Improve check-cfg expected names diagnostic #136016

Urgau · 2025-01-24T17:30:47Z

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, which is reinforced for Cargo users by adding a note in the Cargo check-cfg specific section.

Fixes #135995
r? @jieyouxu

rust-lang#135995

jyn514 · 2025-01-24T17:36:18Z

This is not specific to the one test, it applies to nearly all the tests in the check-cfg directory.

Urgau · 2025-01-24T17:50:48Z

Many of the other tests have custom (non-builtin) check-cfgs which are reflected in the output. I can take a look at trimming those that never uses it but it will never be zero tests.

jyn514 · 2025-01-24T17:52:15Z

Right. this is why i suggested trimming builtin cfgs in the diagnostics, not during normalization, because they are noisy and i don't think they're actually very helpful.

Urgau · 2025-01-24T18:51:09Z

Oh, you mean only trimming the builtins but still showing the custom ones (if there any)?

I though you meant removing the entire line. I kind of like this idea, and if the users wants to see the builtins, aka well known, they can see the full list on the rustc book.

Urgau · 2025-01-25T12:48:09Z

tests/ui/check-cfg/cargo-feature.none.stderr

   |
 LL | #[cfg(tokio_unstable)]
   |       ^^^^^^^^^^^^^^
   |
-   = help: expected names are: `clippy`, `debug_assertions`, `doc`, `doctest`, `feature`, `fmt_debug`, `miri`, `overflow_checks`, `panic`, `proc_macro`, `relocation_model`, `rustfmt`, `sanitize`, `sanitizer_cfi_generalize_pointers`, `sanitizer_cfi_normalize_integers`, `target_abi`, `target_arch`, `target_endian`, `target_env`, `target_family`, `target_feature`, `target_has_atomic`, `target_has_atomic_equal_alignment`, `target_has_atomic_load_store`, `target_os`, `target_pointer_width`, `target_thread_local`, `target_vendor`, `ub_checks`, `unix`, and `windows`
+   = help: expected names are: `docsrs`, `feature`, and `test` and 30 more


With this PR, Cargo users would now only see those 3 cfgs (which are Cargo defined) instead of the long list.

Urgau · 2025-01-25T12:51:07Z

tests/ui/check-cfg/well-known-names.stderr

+LL | #[cfg(list_all_well_known_cfgs)]
+   |       ^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: expected names are: `clippy`
+`debug_assertions`
+`doc`
+`doctest`
+`fmt_debug`
+`miri`
+`overflow_checks`
+`panic`
+`proc_macro`
+`relocation_model`
+`rustfmt`
+`sanitize`
+`sanitizer_cfi_generalize_pointers`
+`sanitizer_cfi_normalize_integers`
+`target_abi`
+`target_arch`
+`target_endian`
+`target_env`
+`target_family`
+`target_feature`
+`target_has_atomic`
+`target_has_atomic_equal_alignment`
+`target_has_atomic_load_store`
+`target_os`
+`target_pointer_width`
+`target_thread_local`
+`target_vendor`
+`ub_checks`
+`unix`, and `windows`


For anti-regression and explicit-ness purpose I kept one place with the full list and applied the same trick as in #133710 to reduce merge conflicts.

Urgau · 2025-01-25T12:57:44Z

tests/ui/check-cfg/raw-keywords.edition2021.stderr

@@ -14,7 +14,7 @@ warning: unexpected `cfg` condition name: `r#false`
 LL | #[cfg(r#false)]
   |       ^^^^^^^
   |
-   = help: expected names are: `r#async`, `clippy`, `debug_assertions`, `doc`, `doctest`, `edition2015`, `edition2021`, `fmt_debug`, `miri`, `overflow_checks`, `panic`, `proc_macro`, `relocation_model`, `rustfmt`, `sanitize`, `sanitizer_cfi_generalize_pointers`, `sanitizer_cfi_normalize_integers`, `target_abi`, `target_arch`, `target_endian`, `target_env`, `target_family`, `target_feature`, `target_has_atomic`, `target_has_atomic_equal_alignment`, `target_has_atomic_load_store`, `target_os`, `target_pointer_width`, `target_thread_local`, `target_vendor`, `r#true`, `ub_checks`, `unix`, and `windows`
+   = help: expected names are: `r#async`, `edition2015`, `edition2021`, and `r#true` and 30 more


This double and ... and 30 more is unfortunate (and pre-existing). DiagSymbolList doesn't gives us enough control to says that the list has been truncated by X amount, and even if it did, icu_list doesn't seems to have a way to say that the list has been truncated either.

We could hack it around in check-cfg by adding a fake "X more" in the list. Idk if it's worth it.

I think this is totally fine

jieyouxu

