Allow coercions from never-type when ref binding is involved

[Aaron1011]

Fixes #118113

When we type-check a binding that uses ref patterns, we do not perform coercions between the expression type and the pattern type.

However, this has the unfortunate result of disallow code like: let Foo { ref my_field } = diverging_expr();. This code is accepted without the ref keyword.

We now explicitly allow coercions when the expression type is ! In all other cases, we still avoid performing coersions. This avoids causing broader changes in type-checking (with potential soundness implications for 'ref mut'),

[rustbot]

r? @compiler-errors

(rustbot has picked a reviewer for you, use r? to override)

[compiler-errors]

I've only got one concern about changing the check_expr_with_needs.

---

Can you add a test where the source type is an associated type that normalizes to !?

trait Call {
  type Out;
  fn call(self) -> Self::Out;
}

impl<F: FnOnce() -> T, T> Call for F {
  type Out = T;
  fn call(self) -> T { (self)() }
}

pub struct Foo {
    bar: u8
}

fn diverge() -> ! { todo!() }

#[allow(unused_variables)]
fn main() {
    let Foo { ref bar } = diverge.call();
}

Just so we don't make sure to subtly regress this when lazy normalization comes around.

[compiler-errors]

Huh, I wonder if this has subtle inference implications. Did you look into this?

Also, why did you add this?

[Aaron1011]

This is the call made by check_expr_coercible_to_type (which we call in the non-bindings case) - I wanted to align them more closely.

[Aaron1011]

@compiler-errors I've added an additional test

[compiler-errors]

Sorry for taking so long to get to this. I think this behavior is fine, but I don't feel comfortable approving this without some consensus. I'll start a types FCP.

@rfcbot fcp merge

See description and test for what this enables. Seems pretty clear to me.

[rfcbot]

Team member @compiler-errors has proposed to merge this. The next step is review by the rest of the tagged team members:

• @BoxyUwU
• @aliemjay
• @compiler-errors
• @jackh726
• @lcnr
• @nikomatsakis
• @oli-obk
• @spastorino

Concerns:

• coerce place vs value (Allow coercions from never-type when ref binding is involved #118270 (comment))
• want to look into always enabling coercions resolved by Allow coercions from never-type when ref binding is involved #118270 (comment)

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[oli-obk]

Suggested change

	};
	}

[BoxyUwU]

this should possibly be a lang question too as to whether we would want the behaviour to be a check for Ty::Never instead of the type being uninhabited

[lcnr]

@rfcbot concern want to look into always enabling coercions

even if there is a ref we should still detect any unsoundness in thir unsafeck or borrowck, so I don't think the original concerns still apply. Want to look into this. Specialcasing only ! here feels less than ideal

[Nadrieril]

We don't coerce uninhabited types in general today, only !; we should keep that here

[matthewjasper]

This seems the exact opposite decision to the one made in #117288 (comment)

[Nadrieril]

In my understanding, this is consistent: #117288 is about whether constructing a place of type ! should coerce, and the decision was "no". This applies because a _ pattern doesn't do anything to the place. This current issue however involves an explicit pattern, which counts as accessing the place. It's the access that triggers the NeverToAny. This is comparable to this case mentioned in #117288 (comment), where NeverToAny does happen:

let _val: () = *x; //~ Insta-UB.

[lcnr]

@rfcbot resolve want to look into always enabling coercions

while I still lack quite a lot of context here, I will sadly not get to actually spend significantly more time on this :/ relying on

In my understanding, this is consistent: #117288 is about whether constructing a place of type ! should coerce, and the decision was "no". This applies because a _ pattern doesn't do anything to the place. This current issue however involves an explicit pattern, which counts as accessing the place.

cc @RalfJung @digama0 who seem to have a lot more context here given their comments in #117288.

I think that assuming we actually convert the place to a value in the following example, I feel fairly confident that it's a step in the right direction, regardless of whether we want to also allow more general coercions if there are ref bindings in the future:

struct Foo(u8);
// This currently does not coerce but afaict will with this PR.
// Please add this as a test.
fn should_coerce_with_this_pr() {
    let ref foo: Foo = loop {};
}

// this currently coerces and afaict it should ideally
// not do so according to RalfJ in #117288.
fn already_coerces() {
    let _: Foo = loop {};
}

Can I get some confirmation whether let ref foo: Foo = expr; converts expr to a value?¹ Note that due to the ref we refer to the place from expr:

use std::cell::Cell;

fn ref_pat(x: &Cell<bool>) {
    let ref new_val_same_place = *x;
    new_val_same_place.set(true);
}

fn main() {
    let x = Cell::new(false);
    ref_pat(&x);
    assert!(x.get());
}

Footnotes

1. afaict equivalent question: should let ref x: ! = *ptr_to_never be UB? ↩

[RalfJung]

It's not at all clear to me that let ref x = expr should be constructing a value -- AFAIK it is equivalent to let x = &expr, which evaluates expr as a place only.

For let Foo { ref my_field } = expr;, that seems equivalent to let my_field = &expr.my_field. expr is in place position here so again, I don't think this constructs a value.

[RalfJung]

The example in #118113 is a bit different, since expr there is a function call expression, which is a value expression, so indeed if it has type ! then it can be coerced to anything.

But I don't think we want to accept code like this:

let raw_ptr = 16 as *const !;
let Foo { ref my_field } = *raw_ptr;

This is taking a place of type ! and trying to use it as a Foo; that's just nonsense that we should reject.

[lcnr]

@RalfJung to make sure I understand you correctly:

First a question: is there currently a consensus whether a place expression of type ! is unreachable? Afaict the consensus is that it is not. A place of type ! is totally acceptable and only causes UB when converted to a value. let _ = *(&1 as *const u32 as *const !); is not UB.

It's not at all clear to me that let ref x = expr should be constructing a value -- AFAIK it is equivalent to let x = &expr, which evaluates expr as a place only.

For let Foo { ref my_field } = expr;, that seems equivalent to let my_field = &expr.my_field. expr is in place position here so again, I don't think this constructs a value.

You believe that let ref var = place_expr does not convert place_expr to a value and neither does let Foo { ref my_field } = *raw_ptr;.

You therefore believe that the PR as is is not desirable. We should only apply coercions from ! to any other type if the source is an expression, not a place?

Assuming that 1) we have not yet decided that a place of type ! is UB (or even decided the opposite) and 2) this PR would result in coercions from places to pattern to be unique for ! by adding a place_to_value conversion not present for other types:

@rfcbot concern coerce place vs value

I am not confident that I fully understand what is going on here and how this fits into the greater picture, but I would like to definitely involve @rust-lang/opsem to make sure the behavior here is consistent

[digama0]

I believe @RalfJung and I were in agreement on #117288 that constructing places of type ! should not be UB, and the lang team conclusion at the end is also in accordance with this, but AFAIK there has been no FCP about it. (The disagreement was about whether coercions can be applied to never places, which necessarily require place-to-value conversion and therefore cause UB.) The lang team conclusion seems to be in favor of removing coercions on never places completely, meaning you need { *x } to get a value before coercions trigger. This is not consistent with current stable behavior, so this may need to be done over an edition boundary.

As it relates to this PR, I think it's okay provided that it only triggers on expressions that are already value expressions (like diverging_expr()). Triggering on *x where x: *const ! would be exactly the opposite of the conclusion in #117288.

[RalfJung]

A place of type ! is totally acceptable and only causes UB when converted to a value. let _ = *(&1 as *const u32 as *const !); is not UB.

I think that is the general opinion in @rust-lang/opsem, yes.

You therefore believe that the PR as is is not desirable. We should only apply coercions from ! to any other type if the source is an expression, not a place?

If the source is a value expression. Yes.

[bors]

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

[compiler-errors]

Having re-familiarized myself with #117288 and the current hazards the HIR typeck introduces due to its not distinguishing place and value expressions sufficiently w.r.t. divergence, never coercions, and pointers-to-never, I'd rather we close this until we figure out that story first.

[lcnr]

given the blocking concern and that quite a lot of time elapsed since first opening this PR, I agree with @compiler-errors and am going to cancel the FCP for now. Given the concern by @compiler-errors I would like us to also properly figure out how to handle the place/value distinction in general before landing this change.

@rfcbot cancel

[rfcbot]

@lcnr proposal cancelled.

[alex-semenyuk]

@lcnr
From wg-triage. Are we interesting in this PR? if so might make sense to move on blocking status?

[lcnr]

This PR is blocked on only allowing coercions from values, but it should be possible to implement this rn 🤔 i think waiting-on-author is correct here