interpret: properly check for inhabitedness of nested references#156977
Merged
rust-bors[bot] merged 3 commits intoJul 17, 2026
Conversation
Collaborator
rustbot
added
S-waiting-on-review
RalfJung
commented
May 26, 2026
Comment on lines
+282
to
+287
| ty::Coroutine(..) => { | ||
| true // FIXME should these really be trivially inhabited? | ||
| } | ||
| ty::CoroutineClosure(..) => { | ||
| true // FIXME should these really be trivially inhabited? | ||
| } |
Member
Author
There was a problem hiding this comment.
I wasn't sure how to recurse into these. Are coroutines always inhabited via a trivial start state, or can they be uninhabited due to capturing ! as an "upvar"? How do coroutine closures work?
Member
Author
There was a problem hiding this comment.
Looks like yes they can be uninhabited
Coroutine(DefId(0:13 ~ diverges[9ce3]::async_let::{closure#0}), [(), std::future::ResumeTy, (), !, (Void,)]) is ABI-uninhabited but not opsem-uninhabited?
Member
Author
There was a problem hiding this comment.
I now made them all check the upvar_tys, but I am not entirely sure if that is enough.
Contributor
There was a problem hiding this comment.
For coroutines, checking upvars is enough. The coroutine state is pretty much struct { upvars, enum { Unresumed, Returned, Panicked, Suspend0 { .. }, Suspent1 { .. }, .. } }
Note that #135527 proposes to change this to enum { Unresumed { upvars }, Returned, Panicked, Suspend0 { .. }, Suspent1 { .. }, .. }. Starting from that PR, the enum state will be inhabited in all cases.
Coroutine closures work exactly like closures.
RalfJung
commented
May 26, 2026
RalfJung
commented
May 26, 2026
| len.try_to_target_usize(tcx).unwrap() == 0 | ||
| || is_opsem_inhabited_recursor(elem, tcx, root, adt_handler) | ||
| } | ||
| ty::Pat(inner, _pat) => is_opsem_inhabited_recursor(inner, tcx, root, adt_handler), |
Member
Author
There was a problem hiding this comment.
I guess in theory the pattern could make a type uninhabited... so technically if we ever want to use that for the opsem we have to add it has a check here before pattern types get stabilized.
Contributor
There was a problem hiding this comment.
none of the current patterns can, but yes, that may be possible in the future
RalfJung
commented
May 26, 2026
| /// | ||
| /// When we git an ADT, we call `adt_handler`, giving it as its last argument a closure that it | ||
| /// can invoke to continue the recursion. | ||
| fn is_opsem_inhabited_recursor<'tcx>( |
Member
Author
There was a problem hiding this comment.
Do we need to do something to bound this recursion? We already stop when encountering the same ADT again, so recursion is bounded by the depth of ADT field types until it comes back to the original type.
Do we need to do some stack growing magic?
Contributor
There was a problem hiding this comment.
it's possible this can't be hit due to other stack growth protections, but theoretically you can create a very nested tuple, or just &&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&T with a lot of repetitions of the reference. I'd honestly just wait for an issue
This comment has been minimized.
This comment has been minimized.
RalfJung
force-pushed
the
interpret-opsem-inhabited
branch
from
May 26, 2026 15:51
4e90bd5 to
e6d1440
Compare
This comment has been minimized.
This comment has been minimized.
RalfJung
force-pushed
the
interpret-opsem-inhabited
branch
from
May 26, 2026 17:12
e6d1440 to
33dc892
Compare
This comment has been minimized.
This comment has been minimized.
RalfJung
force-pushed
the
interpret-opsem-inhabited
branch
from
May 26, 2026 18:07
33dc892 to
846fa4f
Compare
This comment has been minimized.
This comment has been minimized.
RalfJung
force-pushed
the
interpret-opsem-inhabited
branch
from
May 26, 2026 20:45
846fa4f to
8d272fd
Compare
This comment has been minimized.
This comment has been minimized.
RalfJung
force-pushed
the
interpret-opsem-inhabited
branch
from
May 26, 2026 21:42
8d272fd to
06e6db9
Compare
This comment has been minimized.
This comment has been minimized.
RalfJung
force-pushed
the
interpret-opsem-inhabited
branch
from
May 27, 2026 06:27
06e6db9 to
f328265
Compare
oli-obk
requested changes
May 27, 2026
| /// | ||
| /// When we git an ADT, we call `adt_handler`, giving it as its last argument a closure that it | ||
| /// can invoke to continue the recursion. | ||
| fn is_opsem_inhabited_recursor<'tcx>( |
Contributor
There was a problem hiding this comment.
it's possible this can't be hit due to other stack growth protections, but theoretically you can create a very nested tuple, or just &&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&T with a lot of repetitions of the reference. I'd honestly just wait for an issue
| len.try_to_target_usize(tcx).unwrap() == 0 | ||
| || is_opsem_inhabited_recursor(elem, tcx, root, adt_handler) | ||
| } | ||
| ty::Pat(inner, _pat) => is_opsem_inhabited_recursor(inner, tcx, root, adt_handler), |
Contributor
There was a problem hiding this comment.
none of the current patterns can, but yes, that may be possible in the future
rustbot
added
S-waiting-on-author
theemathas
reviewed
May 27, 2026
RalfJung
force-pushed
the
interpret-opsem-inhabited
branch
from
May 27, 2026 14:28
f328265 to
bffa3cd
Compare
RalfJung
commented
May 27, 2026
| // Bailing out here is unfortuante as it means that the recursion limit affects the | ||
| // operational semantics... but what else could we do? | ||
| return true; | ||
| } |
Member
Author
There was a problem hiding this comment.
I pushed a proper recursion check including a recursion limit check.
But... I am not sure it's sound. This means that depending on the recursion limit, different crates might consider the same type inhabited or not.
OTOH if we just hard-code e.g. 256 here, then with a sufficiently high query recursion limit one can write a 257-level-nested type (Wrap<Wrap<...<Wrap<!>>...>>) that layout will still consider uninhabited but this check will give up.
Contributor
There was a problem hiding this comment.
Why exactly is this check soundness-critical? I don't quite follow...
As far as I can tell, it seems like this is just a courtesy best-effort check for UB in consteval, and a sanity check after computing layout?
Member
Author
There was a problem hiding this comment.
This is also used by Miri so it is supposed to define what is and isn't UB.
Also our query system relies on cross-crate-consistent results, doesn't it?
Member
Author
There was a problem hiding this comment.
@camsteffen proposed to do something more like inhabited_predicate_adt. I must admit that I do not understand how that query works -- I can see that it produces a predicate and ships it to the trait solver, but how is that helpful for recursive types?
Contributor
There was a problem hiding this comment.
Sorry, that was kind of a red herring.
More to the point is check_representability. That query uses query cycle detection to detect an infinite sized type and abort compilation.
So I think we could have a check_opsem_inhabited query which also has query cycle detection, but instead of aborting, return OpsemInhabited::False or something like that for the query result.
check_representability uses params_in_repr, so check_opsem_inhabited could use something similar like params_in_repr_or_ref to include references.
Member
There was a problem hiding this comment.
I believe that the algorithm you proposed would say that
&Wrap<Wrap<!>>is inhabited, which doesn't seem like a particularly strange edge case. @WaffleLapkin
I personally think that it's "fine", especially given that this improves over the status quo. But I suppose we could also allow recursing through the same definition a set number of times (i.e. setting a recursion limit higher than 1)...
Member
Author
There was a problem hiding this comment.
@WaffleLapkin I like that first version, and I think I can combine it with an idea I had to avoid the problem @theemathas mentioned. This avoids any dependency on the recursion limit while also still satisfying the desired implication "layout inhabited => opsem inhabited". I pushed that now.
Contributor
There was a problem hiding this comment.
Another idea, that lcnr proposed, is that we can error on overflow. I.e. when computing inhabitedness of a type, if we reach the recursion limit, emit a hard error.
Since an error prevents the code from being compiled, there is no problem with linking two different crates with different recursion limits -- they either both get the same result, or one of them doesn't compile.
We can still support (assume inhabited) types which refer to themselves directly like
struct A(&'static A);And error only if the same type is mentioned with different generic parameters and overflows:
struct B<T: 'static>(&'static B<(T, T)>);This is technically a breaking change, but it seems fine not to support types which infinitely grow like this (or am I missing something?).
I personally fairly strongly prefer checking for overflow and hard erroring when encountering the recursion limit instead of having WrapRef<WrapRef<!>> be considered to be inhabited by MIRI and to not allow optimizations for it?
I do feel that the examples from #138599 should just errors. It's kind of brittle to support these, as it relies on the type with diverging recursion to be trivially sized. I don't want us to have to be careful with the way sizedness and its fastpaths are handled because of this.
@RalfJung is it a strong preference on your end to not do it based on tracking recursion depth which increases whenever we recur into an adt?
Does the fact that WrapRef<WrapRef<!>> is opsem inhabited mean the following optimization is now unsound? I don't know whether we currently allow this opt
#![feature(never_type)]
#[derive(Clone, Copy)]
#[repr(transparent)]
struct WrapRef<T: 'static>(&'static T);
fn foo<T: Copy>(x: WrapRef<T>) {
// is it an acceptable optimization to move the assignment to `y` to here?
//
// Because if I understand this change correctly, `y` would be opsem
// uninhabited while `x` would be inhabited?
abort();
let y: T = *x.0;
}
fn abort() {
std::process::abort();
}
fn main() {
// SAFETY: Layout is the same here? xd
unsafe {
foo(std::mem::transmute::<&&(), WrapRef<WrapRef<!>>>(&&()))
};
}
Member
Author
There was a problem hiding this comment.
I personally fairly strongly prefer checking for overflow and hard erroring when encountering the recursion limit of having WrapRef<WrapRef<!>> be considered to be inhabited by MIRI and to not allow optimizations for it?
There's a word missing here I think, so I cannot fully parse this sentence...
But, in terms of hard erroring, I would find it strange if the interpreter validity check is load-bearing for getting hard errors for certain types. We don't even run this validity check on many types during normal compilation. And it is quite problematic if a program that would usually compile then fails to compile in Miri as that makes it impossible to use Miri to check that program for UB, so this is also something I'd like to avoid.
is it a strong preference on your end to not do it based on tracking recursion depth which increases whenever we recur into an adt?
I don't want the interpreter validity check to be the only place where this errors. If these types all error during normal compilation for normal type system reasons, that's entirely fine for me. But we don't call is_opsem_inhabited during normal compilation (except for consts), and we shouldn't, so there needs to be somewhere else that is responsible for ensuring "no infinite types" if that's a property we want.
I don't have a strong opinion on whether we have that property or not. I think we can deal with not having it, but some things get simpler if we do have it.
Does the fact that WrapRef<WrapRef<!>> is opsem inhabited mean the following optimization is now unsound?
This optimization is unsound for a multitude of reasons. For instance, it is unsound for T = bool because we don't require an &bool to actually point to a valid bool. And with this PR it is also invalid for T = WrapRef<!>, yes,
Contributor
There was a problem hiding this comment.
👍
yeah, I do want infinitely expanding types to be an error in general, and only dislike this PR out of "it is avoiding erroring on them explicitly".
Feels fine to merge this as is, opened #158908 for the more general issue
This comment has been minimized.
This comment has been minimized.
RalfJung
force-pushed
the
interpret-opsem-inhabited
branch
from
May 27, 2026 14:56
bffa3cd to
2552ff6
Compare
This comment has been minimized.
This comment has been minimized.
Collaborator
|
This PR changes a file inside |
Member
Author
No I won't. This is bad advice in this case: the underlying cause of the crash has not been fixed. The PR just somehow makes the compiler not reach the crashy code any more for that particular example. I don't think the example tests anything meaningful that is worth adding to the test suite. |
Member
Author
|
@WaffleLapkin just making sure you're aware that I replied to your review above. :) |
Member
WaffleLapkin
approved these changes
Jul 15, 2026
| } | ||
| } | ||
|
|
||
| fn is_opsem_inhabited_raw<'tcx>( |
Member
There was a problem hiding this comment.
I meant something like this: 5c3fe93. Now, my change removes the query (wrote it hastily before needing to leave), which is probably still needed, but it can easily be added inside of Ty::is_opsem_inhabited.
I'm not sure that "handle easy cases directly" is worth the complexity with the callback... And even then, it can probably be expressed in simpler ways.
This comment has been minimized.
This comment has been minimized.
RalfJung
force-pushed
the
interpret-opsem-inhabited
branch
from
July 15, 2026 21:36
a08ecfe to
d65ad07
Compare
Collaborator
|
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. |
Member
Author
Thanks! If you prefer the version with more code duplication and fewer closures, I'm happy to make a follow-up PR. If someone has an idea how we could avoid both the closures (or an equivalent trait which I don't think would be any more clear) and the code duplication that'd be amazing but I have no idea how to do that. :) @bors r=WaffleLapkin |
Contributor
rust-bors
Bot
added
S-waiting-on-bors
jhpratt
added a commit
to jhpratt/rust
that referenced
this pull request
Jul 17, 2026
…, r=WaffleLapkin interpret: properly check for inhabitedness of nested references This implements the opsem from the ongoing FCP in rust-lang/unsafe-code-guidelines#413. The bit we were previously missing is that transmuting a `&&!` into existence was not caught as being immediate UB -- only the `&!` case behaved as expected. I did not adjust the layout computation because when we compute the layout of `&T`, we cannot know the layout of `T` (as that might be recursive). r? @oli-obk
rust-bors Bot
pushed a commit
that referenced
this pull request
Jul 17, 2026
Rollup of 10 pull requests Successful merges: - #156977 (interpret: properly check for inhabitedness of nested references) - #159365 (fix: point at method call chain when a return-position `impl Trait` assoc type diverges) - #159402 (Clarify safety requirements for SIMD shl/shr and masked load/store) - #159410 (rustdoc: remove old `--emit` types) - #159302 (Implement `Debug` helpers via `Cell`) - #159386 (add a fallback for `fmuladdf*`) - #159391 (Update tests for LLVM 23) - #159400 (Update books) - #159401 (Gate `tests/debuginfo/function-call.rs` on min GDB 15.1) - #159404 ([aarch64][win] Pass oversized c-variadic args indirectly on Arm64EC)
This comment has been minimized.
This comment has been minimized.
rust-bors
Bot
added
merged-by-bors
Contributor
Contributor
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 3d50c25 (parent) -> 4a9d536 (this PR) Test differencesShow 22 test diffsStage 1
Stage 2
Additionally, 14 doctest diffs were found. These are ignored, as they are noisy. Job group index
Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 4a9d5368df6450bbd2fc8dde4508c3e5d83bb19d --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
Collaborator
|
Finished benchmarking commit (4a9d536): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (secondary 1.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -11.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.1%, secondary -0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 489.9s -> 491.028s (0.23%) |
Collaborator
|
A job failed! Check out the build log: (web) (plain enhanced) (plain) Click to see the possible cause of the failure (guessed by this bot) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
11 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
View all comments
This implements the opsem from the ongoing FCP in rust-lang/unsafe-code-guidelines#413. The bit we were previously missing is that transmuting a
&&!into existence was not caught as being immediate UB -- only the&!case behaved as expected.I did not adjust the layout computation because when we compute the layout of
&T, we cannot know the layout ofT(as that might be recursive).r? @oli-obk