Error if any invalid keys are defined when inheriting a dependency #2

Muscraft · 2022-04-25T22:36:38Z

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

PRs in this RFC:

Base Implementation
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

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 · 2022-04-26T01:30:00Z

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

epage · 2022-04-26T01:53:07Z

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 · 2022-04-26T02:04:33Z

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 · 2022-04-26T02:15:42Z

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 · 2022-04-26T02:20:14Z

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 · 2022-04-27T01:26:53Z

tests/testsuite/cargo_add.rs

+#[cargo_test]
+fn invalid_key_inherit_dependency() {


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

I can add a test for this

epage · 2022-04-27T01:33:08Z

src/cargo/ops/cargo_add/mod.rs

@@ -284,6 +284,22 @@ fn resolve_dependency(
 // Checking for a workspace dependency happens first since a member could be specified
 // in the workspace dependencies table as a dependency
 if let Some(_dep) = find_workspace_dep(dependency.toml_key(), ws.root_manifest()).ok() {
+ let err_msg = "cannot be defined when inheriting a workspace dependency";


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

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

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.

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

epage · 2022-04-27T01:35:19Z

src/cargo/ops/cargo_add/mod.rs

+ if arg.default_features.is_some() {
+ anyhow::bail!("`default-features` {}", err_msg)
+ }
+ if arg.registry.is_some() {
+ anyhow::bail!("`registry` {}", err_msg)
+ }
+ if arg.branch.is_some() {
+ anyhow::bail!("`branch` {}", err_msg)
+ }
+ if arg.tag.is_some() {
+ anyhow::bail!("`tag` {}", err_msg)
+ }
+ if arg.rev.is_some() {
+ anyhow::bail!("`rev` {}", err_msg)
+ }


. 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

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

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.

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

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

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

epage · 2022-04-27T12:12:46Z

src/cargo/ops/cargo_add/mod.rs

+ format!(
+ "cannot override workspace dependency with `{}`, \
+ either change `workspace.dependencies.{}.{}` \
+ or define the dependency exclusively in the package's manifest",
+ key, toml_key, key
+ )


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",

I'll change it to this

epage · 2022-04-27T12:14:15Z

tests/snapshots/add/invalid_key_overwrite_inherit_dependency.in/primary/Cargo.toml

+
+foo.workspace = true


Is this missing a [dependencies]?

It looks like it is

epage · 2022-04-27T12:16:18Z

src/cargo/ops/cargo_add/mod.rs

+ }
+ // rename is `package`
+ if arg.rename.is_some() {
+ anyhow::bail!("{}", err_msg(toml_key, "rename"))


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

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 · 2022-04-27T12:18:50Z

tests/testsuite/cargo_add.rs

+ snapbox::cmd::Command::cargo()
+ .masquerade_as_nightly_cargo()
+ .arg("add")
+ .args(["foo", "--registry", "registry", "-p", "bar"])


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

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

Yes, please

epage · 2022-04-27T12:51:51Z

src/cargo/ops/cargo_add/mod.rs

+ if let Some(Source::Workspace(_)) = old_dep.source() {
+ check_invalid_ws_keys(old_dep.toml_key(), arg)?;
+ }


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

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

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.

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

…endency

Cargo add support for workspace inheritance Tracking issue: #8415 RFC: rust-lang/rfcs#2906 This PR adds all the required support for workspace inheritance within `cargo-add`. It was split up across a few different PRs as it required `snapbox` support from #10581 and a new `toml_edit` version from #10603. `@epage` and I decided to go ahead with this PR and add in some of the changes those PRs made. `@epage's` name on the commits is from helping to rewrite commits and some very minor additions. Changes: - #10585 - Muscraft#1 - Muscraft#3 - Muscraft#2 - Muscraft#4 r? `@epage`

Muscraft force-pushed the cargo-add-support branch from 049f6eb to 7b64cb8 Compare April 25, 2022 23:19

Muscraft force-pushed the error-invalid-keys branch from ed0a124 to 06ecb3e Compare April 25, 2022 23:20

epage suggested changes Apr 26, 2022

View reviewed changes

Muscraft force-pushed the cargo-add-support branch from bfb9def to 7914967 Compare April 27, 2022 00:08

Muscraft changed the title ~~Error if any invalid keys are defined with workspace = true for `ca…~~ Error if any invalid keys are defined when inheriting dependency Apr 27, 2022

Muscraft changed the title ~~Error if any invalid keys are defined when inheriting dependency~~ Error if any invalid keys are defined when inheriting a dependency Apr 27, 2022

Muscraft force-pushed the error-invalid-keys branch from 06ecb3e to a13ef84 Compare April 27, 2022 01:07

epage reviewed Apr 27, 2022

View reviewed changes

Muscraft force-pushed the error-invalid-keys branch from a13ef84 to 5c4bcca Compare April 27, 2022 02:40

epage reviewed Apr 27, 2022

View reviewed changes

Muscraft force-pushed the error-invalid-keys branch from 5c4bcca to 3b875d5 Compare April 27, 2022 12:41

epage reviewed Apr 27, 2022

View reviewed changes

epage approved these changes Apr 27, 2022

View reviewed changes

Error if any invalid keys are defined when inheriting a workspace dep…

74d7f69

…endency

Muscraft force-pushed the error-invalid-keys branch from 3b875d5 to 74d7f69 Compare April 27, 2022 15:34

Muscraft merged this pull request into cargo-add-support Apr 27, 2022

This was referenced Apr 27, 2022

Support overwriting a foo.workspace = true with a dependency from a… #4

Merged

Cargo add support for workspace inheritance rust-lang/cargo#10606

Merged

Muscraft deleted the error-invalid-keys branch May 27, 2022 01:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Error if any invalid keys are defined when inheriting a dependency #2

Error if any invalid keys are defined when inheriting a dependency #2

Muscraft commented Apr 25, 2022 •

edited

Loading

epage left a comment

Muscraft commented Apr 26, 2022

epage commented Apr 26, 2022

Muscraft commented Apr 26, 2022

epage commented Apr 26, 2022

Muscraft commented Apr 26, 2022

epage Apr 27, 2022

Muscraft Apr 27, 2022

epage Apr 27, 2022

Muscraft Apr 27, 2022

epage Apr 27, 2022

Muscraft Apr 27, 2022

epage Apr 27, 2022

Muscraft Apr 27, 2022

epage Apr 27, 2022

Muscraft Apr 27, 2022

Muscraft Apr 27, 2022 •

edited

Loading

epage Apr 27, 2022

epage Apr 27, 2022

Muscraft Apr 27, 2022

epage Apr 27, 2022

Muscraft Apr 27, 2022

epage Apr 27, 2022

Muscraft Apr 27, 2022

epage Apr 27, 2022

Muscraft Apr 27, 2022

epage Apr 27, 2022

epage Apr 27, 2022

Muscraft Apr 27, 2022

epage Apr 27, 2022

Muscraft Apr 27, 2022

epage left a comment

epage left a comment

		#[cargo_test]
		fn invalid_key_inherit_dependency() {


		foo.workspace = true

Error if any invalid keys are defined when inheriting a dependency #2

Error if any invalid keys are defined when inheriting a dependency #2

Conversation

Muscraft commented Apr 25, 2022 • edited Loading

epage left a comment

Choose a reason for hiding this comment

Muscraft commented Apr 26, 2022

epage commented Apr 26, 2022

Muscraft commented Apr 26, 2022

epage commented Apr 26, 2022

Muscraft commented Apr 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Muscraft Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epage left a comment

Choose a reason for hiding this comment

epage left a comment

Choose a reason for hiding this comment

Muscraft commented Apr 25, 2022 •

edited

Loading

Muscraft Apr 27, 2022 •

edited

Loading