Automatically taint InferCtxt when errors are emitted #126996

oli-obk · 2024-06-26T16:14:49Z

Basically InferCtxt::dcx now returns a DiagCtxt that refers back to the Cell<Option<ErrorGuaranteed>> of the InferCtxt and thus when invoking Diag::emit, and the diagnostic is an error, we taint the InferCtxt directly.

That change on its own has no effect at all, because InferCtxt already tracks whether errors have been emitted by recording the global error count when it gets opened, and checking at the end whether the count changed. So I removed that error count check, which had a bit of fallout that I immediately fixed by invoking InferCtxt::dcx instead of TyCtxt::dcx in a bunch of places.

The remaining new errors are because an error was reported in another query, and never bubbled up. I think they are minor enough for this to be ok, and sometimes it actually improves diagnostics, by not silencing useful diagnostics anymore.

fixes #126485 (cc @olafes)

There are more improvements we can do (like tainting in hir ty lowering), but I would rather do that in follow up PRs, because it requires some refactorings.

…tead of TyCtxt

…f TyCtxt

…ead of TyCtxt

…f TyCtxt

rustbot · 2024-06-26T16:14:58Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in need_type_info.rs

cc @lcnr

oli-obk · 2024-06-26T16:14:59Z

@bors try @rust-timer queue

Automatically taint InferCtxt when errors are emitted r? `@nnethercote` Basically `InferCtxt::dcx` now returns a `DiagCtxt` that refers back to the `Cell<Option<ErrorGuaranteed>>` of the `InferCtxt` and thus when invoking `Diag::emit`, and the diagnostic is an error, we taint the `InferCtxt` directly. That change on its own has no effect at all, because `InferCtxt` already tracks whether errors have been emitted by recording the global error count when it gets opened, and checking at the end whether the count changed. So I removed that error count check, which had a bit of fallout that I immediately fixed by invoking `InferCtxt::dcx` instead of `TyCtxt::dcx` in a bunch of places. The remaining new errors are because an error was reported in another query, and never bubbled up. I think they are minor enough for this to be ok, and sometimes it actually improves diagnostics, by not silencing useful diagnostics anymore. fixes rust-lang#126485 (cc `@olafes)` There are more improvements we can do (like tainting in hir ty lowering), but I would rather do that in follow up PRs, because it requires some refactorings.

bors · 2024-06-26T16:16:10Z

⌛ Trying commit 86c8eae with merge c8dd503...

oli-obk · 2024-06-26T16:16:33Z

compiler/rustc_infer/src/infer/mod.rs

-        if let Some(guar) = self.tainted_by_errors.get() {
-            Some(guar)
-        } else if self.dcx().err_count_excluding_lint_errs() > self.err_count_on_creation {
-            // Errors reported since this infcx was made. Lint errors are
-            // excluded to avoid some being swallowed in the presence of
-            // non-lint errors. (It's arguable whether or not this exclusion is
-            // important.)
-            let guar = self.dcx().has_errors().unwrap();
-            self.set_tainted_by_errors(guar);
-            Some(guar)
-        } else {
-            None
-        }
+        self.tainted_by_errors.get()


This is the core change that causes diagnostics changes

oli-obk · 2024-06-26T16:17:55Z

compiler/rustc_infer/src/infer/mod.rs

-    pub fn dcx(&self) -> DiagCtxtHandle<'tcx> {
-        self.tcx.dcx()
+    pub fn dcx(&self) -> DiagCtxtHandle<'_> {
+        self.tcx.dcx().taintable_handle(&self.tainted_by_errors)


This is what enables the auto-tainting (as long as one goes through this dcx). I'm looking into writing a lint that detects when you invoke dcx on something that does not taint, while a tainting thing is available

bors · 2024-06-26T18:00:00Z

☀️ Try build successful - checks-actions
Build commit: c8dd503 (c8dd503c26d86d0acf80b245ec20dc3d26b437a5)

rust-timer · 2024-06-26T19:33:13Z

Finished benchmarking commit (c8dd503): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.5%	[0.4%, 0.9%]	7
Improvements ✅ (primary)	-0.2%	[-0.2%, -0.2%]	1
Improvements ✅ (secondary)	-0.3%	[-0.4%, -0.2%]	2
All ❌✅ (primary)	-0.2%	[-0.2%, -0.2%]	1

Max RSS (memory usage)

