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

`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-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.

Zalathar · 2026-03-04T06:15:04Z

compiler/rustc_middle/src/query/plumbing.rs

+                $(#[$attr])*
+                #[inline(always)]
+                pub fn $name(self, key: query_helper_param_ty!($($K)*)) {
+                    crate::query::inner::query_ensure_ok_or_done(
+                        self.tcx,
+                        &self.tcx.query_system.query_vtables.$name,
+                        $crate::query::IntoQueryParam::into_query_param(key),
+                        $crate::query::EnsureMode::Ok,
+                    )
+                }


Question: Would it make sense to forbid ensure_ok calls for queries that return Result<_, ErrorGuaranteed>?

(By gating this method with #[cfg(not($returns_error_guaranteed))].)

I haven't fully considered the consequences, but intuitively it feels like a footgun to allow such calls.

I tried this before filing the PR and had problems here. Currently that forces tcx.foo() being supported for every query. It's possible this might be adjusted away, but it felt like a bridge too far for this PR.

To clarify, I meant forbidding tcx.ensure_ok().$query(..), not tcx.$query(..).

But it turns out that I was planning to get rid of call_query_method_fn anyway:

Get rid of QueryVTable::call_query_method_fn #153387

Sorry I misread. Back to the original question: I tried forbidding it like you suggested. There are two places it causes problems. A line in compiler/rustc_interface/src/passes.rs:

tcx.ensure_ok().mir_borrowck(def_id);

and a line in src/librustdoc/core.rs:

tcx.sess.time("wf_checking", || tcx.ensure_ok().check_type_wf(()));

Both of these queries return Result<_, ErrorGuaranteed> but neither was marked with return_result_from_ensure_ok. They could be changed to use ensure_result() (which requires a let _ = ...) but I don't think it's worthwhile. After all, every query that returns a value (i.e. most of them) can be called with ensure_ok, so it seems weird to ban ensure_ok just for the Result<_, ErrorGuaranteed> ones.

compiler/rustc_macros/src/lib.rs

`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_result_with_error_guarantee`. (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-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%)

nnethercote added 2 commits March 4, 2026 16:29

Tweak control flow in query_* functions.

28848d7

`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`.

rustbot assigned Zalathar Mar 4, 2026

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

Uh oh!

Conversation

nnethercote commented Mar 4, 2026

Uh oh!

rustbot commented Mar 4, 2026

Uh oh!

rustbot commented Mar 4, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nnethercote Mar 4, 2026 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nnethercote Mar 4, 2026 •

edited

Loading