@bors2 try @rust-timer queue

I also want to crater this since it may cause some code to fail due to new ambiguity and missing imports.

☀️ Try build successful (CI)
Build commit: 987a165 (987a165cfab916796a8315782b83ac460a651ce2, parent: fce0e74720d199eb7839fdb51af35ac5226da178)

Finished benchmarking commit (987a165): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

Max RSS (memory usage)

Cycles

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 464.993s -> 463.982s (-0.22%)
Artifact size: 377.36 MiB -> 377.47 MiB (0.03%)

@craterbot check

👌 Experiment pr-145262 created and queued.
🤖 Automatically detected try build 987a165
🔍 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

0.0% perf regression is definitely important good job perfbot 👍

This code compiles on 1.89.0. But it doesn't compile on 1.88.0, and doesn't compile with this PR. Is this intended? I don't think the test checks for this case properly.

pub mod module {
    pub trait Trait {
        fn method(&self);
    }
}

// No import of Trait
use std::ops::Deref;

pub fn foo(x: impl Deref<Target: module::Trait>) {
    x.method();
}

This code compiles on 1.89.0. But it doesn't compile on 1.88.0, and doesn't compile with this PR. Is this intended?

Yes, this is intended breakage.

The whole point of this PR is that there was a regression in 1.89 where we were accepting code that we shouldn't have. As a side effect, this regression also affects how we compute the lint for unused imports, but that's kinda the least important part of this PR.

This is the whole point of the crater run I started. I'd like to ensure that nobody is relying on this behavior.

I don't think the test checks for this case properly.

The test I included is effectively equivalent to that one, since they are both originating from the same root cause around how we treat where clauses from param types specially in method selection. Both the test I included and the one you've shared go from fail (1.88) -> pass (1.89) -> fail (this PR).

Given that there was an accidental stabilization here and that we'll be accepting a breaking change to take it back, let's go ahead and nominate for review.

@rustbot labels +I-lang-nominated

It hasn't been out there long, but it is particularly the kind of thing that would be easy for people to unknowingly rely on.

In fact, I'm going to go ahead kick off the proposed FCP because I don't suspect the crater run is going to tell us very much. The release has been out for less than a week. Most of the people who are going to accidentally rely on this probably haven't done so yet¹ -- they'll do it after we land this but before we release it and they adopt that release.

@rfcbot fcp merge

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

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

No concerns currently listed.

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!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

Interestingly, the accidental stabilization didn't cause all non-param WCs to be treated as "inherent". I'd be curious to know what defined the exact edges of this. E.g.:

use core::ops::Deref;
mod m {
    pub trait TrA { fn f(&self); }
}

fn g1<T: Deref<Target: m::TrA> + Deref<Target = U>, U>(x: &T::Target) {
    x.f(); //~ OK
}

fn g2<T: Deref<Target: m::TrA>>(x: &T::Target) {
    x.f();//~ ERROR
}

fn g3<T: Deref<Target: m::TrA> + Deref<Target = U>, U>(x: T) {
    x.f(); //~ OK
}

fn g4<T: Deref<Target: m::TrA>>(x: T) {
    x.f(); //[v1.89]~ OK
}

One would kind of expect g2 to be accepted under the same reasoning that g4 is, but it's not.

It's just autoderefs that start from param types that are treated as inherent. In g2, we do not start with a param type and deref it.

In g1 and g3, those are both inherent because we end up just with a bound on U.

Thanks, makes sense.

Neither here nor there as far as this issue goes, but it made me contemplate that one can induce inherent treatment of a trait method (in any version) with an irrelevant use of APIT, e.g.:

use core::ops::Deref;
trait Any {}
impl<T: ?Sized> Any for T {}
mod m { pub trait TrA { fn f(&self); } }
fn g5<T: Deref<Target: m::TrA> + Deref<Target = impl Any>>(x: &T::Target) {
    x.f(); //~ OK
}

I understand why that is, but it's pretty odd. It's been on my mind to maybe try to do something about this sort of thing, e.g. by indeed expanding our "inherent" treatment to non-param WC bounds. But, of course, if we do, we should do so intentionally.

As a procedural comment, I believe the types team should have been involved in the FCP.

I personally would have liked to have it be fully the responsibility of the types team given that it's a subtle detail/bug of the method selection/type checking. But it does feel user facing enough that I think T-lang also being involved is correct.

I don't think this change is significant or contentious enough to necessarily require a new FCP, so this comment only serves as a "I don't want the T-types to lose ownership of method selection going forward".