Refactor is_opsem_inhabited by camsteffen · Pull Request #159625 · rust-lang/rust

camsteffen · 2026-07-20T20:41:26Z

View all comments

Refactor of #156977

Introduce a struct to generally simplify the recursion.

r? WaffleLapkin
cc @RalfJung

rustbot · 2026-07-20T20:41:31Z

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

RalfJung

Okay yeah that is less confusing than my closure nest. :)

@rustbot author

View changes since this review

RalfJung · 2026-07-21T08:17:32Z

@@ -251,107 +248,125 @@ fn inhabited_predicate_type<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) -> InhabitedP

 /// Recurse over a type to determine whether it is inhabited on the opsem level.
 /// See `is_opsem_inhabited` above for the spec of what we compute.


This comment does not really make sense on a type IMO. It should probably go on the ty method, with another brief comment here explaining the point of this type.

Tweaked the comment. I think the method is self documenting enough after renaming it?

The type doesn't compute anything though.

I'd suggest renaming to OpsemInhabitedCtx and then having the docs be something like

/// Context for computing whether a type is inhabited on the opsem level.

RalfJung · 2026-07-21T08:17:47Z

+}
+
+#[derive(Default)]
+struct OpsemInhabitedInQuery {


What's this type for? Needs a doc comment either here or at the in_query field.

Or you can avoid this type entirely by making stop_at_ref a field of OpsemInhabited (which will also simplify some code a bit) and then just having seen: Option<FxHashSet> in OpsemInhabited.

Added comments. I wanted the structure to be explicit about in-query vs. not-in-query modalities. OTOH I think the fact that only ty::Adt triggers the query is an implementation detail that could be tweaked. For example ty::Tuple could also trigger the query.

🤷 I still think this would be simpler with stop_at_ref in the main ctx type and without a second type, and I don't see what this has to do with tuples triggering or not triggering the query, but I don't care strong enough to block the PR over it.

Yeah I guess I over-engineered it. I flattened it.

rustbot · 2026-07-21T08:30:48Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

RalfJung · 2026-07-21T08:30:58Z

r? me

camsteffen · 2026-07-21T16:29:15Z

@rustbot ready

RalfJung · 2026-07-21T17:07:56Z

+        let seen = self.seen.as_mut().unwrap();
+        // Remove the type again so that we allow it to appear on other branches.
+        if new_adt {
+            seen.remove(&adt_def.did());


Suggested change

let seen = self.seen.as_mut().unwrap();

// Remove the type again so that we allow it to appear on other branches.

if new_adt {

seen.remove(&adt_def.did());

// Remove the type again so that we allow it to appear on other branches.

if new_adt {

self.seen.as_mut().unwrap().remove(&adt_def.did());

Or if you want a let-binding, move it inside the if, since it is only needed there.

View changes since the review

RalfJung

Thanks. :)
r=me with last nit resolved.

View changes since this review

RalfJung · 2026-07-21T18:22:58Z

@bors r+ rollup

rust-bors · 2026-07-21T18:23:01Z

📌 Commit 7f2417b has been approved by RalfJung

It is now in the queue for this repository.

@RalfJung

…r, r=RalfJung Refactor is_opsem_inhabited Refactor of rust-lang#156977 Introduce a struct to generally simplify the recursion. r? WaffleLapkin cc @RalfJung

@RalfJung

…r, r=RalfJung Refactor is_opsem_inhabited Refactor of rust-lang#156977 Introduce a struct to generally simplify the recursion. r? WaffleLapkin cc @RalfJung

@RalfJung

…r, r=RalfJung Refactor is_opsem_inhabited Refactor of rust-lang#156977 Introduce a struct to generally simplify the recursion. r? WaffleLapkin cc @RalfJung

Rollup of 12 pull requests Successful merges: - #159582 (Sync from portable simd 2026 07 20) - #158890 (Fix splat v0 mangling) - #159411 ([rustdoc] Correctly handle output options with --show-coverage) - #159647 (Remove `early_exit` closures) - #159656 (define a `Simd` type in `minicore`) - #156474 (Add paths for linked associated items) - #159211 (Fix debuginfo argument when invoking LLBC linker) - #159527 (Bring runtime symbols statics on par with foreign functions) - #159625 (Refactor is_opsem_inhabited) - #159652 (Remove `cfg(not(no_global_oom_handling))` from the `Drop` impl of `UniqueArcUninit`) - #159657 (Remove the blanket `#![cfg_attr(test, allow(unused))]` from bootstrap ) - #159663 (Add layout cycle hang regression test)

@RalfJung

…r, r=RalfJung Refactor is_opsem_inhabited Refactor of rust-lang#156977 Introduce a struct to generally simplify the recursion. r? WaffleLapkin cc @RalfJung

Rollup of 13 pull requests Successful merges: - #159582 (Sync from portable simd 2026 07 20) - #158890 (Fix splat v0 mangling) - #159614 (debuginfo generation for unsafe binders) - #159647 (Remove `early_exit` closures) - #159656 (define a `Simd` type in `minicore`) - #156474 (Add paths for linked associated items) - #159211 (Fix debuginfo argument when invoking LLBC linker) - #159527 (Bring runtime symbols statics on par with foreign functions) - #159625 (Refactor is_opsem_inhabited) - #159629 (Add more splat rust-call regression tests) - #159652 (Remove `cfg(not(no_global_oom_handling))` from the `Drop` impl of `UniqueArcUninit`) - #159657 (Remove the blanket `#![cfg_attr(test, allow(unused))]` from bootstrap ) - #159663 (Add layout cycle hang regression test)

@RalfJung

Rollup merge of #159625 - camsteffen:opsem-inhabited-refactor, r=RalfJung Refactor is_opsem_inhabited Refactor of #156977 Introduce a struct to generally simplify the recursion. r? WaffleLapkin cc @RalfJung

rustbot assigned WaffleLapkin Jul 20, 2026

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 Jul 20, 2026