correctly handle normalization in implied bounds query

[aliemjay]

One subtle bug with the implied_outlives_bounds is that it adds implied bounds from trait impls. Example:

trait Trait { type Ty; }
impl<T: 'static> Trait for T { type Ty = (); }
// implied_outlives_bounds(Vec<<T as Trait>::Ty>) = [T: 'static,]
// note Vec<_> is necessary 

This is not a user-visible bug only because wfcheck currently computes implied bounds for fully normalized types only. (edit: this is not correct unfortunately. See #109628) The only place where we use implied bounds for unnormalized types during HIR analysis is in compare_predicate_entailment.

This PR fixes this by using wf::unnormalized_obligations instead of wf::obligations.

Fixes #109628.
Fixes #109799.

Additional changes :

• rework to handle inference vars and normalization more explicitly and rigorously.
• add implied bounds from the input types after normalization.

r? @lcnr

[lcnr]

that's really cool 👍 i remember that we still have that issue during borrowck

use std::any::Any;

trait Trait {
    type Assoc;
}

impl<T: 'static> Trait for T {
    type Assoc = &'static T; 
}

fn implies_t_static<T>(t: T, x: <T as Trait>::Assoc) -> Box<dyn Any> {
    Box::new(t)
}

I forgot why exactly we had to add both the normalized and the unnormalized signature types for implied bounds, maybe that was also an issue solved by opportunistically resolving region vars in the query response?

why is this still a draft?

[aliemjay]

i remember that we still have that issue during borrowck

@lcnr I breaked this code in 00a80b0 only to assess the impact with crater. That said, this only breaks borrowck -- we still accept this in wfcheck:

trait Trait {
    type Assoc;
}

impl<T: 'static> Trait for Box<T> {
    type Assoc = &'static T; 
}

impl<T> Trait for (T, <Box<T> as Trait>::Assoc) {
    type Assoc = &'static T; // We can still assume T: 'static by wfcheck
}

[workingjubilee]

@bors try

[bors]

⌛ Trying commit 00a80b04fd4235d74626d6e7071e4c58803cbd2f with merge 78392dafb07418956e6952e68a24a94edd0c8dc6...

[bors]

☀️ Try build successful - checks-actions
Build commit: 78392dafb07418956e6952e68a24a94edd0c8dc6 (78392dafb07418956e6952e68a24a94edd0c8dc6)

[lcnr]

@craterbot check

[craterbot]

👌 Experiment pr-109482 created and queued.
🤖 Automatically detected try build 78392dafb07418956e6952e68a24a94edd0c8dc6
🔍 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

[craterbot]

🚧 Experiment pr-109482 is now running

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

[craterbot]

🎉 Experiment pr-109482 is completed!
📊 2002 regressed and 3 fixed (259262 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

[bors]

☀️ Try build successful - checks-actions
Build commit: 38d217432297792fdf0e298115e9fe95b714ee8f (38d217432297792fdf0e298115e9fe95b714ee8f)

[compiler-errors]

@craterbot run mode=check-only crates=https://crater-reports.s3.amazonaws.com/pr-109482/retry-regressed-list.txt

[craterbot]

👌 Experiment pr-109482-1 created and queued.
🤖 Automatically detected try build 38d217432297792fdf0e298115e9fe95b714ee8f
🔍 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

[craterbot]

🚧 Experiment pr-109482-1 is now running

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

[craterbot]

🎉 Experiment pr-109482-1 is completed!
📊 428 regressed and 0 fixed (2002 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

[lcnr]

We currently consider bounds only on impls used during normalization while computing implied bounds. This means that removing bounds on impls is a theoretical breaking change.

This PR causes us to only consider implied bounds which are from WF requirements directly and stops considering bounds from normalization. This allows users to safely remove (region) bounds from their impls.

The following code currently compiles but will break with this change:

use core::any::Any;

trait Trait {
    type Assoc;
}

impl<X: 'static> Trait for Vec<X> {
    type Assoc = ();
}

struct Foo<T: Trait>(T)
where
    T::Assoc: Clone; // any predicate using `T::Assoc` works here

fn foo<X>(x: X, _: Foo<Vec<X>>) -> Box<dyn Any> {
    Box::new(x) // gets to assume `X: 'static` from the bound on the impl
}

This is relied on by 2 crates:

• https://github.com/Syndim/actix-mqtt-client
• https://github.com/bevyengine/bevy

This breakage is fixing a previously underspecified part of the language and is therefore acceptable. I think we should open PRs for both of these crates as soon as possible, fixing this issue. Maybe even asking the bevy devs to emit point releases for some previous versions, and then land this at the start of the next nightly cycle: April 14th.

going to start a types FCP after we have the PRs up to fix these 2 crates

[lcnr]

alright, bevy is very annoying to fix. We would have to add 'static bounds to a lot of structs and that also leaks to crates using bevy, so users may also be required to add 'static.

Considering that region bounds on trait definitions are already used as implied bounds somewhere?

use std::any::Any;

trait Trait: 'static {}

fn foo<T: Trait>(x: T) -> Box<dyn Any> {
    Box::new(x)
}

Maybe we should apply this consistently to trait predicates in implied bounds, looking at the nested predicates of trait bounds and considering the outlives predicates... not sure why that isn't currently happening/what exactly is happening here.

With this we should only have to add a 'static bound to trait WorldQuery to bevy and get it implied everywhere else.

[aliemjay]

closing in favor of the lint version #109763.