Skip to content

Provide a better error message when mixing dep: with /#11172

Merged
bors merged 1 commit intorust-lang:masterfrom
ehuss:namespaced-feature-err-help
Oct 3, 2022
Merged

Provide a better error message when mixing dep: with /#11172
bors merged 1 commit intorust-lang:masterfrom
ehuss:namespaced-feature-err-help

Conversation

@ehuss
Copy link
Contributor

@ehuss ehuss commented Oct 3, 2022

Features of the form dep:foo/feature aren't accepted as valid syntax. This generated a somewhat confusing error message of:

feature `f1` includes `dep:bar/bar-feat`, but `dep:bar` is not a dependency

This PR adds a more targeted error message that provides some suggestions on how to fix it.

There is more context in #9574 as to why the syntax is the way it is.

@rust-highfive
Copy link

rust-highfive commented Oct 3, 2022

r? @weihanglo

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 3, 2022
weihanglo
weihanglo approved these changes Oct 3, 2022
View reviewed changes
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This patch looks fine to me. Thank you!

One question. Should Cargo errors out earlier in FeatureValue::new if there is a syntax error? It might lose some contexts to provide a good error message, but I found that FeatureValue::new is used in multiple places. Does it deserve a consistent validation logic throughout?

@weihanglo
Copy link
Member

weihanglo commented Oct 3, 2022

Semi-related to this issue. Some other cases emit confusing error messages are:

  • f1 = ["dep:]
  • f1 = ["dep:/]
  • f1 = ["bar/]
  • f1 = ["/feat]
  • f1 = ["/"]

Should we handle this in the follow-up?

@ehuss
Copy link
Contributor Author

ehuss commented Oct 3, 2022

It might be better to have FeatureValue::new return a structured error type, and then handle it in the callers. Unfortunately it is called in a bunch of places, and getting good diagnostics would be a bit more difficult. For example, the errors in build_feature_map need to see if some other feature matches some criteria. In order to do that, it needs to parse all the features and build some maps. If FeatureValue::new returned an error, I think it would be a bit more difficult to handle. Like, perhaps it could keep a map of failed parses to be deferred, then build the other maps with the ones that parse successfully, and then check the failed ones? Overall I think it will be more difficult to handle, though it may be safer.

As for the other invalid forms, it probably wouldn't hurt to have better error messages for those. I don't expect users to enter an invalid form like those, so I don't think it is important. But if someone wants to add more checks, I think that would be fine.

@weihanglo
Copy link
Member

weihanglo commented Oct 3, 2022

Make sense. Thank you!

@bors r+

@bors
Copy link
Contributor

bors commented Oct 3, 2022

📌 Commit fc1f5a4 has been approved by weihanglo

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 3, 2022
@bors
Copy link
Contributor

bors commented Oct 3, 2022

⌛ Testing commit fc1f5a4 with merge 0b84a35...

@bors
Copy link
Contributor

bors commented Oct 3, 2022

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 0b84a35 to master...

@bors bors merged commit 0b84a35 into rust-lang:master Oct 3, 2022
weihanglo added a commit to weihanglo/rust that referenced this pull request Oct 4, 2022
@weihanglo
Update cargo
c1b4f11 
8 commits in f5fed93ba24607980647962c59863bbabb03ce14..0b84a35c2c7d70df4875a03eb19084b0e7a543ef
2022-09-27 12:03:57 +0000 to 2022-10-03 19:13:21 +0000

- Provide a better error message when mixing dep: with / (rust-lang/cargo#11172)
- Remove lingering unstable flag `-Zfeatures` (rust-lang/cargo#11168)
- Tweak wording (rust-lang/cargo#11164)
- Expose libgit2-sys/vendored feature as vendored-libgit2 (rust-lang/cargo#11162)
- refactor(cli): Upgrade to clap v4 (rust-lang/cargo#11159)
- Expose guide to adding a new edition as rustdoc (rust-lang/cargo#11157)
- Remove `multitarget` from -Zhelp (rust-lang/cargo#11158)
- Remove outdated comments (rust-lang/cargo#11155)
@weihanglo weihanglo mentioned this pull request Oct 4, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 5, 2022
@bors
Auto merge of rust-lang#102677 - weihanglo:update-cargo, r=ehuss
d8613f7 
Update cargo

8 commits in f5fed93ba24607980647962c59863bbabb03ce14..0b84a35c2c7d70df4875a03eb19084b0e7a543ef 2022-09-27 12:03:57 +0000 to 2022-10-03 19:13:21 +0000

- Provide a better error message when mixing dep: with / (rust-lang/cargo#11172)
- Remove lingering unstable flag `-Zfeatures` (rust-lang/cargo#11168)
- Tweak wording (rust-lang/cargo#11164)
- Expose libgit2-sys/vendored feature as vendored-libgit2 (rust-lang/cargo#11162)
- refactor(cli): Upgrade to clap v4 (rust-lang/cargo#11159)
- Expose guide to adding a new edition as rustdoc (rust-lang/cargo#11157)
- Remove `multitarget` from -Zhelp (rust-lang/cargo#11158)
- Remove outdated comments (rust-lang/cargo#11155)
@ehuss ehuss added this to the 1.66.0 milestone Oct 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@weihanglo weihanglo weihanglo approved these changes

Assignees

@weihanglo weihanglo

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

1.66.0

Development

Successfully merging this pull request may close these issues.

4 participants

@ehuss @rust-highfive @weihanglo @bors