-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
check-cfg
tests are prone to merge conflicts
#135995
Comments
cc @Urgau do you have any preferences for this? |
I prefer if we normalize the output, it's irrelevant for the test. I had a few sparse minutes and did it my-self in #136016. |
When I ported some code base as part the stabilization effort, having that list was helpful for the custom cfgs. It should also be noted that the list is only printed once (always the first warning) as to reduce the visual impact. |
Btw, we already have a limit, it's currently 35.
|
Rollup merge of rust-lang#136016 - Urgau:check-cfg-allow-test-improv, r=jieyouxu Improve check-cfg expected names diagnostic This PR improves the check-cfg `allow-same-level` test by ~~normalizing it's output and by~~ adding more context to the test. It also filters the well known cfgs from the `expected names are` note, as to reduce the size of the diagnostic. Users can still find the full list on the [rustc book](https://doc.rust-lang.org/nightly/rustc/check-cfg.html#well-known-names-and-values), which is reinforced for Cargo users by adding a note in the Cargo check-cfg specific section. Fixes rust-lang#135995 r? `@jieyouxu`
Right now, the tests in
tests/ui/check-cfg/
contain every built-in config known to the compiler, like so:rust/tests/ui/check-cfg/allow-same-level.stderr
Line 7 in 061ee95
this means they have to be updated each time a new builtin cfg is added, which is annoying, prone to merge conflicts, and distracts from what the test is actually doing. Also, it's just not a very good diagnostic? If you name a check-cfg something the compiler doesn't know, an exhaustive list of things it does know is probably not the thing you want.
I suggest one or more of the following:
rust/compiler/rustc_lint/src/early/diagnostics/check_cfg.rs
Line 143 in 061ee95
rust/compiler/rustc_lint/src/early/diagnostics/check_cfg.rs
Line 206 in 061ee95
@rustbot label A-testsuite A-diagnostics F-check-cfg
The text was updated successfully, but these errors were encountered: