Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove implicit names and values from --cfg in --check-cfg #99519

Merged
merged 1 commit into from
Aug 1, 2022

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented Jul 20, 2022

This PR remove the implicit names and values from --cfg in --check-cfg because the behavior is quite surprising but also because it's really easy to inadvertently really on the implicitness and when the --cfg is not set anymore to have an unexpected warning from an unexpected condition that pass with the implicitness.

This change in behavior will also enable us to warn when an unexpected --cfg is passed, ex: the user wrote --cfg=unstabl instead of --cfg=unstable. The implementation of the warning will be done in a follow-up PR.

cc @petrochenkov

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 20, 2022
@rust-highfive
Copy link
Collaborator

r? @JohnTitor

(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 Jul 20, 2022
@est31
Copy link
Member

est31 commented Jul 20, 2022

I'm not a fan of this, even though it would have found the bug of #98147 way earlier. I think it's not worth the cost paid in longer command invocations. Also, when you type --cfg=unstabl, what prevents you from typing that for the check cfg too?

@JohnTitor
Copy link
Member

This is more of a t-compiler PR than docs one, so r? @petrochenkov

@Urgau
Copy link
Member Author

Urgau commented Jul 21, 2022

I'm not a fan of this, even though it would have found the bug of #98147 way earlier. I think it's not worth the cost paid in longer command invocations.

I don't think the cost of having longer CLI args is problematic because:

  • cargo starting from this PR Retry command invocation with argfile cargo#10546 now automatically retry with argument file if there to much argument than the platform support
  • the benefits are important especially the cost of people struggling, they would immediately see a/the problem

Also, when you type --cfg=unstabl, what prevents you from typing that for the check cfg too?

If it's just cmd args nothing, BUT setting the condition means using the cfg in the code, so having #[cfg(unstable)] in the code would warn because the set and expected are unstabl but the code is waiting on unstable you would then see the problem and would be able to fix it immediately, but with the current implicitness you wouldn't have any warn as unstabl would be expected even thought it it's what you want at all.

@est31
Copy link
Member

est31 commented Jul 21, 2022

having #[cfg(unstable)] in the code would warn because the set and expected are unstabl but the code is waiting on unstable you would then see the problem and would be able to fix it immediately, but with the current implicitness you wouldn't have any warn as unstabl would be expected even thought it it's what you want at all.

In the current implementation, if you have #[cfg(unstable)] but set --cfg=unstabl, there will be a warning, it will just complain about #[cfg(unstable)] instead of your --cfg=unstabl. Not optimal but I think it's good enough.

cargo starting from this PR now automatically retry with argument file if there to much argument than the platform support

It's fine that it's technically supported, but I believe that reducing noise in command line arguments should be a goal. Sometimes you do want to run with -v or something and it would be annoying to have even longer command line invocations.

@Urgau
Copy link
Member Author

Urgau commented Jul 21, 2022

having #[cfg(unstable)] in the code would warn because the set and expected are unstabl but the code is waiting on unstable you would then see the problem and would be able to fix it immediately, but with the current implicitness you wouldn't have any warn as unstabl would be expected even thought it it's what you want at all.

In the current implementation, if you have #[cfg(unstable)] but set --cfg=unstabl, there will be a warning, it will just complain about #[cfg(unstable)] instead of your --cfg=unstabl. Not optimal but I think it's good enough.

Not if it's coupled with --check-cfg=names(unstable) or --check-cfg=values(unstable)

My main problem with the implicitness is that it's implicit and leads to unexpected problems that we even saw in the rust-lang/rust, that's why I think being more explicit is better and simpler and will lead to better experience for developers. The increase in command line args is unfortunate but it's for the better and that doesn't mean that arguments could not be improved further down.

cargo starting from this PR now automatically retry with argument file if there to much argument than the platform support

It's fine that it's technically supported, but I believe that reducing noise in command line arguments should be a goal. Sometimes you do want to run with -v or something and it would be annoying to have even longer command line invocations.

I don't know if it should be goal but I certainly don't think --check-cfg argument are noise at all.

@petrochenkov
Copy link
Contributor

I think we can try using these stricter rules while the feature is unstable, and even after that.

It will be a backward compatible change to not require --check-cfging names/values already known through --cfg again.
So we can switch back to the current rules if the stricter rules are too annoying.

I'm going to mark this as waiting on team to gather feedback.

@petrochenkov petrochenkov added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). I-compiler-nominated Nominated for discussion during a compiler team meeting. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 21, 2022
@est31
Copy link
Member

