Only invoke `decorate` if the diag can eventually be emitted #122578

jieyouxu · 2024-03-16T03:30:33Z

Lints can call trimmed_def_paths, such as through manual implementations of LintDiagnostic and calling def_path_str.

rust/compiler/rustc_lint/src/lints.rs

Lines 1834 to 1839 in 05a2be3

    
           // Needed because of def_path_str 
        
           impl<'a> LintDiagnostic<'a, ()> for UnusedDef<'_, '_> { 
        
               fn decorate_lint<'b>(self, diag: &'b mut Diag<'a, ()>) { 
        
                   diag.arg("pre", self.pre); 
        
                   diag.arg("post", self.post); 
        
                   diag.arg("def", self.cx.tcx.def_path_str(self.def_id));

The emission of a lint eventually relies on TyCtxt::node_lint, which has a decorate closure which is responsible for decorating the diagnostic with "lint stuff". node_lint in turn relies on lint_level. Within lint_level, decorate is eventually called just before Diag::emit is called to decorate the diagnostic. However, if -A warnings or --cap-lint=allow are set, or if the unused_must_use lint is explicitly allowed, then decorate would be called, which would call def_path_str, but the diagnostic would never be emitted and hence would trigger the must_produce_diag ICE.

To avoid calling decorate when we don't eventually emit the diagnostic, we check that:

if --force-warn is specified, then call decorate; otherwise
if we can emit warnings (or higher), then call decorate.

Fixes #121774.

rustbot · 2024-03-16T03:30:41Z

r? @fee1-dead

rustbot has assigned @fee1-dead.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

fee1-dead · 2024-03-16T10:19:50Z

compiler/rustc_middle/src/lint.rs

+        // Finally, run `decorate`. This is guarded by a `can_emit_warnings()` check so that any
+        // `def_path_str` called within `decorate` won't trigger a `must_produce_diag` ICE if the
+        // `err` isn't eventually emitted (e.g. due to `-A warnings`). If an `err` is force-warn,
+        // it's going to be emitted anyway.
+        if matches!(err_level, rustc_errors::Level::ForceWarning(_))
+            || sess.dcx().can_emit_warnings()
+        {
+            decorate(&mut err);
+        }
+


This isn't entirely correct. The test attached below ICEs after your PR because decorate is incorrectly skipped. can_emit_warnings is only relevant when the error level is Warning.

//@ compile-flags: -Dunused_must_use -Awarnings --crate-type=lib //@ check-pass #[must_use] fn f() {} pub fn g() { f(); }

Also please add a test with --force-warn as well.

This isn't entirely correct. The test attached below ICEs after your PR because decorate is incorrectly skipped. can_emit_warnings is only relevant when the error level is Warning.

Thanks for pointing that out. I forgor to also invoke decorate for the actual Level::Error case (which lint levels Deny and Forbid correspond to).

Added a test case with --force-warn.

fee1-dead · 2024-03-16T10:20:59Z

tests/ui/lint/decorate-def-path-str-ice.rs

+// Checks that compiling this file with
+// `-Dunused_must_use -Awarnings --cap-lints=warn --crate-type=lib` does not ICE when emitting
+// diagnostics.


Seems a bit redundant given compile-flags below. It would probably be better to write some notes explaining why there is an ICE.

I added some notes explaining why the ICE is triggered in the tests/ui/lint/decorate-ice/decorate-def-path-str-ice.rs test instead of repeating what the compile-flags say (lol).

jieyouxu · 2024-03-16T14:58:24Z

I need to think about how to guard the decorate invocation a bit more.

jieyouxu · 2024-03-16T15:15:49Z

compiler/rustc_middle/src/lint.rs

+            if matches!(err_level, ELevel::ForceWarning(_) | ELevel::Error)
+                || sess.dcx().can_emit_warnings()


This check seems to pass the ui test suite including the -Dunused_must_use case, but I'm still not very confident in this check.

I think it would be best if we instead used an if statement to consider when to skip calling decorate.

Something like

let skip = err_level == rustc_errors::Warning && !sess.dcx().can_emit_warnings(); if !skip { decorate(&mut err); }

Which would have better maintainability.

jieyouxu · 2024-03-16T17:13:52Z

@rustbot ready

fee1-dead · 2024-03-17T04:40:56Z

compiler/rustc_middle/src/lint.rs

+            if matches!(err_level, ELevel::ForceWarning(_) | ELevel::Error)
+                || sess.dcx().can_emit_warnings()


I think it would be best if we instead used an if statement to consider when to skip calling decorate.

Something like

let skip = err_level == rustc_errors::Warning && !sess.dcx().can_emit_warnings(); if !skip { decorate(&mut err); }

Which would have better maintainability.

jieyouxu · 2024-03-17T15:09:27Z

Changed the guard for decorate to be based on when not to skip instead.

fee1-dead

Thanks!

@bors r+

fee1-dead · 2024-03-17T16:49:07Z

@bors r+

bors · 2024-03-17T16:49:10Z

📌 Commit bdab02c has been approved by fee1-dead

It is now in the queue for this repository.

…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#120640 (Mark UEFI std support as WIP) - rust-lang#121862 (Add release notes for 1.77.0) - rust-lang#122572 (add test for rust-lang#122301 to cover behavior that's on stable) - rust-lang#122578 (Only invoke `decorate` if the diag can eventually be emitted) - rust-lang#122615 (Mention Zalathar for coverage changes) - rust-lang#122636 (some minor code simplifications) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#122578 - jieyouxu:guard-decorate, r=fee1-dead Only invoke `decorate` if the diag can eventually be emitted Lints can call [`trimmed_def_paths`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/print/fn.trimmed_def_paths.html#), such as through manual implementations of `LintDiagnostic` and calling `def_path_str`. https://github.com/rust-lang/rust/blob/05a2be3def211255dc7640b006ac10f0f02baf5c/compiler/rustc_lint/src/lints.rs#L1834-L1839 The emission of a lint eventually relies on [`TyCtxt::node_lint`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/context/struct.TyCtxt.html#method.node_lint), which has a `decorate` closure which is responsible for decorating the diagnostic with "lint stuff". `node_lint` in turn relies on [`lint_level`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/lint/fn.lint_level.html). Within `lint_level`, `decorate` is eventually called just before `Diag::emit` is called to decorate the diagnostic. However, if `-A warnings` or `--cap-lint=allow` are set, or if the unused_must_use lint is explicitly allowed, then `decorate` would be called, which would call `def_path_str`, but the diagnostic would never be emitted and hence would trigger the `must_produce_diag` ICE. To avoid calling `decorate` when we don't eventually emit the diagnostic, we check that: - if `--force-warn` is specified, then call `decorate`; otherwise - if we can emit warnings (or higher), then call `decorate`. Fixes rust-lang#121774.

rustbot assigned fee1-dead Mar 16, 2024

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 Mar 16, 2024

fee1-dead requested changes Mar 16, 2024

View reviewed changes

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 Mar 16, 2024

This comment has been minimized.

Sign in to view

jieyouxu force-pushed the guard-decorate branch from 714fbd3 to 0bb5722 Compare March 16, 2024 15:14

jieyouxu commented Mar 16, 2024

View reviewed changes

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 Mar 16, 2024

fee1-dead requested changes Mar 17, 2024

View reviewed changes

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 Mar 17, 2024

jieyouxu added 3 commits March 17, 2024 14:41

Only invoke decorate if the diag can eventually be emitted

772d859

Invoke decorate when error level is beyond warning, including error

60de755

Guard decorate on when not to skip instead

bdab02c

jieyouxu force-pushed the guard-decorate branch from 0bb5722 to bdab02c Compare March 17, 2024 15:08

fee1-dead approved these changes Mar 17, 2024

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 17, 2024

matthiaskrgr mentioned this pull request Mar 17, 2024

Rollup of 6 pull requests #122653

Merged

bors merged commit 8e748c0 into rust-lang:master Mar 17, 2024

rustbot added this to the 1.79.0 milestone Mar 17, 2024

jieyouxu deleted the guard-decorate branch May 28, 2024 03:23

	// Needed because of def_path_str
	impl<'a> LintDiagnostic<'a, ()> for UnusedDef<'_, '_> {
	fn decorate_lint<'b>(self, diag: &'b mut Diag<'a, ()>) {
	diag.arg("pre", self.pre);
	diag.arg("post", self.post);
	diag.arg("def", self.cx.tcx.def_path_str(self.def_id));

		if matches!(err_level, ELevel::ForceWarning(_) \| ELevel::Error)
		\|\| sess.dcx().can_emit_warnings()

Uh oh!

Only invoke decorate if the diag can eventually be emitted #122578

Only invoke decorate if the diag can eventually be emitted #122578

Uh oh!

Conversation

jieyouxu commented Mar 16, 2024

Uh oh!

rustbot commented Mar 16, 2024

Uh oh!

fee1-dead Mar 16, 2024

Choose a reason for hiding this comment

Uh oh!

fee1-dead Mar 16, 2024

Choose a reason for hiding this comment

Uh oh!

jieyouxu Mar 16, 2024

Choose a reason for hiding this comment

Uh oh!

jieyouxu Mar 16, 2024

Choose a reason for hiding this comment

Uh oh!

fee1-dead Mar 16, 2024

Choose a reason for hiding this comment

Uh oh!

jieyouxu Mar 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

jieyouxu commented Mar 16, 2024

Uh oh!

jieyouxu Mar 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fee1-dead Mar 17, 2024

Choose a reason for hiding this comment

Uh oh!

jieyouxu commented Mar 16, 2024

Uh oh!

fee1-dead Mar 17, 2024

Choose a reason for hiding this comment

Uh oh!

jieyouxu commented Mar 17, 2024

Uh oh!

fee1-dead left a comment

Choose a reason for hiding this comment

Uh oh!

fee1-dead commented Mar 17, 2024

Uh oh!

bors commented Mar 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Only invoke `decorate` if the diag can eventually be emitted #122578

Only invoke `decorate` if the diag can eventually be emitted #122578

jieyouxu Mar 16, 2024 •

edited

Loading

jieyouxu Mar 16, 2024 •

edited

Loading