Error if any invalid keys are defined when inheriting a dependency

[Muscraft]

Tracking issue: rust-lang#8415
RFC: rust-lang/rfcs#2906

PRs in this RFC:

• Base Implementation
  • Part 1 of RFC2906 - Packages can inherit fields from their root workspace rust-lang/cargo#10497
  • Part 2 of RFC2906 -- allow inheriting from a different Cargo.toml rust-lang/cargo#10517
  • Part 3 of RFC2906 - Add support for inheriting license-path, and depednency.path rust-lang/cargo#10538
  • Part 4 of RFC2906 - Add support for inheriting readme rust-lang/cargo#10548
• New Fields
  • Part 5 of RFC2906 - Add support for inheriting rust-version rust-lang/cargo#10563
  • Part 7 of RFC2906 - Add support for inheriting exclude and include rust-lang/cargo#10565
• Update inheritance source
  • Part 6 of RFC2906 - Switch the inheritance source from workspace to… rust-lang/cargo#10564
• Optimizations
  • Part 8 of RFC2906 - Keep InheritableFields in a LazyCell inside `… rust-lang/cargo#10568
• Documentation changes
  • Prefer key.workspace = true to key = { workspace = true } rust-lang/cargo#10584
• cargo-add support
  • Allow running cargo add foo when a foo.workspace = true already exists rust-lang/cargo#10585
  • Show the merged set of activated features to the user when inheriting… #1
  • Detect cargo add foo will inherit a workspace dependency #3

Changes:

• Added checks for invalid keys (default-features, registry, branch, tag, rev) when inheriting a dependency from a workspace

Remaining implementation work for the RFC

• cargo-add support, see epage's comment
• Optimizations as needed

r? @epage

[epage]

We will have everything pre-parsed by the regular parser, so we shouldn't need to error check this.

In my re-wording, I think some context was lost. We need to error if the user sets any flags on the command-line that are incompatible with workspace = true.

[Muscraft]

So the error should be during the parsing of the CLI flags?

[epage]

So the error should be during the parsing of the CLI flags?

It has to happen where we have access to the CLI flags and we know its a workspace dependency. Note that we pass in a struct to the ops that basically represents the CLI, so we know the details.

Keep in mind that right now we are just loading an existing workspace entry. We'll soon also take the name and look up to find if its a workspace dependency.

[Muscraft]

Would it make sense to wait until after adding in where we take the name and look up to find if it's a workspace dependency, to do this?

[epage]

We can implement the PRs in any order so long as we keep the work for both in mind though doing the other PR first will make this one more obvious for what to do

[Muscraft]

I agree that one of the others would help make this slightly more clear. I'll do one of those and then come back to this.

[epage]

What about the case where the dependency already exists and we use one of these keys?

[Muscraft]

I can add a test for this

[epage]

What if we rephrase this as

 "cannot override workspace dependency with `{}`, either change `workspace.dependencies.{}.{}` or define the dependency exclusively in the package's manifest"

• I feel like "defined" is confusing language in this context
• I feel like talking in terms of "overriding" is more specific and clear than generically talking about "inheriting"
• This provides a remediation

[Muscraft]

Does this override anything? To me, this only runs when it is not overriding something since it would've been found by get_existing_dependency. As for changing it I agree that we should change it to something else

[epage]

I guess there are two different kinds of overriding to talk about. This isn't writing over an existing fields value with a different one but it is instead overriding the field it inherits from workspace.dependencies.

[Muscraft]

Yeah, that makes sense. I'll change it to what you said

[epage]

. This means you cannot define workspace with keys like version, registry, registry-index, path, git, branch, tag, rev, or package.

What about checking for

• path
• git
• version
• package

[Muscraft]

The only package that is used here is to tell cargo add what package to add the dependency too. The others should be checked before this since they all would set a source from what I can tell

[epage]

The only package that is used here is to tell cargo add what package to add the dependency too.

Try a test case where you cargo add --rename foo foo-alt where foo is a workspace dependency and foo-alt is a workspace member.

The others should be checked before this since they all would set a source from what I can tell

Thats right, we either do or will cover this with testing switching sources. Probably good to add a comment about it.

[Muscraft]

Try a test case where you cargo add --rename foo foo-alt where foo is a workspace dependency and foo-alt is a workspace member.

So package is rename? or at least that's the way it looks to me

Thats right, we either do or will cover this with testing switching sources. Probably good to add a comment about it.

I'll add a comment about this

[Muscraft]

I also do not know if we need branch, tag, or rev, since they can't be defined on their own

[epage]

I was thinking similar about branch, tag, and rev.

[epage]

nit: this might read better if using the new format macro feature

            "cannot override workspace dependency with `{field}`, \
            either change `workspace.dependencies.{toml_key}.{field}` \
            or define the dependency exclusively in the package's manifest",

[Muscraft]

I'll change it to this

[epage]

Is this missing a [dependencies]?

[Muscraft]

It looks like it is

[epage]

This is a case where the argument and field diverge (--rename to cause package to be set)

In general, I feel like the error message will be easier to read with arguments

error: cannot override workspace dependency with `--rename`, either change `workspace.dependencies.foo.package` or define the dependency exclusively in the package's manifest

[Muscraft]

fn err_msg(toml_key: &str, flag: &str, field: &str) -> String {
        format!(
            "cannot override workspace dependency with `{flag}`, \
            either change `workspace.dependencies.{toml_key}.{field}` \
            or define the dependency exclusively in the package's manifest"
        )
    }

    if arg.default_features.is_some() {
        anyhow::bail!("{}", err_msg(toml_key, "--default-features", "default-features"))
    }
    if arg.registry.is_some() {
        anyhow::bail!("{}", err_msg(toml_key, "--registry", "registry"))
    }
    // rename is `package`
    if arg.rename.is_some() {
        anyhow::bail!("{}", err_msg(toml_key, "--rename", "package"))
    }

[epage]

Is there a reason this test is using --registry? I think it can obscure what you are trying to test by changing more variables.

[Muscraft]

There is no specific reason that used --registry here, do you think I should change it to --default-features?

[epage]

Yes, please

[epage]

What if we combined the two and moved this to the end of the function?

• Check the source on dependency instead of old_dep
• Condition the check on the source being a Workspace

[Muscraft]

That works for me. Does moving it to after if dependency.source().is_none(), instead of the end of the function work?

[epage]

It doesn't have to be literally at the end of the function but among the clauses at the end of the function.

Stylistically, make sure it has newlines before and after since its not logically coupled to any of the blocks.

[Muscraft]

That's how I have it! I'll push the changes and merge it then!

[epage]

Once/if the checks get consolidated

[epage]

Ok, this time I remembered to actually approve