Overhaul `ensure_ok` by nnethercote · Pull Request #153383 · rust-lang/rust

nnethercote · 2026-03-04T06:01:19Z

The interaction of ensure_ok and the return_result_from_ensure_ok query modifier is weird and hacky. This PR cleans it up. Details in the individual commits.

r? @Zalathar

rustbot · 2026-03-04T06:01:23Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

Some changes occurred to constck

cc @fee1-dead

rustbot · 2026-03-04T06:01:25Z

Zalathar is not on the review rotation at the moment.
They may take a while to respond.

compiler/rustc_middle/src/query/plumbing.rs

compiler/rustc_macros/src/lib.rs

nnethercote · 2026-03-04T09:47:35Z

@bors try @rust-timer queue

Overhaul `ensure_ok`

rust-bors · 2026-03-04T11:58:25Z

☀️ Try build successful (CI)
Build commit: 61d1293 (61d12939576562cd1a24b416ffcb69ce1ef6562e, parent: d933cf483edf1605142ac6899ff32536c0ad8b22)

petrochenkov · 2026-03-04T12:09:59Z

compiler/rustc_middle/src/query/inner.rs

-    if try_get_cached(tcx, &query.cache, &key).is_none() {
-        (query.execute_query_fn)(tcx, DUMMY_SP, key, QueryMode::Ensure { ensure_mode });
+    match try_get_cached(tcx, &query.cache, &key) {
+        Some(_value) => {}


I suspect this is going to be reverted next time anyone runs clippy on the compiler.

I tried clippy on a test program with the same structure and it didn't complain.

rust-timer · 2026-03-04T12:38:09Z

Finished benchmarking commit (61d1293): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.1%	[-0.1%, -0.1%]	1
Improvements ✅ (secondary)	-0.2%	[-0.2%, -0.1%]	3
All ❌✅ (primary)	-0.1%	[-0.1%, -0.1%]	1

Max RSS (memory usage)

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

Cycles

Results (secondary -1.9%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.8%	[1.3%, 4.7%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-15.7%	[-15.7%, -15.7%]	1
All ❌✅ (primary)	-	-	0

Binary size

Results (primary 0.1%, secondary 0.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.1%	[0.0%, 0.1%]	23
Regressions ❌ (secondary)	0.1%	[0.0%, 0.1%]	46
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[0.0%, 0.1%]	23

Bootstrap: 481.183s -> 478.51s (-0.56%)
Artifact size: 394.90 MiB -> 395.01 MiB (0.03%)

Zalathar · 2026-03-07T02:02:20Z

compiler/rustc_hir_analysis/src/lib.rs

+        let _: R = tcx.ensure_result().check_type_wf(());

        for &trait_def_id in tcx.all_local_trait_impls(()).keys() {
-            let _: R = tcx.ensure_ok().coherent_trait(trait_def_id);
+            let _: R = tcx.ensure_result().coherent_trait(trait_def_id);
        }
        // these queries are executed for side-effects (error reporting):
-        let _: R = tcx.ensure_ok().crate_inherent_impls_validity_check(());
-        let _: R = tcx.ensure_ok().crate_inherent_impls_overlap_check(());
+        let _: R = tcx.ensure_result().crate_inherent_impls_validity_check(());
+        let _: R = tcx.ensure_result().crate_inherent_impls_overlap_check(());


Future work: If we're calling ensure_result() and then immediately discarding the result, then maybe we should just be calling the plain ensure_ok() instead. That wasn't possible before this PR, but it becomes possible now.

(But I'm fine with not worrying about that for this PR.)

compiler/rustc_middle/src/query/plumbing.rs

Zalathar · 2026-03-07T02:16:26Z

I don't love the ensure_ok/ensure_result naming convention. But I don't have good suggestions for better names, and those names seem good enough to not be worth blocking this PR on. We can always revisit the names later.

`query_get_at`, `query_ensure`, and `query_ensure_error_guarantee` are very similar functions, but they all use different control flow styles which obscures the similarities. This commit rewrites them to all use a `match`.

Two places where `ensure_ok` can be used but currently isn't. (These queries are marked with `return_result_from_ensure_ok`, which means that `ensure_ok` returns `Result<(), ErrorGuaranteed>`.) This is potentially a perf win, because `ensure_ok` query calls can be faster.

rustbot · 2026-03-07T10:31:13Z

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

nnethercote · 2026-03-07T10:32:23Z

I addressed the nit about the comment. Should be good to go now.

Zalathar · 2026-03-07T11:07:47Z

I think there’s some stale text in the final commit message; it mentions returns_result_with_error_guarantee, which doesn’t match the actual synthetic modifier name.

r=me after fixing that.

`ensure_ok` provides a special, more efficient way of calling a query when its return value isn't needed. But there is a complication: if the query is marked with the `return_result_from_ensure_ok` modifier, then it will return `Result<(), ErrorGuaranteed`. This is clunky and feels tacked on. It's annoying to have to add a modifier to a query to declare information present in its return type, and it's confusing that queries called via `ensure_ok` have different return types depending on the modifier. This commit: - Eliminates the `return_result_from_ensure_ok` modifier. The proc macro now looks at the return type and detects if it matches `Result<_, ErrorGuarantee>`. If so, it adds the modifier `returns_error_guaranteed`. (Aside: We need better terminology to distinguish modifiers written by the user in a `query` declaration (e.g. `cycle_delayed_bug`) from modifiers added by the proc macro (e.g. `cycle_error_handling`.)) - Introduces `ensure_result`, which replaces the use of `ensure_ok` for queries that return `Result<_, ErrorGuarantee>`. As a result, `ensure_ok` can now only be used for the "ignore the return value" case.

nnethercote · 2026-03-07T22:42:07Z

it mentions returns_result_with_error_guarantee

Good catch. That was the name I originally used. I changed it because it was so long that it was causing line breaks. I have updated.

@bors r=Zalathar

rust-bors · 2026-03-07T22:42:10Z

📌 Commit d4503b0 has been approved by Zalathar

It is now in the queue for this repository.

rust-bors · 2026-03-08T10:45:58Z

☀️ Test successful - CI
Approved by: Zalathar
Duration: 3h 42m 16s
Pushing c7b206b to main...

github-actions · 2026-03-08T10:48:44Z

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing c3d0140 (parent) -> c7b206b (this PR)

Test differences

Show 172 test diffs

172 doctest diffs were found. These are ignored, as they are noisy.

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard c7b206bba4434ed928e9615f31082a9ef3f67c27 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

dist-aarch64-linux: 1h 49m -> 2h 54m (+59.8%)
dist-aarch64-apple: 2h 20m -> 1h 50m (-21.3%)
x86_64-gnu-llvm-20-2: 1h 27m -> 1h 38m (+12.2%)
dist-arm-linux-musl: 1h 42m -> 1h 31m (-11.0%)
aarch64-msvc-1: 2h 7m -> 1h 55m (-9.8%)
dist-apple-various: 1h 52m -> 2h 3m (+9.4%)
dist-x86_64-netbsd: 1h 21m -> 1h 28m (+9.0%)
dist-x86_64-freebsd: 1h 30m -> 1h 22m (-8.6%)
pr-check-1: 31m 39s -> 34m 4s (+7.7%)
x86_64-gnu-miri: 1h 34m -> 1h 27m (-7.1%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

rust-timer · 2026-03-08T11:27:08Z

Finished benchmarking commit (c7b206b): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.3%	[-0.3%, -0.3%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -0.0%, secondary -0.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.5%	[2.5%, 2.5%]	1
Regressions ❌ (secondary)	2.0%	[0.6%, 3.2%]	3
Improvements ✅ (primary)	-1.3%	[-1.4%, -1.1%]	2
Improvements ✅ (secondary)	-7.1%	[-7.1%, -7.1%]	1
All ❌✅ (primary)	-0.0%	[-1.4%, 2.5%]	3

Cycles

Results (primary 2.9%, secondary 2.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.9%	[2.9%, 2.9%]	1
Regressions ❌ (secondary)	4.9%	[4.6%, 5.2%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.6%	[-2.6%, -2.6%]	1
All ❌✅ (primary)	2.9%	[2.9%, 2.9%]	1

Binary size

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

Bootstrap: 481.476s -> 488.157s (1.39%)
Artifact size: 397.01 MiB -> 395.01 MiB (-0.51%)

rustbot assigned Zalathar Mar 4, 2026

Zalathar reviewed Mar 4, 2026

View reviewed changes

compiler/rustc_middle/src/query/plumbing.rs Show resolved Hide resolved

Zalathar reviewed Mar 4, 2026

View reviewed changes

compiler/rustc_macros/src/lib.rs Outdated Show resolved Hide resolved

nnethercote mentioned this pull request Mar 4, 2026

Check stalled coroutine obligations eagerly #152327

Merged

nnethercote force-pushed the overhaul-ensure_ok branch from edf3729 to bde722a Compare March 4, 2026 09:47

This comment has been minimized.

Sign in to view

rust-bors bot pushed a commit that referenced this pull request Mar 4, 2026

Auto merge of #153383 - nnethercote:overhaul-ensure_ok, r=<try>

61d1293

Overhaul `ensure_ok`

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 4, 2026

This comment has been minimized.

Sign in to view

petrochenkov reviewed Mar 4, 2026

View reviewed changes

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 4, 2026

Zalathar reviewed Mar 7, 2026

View reviewed changes

compiler/rustc_middle/src/query/plumbing.rs Outdated Show resolved Hide resolved

nnethercote added 2 commits March 7, 2026 21:29

Tweak control flow in query_* functions.

0ffb4ad

`query_get_at`, `query_ensure`, and `query_ensure_error_guarantee` are very similar functions, but they all use different control flow styles which obscures the similarities. This commit rewrites them to all use a `match`.

nnethercote force-pushed the overhaul-ensure_ok branch from bde722a to 799ad7a Compare March 7, 2026 10:31

nnethercote force-pushed the overhaul-ensure_ok branch from 799ad7a to d4503b0 Compare March 7, 2026 22:41

rust-bors bot 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 Mar 7, 2026

This comment has been minimized.

Sign in to view

rust-bors bot added merged-by-bors This PR was explicitly merged by bors. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 8, 2026

rust-bors bot merged commit c7b206b into rust-lang:main Mar 8, 2026
12 checks passed

rustbot added this to the 1.96.0 milestone Mar 8, 2026

rust-bors bot mentioned this pull request Mar 8, 2026

Remove the rustc_data_structures::assert_matches! re-exports #153553

Merged

nnethercote deleted the overhaul-ensure_ok branch March 8, 2026 11:55

Uh oh!

Conversation

nnethercote commented Mar 4, 2026

Uh oh!

rustbot commented Mar 4, 2026

Uh oh!

rustbot commented Mar 4, 2026

Uh oh!

Uh oh!

Uh oh!

nnethercote commented Mar 4, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

rust-bors bot commented Mar 4, 2026

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rust-timer commented Mar 4, 2026

Overall result: ✅ improvements - no action needed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Zalathar commented Mar 7, 2026

Uh oh!

rustbot commented Mar 7, 2026

Uh oh!

nnethercote commented Mar 7, 2026

Uh oh!

Zalathar commented Mar 7, 2026

Uh oh!

nnethercote commented Mar 7, 2026

Uh oh!

rust-bors bot commented Mar 7, 2026

Uh oh!

This comment has been minimized.

rust-bors bot commented Mar 8, 2026

Uh oh!

Uh oh!

github-actions bot commented Mar 8, 2026

Test differences

Job duration changes

Uh oh!

rust-timer commented Mar 8, 2026

Overall result: ✅ improvements - no action needed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants