change method resolution to constrain hidden types instead of rejecting method candidates

[oli-obk]

Some of these are in probes and may affect inference. This is therefore a breaking change.

This allows new code to compile on stable:

trait Trait {}

impl Trait for u32 {}

struct Bar<T>(T);

impl Bar<u32> {
    fn foo(self) {}
}

fn foo(x: bool) -> Bar<impl Sized> {
    if x {
        let x = foo(false);
        x.foo();
        //^ this used to not find the `foo` method, because while we did equate `x`'s type with possible candidates, we didn't allow opaque type inference while doing so
    }
    todo!()
}

r? @compiler-errors

fixes #121404

cc #116652

[oli-obk]

@rfcbot merge

This allows new code to compile on stable. It is a necessary step towards making the old solver more similar to the new solver (and subsequently unblocking stabilizing impl-trait-in-associated-types). See the PR's main post for an example of the new kind of code that is now accepted

[rfcbot]

Team member @oli-obk 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:

• unexpected-self-constraint resolved by change method resolution to constrain hidden types instead of rejecting method candidates #123962 (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.

[compiler-errors]

This causes (TAIT) code to break, like what caused that CI failure. Pls update the FCP to include an example of what becomes ambiguous during method probe.

[lcnr]

please also add a revision for -Znext-solver to the added tests

[lcnr]

Suggested change

	//! check that we do not unify `Bar<Foo>` with `BAr<u32>`, even though the
	//! check that we do not unify `Bar<Foo>` with `Bar<u32>`, even though the

[lcnr]

not resolved, also this comment feels outdated 🤔 isn't the cause here that the Bar<u32> impl does not define Foo?

[oli-obk]

I added more tests to cover the case where it actually defines Foo

[jackh726]

But it is also a breaking change, since &self and &mut self method calls on recursive RPIT function calls now constrain the hidden type to &_ and &mut _ respectively. This is not what users really expect or want from this, but there's way around this.

This is concerning to me and I would prefer us to outright error of ambiguity here than constrain incorrectly.

[oli-obk]

This is concerning to me and I would prefer us to outright error of ambiguity here than constrain incorrectly.

That's not easy to do in the old solver, and likely impossible to do without major downsides. While we could attempt to move the initial method probing into a canonical query, that would mess with inference as we would not have access to the trait bounds on infer vars anymore.

Note that ne new solver handles this correctly

[oli-obk]

@bors try

[bors]

⌛ Trying commit af980d5 with merge fe30d25...

[jackh726]

This is concerning to me and I would prefer us to outright error of ambiguity here than constrain incorrectly.

That's not easy to do in the old solver, and likely impossible to do without major downsides. While we could attempt to move the initial method probing into a canonical query, that would mess with inference as we would not have access to the trait bounds on infer vars anymore.

Note that ne new solver handles this correctly

The test I'm focusing on here is tests/ui/impl-trait/method-resolution4.rs. Today, this works exactly as I would expect on stable. With both this PR and with -Znext-solver, it fails. So, this is not a case where I think the new solver behaves as expected. (Similar with tests/ui/type-alias-impl-trait/method_resolution_trait_method_from_opaque.rs).

The problem I have is: if we make this change and break these cases today, we can't in the future unbreak these cases in the future in a backwards-compatible manner I think, even though as we've both said this is not what users expect.

I think it would be good if you can make a separate PR just adding the tests, so we can more clearly see what would be changing in the future.

[jackh726]

I actually do feel strongly about this, so I'm going to go ahead and raise a concern.

@rfcbot concern unexpected-self-constraint

[bors]

☀️ Try build successful - checks-actions
Build commit: fe30d25 (fe30d255593ff5176e4ada5fb82b6b6276de3f85)

[oli-obk]

@craterbot check

[craterbot]

👌 Experiment pr-123962 created and queued.
🤖 Automatically detected try build fe30d25
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[oli-obk]

@bors r=lcnr

[bors]

📌 Commit 9cf60ee has been approved by lcnr

It is now in the queue for this repository.