compiletest: Make diagnostic kind mandatory on line annotations

[petrochenkov]

Most of annotations have them, this PR adds all the missing ones.

Also, parsing of diagnostic kinds is made stricter to not accept things like Error or error:.

One major issue in doing this is that NOTE and HELP annotations are viral.
If you add one such annotation to a file, then you must annotate all the other NOTEs and HELPs annotations in that file.
Empty kind was often used as an opt out from that rule - we use an empty kind instead of NOTE and voila, other NOTEs no longer need to be annotated.

For now, I've introduced NOTE_NONVIRAL and HELP_NONVIRAL annotations to reproduce the old behavior while still specifying the kind, but I wonder, what would be the best solution here longer term.

• Maybe be just add the remaining NOTEs and HELPs and deal with it? (Not in this PR, there are many of them.)
• Or maybe make them non-viral (this is how it's been a few years ago).
• Or maybe make them less viral - at the scope of "the same top level diagnostic" (if that makes sense), and not the whole file.
• Or keep the NONVIRAL flavors.

For statistics, there are currently 3696 NOTEs and 648 NOTE_NONVIRALs in the UI test suite.
Also 1824 HELPs and 12 HELP_NONVIRALs (thus for HELP specifically the "add remaining annotations" solution looks quite reasonable).

r? @jieyouxu

[rustbot]

Some changes occurred in src/tools/compiletest

cc @jieyouxu

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

[RalfJung]

Currently, compiletest has the useful feature where I can match on some NOTE-level annotations without matching on all of them by writing //~ text goes here rather than //~ NOTE: text goes here. We're using this feature in the const tests, and I don't think we should remove it.

So if the diagnostic kind/level is required, how do I write a test like that?

---

FWIW, tree-wide changes like this and #139448 require an MCP in my eyes.

[petrochenkov]

@RalfJung That's the main unresolved issue, see the PR description and the discussion of NOTE_NONVIRAL in it.

[petrochenkov]

Or maybe make them less viral - at the scope of "the same top level diagnostic" (if that makes sense), and not the whole file.

So, I implemented this alternative in the second commit f22f55d and it kind of works.
If you add a NOTE, you are only required to add remaining NOTEs to the same error, not to the whole file.

Without it UI tests suite requires adding 1624 missing NOTE annotations, and with it - 978.

[jieyouxu]

Yeah, I think this deserves an MCP, mostly to figure out the ergonomics/design of this from the POV of compiler contributors who write a lot of tests. At the moment, likewise, I'm kinda dilly-dallying between the options. I.e. there's 2 potentially competing use cases that we need to account for:

1. I want to match on some NOTE/HELP annotations without being required to match on all NOTE/HELP annotations.
   • Adjacently related is Add a dont-check-error-annotations to compiletest #132647 where @BoxyUwU had a case where they kinda wanted //@ dont-check-error-annotations or even sth like //~ ERROR_NONVIRAL. But that's for ERROR-level diagnostics which is typically less noisy than NOTE/HELP.
2. But in some cases I may want NOTE/HELP annotations to be exhaustive.

---

One possible alternative solution (yet another option) I can think of is sth like

//@ non-viral-note
//@ non-viral-help

Where,

• By default, if these directives are not used, then //~ NOTE and //~ HELP annotations are viral (like current behavior).
• However, the test writer may opt-out of the virality for {NOTE,HELP} annotation exhaustiveness by specifying the directives.

At the cost of even more directives of course.

[RalfJung]

ui_test has a separate directive, //@require-annotations-for-level: WARN, that can override the usually "automatically inferred" annotation level for which all annotations have to be present.

Or maybe make them non-viral (this is how it's been a few years ago).

That seems oddly inconsistent, and may accidentally weaken some existing tests.

Or maybe make them less viral - at the scope of "the same top level diagnostic" (if that makes sense), and not the whole file.

It's hard for me to predict the impact on that, but I think the const tests probably have notes in the same top-level diagnostic that we don't want to have to list in the test file. Also, this too could accidentally weaken existing tests.

[petrochenkov]

After considering the alternatives, I think we should adopt something equivalent to //@require-annotations-for-level: LEVEL.

We cannot adopt it exactly, because in compiletest diagnostic kinds are not ordered, and do not use the "if a level is specified explicitly, all diagnostics of that level or higher need an annotation" scheme for requiring annotations.
But we can instead revert it and make it work on a single level, rather than "level and higher".

This would be useful not just for NOTEs and HELPs, but for ERRORs as well in case of target-dependent diagnostics (see examples in 4916d44).

---

Why not the alternatives:

• "make NOTE/HELP less viral - at the scope of "the same top level diagnostic" - not very predictable, and I don't like that the implementation f22f55d requires introducing a tree structure for diagnostics in compiletes.
• NOTE_NONVIRAL - if NOTE is viral for the whole file, then we cannot have both NOTEs and NOTE_NONVIRALs in the same file, so we can have just a single directive for a file changing the meaning of NOTE.

[RalfJung]

We cannot adopt it exactly, because in compiletest diagnostic kinds are not ordered, and do not use the "if a level is specified explicitly, all diagnostics of that level or higher need an annotation" scheme for requiring annotations.

They are not? How does it work instead? ui_test is modeled after how we thought compiletest works.^^

[petrochenkov]

We cannot adopt it exactly, because in compiletest diagnostic kinds are not ordered, and do not use the "if a level is specified explicitly, all diagnostics of that level or higher need an annotation" scheme for requiring annotations.

They are not? How does it work instead? ui_test is modeled after how we thought compiletest works.^^

• ERROR and WARN are always required
• HELP is required if any HELP in the file is specified
• NOTE is required if any NOTE in the file is specified
• SUGGESTION is never required

[RalfJung]

Interesting. @oli-obk maybe ui_test should match that to avoid unnecessary confusion. (Probably best to await the outcome of this discussion and then considering syncing with whatever compiletest ends up doing.)

[oli-obk]

Yea well match whatever comes out of that

[petrochenkov]

Some non-controversial parts are extracted into #139485.

[petrochenkov]

After considering the alternatives, I think we should adopt something equivalent to //@require-annotations-for-level: LEVEL.

We cannot adopt it exactly, because in compiletest diagnostic kinds are not ordered, and do not use the "if a level is specified explicitly, all diagnostics of that level or higher need an annotation" scheme for requiring annotations. But we can instead revert it and make it work on a single level, rather than "level and higher".

This would be useful not just for NOTEs and HELPs, but for ERRORs as well in case of target-dependent diagnostics (see examples in 4916d44).

Why not the alternatives:

• "make NOTE/HELP less viral - at the scope of "the same top level diagnostic" - not very predictable, and I don't like that the implementation f22f55d requires introducing a tree structure for diagnostics in compiletes.
• NOTE_NONVIRAL - if NOTE is viral for the whole file, then we cannot have both NOTEs and NOTE_NONVIRALs in the same file, so we can have just a single directive for a file changing the meaning of NOTE.

Implemented in #139489.

[bors]

☔ The latest upstream changes (presumably #139482) made this pull request unmergeable. Please resolve the merge conflicts.

[rust-log-analyzer]

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

Click to see the possible cause of the failure (guessed by this bot)

#19 exporting to docker image format
#19 sending tarball 20.2s done
#19 DONE 27.9s
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-19]
[CI_JOB_NAME=x86_64-gnu-llvm-19]
debug: `DISABLE_CI_RUSTC_IF_INCOMPATIBLE` configured.
---
sccache: Listening on address 127.0.0.1:4226
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-19', '--enable-llvm-link-shared', '--set', 'rust.randomize-layout=true', '--set', 'rust.thin-lto-import-instr-limit=10', '--set', 'build.print-step-timings', '--enable-verbose-tests', '--set', 'build.metrics', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--set', 'rust.lld=false', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: build.build          := x86_64-unknown-linux-gnu
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-19/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.randomize-layout := True
configure: rust.thin-lto-import-instr-limit := 10
---
  Number of decisions:   4447
  longest path:          1159 (code:    152)
  longest backtrack:       66 (code:    428)
Shared 86733 out of 152951 states by creating 14756 new states, saving 71977
/checkout/obj/build/x86_64-unknown-linux-gnu/gcc/src/gcc/expmed.cc: In function ‘rtx_def* extract_bit_field_1(rtx, poly_uint64, poly_uint64, int, rtx, machine_mode, machine_mode, bool, bool, rtx_def**)’:
/checkout/obj/build/x86_64-unknown-linux-gnu/gcc/src/gcc/expmed.cc:1864:45: warning: ‘*(unsigned int*)((char*)&imode + offsetof(scalar_int_mode, scalar_int_mode::m_mode))’ may be used uninitialized [-Wmaybe-uninitialized]
 1864 |       rtx sub = extract_bit_field_as_subreg (mode1, op0, imode,
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
 1865 |                                              bitsize, bitnum);
      |                                              ~~~~~~~~~~~~~~~~
/checkout/obj/build/x86_64-unknown-linux-gnu/gcc/src/gcc/expmed.cc:1824:19: note: ‘*(unsigned int*)((char*)&imode + offsetof(scalar_int_mode, scalar_int_mode::m_mode))’ was declared here
 1824 |   scalar_int_mode imode;
      |                   ^~~~~
/checkout/obj/build/x86_64-unknown-linux-gnu/gcc/src/gcc/gimple-range-gori.cc: In member function ‘void range_def_chain::dump(FILE*, basic_block, const char*)’:
/checkout/obj/build/x86_64-unknown-linux-gnu/gcc/src/gcc/gimple-range-gori.cc:319:19: warning: format not a string literal and no format arguments [-Wformat-security]
  319 |           fprintf (f, prefix);
      |           ~~~~~~~~^~~~~~~~~~~
---
                 from /checkout/obj/build/x86_64-unknown-linux-gnu/gcc/src/gcc/analyzer/region-model.h:33,
                 from /checkout/obj/build/x86_64-unknown-linux-gnu/gcc/src/gcc/analyzer/access-diagram.cc:39:
In constructor ‘ana::byte_range::byte_range(ana::byte_offset_t, ana::byte_size_t)’,
    inlined from ‘virtual text_art::table ana::string_literal_spatial_item::make_table(const ana::bit_to_table_map&, text_art::style_manager&) const’ at /checkout/obj/build/x86_64-unknown-linux-gnu/gcc/src/gcc/analyzer/access-diagram.cc:1812:18:
/checkout/obj/build/x86_64-unknown-linux-gnu/gcc/src/gcc/analyzer/store.h:312:5: warning: ‘size_in_bytes.generic_wide_int<fixed_wide_int_storage<128> >::fixed_wide_int_storage<128>.fixed_wide_int_storage<128>::val[1]’ may be used uninitialized [-Wmaybe-uninitialized]
  312 |     m_size_in_bytes (size_in_bytes)
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/checkout/obj/build/x86_64-unknown-linux-gnu/gcc/src/gcc/analyzer/access-diagram.cc: In member function ‘virtual text_art::table ana::string_literal_spatial_item::make_table(const ana::bit_to_table_map&, text_art::style_manager&) const’:
/checkout/obj/build/x86_64-unknown-linux-gnu/gcc/src/gcc/analyzer/access-diagram.cc:1808:28: note: ‘size_in_bytes.generic_wide_int<fixed_wide_int_storage<128> >::fixed_wide_int_storage<128>.fixed_wide_int_storage<128>::val[1]’ was declared here
 1808 |                byte_size_t size_in_bytes
      |                            ^~~~~~~~~~~~~
/checkout/obj/build/x86_64-unknown-linux-gnu/gcc/src/gcc/diagnostic.cc: In function ‘void fancy_abort(const char*, int, const char*)’:
/checkout/obj/build/x86_64-unknown-linux-gnu/gcc/src/gcc/diagnostic.cc:1677:15: warning: format not a string literal and no format arguments [-Wformat-security]
 1677 |       fnotice (stderr, diagnostic_kind_text[DK_ICE]);
---
/checkout/obj/build/x86_64-unknown-linux-gnu/gcc/src/gcc/gcc.cc:7930:9: warning: ignoring return value of ‘ssize_t write(int, const void*, size_t)’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
 7930 |   write (fd, "\n\n", 2);
      |   ~~~~~~^~~~~~~~~~~~~~~
/checkout/obj/build/x86_64-unknown-linux-gnu/gcc/src/gcc/gcc.cc: In member function ‘void driver::final_actions() const’:
/checkout/obj/build/x86_64-unknown-linux-gnu/gcc/src/gcc/gcc.cc:9307:13: warning: ignoring return value of ‘int truncate(const char*, __off_t)’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
 9307 |     truncate(totruncate_file, 0);
      |     ~~~~~~~~^~~~~~~~~~~~~~~~~~~~
In file included from /checkout/obj/build/x86_64-unknown-linux-gnu/gcc/src/gcc/jit/jit-playback.h:31,
                 from /checkout/obj/build/x86_64-unknown-linux-gnu/gcc/src/gcc/jit/jit-builtins.cc:24:
/checkout/obj/build/x86_64-unknown-linux-gnu/gcc/src/gcc/jit/jit-recording.h: In member function ‘virtual bool gcc::jit::recording::type::is_same_type_as(gcc::jit::recording::type*)’:
/checkout/obj/build/x86_64-unknown-linux-gnu/gcc/src/gcc/jit/jit-recording.h:640:20: warning: suggest parentheses around ‘&&’ within ‘||’ [-Wparentheses]
---
/checkout/obj/build/x86_64-unknown-linux-gnu/gcc/src/gcc/lto/lto-common.cc: In function ‘void lto_resolution_read(splay_tree, FILE*, lto_file*)’:
/checkout/obj/build/x86_64-unknown-linux-gnu/gcc/src/gcc/lto/lto-common.cc:2091:10: warning: ignoring return value of ‘int fscanf(FILE*, const char*, ...)’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
 2091 |   fscanf (resolution, " ");   /* Read white space.  */
      |   ~~~~~~~^~~~~~~~~~~~~~~~~
/checkout/obj/build/x86_64-unknown-linux-gnu/gcc/src/gcc/lto/lto-common.cc:2093:9: warning: ignoring return value of ‘size_t fread(void*, size_t, size_t, FILE*)’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
 2093 |   fread (obj_name, sizeof (char), name_len, resolution);
      |   ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/checkout/obj/build/x86_64-unknown-linux-gnu/gcc/src/gcc/lto/lto-common.cc:2113:10: warning: ignoring return value of ‘int fscanf(FILE*, const char*, ...)’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
 2113 |   fscanf (resolution, "%u", &num_symbols);
      |   ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /checkout/obj/build/x86_64-unknown-linux-gnu/gcc/src/gcc/jit/jit-recording.cc:32:
---
Applying io_quotes_use            to linux/blkzoned.h
Applying io_quotes_use            to linux/ipmi.h
Applying io_quotes_use            to linux/psp-dbc.h
Applying io_quotes_use            to linux/bt-bmc.h
Applying io_quotes_use            to linux/tps6594_pfsm.h
Applying io_quotes_use            to linux/cxl_mem.h
Applying io_quotes_use            to linux/wmi.h
Applying io_quotes_use            to linux/auto_fs.h
Applying io_quotes_use            to linux/mmtimer.h
Applying io_quotes_use            to linux/f2fs.h
Applying io_quotes_use            to linux/vhost.h
---
Applying io_quotes_use            to sound/asound.h
Applying io_quotes_use            to sound/compress_offload.h
Applying hpux8_bogus_inlines      to math.h
Applying pthread_incomplete_struct_argument to pthread.h
Fixed:  pthread.h
Applying io_quotes_use            to misc/mrvl_cn10k_dpi.h
Applying io_quotes_use            to misc/ocxl.h
Applying io_quotes_use            to misc/cxl.h
Applying io_quotes_use            to misc/xilinx_sdfec.h
Applying io_quotes_def            to unicode/platform.h
Applying sun_malloc               to malloc.h
Applying io_quotes_use            to scsi/cxlflash_ioctl.h
---
Applying machine_name             to x86_64-linux-gnu/bits/unistd_ext.h
Applying io_quotes_use            to x86_64-linux-gnu/asm/mtrr.h
Applying io_quotes_use            to x86_64-linux-gnu/asm/amd_hsmp.h
Applying machine_name             to openssl/e_os2.h
Applying io_quotes_use            to drm/xe_drm.h
Applying io_quotes_use            to drm/radeon_drm.h
Applying io_quotes_use            to drm/panfrost_drm.h
Applying io_quotes_use            to drm/etnaviv_drm.h
Applying io_quotes_use            to drm/lima_drm.h
Applying io_quotes_use            to drm/qaic_accel.h
Applying io_quotes_use            to drm/vc4_drm.h
Applying io_quotes_use            to drm/i915_drm.h
Applying io_quotes_use            to drm/omap_drm.h
Applying io_quotes_use            to drm/pvr_drm.h
Applying io_quotes_use            to drm/amdgpu_drm.h
Applying io_quotes_use            to drm/vgem_drm.h
Applying io_quotes_use            to drm/msm_drm.h
Applying io_quotes_use            to drm/v3d_drm.h
Applying io_quotes_use            to drm/exynos_drm.h
Applying io_quotes_use            to drm/nouveau_drm.h
Applying io_quotes_use            to drm/drm.h
Applying io_quotes_use            to drm/habanalabs_accel.h
Applying io_quotes_use            to drm/tegra_drm.h
Applying io_quotes_use            to rdma/rdma_user_ioctl.h
cc1: note: self-tests are not enabled in this build
/checkout/obj/build/x86_64-unknown-linux-gnu/gcc/src/c++tools/server.cc: In function ‘void server(bool, int, module_resolver*)’:
/checkout/obj/build/x86_64-unknown-linux-gnu/gcc/src/c++tools/server.cc:620:10: warning: ignoring return value of ‘int pipe(int*)’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
---
failures:

---- [ui] tests/ui/feature-gates/feature-gate-cfg-target-thread-local.rs stdout ----

thread '[ui] tests/ui/feature-gates/feature-gate-cfg-target-thread-local.rs' panicked at src/tools/compiletest/src/errors.rs:40:18:
unexpected diagnostic kind ``, expected `ERROR`, `WARN`, `NOTE`, `HELP` or `SUGGESTION`
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    [ui] tests/ui/feature-gates/feature-gate-cfg-target-thread-local.rs

[petrochenkov]

Closing in favor of #139720.