Skip to content

fix(interp): Treat UnsafeBinder as Compound Type in try_visit_primitive - #153458

Open
TKanX wants to merge 1 commit into
rust-lang:mainfrom
TKanX:bugfix/153362-ice-unsafe-binder-validity
Open

fix(interp): Treat UnsafeBinder as Compound Type in try_visit_primitive#153458
TKanX wants to merge 1 commit into
rust-lang:mainfrom
TKanX:bugfix/153362-ice-unsafe-binder-validity

Conversation

@TKanX

@TKanXTKanX commented Mar 5, 2026•
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

View all comments

Summary:

Fixes ICE in const eval validity checking when a value contains an UnsafeBinder-typed field.

try_visit_primitive in validity.rs had ty::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 move UnsafeBinder into the compound types arm.

Closes#153362

r? @dingxiangfei2009
cc @matthiaskrgr

@rustbotrustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 5, 2026
@TKanX
TKanX marked this pull request as ready for review March 5, 2026 19:34
@rustbot

rustbot commented Mar 5, 2026

Copy link
Copy Markdown
Collaborator

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

@rustbotrustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 5, 2026
@Kivooeo

Kivooeo commented Mar 5, 2026

Copy link
Copy Markdown
Member

can you explain a bit your choice of this match arm?

@TKanX

TKanX commented Mar 5, 2026

Copy link
Copy Markdown
ContributorAuthor

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 false and letting walk_value recurse via FieldsShape is correct (similar to Pat or Closure).

Happy to be corrected if there's something specific.

@EnselicEnselic added the A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) label Apr 28, 2026
@wesleywiser

wesleywiser commented Apr 30, 2026

Copy link
Copy Markdown
Member

r? rust-lang/compiler

dingxiangfei2009
dingxiangfei2009 reviewed Apr 30, 2026
View reviewed changes
Comment threadcompiler/rustc_const_eval/src/interpret/validity.rs Outdated
| ty::Dynamic(..)
| ty::Closure(..)
| ty::Pat(..)
| ty::UnsafeBinder(..)

@dingxiangfei2009dingxiangfei2009Apr 30, 2026•
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

View changes since the review

dingxiangfei2009
dingxiangfei2009 reviewed Apr 30, 2026
View reviewed changes
Comment threadtests/ui/unsafe-binders/const-eval-validity.rs
#![allow(incomplete_features)]

struct ThinDst {
b: unsafe<> (),

@dingxiangfei2009dingxiangfei2009Apr 30, 2026•
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... so if a raw pointer, instead of a unit, is behind the binder, we probably should also check the reference here, or?

View changes since the review

oli-obk
oli-obk reviewed Apr 30, 2026
View reviewed changes
Comment threadtests/ui/unsafe-binders/const-eval-validity.rs

@oli-obkoli-obkApr 30, 2026•
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

View changes since the review

@TKanX

TKanX commented May 19, 2026

Copy link
Copy Markdown
ContributorAuthor

@rustbot author

@rustbotrustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 19, 2026
@rustbot

rustbot commented May 19, 2026

Copy link
Copy Markdown
Collaborator

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

@JohnTitor

JohnTitor commented May 19, 2026

Copy link
Copy Markdown
Member

r? dingxiangfei2009

@TKanX
TKanXforce-pushed the bugfix/153362-ice-unsafe-binder-validity branch from cc2d7ce to 321a95fCompareMay 19, 2026 10:22
@rustbot

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@TKanX
TKanXforce-pushed the bugfix/153362-ice-unsafe-binder-validity branch from 321a95f to 3941d0bCompareMay 19, 2026 12:11
@rustbot

rustbot commented May 19, 2026

Copy link
Copy Markdown
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.

@TKanX

TKanX commented May 19, 2026

Copy link
Copy Markdown
ContributorAuthor

@rustbot ready

@rustbotrustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 19, 2026
@rustbotrustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 19, 2026
@apiraino

apiraino commented Jun 4, 2026•
edited
Loading

Copy link
Copy Markdown
Contributor

visited during t-compiler triage meeting on Zulip#t-compiler/meetings > [weekly] 2026-04-30 @ 💬, rolling the reviewer

r? compiler

@rust-langrust-lang deleted a comment from rustbotJun 4, 2026
@rust-langrust-lang deleted a comment from rustbotJun 4, 2026
@JonathanBrouwer

JonathanBrouwer commented Jun 4, 2026

Copy link
Copy Markdown
Member

r? @BoxyUwU
I'm not familiar enough with const eval to review this

@rustbot

rustbot commented Jun 4, 2026

Copy link
Copy Markdown
Collaborator

BoxyUwU is currently at their maximum review capacity.
They may take a while to respond.

@JonathanBrouwer

JonathanBrouwer commented Jun 4, 2026

Copy link
Copy Markdown
Member

Ah maybe then
r? @oli-obk

@rustbotrustbot assigned oli-obk and unassigned BoxyUwUJun 4, 2026
@oli-obk

oli-obk commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Oh I didn't see this yet. There's also #156373 which was created later

oli-obk
oli-obk reviewed Jun 9, 2026
View reviewed changes
Comment threadcompiler/rustc_const_eval/src/interpret/validity.rs
Comment on lines -994 to +1000
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)
}

@oli-obkoli-obkJun 9, 2026•
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 changes since the review

@oli-obkoli-obk added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 9, 2026
@rust-bors

rust-borsBot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

☔ The latest upstream changes (presumably #159246) made this pull request unmergeable. Please resolve the merge conflicts by rebasing.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oli-obkoli-obkoli-obk left review comments
@dingxiangfei2009dingxiangfei2009Awaiting requested review from dingxiangfei2009

At least 1 approving review is required to merge this pull request.

Assignees

@oli-obkoli-obk

Labels

A-const-evalArea: Constant evaluation, covers all const contexts (static, const fn, ...)S-waiting-on-authorStatus: This is awaiting some action (such as code changes or more information) from the author.T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[ICE]: try_visit_primitive() on unsafe binder

12 participants

@TKanX@rustbot@Kivooeo@wesleywiser@JohnTitor@rust-log-analyzer@apiraino@JonathanBrouwer@oli-obk@dingxiangfei2009@Enselic@BoxyUwU