est31 commented Jul 21, 2022

Going both ways is backwards compatible as per Rust's semver rules, because it's a lint. But yes, breakage is lower one way than going the other way.

@apiraino
Copy link
Contributor

Visiting after T-compiler meeting (see Zulip notes).

The opinion is to tentatively move forward with --check-cfg and test the waters while it's unstable

@rustbot label -I-compiler-nominated

@rustbot rustbot removed the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Jul 29, 2022
@Urgau
Copy link
Member Author

Urgau commented Jul 30, 2022

Based on the previous comment, this is no longer waiting on team, back on the review queue.

@rustbot label -S-waiting-on-team +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jul 30, 2022
@petrochenkov
Copy link
Contributor

Okay, the implementation is trivial so @bors r+

@bors
Copy link
Contributor

bors commented Jul 30, 2022

📌 Commit ebf4cc3 has been approved by petrochenkov

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 Jul 30, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 1, 2022
…askrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#99519 (Remove implicit names and values from `--cfg` in `--check-cfg`)
 - rust-lang#99620 (`-Z location-detail`: provide option to disable all location details)
 - rust-lang#99932 (Fix unwinding on certain platforms when debug assertions are enabled)
 - rust-lang#99973 (Layout things)
 - rust-lang#99980 (Remove more Clean trait implementations)
 - rust-lang#99984 (Fix compat.rs for `cfg(miri)`)
 - rust-lang#99986 (Add wrap suggestions for record variants)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e820ecd into rust-lang:master Aug 1, 2022
@rustbot rustbot added this to the 1.64.0 milestone Aug 1, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 31, 2022
…henkov

Add warning against unexpected --cfg with --check-cfg

This PR adds a warning when an unexpected `--cfg` is specified but not in the specified list of `--check-cfg`.

This is the follow-up PR I mentioned in rust-lang#99519.

r? `@petrochenkov`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 1, 2022
…henkov

Add warning against unexpected --cfg with --check-cfg

This PR adds a warning when an unexpected `--cfg` is specified but not in the specified list of `--check-cfg`.

This is the follow-up PR I mentioned in rust-lang#99519.

r? `@petrochenkov`
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Sep 1, 2022
…henkov

Add warning against unexpected --cfg with --check-cfg

This PR adds a warning when an unexpected `--cfg` is specified but not in the specified list of `--check-cfg`.

This is the follow-up PR I mentioned in rust-lang#99519.

r? ``@petrochenkov``
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Sep 1, 2022
…henkov

Add warning against unexpected --cfg with --check-cfg

This PR adds a warning when an unexpected `--cfg` is specified but not in the specified list of `--check-cfg`.

This is the follow-up PR I mentioned in rust-lang#99519.

r? ```@petrochenkov```
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Sep 1, 2022
…henkov

Add warning against unexpected --cfg with --check-cfg

This PR adds a warning when an unexpected `--cfg` is specified but not in the specified list of `--check-cfg`.

This is the follow-up PR I mentioned in rust-lang#99519.

r? ````@petrochenkov````
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Sep 1, 2022
…henkov

Add warning against unexpected --cfg with --check-cfg

This PR adds a warning when an unexpected `--cfg` is specified but not in the specified list of `--check-cfg`.

This is the follow-up PR I mentioned in rust-lang#99519.

r? `````@petrochenkov`````
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Sep 2, 2022
…henkov

Add warning against unexpected --cfg with --check-cfg

This PR adds a warning when an unexpected `--cfg` is specified but not in the specified list of `--check-cfg`.

This is the follow-up PR I mentioned in rust-lang#99519.

r? `@petrochenkov`
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 3, 2022
…nkov

Add warning against unexpected --cfg with --check-cfg

This PR adds a warning when an unexpected `--cfg` is specified but not in the specified list of `--check-cfg`.

