Add future-incompat warning against keywords in cfgs and add raw-idents #14671
Conversation
rustbot
added
A-build-scripts
Urgau
force-pushed
the
cfg-keyword-and-raw-idents
branch
2 times, most recently
from
89428c0
to
a02fbf2
Compare
Urgau
force-pushed
the
cfg-keyword-and-raw-idents
branch
2 times, most recently
from
0087199
to
aa8bb59
Compare
Urgau
force-pushed
the
cfg-keyword-and-raw-idents
branch
from
aa8bb59
to
0569db6
Compare
| @@ -550,9 +550,26 @@ fn cfg_keywords() { | |||
|
|
|||
| p.cargo("check") | |||
| .with_stderr_data(str![[r#" | |||
| [WARNING] [[ROOT]/foo/Cargo.toml] future-incompatibility: `cfg(async)` is deprecated as `async` is a keyword and not an identifier and should not have have been accepted in this position. | |||
There was a problem hiding this comment.
Rustc makes this conditioned on the edition
Compare
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=3e7b39ec2db740f36b54d9fd0e93eaaa
with
https://play.rust-lang.org/?version=stable&mode=debug&edition=2015&gist=8e0ec47ce0d8c34e642dbcdc5abf622c
We can make this dependent on the edition for Cargo.toml but not .cargo/config.toml. We'll need to figure out how we want to handle this
There was a problem hiding this comment.
One solution is to make this only relevant for true / false which is what I had scoped my comments to in #14649 (comment)
There was a problem hiding this comment.
I noticed that to (and forgot to mentioned it, my bad). Idk how Cargo should handle edition keywords.
However I don't think we should only do it for true/false, the other ones are wrong (even if some are not only wrong in some peculiar way), it would inconsistent to allow some and warn on the others.
My stance on the PR is to consider then independently of their "edition"-ness, which could be reversed by excluding any edition keyword until we figure out how to handle them (which would only remove a few of them and keep the vast majority).
There was a problem hiding this comment.
The conversation on this was specifically scoped to "we shouldn't break people now for true / false"
If nothing else, expanding the scope beyond that will mire this in decisions that need to be made vs a simple sign off.
There was a problem hiding this comment.
I'm not in a rush.
I think it would be better to handle all of them in one go, but if you specifically only want to handle true/false I can separate the others keywords and do a follow up PR with them.
There was a problem hiding this comment.
I'd prefer it if we split these conversations up.
What does this PR try to resolve?
This PR tries to address this thread #14649 (comment) in #14649 regarding
cfg(true)/cfg(false)(and keywords more generally) which are wrongly accepted1 as ident.To address this, this PR does two things:
r#true) add suggest-it in the warningHow should we test and review this PR?
This PR should be reviewed commit-by-commit.
Tests are included in preliminary commits.
Additional information
I added a new struct for representing
Idents which is rawness aware.Which implied updating
cargo-platformto0.2.0due to the API changes.r? @epage
Footnotes
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=ccfb9c894dbf14e791c8ae7e4798efd0 ↩