Results (secondary 4.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.1%	[2.5%, 5.8%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results (secondary -3.6%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.6%	[-5.2%, -2.6%]	3
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 694.171s -> 694.347s (0.03%)
Artifact size: 324.67 MiB -> 326.84 MiB (0.67%)

nnethercote · 2024-06-27T03:38:44Z

compiler/rustc_mir_dataflow/src/impls/initialized.rs

    mdpe: &'a MoveDataParamEnv<'tcx>,
    skip_unreachable_unwind: bool,
 }

-impl<'a, 'tcx> MaybeInitializedPlaces<'a, 'tcx> {
-    pub fn new(tcx: TyCtxt<'tcx>, body: &'a Body<'tcx>, mdpe: &'a MoveDataParamEnv<'tcx>) -> Self {
+impl<'a, 'mir, 'tcx> MaybeInitializedPlaces<'a, 'mir, 'tcx> {


I will make a plea for better lifetime names than 'a. In a type with multiple lifetimes, I don't like when some of the names are useful ('mir) and some are not ('a). Elaborator<'a, 'b, 'mir, 'tcx> is a good (well, bad) example.

I know some of the 'a names are pre-existing but we can do better for the new ones. Would 'mdpe be a good name here instead of 'a? Or 'env? Anything even slightly descriptive would be better than 'a. And I think that would propagate to a lot of the other changes in this commit.

It is very common to use 'a lifetime. We should either change this across rustc (I mildly disagree that we should) or not do it.

But I def don't want to do it in this PR

compiler/rustc_borrowck/src/borrowck_errors.rs

nnethercote · 2024-06-27T03:46:17Z

compiler/rustc_hir_typeck/src/fn_ctxt/mod.rs

@@ -144,7 +144,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
        }
    }

-    pub(crate) fn dcx(&self) -> DiagCtxtHandle<'tcx> {
+    pub(crate) fn dcx(&self) -> DiagCtxtHandle<'a> {


'root_ctxt (or 'rcx) would be a better lifetime name than 'a for FnCtxt.

Preexisting and very 'root_ctxt is very verbose. I don't really think we should do it. 'rcx is not really better than 'a. We could use 'infcx, but it's used everywhere and does not spark joy. If someone wants to do that change, I won't stop them, but I don't want to do it, and definitely not in this PR.

nnethercote · 2024-06-27T03:47:19Z

compiler/rustc_infer/src/infer/error_reporting/mod.rs

@@ -139,7 +139,7 @@ pub struct TypeErrCtxt<'a, 'tcx> {
 }

 impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {


Hmm, hard to improve on 'a for TypeErrCtxt due to it being used in multiple fields.

compiler/rustc_errors/src/lib.rs

nnethercote · 2024-06-27T03:58:25Z

compiler/rustc_errors/src/lib.rs

@@ -417,6 +418,7 @@ pub struct DiagCtxt {
 #[derive(Copy, Clone)]
 pub struct DiagCtxtHandle<'a> {
    dcx: &'a DiagCtxt,
+    tainted_with_errors: Option<&'a Cell<Option<ErrorGuaranteed>>>,


Comments, please! This needs some explanation.

nnethercote · 2024-06-27T04:00:03Z

Will this change mean set_tainted_by_errors can be removed?

nnethercote · 2024-06-27T04:02:05Z

To play devil's advocate: why is this approach needed for InferCtxt when it's not needed for other parts of the compiler? Is it because it currently is messy and uses error counting and this PR at least improve things, even if it's not as clean as just propagating ErrorGuaranteed around like other parts of the compiler do?

oli-obk · 2024-06-27T08:26:55Z

Will this change mean set_tainted_by_errors can be removed?

No, there are like 10 sites where we taint from a delayed bug, no clue what's up there, need to look at them. Also we sometimes taint when obtaining an ErrorGuaranteed from other queries.

oli-obk · 2024-06-27T08:30:19Z

To play devil's advocate: why is this approach needed for InferCtxt when it's not needed for other parts of the compiler? Is it because it currently is messy and uses error counting and this PR at least improve things, even if it's not as clean as just propagating ErrorGuaranteed around like other parts of the compiler do?

Oh I def plan on doing that for other sites, too.

The alternative is to rely on everyone either tainting when they emit an error or producing an error type. It's not always desirable to emit an error type (e.g. there is no reason to make x in let x: u32 = (); have the error type, that just silences follow up errors, when we know it's u32) and it just doesn't scale having a social requirement to always taint when you emit an error. This PR automates that process.

lcnr · 2024-06-27T09:07:44Z

happy with the general approach 👍 I personally think the current approach of using 'a for "boring" lifetimes is fine. imo using 'tcx is meaningful because that's the lifetime you should return for interned stuff. THis is different from the 'a in &'a InferCtxt<'tcx> where we never actually need stuff to live for that lifetime.

I think my general approach is something like: if we expect to be able to elide the lifetime in (nearly) all usages of the type, it can be non-descriptive.

nnethercote · 2024-07-01T05:49:00Z

ugh, whatever

@bors r+

bors · 2024-07-01T05:49:02Z

📌 Commit bd111f5 has been approved by nnethercote

It is now in the queue for this repository.

bors · 2024-07-01T06:36:02Z

⌛ Testing commit bd111f5 with merge 7b21c18...

bors · 2024-07-01T08:50:39Z

☀️ Test successful - checks-actions
Approved by: nnethercote
Pushing 7b21c18 to master...

rust-timer · 2024-07-01T10:06:22Z

Finished benchmarking commit (7b21c18): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.5%	[0.4%, 0.9%]	7
Improvements ✅ (primary)	-0.2%	[-0.2%, -0.2%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.2%	[-0.2%, -0.2%]	1

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results (secondary -2.5%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.5%	[-2.6%, -2.2%]	3
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 697.828s -> 699.484s (0.24%)
Artifact size: 324.64 MiB -> 326.66 MiB (0.62%)

pnkfelix · 2024-07-03T02:58:57Z

Visiting for weekly rustc-perf triage

all regressions are to secondary match-stress benchmark, and were anticipated during a perf run during review
marking as triaged.

@rustbot label: +perf-regression-triaged

oli-obk added 6 commits June 26, 2024 16:01

Split lifetimes on mir borrowck dataflow

81695a1

Restrict diagnostic context lifetime of mir borrowck to InferCtxt ins…

ef55919

…tead of TyCtxt

Restrict diagnostic context lifetime of FnCtxt to InferCtxt instead o…

f3d9523

…f TyCtxt

Restrict diagnostic context lifetime of TypeErrCtxt to InferCtxt inst…

79ac898

…ead of TyCtxt

Restrict diagnostic context lifetime of InferCtxt to itself instead o…

5988078

…f TyCtxt

Automatically taint InferCtxt when errors are emitted

86c8eae

rustbot assigned nnethercote Jun 26, 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 Jun 26, 2024

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 26, 2024

oli-obk commented Jun 26, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jun 26, 2024

nnethercote reviewed Jun 27, 2024

View reviewed changes

compiler/rustc_borrowck/src/borrowck_errors.rs Show resolved Hide resolved

nnethercote reviewed Jun 27, 2024

View reviewed changes

compiler/rustc_errors/src/lib.rs Show resolved Hide resolved

nnethercote reviewed Jun 27, 2024

View reviewed changes

oli-obk added 2 commits June 27, 2024 09:07

Rename 'cx to 'infcx

ba198af

Document new field and function

bd111f5

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 Jul 1, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 1, 2024

bors merged commit 7b21c18 into rust-lang:master Jul 1, 2024
7 checks passed

rustbot added this to the 1.81.0 milestone Jul 1, 2024

bors mentioned this pull request Jul 1, 2024

Disallow hidden references to mutable static #124895

Merged

oli-obk deleted the do_not_count_errors branch July 1, 2024 09:08

matthiaskrgr mentioned this pull request Jul 1, 2024

ICE: Size != Size #127219

Closed

rustbot added the perf-regression-triaged The performance regression has been triaged. label Jul 3, 2024

lqd mentioned this pull request Jul 3, 2024

ICE: type mismatch when copying! #127268

Closed

GrigorenkoPV mentioned this pull request Jul 6, 2024

ICE: Got a scalar pair where a scalar was expected #127299

Closed

matthiaskrgr mentioned this pull request Jul 6, 2024

ICE: sig.c_variadic || extra_args.is_empty() #127423

Open

matthiaskrgr mentioned this pull request Sep 18, 2024

ICE: there are no vtables for unsized types #130521

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automatically taint InferCtxt when errors are emitted #126996

Automatically taint InferCtxt when errors are emitted #126996

oli-obk commented Jun 26, 2024

rustbot commented Jun 26, 2024

oli-obk commented Jun 26, 2024

This comment has been minimized.

bors commented Jun 26, 2024

oli-obk Jun 26, 2024

oli-obk Jun 26, 2024

bors commented Jun 26, 2024

This comment has been minimized.

rust-timer commented Jun 26, 2024

nnethercote Jun 27, 2024 •

edited

Loading

oli-obk Jun 27, 2024

nnethercote Jun 27, 2024

oli-obk Jun 27, 2024 •

edited

Loading

nnethercote Jun 27, 2024

nnethercote Jun 27, 2024

nnethercote commented Jun 27, 2024

nnethercote commented Jun 27, 2024

oli-obk commented Jun 27, 2024

oli-obk commented Jun 27, 2024

lcnr commented Jun 27, 2024

nnethercote commented Jul 1, 2024

bors commented Jul 1, 2024

bors commented Jul 1, 2024

bors commented Jul 1, 2024

rust-timer commented Jul 1, 2024

pnkfelix commented Jul 3, 2024

		@@ -139,7 +139,7 @@ pub struct TypeErrCtxt<'a, 'tcx> {
		}

		impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {

Automatically taint InferCtxt when errors are emitted #126996

Automatically taint InferCtxt when errors are emitted #126996

Conversation

oli-obk commented Jun 26, 2024

rustbot commented Jun 26, 2024

oli-obk commented Jun 26, 2024

This comment has been minimized.

bors commented Jun 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Jun 26, 2024

This comment has been minimized.

rust-timer commented Jun 26, 2024

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

nnethercote Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nnethercote commented Jun 27, 2024

nnethercote commented Jun 27, 2024

oli-obk commented Jun 27, 2024

oli-obk commented Jun 27, 2024

lcnr commented Jun 27, 2024

nnethercote commented Jul 1, 2024

bors commented Jul 1, 2024

bors commented Jul 1, 2024

bors commented Jul 1, 2024

rust-timer commented Jul 1, 2024

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

pnkfelix commented Jul 3, 2024

nnethercote Jun 27, 2024 •

edited

Loading

oli-obk Jun 27, 2024 •

edited

Loading