move leak-check to during coherence, candidate eval

[nikomatsakis]

Implementation of MCP rust-lang/compiler-team#295.

I'd like to do a crater run on this.

Note to @rust-lang/lang: This PR is a breaking change (bugfix). It causes tests like the following to go from a future-compatibility warning #56105 to a hard error:

trait Trait {}
impl Trait for for<'a, 'b> fn(&'a u32, &'b u32) {}
impl Trait for for<'c> fn(&'c u32, &'c u32) {} // now rejected, used to warn

I am not aware of any instances of this code in the wild, but that is why we are doing a crater run. The reason for this change is that those two types are, in fact, the same type, and hence the two impls are overlapping.

There will still be impls that trigger #56105 after this lands, however -- I hope that we will eventually just accept those impls without warning, for the most part. One example of such an impl is this pattern, which is used by wasm-bindgen and other crates as well:

trait Trait {}
impl<T> Trait for fn(&T) { }
impl<T> Trait for fn(T) { } // still accepted, but warns

[rust-highfive]

r? @varkor

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

[nikomatsakis]

r? @matthewjasper

[nikomatsakis]

@bors try

[bors]

⌛ Trying commit 097b7686a20f091e5da71e83a28642237bc6c537 with merge e2aab98dd8975a0990f9fbb79b3ea4b40559a184...

[nikomatsakis]

Whoops, I just noticed that I forgot to remove those first 2 commits from the history. I'll leave it as is for now so as not to disturb the try run, but we should clean that up before landing.

[Aaron1011]

@nikomatsakis: What would it mean to accept the impl

trait Trait {}
impl<T> Trait for fn(&T) { }
impl<T> Trait for fn(T) { } // still accepted, but warns

without warnings? Would the impl<T> Trait for fn(&T) { } essentialy be ignored, or would it implicitly 'specialize' impl<T> Trait for fn(T) { } without actually using #![feature(specialization)])?

[nikomatsakis]

@Aaron1011

Neither, they would be considered distinct types. In particular, the first type fn(T) cannot ever be equal to the second type for<'a> fn(&'a U) because there is no value of T that can name 'a.

The reason for the warning, however, is that we cannot yet distinguish that type from those where implied bounds lets us figure out that 'a is equal to some other region that T could name (I give some examples in the MCP, I think).

[nikomatsakis]

I can't tell if the try build succeeded or failed :) or maybe it's still pending?

[Aaron1011]

Neither, they would be considered distinct types. In particular, the first type fn(T) cannot ever be equal to the second type for<'a> fn(&'a U) because there is no value of T that can name 'a.

I see. Does that mean that it's impossible to write a blanket impl (or collection of blanket impls) that apply to all potential fn(T)s? For example, neither impl<T> Trait for fn(&T) { } or impl<T> Trait for fn(T) { } would apply to fn((&u8, &u8))

[nikomatsakis]

@Aaron1011

Does that mean that it's impossible to write a blanket impl (or collection of blanket impls) that apply to all potential fn(T)s?

Yes, that is presently impossible. You might be interested though in this proto-proposal, which addresses some related use cases. (I've been wanting to try and move that forward, I was sort of waiting until we establish the new lang team process, though it's probably not necessary, I'd love to see someone experiment with it...)

[Mark-Simulacrum]

@craterbot run mode=check-only start=master#7f940ef5d91b53e889f111f27e00849f2f5ae4a2 end=try#e2aab98dd8975a0990f9fbb79b3ea4b40559a184

[craterbot]

👌 Experiment pr-72493 created and queued.
🔍 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

[bors]

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

[craterbot]

🚧 Experiment pr-72493 is now running

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

[lqf96]

@Aaron1011 In the long term I think this can be solved by introducing variadic lifetimes. For example, you would write something like impl<'...a, T> Trait for fn(T<'...a>) so that your implementation can apply to any type that takes zero or more lifetime parameters. The '...a lifetime parameter pack would capture all the lifetime parameters and their constraints of the T type.

[craterbot]

🎉 Experiment pr-72493 is completed!
📊 1 regressed and 2 fixed (106133 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[matthewjasper]

Regressions/fixes all look spurious.

[nikomatsakis]

@bors r=matthewjasper

[bors]

📌 Commit d57689f has been approved by matthewjasper

[QuineDot]

The lint should be updated to reflect the current direction of the language. Namely, this:

trait Trait {}
impl<T> Trait for fn(&T) { }
impl<T> Trait for fn(T) { } 

Says

  = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!

But as I understand it, the goal is to support that indefinitely.