correctly dedup ExistentialPredicates

[lcnr]

fixes the underlying issue of the regression in #59326 and removes the stopgap added in #73485.

r? @nikomatsakis cc @estebank

[lcnr]

I think this test changes because ExistentialPredicate::stable_cmp doesn't actually look at its substs. This seems kind of incorrect to me 🤔

[estebank]

I think here we need to look for conflicting existential_projections and emit an error like E0271. I think the change to dedup below makes it consider two projections on the same associated type but with different values to be the same.

I would also imagine that if you try this code out you will also get a "successful" compilation.

[lcnr]

I would also imagine that if you try this code out you will also get a "successful" compilation.

That code compiles correctly errors, added it as a test. I still fear that the current dedup ends up being unsound though 🤔

[estebank]

I see what's happening. In the type test that is now passing we probably shouldn't allow people to override the type params that are set in the type, and error when that happens. @nikomatsakis might have better insight on what we should continue to do, but the more I look at how we treat type aliases the more I want to revamp them.

(I failed to publish this back on the 28th)

[nikomatsakis]

Why do you think it's unsound?

[lcnr]

List<ExistentialPredicate> for dyn A + B previously was [Trait(A), Trait(B)] and this PR changes this to just [Trait(A)]. This made me slightly worried as I wasn't sure how we handle wf for trait objects.

I spend a bit more time looking at this and strongly believe that this PR is not concerning here
as we emit the errors before using stable_cmp, so anything we may "incorrectly" dedup here
already caused an error.

Added a comment summarizing my current understanding

[nikomatsakis]

Huh, I guess I better look at how stable_cmp is defined.

[nikomatsakis]

OK, yeah, this seems like not what I expected. =)

[lcnr]

Okay, so I've rebased this, I think the current state is sound, if somewhat 👻 spooky 👻 Not
sure what's the least invasive way to clean this up though...

[nikomatsakis]

Sorry, I'll try to give useful feedback as soon as I can ...

[bors]

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

[bors]

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

[bors]

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

[crlf0710]

Ping from triage, there's merge conflicts now.

[lcnr]

This is the only relevant test which fails if we check that stable_cmp doesn't remove unique predicates:

// check-pass
trait Service {
    type S;
}

trait Framing {
    type F;
}

impl Framing for () {
    type F = ();
}

trait HttpService<F: Framing>: Service<S = F::F> {}

type BoxService = Box<dyn HttpService<(), S = ()>>;

fn build_server<F: FnOnce() -> BoxService>(_: F) {}

fn make_server<F: Framing>() -> Box<dyn HttpService<F, S = F::F>> {
    unimplemented!()
}

fn main() {
    build_server(|| make_server())
}

It fails with

error: internal compiler error: unexpected existential predicates: ExistentialProjection { item_def_id: DefId(0:4 ~ issue_59326_compile_run[317d]::Service[0]::S[0]), substs: [], ty: <() as Framing>::F }, ExistentialProjection { item_def_id: DefId(0:4 ~ issue_59326_compile_run[317d]::Service[0]::S[0]), substs: [], ty: () }
   |
   = note: delayed at compiler/rustc_middle/src/ty/sty.rs:695:30

i.e. one of the predicates is not normalized enough.

Tbh I think that we should slowly start to emit a future compat lint and then error here, as dyn HttpService should not be able set the associated type of Service here IMO, considering that it is already set once by HttpService itself.

IMO we are getting dangerously close to being unsound here:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=20ba57f1c6c3bcca62b495a871ff3743

trait Foo {
    type F;
}

impl<T> Foo for T {
    type F = T;
}

trait Bar: Foo<F = u32> {}
impl<T: Foo<F = u32>> Bar for T {}

fn main() {
    let y: i32 = 7;
    let z: Box<dyn Bar<F = i32>> = Box::new(y);
}

luckily results in

error[E0271]: type mismatch resolving `<i32 as Foo>::F == u32`
  --> src/main.rs:14:36
   |
14 |     let z: Box<dyn Bar<F = i32>> = Box::new(y);
   |                                    ^^^^^^^^^^^ expected `u32`, found `i32`
   |
   = note: required for the cast to the object type `dyn Bar<F = i32, F = u32>`

[bors]

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[lcnr]

closing this for now, I feel like we probably should instead try to change the way existential types are handled in a more fundamental way which means that keeping this PR open isn't really too helpful.