Emit error on [target.'cfg(debug_assertions)'.dependencies] and similar

[benediktwerner]

Closes #7634

Cargo now emits an error if an unsupported cfg key or name is used for specifying a target.

This PR also updates the docs to clarify this behavior.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[benediktwerner]

Ok, it looks like a few of the tests are using invalid keys. Is there a reason for this, like a way to provide custom cfg values? Should I fix the tests or should cargo maybe only error on debug_assertions etc. and feature = ...?

[alexcrichton]

Thanks for the PR! I think we'll probably want a blacklist as opposed to a whitelist of names, since you can also affect dependency selection with RUSTFLAGS. That means that I don't think we can write down an exhaustive whitelist of names that can be included and matched on

[benediktwerner]

Ok, makes sense, I changed it to only error on test, debug_assertions, proc_macro and feature = ....

[alexcrichton]

Looks good to me! I'm curious though, I would suspect that this should start out as a warning, but @ehuss could you elaborate why you thought it should be an error?

[ehuss]

why you thought it should be an error?

I don't feel strongly about it, it's just that it is an invalid configuration that is not going to work correctly. I'd be happy with a warning. I did a quick scan on crates.io, and found the following manifests that use one of these patterns:

debug_assertions:

• rocket_contrib 0.4.2

test:

• com_ptr 0.1.2
• tokio-net-0.2.0-alpha.6

feature:

• aes-0.3.2
• aes-ctr-0.3.0
• collision-0.20.1
• font-rs-0.1.3
• hyper-thread-random-0.4.2
• iban_validate-3.0.0
• instant-0.1.2
• label_attribute-0.1.1
• ldap3-0.6.1
• lokacore-0.3.0
• mgf-1.3.0
• miscreant-0.4.2
• orbclient-0.3.27
• resid-rs-1.0.3
• rhusics-core-0.7.0
• rhusics-ecs-0.7.0
• sacn-0.4.4
• slice-deque-0.3.0
• tokio-0.2.0-alpha.6
• tracing-0.1.10
• tracing-core-0.1.7
• wasm-bindgen-futures-0.4.4
• xplm-0.2.2
• yew-0.10.0 (.cargo/config)

That's a fairly large number.

I'd also like if this could include more help in the error message, since "invalid" is a little terse. I'm not sure how to word it, but something along the lines to indicate that those keys are not supported by Cargo in target expressions. Particularly feature="…" could maybe use some guidance on what to do instead, though that is only relevant to dependencies. Config files don't have an alternative that I can think of.

[benediktwerner]

Better error messages would definitely be good, but I wasn't quite sure about the best way to do this. It doesn't look like there is any more advanced error reporting mechanism in cargo, right? So should I just make the Display output for the error more explicit?

It might also be a good idea to link to the docs. Maybe something like this: Invalid cfg key: feature. This key is not supported in target configurations. Use the [features] section instead: https://doc.rust-lang.org/cargo/reference/manifest.html#the-features-section and Invalid cfg value: debug_assertions. This value is not supported in target configurations. To learn more visit https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#platform-specific-dependencies

A warning would obviously work as well, but that would require some slight changes since I don't think it's possible to warn from cargo-platforms.

[alexcrichton]

The manifest parsing section of Cargo has the ability to emit warnings, so perhaps that would be a good place to put informational messages?

[benediktwerner]

Ok, I changed it so that it warns when parsing the manifest. Not sure if it makes sense to keep that code in cargo-platform but I also didn't find a better place and in the end, it is about the platform after all.

[alexcrichton]

@bors: r+

Looks great to me, thanks!

[bors]

📌 Commit aa93f61 has been approved by alexcrichton

[bors]

⌛ Testing commit aa93f61 with merge 774d949...

[bors]

☀️ Test successful - checks-azure
Approved by: alexcrichton
Pushing 774d949 to master...