correctly dedup `ExistentialPredicate`s #73815

lcnr · 2020-06-27T21:14:05Z

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

lcnr · 2020-06-27T21:18:33Z

src/test/ui/associated-types/associated-types-overridden-binding-2.rs

 #![feature(trait_alias)]

 trait I32Iterator = Iterator<Item = i32>;

 fn main() {
    let _: &dyn I32Iterator<Item = u32> = &vec![42].into_iter();
-    //~^ ERROR type mismatch


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

estebank · 2020-06-27T22:12:47Z

src/librustc_typeck/astconv.rs

@@ -1814,7 +1815,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
            )


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.

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 🤔

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)

Why do you think it's unsound?

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

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

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

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...

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

bors · 2020-07-01T19:14:54Z

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

bors · 2020-07-06T03:35:08Z

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

bors · 2020-08-22T02:42:57Z

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

crlf0710 · 2020-09-12T02:24:42Z

Ping from triage, there's merge conflicts now.

lcnr · 2020-09-18T21:02:22Z

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 · 2020-09-20T13:08:49Z

☔ 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 · 2020-10-06T13:34:38Z

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.

rust-highfive assigned nikomatsakis Jun 27, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 27, 2020

lcnr commented Jun 27, 2020

View reviewed changes

estebank reviewed Jun 27, 2020

View reviewed changes

lcnr force-pushed the existential-pred branch 2 times, most recently from 93a48f6 to 7d09fdb Compare June 29, 2020 19:34

lcnr force-pushed the existential-pred branch from 7d09fdb to edb65da Compare July 2, 2020 16:30

lcnr force-pushed the existential-pred branch from edb65da to 0f5e9f3 Compare July 6, 2020 07:07

crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 24, 2020

crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 21, 2020

crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 12, 2020

lcnr added 4 commits September 18, 2020 20:57

correctly dedup ExistentialPredicates

5ced845

add ExistentialPredicate compile fail test

c8dba64

add note to ExistentialPRedicate::stable_cmp

7d12e5f

assert previously implicit invariants in stable_cmp

9818bda

lcnr force-pushed the existential-pred branch from 0f5e9f3 to 9818bda Compare September 18, 2020 19:24

lcnr closed this Oct 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

correctly dedup `ExistentialPredicate`s #73815

correctly dedup `ExistentialPredicate`s #73815

lcnr commented Jun 27, 2020 •

edited

Loading

lcnr Jun 27, 2020 •

edited

Loading

estebank Jun 27, 2020

lcnr Jun 28, 2020 •

edited

Loading

estebank Jun 28, 2020

nikomatsakis Jun 29, 2020

lcnr Jun 29, 2020 •

edited

Loading

nikomatsakis Jun 29, 2020

nikomatsakis Jun 29, 2020

lcnr Jul 2, 2020 •

edited

Loading

nikomatsakis Jul 9, 2020

bors commented Jul 1, 2020

bors commented Jul 6, 2020

bors commented Aug 22, 2020

crlf0710 commented Sep 12, 2020

lcnr commented Sep 18, 2020 •

edited

Loading

bors commented Sep 20, 2020

lcnr commented Oct 6, 2020

		@@ -1814,7 +1815,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
		)

correctly dedup ExistentialPredicates #73815

correctly dedup ExistentialPredicates #73815

Conversation

lcnr commented Jun 27, 2020 • edited Loading

lcnr Jun 27, 2020 • edited Loading

Choose a reason for hiding this comment

estebank Jun 27, 2020

Choose a reason for hiding this comment

lcnr Jun 28, 2020 • edited Loading

Choose a reason for hiding this comment

estebank Jun 28, 2020

Choose a reason for hiding this comment

nikomatsakis Jun 29, 2020

Choose a reason for hiding this comment

lcnr Jun 29, 2020 • edited Loading

Choose a reason for hiding this comment

nikomatsakis Jun 29, 2020

Choose a reason for hiding this comment

nikomatsakis Jun 29, 2020

Choose a reason for hiding this comment

lcnr Jul 2, 2020 • edited Loading

Choose a reason for hiding this comment

nikomatsakis Jul 9, 2020

Choose a reason for hiding this comment

bors commented Jul 1, 2020

bors commented Jul 6, 2020

bors commented Aug 22, 2020

crlf0710 commented Sep 12, 2020

lcnr commented Sep 18, 2020 • edited Loading

bors commented Sep 20, 2020

lcnr commented Oct 6, 2020

correctly dedup `ExistentialPredicate`s #73815

correctly dedup `ExistentialPredicate`s #73815

lcnr commented Jun 27, 2020 •

edited

Loading

lcnr Jun 27, 2020 •

edited

Loading

lcnr Jun 28, 2020 •

edited

Loading

lcnr Jun 29, 2020 •

edited

Loading

lcnr Jul 2, 2020 •

edited

Loading

lcnr commented Sep 18, 2020 •

edited

Loading