Do not consider match/let/ref of place that evaluates to ! to diverge, disallow coercions from them too
#129392
Conversation
rustbot
added
S-waiting-on-review
|
@bors try |
…verge-but-more, r=<try> [EXPERIMENT] Do not consider match/let/raw-ref of deref that evalautes to ! to diverge This is the more involved version of rust-lang#129371. It doesn't fully fix rust-lang#117288, since we still need to fix the fact that never type fallback can cause an unintended load via the `NeverToAny` coercion. But I did want to probe crater to see if anyone relies on this behavior currently, since that's almost certainly UB. r? `@ghost`
|
☀️ Try build successful - checks-actions |
compiler-errors
force-pushed
the
raw-ref-op-doesnt-diverge-but-more
branch
from
5250e5d
to
1ee0400
Compare
|
@bors try |
…verge-but-more, r=<try> [EXPERIMENT] Do not consider match/let/raw-ref of deref that evalautes to ! to diverge This is the more involved version of rust-lang#129371. It's pretty ugly rn. r? `@ghost`
|
@bors try |
…verge-but-more, r=<try> [EXPERIMENT] Do not consider match/let/raw-ref of deref that evalautes to ! to diverge This is the more involved version of rust-lang#129371. It's pretty ugly rn. r? `@ghost`
This comment has been minimized.
This comment has been minimized.
|
☀️ Try build successful - checks-actions |
|
@craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
[CRATER] Crater Rollup This is a " crater rollup" of: * rust-lang#126452 * rust-lang#128784 * rust-lang#129392 * rust-lang#129422 * rust-lang#129543 **What is a crater rollup?** It's simply a crater job that is run on all of the containing PRs *together*, and then we can set the crates list for each of these jobs to just the failures after it's done. It should cut out on the bulk of "normal" crates that do nothing and simply just take time to build. r? `@ghost`
[CRATER] Crater Rollup This is a " crater rollup" of: * rust-lang#126452 * rust-lang#128784 * rust-lang#129392 * rust-lang#129422 * rust-lang#129543 * rust-lang#129604 **What is a crater rollup?** It's simply a crater job that is run on all of the containing PRs *together*, and then we can set the crates list for each of these jobs to just the failures after it's done. It should cut out on the bulk of "normal" crates that do nothing and simply just take time to build. r? `@ghost`
[CRATER] Crater Rollup This is a " crater rollup" of: * rust-lang#126452 * rust-lang#128784 * rust-lang#129392 * rust-lang#129422 * rust-lang#129543 * rust-lang#129604 **What is a crater rollup?** It's simply a crater job that is run on all of the containing PRs *together*, and then we can set the crates list for each of these jobs to just the failures after it's done. It should cut out on the bulk of "normal" crates that do nothing and simply just take time to build. r? `@ghost`
|
📝 Configuration of the ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
@craterbot p=1 (Bump priority is this will run quickly.) |
|
📝 Configuration of the ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
|
…verge-but-more, r=<try> Do not consider match/let/ref of place that evaluates to `!` to diverge, disallow coercions from them too Fixes rust-lang#117288. This PR does two things: ### Don't consider `match`/`let`/`ref`/assignment-LHS of a place expression that evaluates to `!` to diverge. Which fixes this unsoundness: ``` fn make_up_a_value<T>() -> T { unsafe { let x: *const ! = &0 as *const u8 as *const !; let _ = *x; } } ``` Before this PR, it was UB since we consider the `unsafe` block to diverge which means the outer block evalutes to `!`, even though we've never actually *read* a value of type `!`. ### Do not perform coercions of those same place expressions. Which fixes this inadvertent, sneaky unsoundness: ``` unsafe { let x: *const ! = &0 as *const u8 as *const !; let _: () = *x; } ``` which is UB because currently rust emits an *implicit* NeverToAny coercion even though we really shouldn't be, since there's no read of the value pointed by `x`. --- Detecting both of these situations is implemented in a heuristic function called `expr_consitutes_read`. It is tasked with detecting the situations where we have a place expression being passed to some parent expression that would not constitute a read necessarily, like a `let _ = *never_ptr` where `never_ptr: *const !`. --- Specifically, for `let` and `match`, we don't consider it to be a read unless any of the subpatterns (i.e. the LHS of the `let` or the arms of the match) constitute a read. Almost all patterns constitute a read except for `_`, an `|` pattern, or the currently experimental `!` pattern. --- I'm not totally certain that this deserves an FCP, since it's really a bugfix for UB. If it does, I'd be comfortable with it being a T-types FCP since we should be responsible with determining which coercions in the type system are sound (similar to how we adjusted subtyping behavior in rust-lang#118247 to be more sound).
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-author
compiler-errors
force-pushed
the
raw-ref-op-doesnt-diverge-but-more
branch
from
26ae1b3
to
a0179f5
Compare
|
@bors try |
…verge-but-more, r=<try> Do not consider match/let/ref of place that evaluates to `!` to diverge, disallow coercions from them too Fixes rust-lang#117288. This PR does two things: ### Don't consider `match`/`let`/`ref`/assignment-LHS of a place expression that evaluates to `!` to diverge. Which fixes this unsoundness: ``` fn make_up_a_value<T>() -> T { unsafe { let x: *const ! = &0 as *const u8 as *const !; let _ = *x; } } ``` Before this PR, it was UB since we consider the `unsafe` block to diverge which means the outer block evalutes to `!`, even though we've never actually *read* a value of type `!`. ### Do not perform coercions of those same place expressions. Which fixes this inadvertent, sneaky unsoundness: ``` unsafe { let x: *const ! = &0 as *const u8 as *const !; let _: () = *x; } ``` which is UB because currently rust emits an *implicit* NeverToAny coercion even though we really shouldn't be, since there's no read of the value pointed by `x`. --- Detecting both of these situations is implemented in a heuristic function called `expr_consitutes_read`. It is tasked with detecting the situations where we have a place expression being passed to some parent expression that would not constitute a read necessarily, like a `let _ = *never_ptr` where `never_ptr: *const !`. --- Specifically, for `let` and `match`, we don't consider it to be a read unless any of the subpatterns (i.e. the LHS of the `let` or the arms of the match) constitute a read. Almost all patterns constitute a read except for `_`, an `|` pattern, or the currently experimental `!` pattern. --- I'm not totally certain that this deserves an FCP, since it's really a bugfix for UB. If it does, I'd be comfortable with it being a T-types FCP since we should be responsible with determining which coercions in the type system are sound (similar to how we adjusted subtyping behavior in rust-lang#118247 to be more sound).
|
☀️ Try build successful - checks-actions |
|
@craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
|
craterbot
added
S-waiting-on-review
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
|
I think all of the crater failures are spurious, including qroc which does look like it's failing for some reason but it doesn't seem to be due to any never type dereferencing 🤔 https://crater-reports.s3.amazonaws.com/pr-129392-1/try%2319c032689f1eaa3070436eefb48228917adf58fa/reg/qroc-0.1.0/log.txt If it shows up again in the failure list after this retry, then I'll look a bit more into it. |
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
|
craterbot
added
S-waiting-on-review
Fixes #117288.
This PR implements a heuristic which disables two things that are currently being performed on the HIR when we have expressions that involve place-like expressions that point to
!. Specifically, it will (in certain cases explained below):(1.) Disable the
NeverToAnycoercion we implicitly insert for!.Which fixes this inadvertent, sneaky unsoundness:
which is UB because currently rust emits an implicit NeverToAny coercion even though we really shouldn't be, since there's no read of the value pointed by
x.(2.) Disable the logic which considers expression which evaluate to
!to diverge, which affects the type returned by the containing block.Which fixes this unsoundness:
We disable these two operations if the expression is a place-like expression (locals, statics, field projections, index operations, and deref operations), and if the parent expression is either:
(1.) the LHS of an assignment
(2.) AddrOf
(3.) A match or let unless all of the patterns consitute a read, which is explained below:
And finally, a pattern currently is considered to constitute a read unless it is a wildcard, or an OR pattern. An OR pattern is considered to constitute a read if all of its subpatterns constitute a read, to remain as conservative as possible in cases like
_ | subpatorsubpat | _.All other patterns are considered currently to constitute a read. Specifically, because
NeverToAnyis a coercion performed on a value and not a place,Struct { .. }on a!type must be a coercion currently, and we currently rely on this behavior to allow us to perform coercions likelet _: i32 = x;wherex: !.This is already considered UB by miri, but also means it does not affect the preexisting UB in this case:
Even though it's likely up for debate since we're not actually reading any data out of the struct, it almost certainly causes inference changes which I do NOT want to fix in this PR.