This is the follow-up PR I mentioned in rust-lang#99519.

r? `@petrochenkov`
@Urgau Urgau deleted the check-cfg-implicit branch May 5, 2023 16:46
Noratrieb added a commit to Noratrieb/rust that referenced this pull request Nov 21, 2023
…trochenkov

Remove `--check-cfg` checking of command line `--cfg` args

Back in rust-lang#100574 we added to the `unexpected_cfgs` lint the checking of `--cfg` CLI arguments and emitted unexpected names and values for them.

The implementation works as expected, but it's usability in particular when using it in combination with Cargo+`RUSTFLAGS` as people who set `RUSTFLAGS=--cfg=tokio_unstable` (or whatever) have `unexpected_cfgs` warnings on all of their crates is debatable. ~~To fix this issue this PR proposes that we split the CLI argument checking into it's own separate allow-by-default lint: `unexpected_cli_cfgs`.~~

~~This has the advantage of letting people who want CLI warnings have them (although not by default anymore), while still linting on every unexpected cfg name and values in the code.~~

After some discussion with the Cargo team ([Zulip thread](https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/check-cfg.20and.20RUSTFLAGS.20interaction)) and member of the compiler team (see below), I propose that we follow the suggestion from `@epage:` never check `--cfg` arguments, but still reserve us the possibility to do it later.

We would still lint on unexpected cfgs found in the source code no matter the `--cfg` args passed. This mean reverting rust-lang#100574 but NOT rust-lang#99519.

r? `@petrochenkov`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 21, 2023
Rollup merge of rust-lang#117522 - Urgau:check-cfg-cli-own-lint, r=petrochenkov

Remove `--check-cfg` checking of command line `--cfg` args

Back in rust-lang#100574 we added to the `unexpected_cfgs` lint the checking of `--cfg` CLI arguments and emitted unexpected names and values for them.

The implementation works as expected, but it's usability in particular when using it in combination with Cargo+`RUSTFLAGS` as people who set `RUSTFLAGS=--cfg=tokio_unstable` (or whatever) have `unexpected_cfgs` warnings on all of their crates is debatable. ~~To fix this issue this PR proposes that we split the CLI argument checking into it's own separate allow-by-default lint: `unexpected_cli_cfgs`.~~

~~This has the advantage of letting people who want CLI warnings have them (although not by default anymore), while still linting on every unexpected cfg name and values in the code.~~

After some discussion with the Cargo team ([Zulip thread](https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/check-cfg.20and.20RUSTFLAGS.20interaction)) and member of the compiler team (see below), I propose that we follow the suggestion from `@epage:` never check `--cfg` arguments, but still reserve us the possibility to do it later.

We would still lint on unexpected cfgs found in the source code no matter the `--cfg` args passed. This mean reverting rust-lang#100574 but NOT rust-lang#99519.

r? `@petrochenkov`
bors pushed a commit to rust-lang/miri that referenced this pull request Nov 21, 2023
Remove `--check-cfg` checking of command line `--cfg` args

Back in rust-lang/rust#100574 we added to the `unexpected_cfgs` lint the checking of `--cfg` CLI arguments and emitted unexpected names and values for them.

The implementation works as expected, but it's usability in particular when using it in combination with Cargo+`RUSTFLAGS` as people who set `RUSTFLAGS=--cfg=tokio_unstable` (or whatever) have `unexpected_cfgs` warnings on all of their crates is debatable. ~~To fix this issue this PR proposes that we split the CLI argument checking into it's own separate allow-by-default lint: `unexpected_cli_cfgs`.~~

~~This has the advantage of letting people who want CLI warnings have them (although not by default anymore), while still linting on every unexpected cfg name and values in the code.~~

After some discussion with the Cargo team ([Zulip thread](https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/check-cfg.20and.20RUSTFLAGS.20interaction)) and member of the compiler team (see below), I propose that we follow the suggestion from `@epage:` never check `--cfg` arguments, but still reserve us the possibility to do it later.

We would still lint on unexpected cfgs found in the source code no matter the `--cfg` args passed. This mean reverting rust-lang/rust#100574 but NOT rust-lang/rust#99519.

r? `@petrochenkov`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants