Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @wesleywiser (or someone else) soon.

Please see the contribution instructions for more information.

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Trying commit a1bf293 with merge 41c8d3a...

I think this is a good change, but maybe a safer intermediate step would be to "assert" that all the queries were run in the "passes" for now. This would ensure that we don't accidentally make a query more lazy, then skip something (accidentally).

Hmm... we don't have the setup for doing that (also doing it at that phase won't work, as it may get run later). I'll see if I can add something to the query system for asserting that a query has been run

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

This would ensure that we don't accidentally make a query more lazy, then skip something (accidentally).

I looked into it, and we're already relying on the mir_built query to run thir_check_unsafety. So we're already assuming that mir_built gets run for all things with bodies. I agree some assertions would be neat, but generating a check-function per query and calling it is a lot. Limiting it to certain queries can be done with more query flags, but that seems like a lot of infrastructure for something that we can be certain won't happen (as we'd already be missing checks in that case).

I'm not sure I agree with the general direction. I'd rather suggest doing the opposite:

• remove useless dependencies from queries -> avoid useless recomputations;
• invoke all the required "check" queries from analysis -> ensure correctness-related stuff are in a single place.

The fact that we run thir_check_unsafety from mir_built is more accidental, due to the fact that mir_built steals THIR. We probably shouldn't really rely on it for correctness. Even if having ensure().mir_borrowck in analysis is enough to make sure that mir_built is computed and thir_check_unsafety too.

The problem with those useless dependencies is that e.g. const eval can run on code that doesn't satisfy various checks, leading to surprising bugs and workarounds in const eval. I agree that mir_built may be too early, but at the very least the mir_drops_elaborated_and_const_checked query should not finish before the various checks on the same def id are done.

I guess my assumption was that at some point all we have to do is to either call the optimized_mir query for the main function and can compile from just the information from that, or call the analysis query to do a check build.

If changing any of this negatively affects incremental, then we should not do it, but I don't see that being an issue in general with this scheme, just with the details of where to add edges.

Finished benchmarking commit (41c8d3a): 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.7%	[0.7%, 0.7%]	1
Regressions (secondary)	0.7%	[0.7%, 0.7%]	1
Improvements (primary)	-0.5%	[-0.6%, -0.5%]	6
Improvements (secondary)	-1.6%	[-1.6%, -1.6%]	1
All (primary)	-0.4%	[-0.6%, 0.7%]	7

Max RSS (memory usage)

Results

Cycles

Results

Everything but assoc_many_items in doc mode is noise.

• remove useless dependencies from queries -> avoid useless recomputations;

about that, do we really add more dependencies with ensure? Doesn't ensure just mean that the ensured query must be run before the current one without any change in the dep-graph otherwise?

Yes, ensure adds a dependency edge. And the calls to ensure just before stealing a value bother me a bit: they introduce a dependency to a value that will not be used, and don't even make sure the code will not be called later. ensure may "just" mark the query as green, without loading any value, to be recomputed later when the value is actually required.

The problem with those useless dependencies is that e.g. const eval can run on code that doesn't satisfy various checks, leading to surprising bugs and workarounds in const eval. I agree that mir_built may be too early, but at the very least the mir_drops_elaborated_and_const_checked query should not finish before the various checks on the same def id are done.

In those cases, we should rely on the trainted_by_errors fields on typeck and MIR body.

One other symptom is the calls to track_errors in rustc_hir_analysis::check_crate that shouldn't exist: typeck should rely on propagated error state via ty::Error and ty::ReError. But that FIXME exists since 2020, so that's not high priority either.

If changing any of this negatively affects incremental, then we should not do it, but I don't see that being an issue in general with this scheme, just with the details of where to add edges.

From the perf results, I have no proof that it does negatively affect incremental, so no serious grounds on which to block this PR. That's also why I frame it as a design question, more than an operational question.

That said, we should merge that PR, as it does not lock anything in a way or the other. I'll have to make my case with an actual PR demonstrating the direction I have in mind .

Seeing #108705, I finally convinced myself that this PR is the right direction. This can help to avoid building MIR for broken code, which is that much potential ICEs and non-sensical diagnostics.

#108118 (comment)
Hmm... we don't have the setup for doing that (also doing it at that phase won't work, as it may get run later). I'll see if I can add something to the query system for asserting that a query has been run

That's what ensure() does: either the query is known green and they replay diagnostics from the previous compilation, or the query is re-executed.

That's what ensure() does: either the query is known green and they replay diagnostics from the previous compilation, or the query is re-executed.

Oh I meant something that asserts it instead of executing it. Though ensuring later will ICE due to stolen stuff, so it results in the same thing

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

r=me when @cjgillot is happy

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

@bors r+

Commit 3344232 has been approved by cjgillot

It is now in the queue for this repository.

Testing commit 3344232 with merge 3462f79...

Test successful - checks-actions
Approved by: cjgillot
Pushing 3462f79 to master...

Finished benchmarking commit (3462f79): comparison URL.

Overall result: regressions and improvements - no action needed

@rustbot label: -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.5%, 0.5%]	1
Improvements (primary)	-	-	0
Improvements (secondary)	-0.3%	[-0.3%, -0.2%]	4
All (primary)	-	-	0

Max RSS (memory usage)

Results

Cycles

Results