Skip to content

Conversation

compiler-errors
Copy link
Member

See #117288.

&raw const EXPR does not constitute a read of EXPR, so if EXPR'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.

@rustbot
Copy link
Collaborator

rustbot commented Aug 21, 2024

r? @davidtwco

rustbot has assigned @davidtwco.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 21, 2024
@compiler-errors
Copy link
Member Author

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 {
Copy link
Member Author

Choose a reason for hiding this comment

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

After #129248, we can take away the unsafe {} so it's especially important this turn into an error :)

@workingjubilee
Copy link
Member

If only the HIR had already made loads explicit... or would that actually handle this?

@compiler-errors
Copy link
Member Author

@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.

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 22, 2024
…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`
@RalfJung
Copy link
Member

RalfJung commented Aug 22, 2024 via email

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.
}
Copy link
Member

Choose a reason for hiding this comment

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

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.
}
Copy link
Member

Choose a reason for hiding this comment

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

Same here, a return type of ! would be more clear I think.

@RalfJung
Copy link
Member

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.

That list is based on MIR; I don't know what the full list is for HIR. #129392 mentions some candidates.

@compiler-errors
Copy link
Member Author

@RalfJung: Doesn't the LHS of an assignment constitute a load? We still have to drop the old value stored in that place, right?

@RalfJung
Copy link
Member

Hm... a drop happens in-place though, via drop_in_place. No value is constructed.

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 22, 2024
…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 added a commit to rust-lang-ci/rust that referenced this pull request Aug 22, 2024
…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`
Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

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.

@compiler-errors
Copy link
Member Author

No need to special case this. I'll implement the "right" way of doing it here: #129392

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants