Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regression in clone impl of enum with variant that has a field with a bivariant substitution #129286

Closed
compiler-errors opened this issue Aug 20, 2024 · 3 comments · Fixed by #129317
Assignees
Labels
A-coercions Area: implicit and explicit `expr as Type` coercions A-variance Area: Variance (https://doc.rust-lang.org/nomicon/subtyping.html) C-bug Category: This is a bug. P-medium Medium priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-types Relevant to the types team, which will review and decide on the PR/issue.

Comments

@compiler-errors
Copy link
Member

compiler-errors commented Aug 20, 2024

I tried this code:

trait Array {
    type Item;
}

trait Platform {
    type Assoc: Array<Item = Self::Assoc2>;
    type Assoc2;
}

struct Message<A, B>
where
    A: Array<Item = B>,
{
    pub field: A,
}

impl<A, B> Clone for Message<A, B>
where
    A: Array<Item = B>,
{
    fn clone(&self) -> Self {
        todo!()
    }
}

fn clone<P: Platform>(x: &Message<P::Assoc, P::Assoc2>) {
    let x: Message<_, _> = Clone::clone(x);
}

I expected it to work.

Instead, this happened:

error[E0282]: type annotations needed
  --> src/lib.rs:31:32
   |
31 |     Enum::Variant(Clone::clone(x))
   |                                ^ cannot infer type for reference `&Message<<P as Platform>::Assoc, _>`

I regressed this in #129059. I have no idea yet why it's happening! This also could be minimized further, I think.

@compiler-errors compiler-errors added regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. C-bug Category: This is a bug. A-coercions Area: implicit and explicit `expr as Type` coercions A-variance Area: Variance (https://doc.rust-lang.org/nomicon/subtyping.html) T-types Relevant to the types team, which will review and decide on the PR/issue. labels Aug 20, 2024
@compiler-errors compiler-errors self-assigned this Aug 20, 2024
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Aug 20, 2024
@compiler-errors compiler-errors added P-medium Medium priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Aug 20, 2024
@compiler-errors
Copy link
Member Author

compiler-errors commented Aug 20, 2024

Weirdly, this also only happens when the Clone::clone call is being directly passed to Enum::Variant. Is this due to Expectation, perhaps?

edit: yep, it has to do with the Expectation. Minimized the code a bit more.

@compiler-errors
Copy link
Member Author

// 3. Check if the formal type is a supertype of the checked one
// and register any such obligations for future type checks
let supertype_error = self.at(&self.misc(provided_arg.span), self.param_env).sup(
DefineOpaqueTypes::Yes,
formal_input_ty,
coerced_ty,
);

lol we're not actually constraining the target of the coercion properly

@compiler-errors
Copy link
Member Author

compiler-errors commented Aug 20, 2024

Minimal:

trait Trait {
    type Item;
}

struct Struct<A: Trait<Item = B>, B> {
    pub field: A,
}

fn identity<T>(x: T) -> T {
    x
}

fn test<A: Trait<Item = B>, B>(x: &Struct<A, B>) {
    let x: &Struct<_, _> = identity(x);
}

bors added a commit to rust-lang-ci/rust that referenced this issue Aug 25, 2024
…g, r=<try>

Use equality when relating formal and expected type in arg checking

rust-lang#129059 uncovered an interesting issue in argument checking. When we check arguments, we create three sets of types:
* Formals
* Expected
* Actuals

The **actuals** are the types of the argument expressions themselves. The **formals** are the types from the signature that we're checking. The **expected** types are the formal types, but passed through `expected_inputs_for_expected_outputs`:

https://github.com/rust-lang/rust/blob/a971212545766fdfe0dd68e5d968133f79944a19/compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs#L691-L725

This method attempts to constrain the formal inputs by relating the [expectation](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_hir_typeck/expectation/enum.Expectation.html) of the call expression and the formal output.

When we check an argument, we get the expression's actual type, and then we first attempt to coerce it to the expected type:

https://github.com/rust-lang/rust/blob/a971212545766fdfe0dd68e5d968133f79944a19/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs#L280-L293

Then we subtype the expected type and the formal type:

https://github.com/rust-lang/rust/blob/a971212545766fdfe0dd68e5d968133f79944a19/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs#L299-L305

However, since we are now recording the right coercion target (since rust-lang#129059), we now end up recording the expected type to the typeck results, rather than the actual.

Since that expected type was [fudged](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_infer/infer/struct.InferCtxt.html#method.fudge_inference_if_ok), it has fresh variables. And since the expected type is only subtyped against the formal type, if that expected type has a bivariant parameter, it will likely remain unconstrained since `Covariant * Bivariant = Bivariant` according to [xform](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/enum.Variance.html#method.xform). This leads to an unconstrained type variable in writeback.

AFAICT, there's no reason for us to be using subtyping here, though. The expected output is already related to the expectation by subtyping:

https://github.com/rust-lang/rust/blob/a971212545766fdfe0dd68e5d968133f79944a19/compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs#L713

So the formals don't need "another" indirection of subtyping in the argument checking... So I've changed it to use equality here. We could alternatively fix this by requiring WF for all the expected types to constrain their bivariant parameters, but this seems a bit overkill.

Fixes rust-lang#129286
bors added a commit to rust-lang-ci/rust that referenced this issue Aug 25, 2024
…g, r=<try>

Use equality when relating formal and expected type in arg checking

rust-lang#129059 uncovered an interesting issue in argument checking. When we check arguments, we create three sets of types:
* Formals
* Expected
* Actuals

The **actuals** are the types of the argument expressions themselves. The **formals** are the types from the signature that we're checking. The **expected** types are the formal types, but passed through `expected_inputs_for_expected_outputs`:

https://github.com/rust-lang/rust/blob/a971212545766fdfe0dd68e5d968133f79944a19/compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs#L691-L725

This method attempts to constrain the formal inputs by relating the [expectation](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_hir_typeck/expectation/enum.Expectation.html) of the call expression and the formal output.

When we check an argument, we get the expression's actual type, and then we first attempt to coerce it to the expected type:

https://github.com/rust-lang/rust/blob/a971212545766fdfe0dd68e5d968133f79944a19/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs#L280-L293

Then we subtype the expected type and the formal type:

https://github.com/rust-lang/rust/blob/a971212545766fdfe0dd68e5d968133f79944a19/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs#L299-L305

However, since we are now recording the right coercion target (since rust-lang#129059), we now end up recording the expected type to the typeck results, rather than the actual.

Since that expected type was [fudged](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_infer/infer/struct.InferCtxt.html#method.fudge_inference_if_ok), it has fresh variables. And since the expected type is only subtyped against the formal type, if that expected type has a bivariant parameter, it will likely remain unconstrained since `Covariant * Bivariant = Bivariant` according to [xform](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/enum.Variance.html#method.xform). This leads to an unconstrained type variable in writeback.

AFAICT, there's no reason for us to be using subtyping here, though. The expected output is already related to the expectation by subtyping:

https://github.com/rust-lang/rust/blob/a971212545766fdfe0dd68e5d968133f79944a19/compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs#L713

So the formals don't need "another" indirection of subtyping in the argument checking... So I've changed it to use equality here. We could alternatively fix this by requiring WF for all the expected types to constrain their bivariant parameters, but this seems a bit overkill.

Fixes rust-lang#129286
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 2, 2024
…g, r=lcnr

Use equality when relating formal and expected type in arg checking

rust-lang#129059 uncovered an interesting issue in argument checking. When we check arguments, we create three sets of types:
* Formals
* Expected
* Actuals

The **actuals** are the types of the argument expressions themselves. The **formals** are the types from the signature that we're checking. The **expected** types are the formal types, but passed through `expected_inputs_for_expected_outputs`:

https://github.com/rust-lang/rust/blob/a971212545766fdfe0dd68e5d968133f79944a19/compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs#L691-L725

This method attempts to constrain the formal inputs by relating the [expectation](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_hir_typeck/expectation/enum.Expectation.html) of the call expression and the formal output.

When we check an argument, we get the expression's actual type, and then we first attempt to coerce it to the expected type:

https://github.com/rust-lang/rust/blob/a971212545766fdfe0dd68e5d968133f79944a19/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs#L280-L293

Then we subtype the expected type and the formal type:

https://github.com/rust-lang/rust/blob/a971212545766fdfe0dd68e5d968133f79944a19/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs#L299-L305

However, since we are now recording the right coercion target (since rust-lang#129059), we now end up recording the expected type to the typeck results, rather than the actual.

Since that expected type was [fudged](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_infer/infer/struct.InferCtxt.html#method.fudge_inference_if_ok), it has fresh variables. And since the expected type is only subtyped against the formal type, if that expected type has a bivariant parameter, it will likely remain unconstrained since `Covariant * Bivariant = Bivariant` according to [xform](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/enum.Variance.html#method.xform). This leads to an unconstrained type variable in writeback.

AFAICT, there's no reason for us to be using subtyping here, though. The expected output is already related to the expectation by subtyping:

https://github.com/rust-lang/rust/blob/a971212545766fdfe0dd68e5d968133f79944a19/compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs#L713

So the formals don't need "another" indirection of subtyping in the argument checking... So I've changed it to use equality here. We could alternatively fix this by requiring WF for all the expected types to constrain their bivariant parameters, but this seems a bit overkill.

Fixes rust-lang#129286
@bors bors closed this as completed in bd53aa3 Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-coercions Area: implicit and explicit `expr as Type` coercions A-variance Area: Variance (https://doc.rust-lang.org/nomicon/subtyping.html) C-bug Category: This is a bug. P-medium Medium priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants