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

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

Merged
merged 2 commits into from
Sep 3, 2022

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented Aug 15, 2022

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 #99519.

r? @petrochenkov

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 15, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 15, 2022
@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 27, 2022
@rustbot rustbot added the A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic label Aug 28, 2022
@rustbot
Copy link
Collaborator

rustbot commented Aug 28, 2022

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @estebank, @TaKO8Ki

@Urgau
Copy link
Member Author

Urgau commented Aug 28, 2022

I've update the implementation to use a early lint pass. Let me know if there is something to change.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 28, 2022
@petrochenkov
Copy link
Contributor

Could you also add a test making sure that crate-level #![allow(unexpected_cfgs)] actually silences the lint?
@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 29, 2022
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Aug 30, 2022

📌 Commit bc68381 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 30, 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
Copy link
Member

@bors r- looks like some tests failed in a rolluup

---- [ui] src/test/ui/check-cfg/mix.rs stdout ----
diff of stderr:

28	LL | #[cfg_attr(uu, test)]
29	   |            ^^
30	
-	warning: unexpected condition value `bar` for condition name `feature`
-	   |
-	   = help: was set with `--cfg` but isn't in the `--check-cfg` expected values
-	
35	warning: unexpected `unknown_name` as condition name
36	   |
37	   = help: was set with `--cfg` but isn't in the `--check-cfg` expected names

+	
+	warning: unexpected condition value `bar` for condition name `feature`
+	   |
+	   = help: was set with `--cfg` but isn't in the `--check-cfg` expected values
38	
39	warning: unexpected `cfg` condition name
40	  --> $DIR/mix.rs:35:10

#101244

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 31, 2022
@Urgau
Copy link
Member Author

Urgau commented Aug 31, 2022

I re-blessed the test output src/test/ui/check-cfg/mix.rs, should be good to go again.

@rustbot ready

@Urgau
Copy link
Member Author

Urgau commented Sep 2, 2022

I've rebased to fix the conflict in compiler/rustc_lint_defs/src/builtin.rs from #94467.

@rustbot ready

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 2, 2022
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Sep 2, 2022

📌 Commit eccdccf has been approved by petrochenkov

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 2, 2022
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
Copy link
Contributor

bors commented Sep 2, 2022

⌛ Testing commit eccdccf with merge be4f3777ade35de16e8a2fdba8acbe40e0ade7b9...

@bors
Copy link
Contributor

bors commented Sep 3, 2022

💔 Test failed - checks-actions

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

The job aarch64-gnu failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
[RUSTC-TIMING] collectionsbenches test:true 66.486
[RUSTC-TIMING] collectionstests test:true 67.485
[RUSTC-TIMING] coretests test:true 93.299
[RUSTC-TIMING] alloc test:true 107.707
##[error]The runner has received a shutdown signal. This can happen when the runner service is stopped, or a manually started runner is canceled.

@Urgau
Copy link
Member Author

Urgau commented Sep 3, 2022

== clock drift check ==
local time: Sat Sep 3 00:01:48 UTC 2022
Session terminated, killing shell... ...killed.
time="2022-09-03T00:01:48Z" level=error msg="error waiting for container: unexpected EOF"
network time:
Error: The operation was canceled.

Seems unrelated to the PR. Could be retried.

@matthiaskrgr
Copy link
Member

@bors retry

@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 Sep 3, 2022
@bors
Copy link
Contributor

bors commented Sep 3, 2022

⌛ Testing commit eccdccf with merge 47d1cdb...

@bors
Copy link
Contributor

bors commented Sep 3, 2022

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 47d1cdb to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 3, 2022
@bors bors merged commit 47d1cdb into rust-lang:master Sep 3, 2022
@rustbot rustbot added this to the 1.65.0 milestone Sep 3, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (47d1cdb): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
3.2% [2.4%, 4.9%] 6
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.7% [-4.3%, -2.8%] 3
All ❌✅ (primary) - - 0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
4.4% [4.4%, 4.4%] 1
Regressions ❌
(secondary)
3.6% [3.6%, 3.6%] 1
Improvements ✅
(primary)
-1.7% [-1.7%, -1.7%] 1
Improvements ✅
(secondary)
-5.5% [-6.7%, -4.4%] 3
All ❌✅ (primary) 1.3% [-1.7%, 4.4%] 2

Footnotes

  1. the arithmetic mean of the percent change 2

  2. number of relevant changes 2

Nilstrieb added a commit to Nilstrieb/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
A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic merged-by-bors This PR was explicitly merged by bors. 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.

None yet

9 participants