Special-case raw ref op for diverging check in HIR typeck#129371
Special-case raw ref op for diverging check in HIR typeck#129371compiler-errors wants to merge 1 commit intorust-lang:masterfrom
Conversation
|
r? @davidtwco rustbot has assigned @davidtwco. Use |
rustbot
added
S-waiting-on-review
|
cc @RalfJung, do you have a strong opinion about this? It seems a bit unfortunate that this is a special case, but raw ref is kinda special anyways. |
| #![feature(never_type)] | ||
|
|
||
| fn make_up_a_value<T>() -> T { | ||
| unsafe { |
There was a problem hiding this comment.
After #129248, we can take away the unsafe {} so it's especially important this turn into an error :)
|
If only the HIR had already made loads explicit... or would that actually handle this? |
|
@workingjubilee: It would probably simplify things a bit, though those loads being explicit would probably make approximately everything else in the HIR much harder to deal with (like dealing w the explicit scopes in THIR, lol). I think the real problem here is that divergence analysis is just a hot mess in HIR typeck. |
…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`
|
I think ideally we would treat all place expressions that we do not load from like this: so also LHS of assignments, and & for references.
Assignments always come with a value expression on the right so there it likely makes no difference. But shouldn't we treat &raw and & the same?
|
| let _: ! = *x; | ||
| // Since `*x` "diverges" in HIR, but doesn't count as a read in MIR, this | ||
| // is unsound since we act as if it diverges but it doesn't. | ||
| } |
There was a problem hiding this comment.
Can you make the return type of this function !? I think that makes it much clearer that we're testing for "HIR detecting divergence" here.
| &raw const *x; | ||
| // Since `*x` is `!`, HIR typeck used to think that it diverges | ||
| // and allowed the block to coerce to any value, leading to UB. | ||
| } |
There was a problem hiding this comment.
Same here, a return type of ! would be more clear I think.
That list is based on MIR; I don't know what the full list is for HIR. #129392 mentions some candidates. |
|
@RalfJung: Doesn't the LHS of an assignment constitute a load? We still have to drop the old value stored in that place, right? |
|
Hm... a drop happens in-place though, via |
…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`
…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`
davidtwco
left a comment
davidtwco
left a comment
There was a problem hiding this comment.
This makes sense to me, r=me after addressing Ralf's comments unless you want to make a bigger change along the lines of Ralf's suggestion.
|
No need to special case this. I'll implement the "right" way of doing it here: #129392 |
5 participants
See #117288.
&raw const EXPRdoes not constitute a read ofEXPR, so ifEXPR's type is!, then we don't want to consider that expression diverging. This was unsound when paired with #129248.This remains unsound for things like matches
let _: ! = *x;, but I wanted to add a test for it.