Provide a better error message when mixing dep: with /

[ehuss]

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]

r? @weihanglo

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

[weihanglo]

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]

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]

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]

Make sense. Thank you!

@bors r+

[bors]

📌 Commit fc1f5a4 has been approved by weihanglo

It is now in the queue for this repository.

[bors]

⌛ Testing commit fc1f5a4 with merge 0b84a35...

[bors]

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