Thanks, I think this should indeed help with merge conflicts quite a lot.

jieyouxu · 2025-01-25T13:41:48Z

tests/ui/check-cfg/raw-keywords.edition2021.stderr

@@ -14,7 +14,7 @@ warning: unexpected `cfg` condition name: `r#false`
 LL | #[cfg(r#false)]
   |       ^^^^^^^
   |
-   = help: expected names are: `r#async`, `clippy`, `debug_assertions`, `doc`, `doctest`, `edition2015`, `edition2021`, `fmt_debug`, `miri`, `overflow_checks`, `panic`, `proc_macro`, `relocation_model`, `rustfmt`, `sanitize`, `sanitizer_cfi_generalize_pointers`, `sanitizer_cfi_normalize_integers`, `target_abi`, `target_arch`, `target_endian`, `target_env`, `target_family`, `target_feature`, `target_has_atomic`, `target_has_atomic_equal_alignment`, `target_has_atomic_load_store`, `target_os`, `target_pointer_width`, `target_thread_local`, `target_vendor`, `r#true`, `ub_checks`, `unix`, and `windows`
+   = help: expected names are: `r#async`, `edition2015`, `edition2021`, and `r#true` and 30 more


I think this is totally fine

jieyouxu · 2025-01-25T13:42:24Z

@bors r+ rollup

bors · 2025-01-25T13:42:27Z

📌 Commit 74223e4 has been approved by jieyouxu

It is now in the queue for this repository.

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#134300 (remove long-deprecated no-op attributes no_start and crate_id) - rust-lang#134373 (Improve and expand documentation of pipes) - rust-lang#135934 (Include missing item in the 1.81 release notes) - rust-lang#136005 (ports last few library files to new intrinsic style) - rust-lang#136016 (Improve check-cfg expected names diagnostic) - rust-lang#136039 (docs: fix typo in std::pin overview) - rust-lang#136056 (Fix typo in const stability error message) r? `@ghost` `@rustbot` modify labels: rollup

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`

Improve check-cfg allow-same-level test

c08cc61

rust-lang#135995

rustbot assigned jieyouxu Jan 24, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 24, 2025

Urgau mentioned this pull request Jan 24, 2025

check-cfg tests are prone to merge conflicts #135995

Closed

Urgau 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 Jan 24, 2025

Urgau added 2 commits January 25, 2025 12:27

Filter well known names from check-cfg diagnostics

c5ea75a

Refer to the well known names from Cargo check-cfg specifics doc

74223e4

Urgau changed the title ~~Improve check-cfg allow-same-level test~~ Improve check-cfg expected names diagnostic Jan 25, 2025

Urgau commented Jan 25, 2025

View reviewed changes

Urgau 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 Jan 25, 2025

jieyouxu approved these changes Jan 25, 2025

View reviewed changes

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 Jan 25, 2025

matthiaskrgr mentioned this pull request Jan 25, 2025

Rollup of 7 pull requests #136070

Merged

bors merged commit dbe911a into rust-lang:master Jan 26, 2025
6 checks passed

rustbot added this to the 1.86.0 milestone Jan 26, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve check-cfg expected names diagnostic #136016

Improve check-cfg expected names diagnostic #136016

Urgau commented Jan 24, 2025 •

edited

Loading

jyn514 commented Jan 24, 2025

Urgau commented Jan 24, 2025

jyn514 commented Jan 24, 2025 •

edited

Loading

Urgau commented Jan 24, 2025

Urgau Jan 25, 2025

Urgau Jan 25, 2025

Urgau Jan 25, 2025

jieyouxu Jan 25, 2025

jieyouxu left a comment

jieyouxu Jan 25, 2025

jieyouxu commented Jan 25, 2025

bors commented Jan 25, 2025

Improve check-cfg expected names diagnostic #136016

Improve check-cfg expected names diagnostic #136016

Conversation

Urgau commented Jan 24, 2025 • edited Loading

jyn514 commented Jan 24, 2025

Urgau commented Jan 24, 2025

jyn514 commented Jan 24, 2025 • edited Loading

Urgau commented Jan 24, 2025

Urgau Jan 25, 2025

Choose a reason for hiding this comment

Urgau Jan 25, 2025

Choose a reason for hiding this comment

Urgau Jan 25, 2025

Choose a reason for hiding this comment

jieyouxu Jan 25, 2025

Choose a reason for hiding this comment

jieyouxu left a comment

Choose a reason for hiding this comment

jieyouxu Jan 25, 2025

Choose a reason for hiding this comment

jieyouxu commented Jan 25, 2025

bors commented Jan 25, 2025

Urgau commented Jan 24, 2025 •

edited

Loading

jyn514 commented Jan 24, 2025 •

edited

Loading