fix(interp): Treat UnsafeBinder as Compound Type in try_visit_primitive#153458
fix(interp): Treat UnsafeBinder as Compound Type in try_visit_primitive#153458TKanX wants to merge 1 commit into
UnsafeBinder as Compound Type in try_visit_primitive#153458Conversation
|
can you explain a bit your choice of this match arm? |
Since layout.rs erases bound regions and delegates entirely to the inner type, I think returning Happy to be corrected if there's something specific. |
|
r? rust-lang/compiler |
| | ty::Dynamic(..) | ||
| | ty::Closure(..) | ||
| | ty::Pat(..) | ||
| | ty::UnsafeBinder(..) |
There was a problem hiding this comment.
Sorry, I am not super sure about the decision here. However, I would like to leave a long overdue opinion and I would like to take this opportunity to learn more about the UnsafeBinder semantics here as well.
In my very rudimentary understanding of an unsafe<..> type and the primitivity property in CTFE is so that it transcends through the binder. For instance the primitivity of unsafe<'a> &'a T is delegated to &'a T, which depends on how well-behaving this underlying reference is. So does this make sense?
| #![allow(incomplete_features)] | ||
|
|
||
| struct ThinDst { | ||
| b: unsafe<> (), |
There was a problem hiding this comment.
... so if a raw pointer, instead of a unit, is behind the binder, we probably should also check the reference here, or?
There was a problem hiding this comment.
While this was the minimal repro for the ICE, fixing the ICE needs to come with some extensive testing of the feature within const validation.
So having tests where a reference behind an unsafe binder points to the wrong memory (e.g. a fn) or points to uninit memory even tho the value can't be uninit if it were a normal reference. There's probably more fun edge cases to handle here
|
@rustbot author |
|
Reminder, once the PR becomes ready for a review, use |
|
r? dingxiangfei2009 |
cc2d7ce to
321a95f
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
321a95f to
3941d0b
Compare
|
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. |
|
@rustbot ready |
|
visited during t-compiler triage meeting on Zulip#t-compiler/meetings > [weekly] 2026-04-30 @ 💬, rolling the reviewer r? compiler |
|
r? @BoxyUwU |
|
|
|
Ah maybe then |
|
Oh I didn't see this yet. There's also #156373 which was created later |
| ty::UnsafeBinder(_) => todo!("FIXME(unsafe_binder)"), | ||
| ty::UnsafeBinder(unsafe_binder_ty) => { | ||
| let inner_ty = | ||
| self.ecx.tcx.instantiate_bound_regions_with_erased((*unsafe_binder_ty).into()); | ||
| let inner = value.transmute(self.ecx.layout_of(inner_ty)?, self.ecx)?; | ||
| self.visit_value(&inner)?; | ||
| interp_ok(true) | ||
| } |
There was a problem hiding this comment.
I can't come up with an example that is problematic, but I think this should just do what https://github.com/rust-lang/rust/pull/156373/changes did and bail on unsafe binders and handle them in the aggregate logic.
View all comments
Summary:
Fixes ICE in const eval validity checking when a value contains an
UnsafeBinder-typed field.try_visit_primitiveinvalidity.rshadty::UnsafeBinder(_) => todo!("FIXME(unsafe_binder)"): a placeholder left by the initial unsafe binders type system implementation (9a1c5eb).The fix is to remove the
todo!()and moveUnsafeBinderinto the compound types arm.Closes #153362
r? @dingxiangfei2009
cc @matthiaskrgr