-
-
Notifications
You must be signed in to change notification settings - Fork 14.1k
Emit check-cfg lints during attribute parsing rather than evaluation
#149215
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
Conversation
This comment has been minimized.
This comment has been minimized.
|
Does it mean we would get warnings for non-active cfg attributes? #[cfg(target_os = "lol")] // warn and not active
#[cfg(unknown)] // currently doesn't warn, since above is not active, but will it with this PR?
fn foo() {}Regarding the non-accessible |
abb82ad to
4ee0f66
Compare
This comment has been minimized.
This comment has been minimized.
4ee0f66 to
f5e9e93
Compare
This comment has been minimized.
This comment has been minimized.
|
Attributes that are configured out (like in your example) are currently not parsed, and therefore this would not cause this warning, preserving the old behavior. I didn't think of this edgecase so I'll make sure to add a test for this. Indeed currently attribute lints are special and not emitted as a builtin lint. I'm gonna play around with this a bit and see if anything is stopping me from changing that |
…r=Urgau Run `eval_config_entry` on all branches so we always emit lints Fixes rust-lang#149090 Ideally I'd have liked to fix this issue using rust-lang#149215, and this is still the long term plan, but this is slightly more annoying to implement than I'd have liked to, and this is also a nice and easy solution to the problem. r? `@tgross35`
Rollup merge of #149380 - JonathanBrouwer:cfg_select_lints, r=Urgau Run `eval_config_entry` on all branches so we always emit lints Fixes #149090 Ideally I'd have liked to fix this issue using #149215, and this is still the long term plan, but this is slightly more annoying to implement than I'd have liked to, and this is also a nice and easy solution to the problem. r? `@tgross35`
Run `eval_config_entry` on all branches so we always emit lints Fixes rust-lang/rust#149090 Ideally I'd have liked to fix this issue using rust-lang/rust#149215, and this is still the long term plan, but this is slightly more annoying to implement than I'd have liked to, and this is also a nice and easy solution to the problem. r? `@tgross35`
|
☔ The latest upstream changes (presumably #149410) made this pull request unmergeable. Please resolve the merge conflicts. |
Run `eval_config_entry` on all branches so we always emit lints Fixes rust-lang/rust#149090 Ideally I'd have liked to fix this issue using rust-lang/rust#149215, and this is still the long term plan, but this is slightly more annoying to implement than I'd have liked to, and this is also a nice and easy solution to the problem. r? `@tgross35`
…elmann Move even more early buffered lints to dyn lint diagnostics Follow-up to #145881 and #145747. I originally wanted to migrate most if not the entire rest of the early buffered lints. However, when trying to migrate the buffered lints used by check-cfg I encountered a roadblock. Namely, it depends on `TyCtxt` (well, `Option<TyCtxt>`) which makes it quite hard to migrate (see also #147634 (comment), #147634 (comment) & #149215). So for now, I won't migrate it (maybe #149215 will find a solution), nor will I migrate the rest since it's quite tedious to migrate these. I'll leave them for future me.
f5e9e93 to
1b85568
Compare
|
☔ The latest upstream changes (presumably #149646) made this pull request unmergeable. Please resolve the merge conflicts. |
1b85568 to
557fcb0
Compare
557fcb0 to
5010285
Compare
|
Rebased on #149662 After that PR, this change becomes trivial, we don't need to move the lints and we have access to |
check-cfg lints to rustc_attr_parsingcheck-cfg during attribute parsing rather than evaluation
check-cfg during attribute parsing rather than evaluationcheck-cfg lints during attribute parsing rather than evaluation
5010285 to
b93aaa7
Compare
|
Some changes occurred in compiler/rustc_attr_parsing Some changes occurred in compiler/rustc_hir/src/attrs |
|
The PR is ready now :) |
| | ^ help: found config with similar value: `target_feature = "a"` | ||
| | | ||
| = help: expected names are: `FALSE` and `test` and 31 more | ||
| = help: to expect this configuration use `--check-cfg=cfg(a)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some small comments. Let me know, if I'm wrong about them I'll approve after :)
| pos: usize, | ||
| ) -> EvalConfigResult { | ||
| let res = self.cfg().cfg_true(&attr, node.node_id(), ShouldEmit::ErrorsAndLints); | ||
| let res = self.cfg().cfg_true(&attr, ShouldEmit::ErrorsAndLints); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we still need the ShouldEmit. You removed the nodeid and that's usually relevant for emitting lints. So maybe we don't need the should_emit anymore either?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you're getting the nodeid out of self now. Maybe should emit as well? Or does that change based on where we call this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ShouldEmit depends on where we call this indeed. Some cfg attributes are evaluated multiple times and we want to be careful to only emit errors once, that's why the ShouldEmit is an argument here
The node.node_id() was actually incorrect, because that turned out to be always the dummy node id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, cool!
|
@rustbot author |
|
Reminder, once the PR becomes ready for a review, use |
|
@rustbot ready |
|
Looks good then I think. @bors r+ rollup |
…szelmann Emit `check-cfg` lints during attribute parsing rather than evaluation The goal of this PR is to make the `eval_config_entry` not have any side effects, by moving the check-cfg lints to the attribute parsing. This also helps ensure we do emit the lint in situations where the attribute happens to be parsed, but never evaluated. cc `@jdonszelmann` `@Urgau` for a vibe check if you feel like it
Rollup of 12 pull requests Successful merges: - #147572 (std: Use more `unix.rs` code on WASI targets) - #148491 ( Correctly provide suggestions when encountering `async fn` with a `dyn Trait` return type) - #149215 (Emit `check-cfg` lints during attribute parsing rather than evaluation) - #149652 (Add release notes for 1.92.0) - #149720 (rustdoc book: mention inner doc attribute) - #149730 (lint: emit proper diagnostic for unsafe binders in improper_ctypes instead of ICE) - #149754 (Retire `opt_str2` from compiletest cli parsing) - #149755 (bootstrap: Use a `CompiletestMode` enum instead of bare strings) - #149763 (Add inline attribute to generated delegation function if needed) - #149772 (test: Add a test for 146133) - #149779 (Fix typo "an" → "and") - #149782 (Remove `[no-mentions]` handler in the triagebot config) r? `@ghost` `@rustbot` modify labels: rollup
Rollup of 11 pull requests Successful merges: - #147585 (Suppress the error for private fields with non_exhaustive attribute) - #149215 (Emit `check-cfg` lints during attribute parsing rather than evaluation) - #149652 (Add release notes for 1.92.0) - #149720 (rustdoc book: mention inner doc attribute) - #149730 (lint: emit proper diagnostic for unsafe binders in improper_ctypes instead of ICE) - #149754 (Retire `opt_str2` from compiletest cli parsing) - #149755 (bootstrap: Use a `CompiletestMode` enum instead of bare strings) - #149763 (Add inline attribute to generated delegation function if needed) - #149772 (test: Add a test for 146133) - #149779 (Fix typo "an" → "and") - #149782 (Remove `[no-mentions]` handler in the triagebot config) Failed merges: - #148491 ( Correctly provide suggestions when encountering `async fn` with a `dyn Trait` return type) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #149215 - JonathanBrouwer:cfg_lints2, r=jdonszelmann Emit `check-cfg` lints during attribute parsing rather than evaluation The goal of this PR is to make the `eval_config_entry` not have any side effects, by moving the check-cfg lints to the attribute parsing. This also helps ensure we do emit the lint in situations where the attribute happens to be parsed, but never evaluated. cc ``@jdonszelmann`` ``@Urgau`` for a vibe check if you feel like it
The goal of this PR is to make the
eval_config_entrynot have any side effects, by moving the check-cfg lints to the attribute parsing. This also helps ensure we do emit the lint in situations where the attribute happens to be parsed, but never evaluated.cc @jdonszelmann @Urgau for a vibe check if you feel like it