Allow Trait inheritance with cycles on associated types take 2 #80732

spastorino · 2021-01-05T20:17:35Z

This reverts the revert of #79209 and fixes the ICEs that's occasioned by that PR exposing some problems that are addressed in #80648 and #79811.
For easier review I'd say, check only the last commit, the first one is just a revert of the revert of #79209 which was already approved.

This also could be considered part or the actual fix of #79560 but I guess for that to be closed and fixed completely we would need to land #80648 and #79811 too.

r? @nikomatsakis
cc @Aaron1011

jyn514 · 2021-01-05T20:27:20Z

@bors try

So people can easily install this with https://github.com/kennytm/rustup-toolchain-install-master (#79560 (comment))

bors · 2021-01-05T20:27:31Z

⌛ Trying commit 958a97b56a7cfdaa35b032fbaa49385c5d08dfe8 with merge c963187c6f959417cbb13a33e9eaea4607696fc4...

spastorino · 2021-01-05T20:37:48Z

@jyn514 yeah that's great. I've force pushed but all I've changed there is the test I've added to be check-pass instead of build-pass, so don't worry about that.

spastorino · 2021-01-06T16:21:44Z

@nikomatsakis, now that this is tested, should be ready for review/merge.

nikomatsakis · 2021-01-16T09:44:06Z

Just to make sure I understand the situation:

changing from Ident to Symbol basically works around the hygiene problems we encountered
but once Add disambiugator to ExpnData #79811 lands, we wouldn't have had those problems in the first place?

Is there a test for the new ICEs we encountered? I couldn't quite tell.

If all that is correct, r=me.

@bors delegate+

bors · 2021-01-16T09:44:08Z

✌️ @spastorino can now approve this pull request

spastorino · 2021-01-16T14:06:27Z

@nikomatsakis what you've said is correct. About the test check the second commit (the one that's not the revert) and at the end you'll find the test.

spastorino · 2021-01-16T14:09:21Z

@bors r=nikomatsakis

bors · 2021-01-16T14:09:22Z

📌 Commit 455a0e1 has been approved by nikomatsakis

… r=nikomatsakis Allow Trait inheritance with cycles on associated types take 2 This reverts the revert of rust-lang#79209 and fixes the ICEs that's occasioned by that PR exposing some problems that are addressed in rust-lang#80648 and rust-lang#79811. For easier review I'd say, check only the last commit, the first one is just a revert of the revert of rust-lang#79209 which was already approved. This also could be considered part or the actual fix of rust-lang#79560 but I guess for that to be closed and fixed completely we would need to land rust-lang#80648 and rust-lang#79811 too. r? `@nikomatsakis` cc `@Aaron1011`

lnicola · 2021-01-17T11:48:30Z

@jyn514 what commit should I use with rustup-toolchain-install-master? Neither 03c0e58 nor 455a0e1 work.

m-ou-se · 2021-01-17T11:57:16Z

@lnicola That only works for commits that have been built by bors. Try the commits mentioned in #80732 (comment).

lnicola · 2021-01-17T12:01:15Z

@spastorino this introduces an error while compiling salsa:

error[E0221]: ambiguous associated type `Group` in bounds of `Q`
   --> src/interned.rs:466:57
    |
466 |             <<Q as QueryDb<'_>>::DynDb as HasQueryGroup<Q::Group>>::group_storage(db);
    |                                                         ^^^^^^^^ ambiguous associated type `Group`
    | 
   ::: src/lib.rs:431:5
    |
431 |     type Group: plumbing::QueryGroup<GroupStorage = Self::GroupStorage>;
    |     --------------------------------------------------------------------
    |     |
    |     ambiguous `Group` from `for<'d> QueryDb<'d>`
    |     ambiguous `Group` from `for<'d> QueryDb<'d>`
    |
help: use fully qualified syntax to disambiguate
    |
466 |             <<Q as QueryDb<'_>>::DynDb as HasQueryGroup<<Q as for<'d> QueryDb<'d>>::Group>>::group_storage(db);
    |                                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: use fully qualified syntax to disambiguate
    |
466 |             <<Q as QueryDb<'_>>::DynDb as HasQueryGroup<<Q as for<'d> QueryDb<'d>>::Group>>::group_storage(db);
    |                                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0221]: ambiguous associated type `Group` in bounds of `Q`
   --> src/interned.rs:478:57
    |
478 |             <<Q as QueryDb<'_>>::DynDb as HasQueryGroup<Q::Group>>::group_storage(db);
    |                                                         ^^^^^^^^ ambiguous associated type `Group`
    | 
   ::: src/lib.rs:431:5
    |
431 |     type Group: plumbing::QueryGroup<GroupStorage = Self::GroupStorage>;
    |     --------------------------------------------------------------------
    |     |
    |     ambiguous `Group` from `for<'d> QueryDb<'d>`
    |     ambiguous `Group` from `for<'d> QueryDb<'d>`
    |
help: use fully qualified syntax to disambiguate
    |
478 |             <<Q as QueryDb<'_>>::DynDb as HasQueryGroup<<Q as for<'d> QueryDb<'d>>::Group>>::group_storage(db);
    |                                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: use fully qualified syntax to disambiguate
    |
478 |             <<Q as QueryDb<'_>>::DynDb as HasQueryGroup<<Q as for<'d> QueryDb<'d>>::Group>>::group_storage(db);
    |                                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0221]: ambiguous associated type `Group` in bounds of `Q`
   --> src/interned.rs:490:57
    |
490 |             <<Q as QueryDb<'_>>::DynDb as HasQueryGroup<Q::Group>>::group_storage(db);
    |                                                         ^^^^^^^^ ambiguous associated type `Group`
    | 
   ::: src/lib.rs:431:5
    |
431 |     type Group: plumbing::QueryGroup<GroupStorage = Self::GroupStorage>;
    |     --------------------------------------------------------------------
    |     |
    |     ambiguous `Group` from `for<'d> QueryDb<'d>`
    |     ambiguous `Group` from `for<'d> QueryDb<'d>`
    |
help: use fully qualified syntax to disambiguate
    |
490 |             <<Q as QueryDb<'_>>::DynDb as HasQueryGroup<<Q as for<'d> QueryDb<'d>>::Group>>::group_storage(db);
    |                                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: use fully qualified syntax to disambiguate
    |
490 |             <<Q as QueryDb<'_>>::DynDb as HasQueryGroup<<Q as for<'d> QueryDb<'d>>::Group>>::group_storage(db);
    |                                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0221]: ambiguous associated type `Group` in bounds of `Q`
   --> src/interned.rs:512:57
    |
512 |             <<Q as QueryDb<'_>>::DynDb as HasQueryGroup<Q::Group>>::group_storage(db);
    |                                                         ^^^^^^^^ ambiguous associated type `Group`
    | 
   ::: src/lib.rs:431:5
    |
431 |     type Group: plumbing::QueryGroup<GroupStorage = Self::GroupStorage>;
    |     --------------------------------------------------------------------
    |     |
    |     ambiguous `Group` from `for<'d> QueryDb<'d>`
    |     ambiguous `Group` from `for<'d> QueryDb<'d>`
    |
help: use fully qualified syntax to disambiguate
    |
512 |             <<Q as QueryDb<'_>>::DynDb as HasQueryGroup<<Q as for<'d> QueryDb<'d>>::Group>>::group_storage(db);
    |                                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: use fully qualified syntax to disambiguate
    |
512 |             <<Q as QueryDb<'_>>::DynDb as HasQueryGroup<<Q as for<'d> QueryDb<'d>>::Group>>::group_storage(db);
    |                                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 4 previous errors

For more information about this error, try `rustc --explain E0221`.
error: could not compile `salsa`

Do you know if that is intended? The two suggestions seem identical. Or is it a single suggestion printed twice?

m-ou-se · 2021-01-17T12:03:33Z

@bors r-

That failed in this rollup: #81102 (comment)

(Thanks @lnicola for figuring it out!)

spastorino · 2021-02-02T20:57:49Z

Have just changed back to Ident given that @Aaron1011 has fixed the original underlying issue.

@lnicola can you tell me how to reproduce the salsa issue? or better said, do you have an MCVE for it?.

lnicola · 2021-02-03T12:20:19Z

@spastorino try this, it was a pain to minimize:

pub struct LookupInternedStorage;

impl<Q> QueryStorageOps<Q> for LookupInternedStorage
where
    Q: Query,
    for<'d> Q: QueryDb<'d>,
{
    fn fmt_index(&self, db: &<Q as QueryDb<'_>>::DynDb) {
        <<Q as QueryDb<'_>>::DynDb as HasQueryGroup<Q::Group>>::group_storage(db);
    }
}

pub trait HasQueryGroup<G>
{
    fn group_storage(&self);
}

pub trait QueryStorageOps<Q>
where
    Q: Query,
{
    fn fmt_index(&self, db: &<Q as QueryDb<'_>>::DynDb);
}

pub trait QueryDb<'d> {
    type DynDb: HasQueryGroup<Self::Group> + 'd;
    type Group;
}

pub trait Query: for<'d> QueryDb<'d> {
}

$ cargo +c963187c6f959417cbb13a33e9eaea4607696fc4 build
   Compiling salsa-min v0.1.0 (/home/grayshade/salsa-min)
error[E0221]: ambiguous associated type `Group` in bounds of `Q`
  --> src/main.rs:9:53
   |
9  |         <<Q as QueryDb<'_>>::DynDb as HasQueryGroup<Q::Group>>::group_storage(db);
   |                                                     ^^^^^^^^ ambiguous associated type `Group`
...
30 |     type Group;
   |     -----------
   |     |
   |     ambiguous `Group` from `for<'d> QueryDb<'d>`
   |     ambiguous `Group` from `for<'d> QueryDb<'d>`
   |
help: use fully qualified syntax to disambiguate
   |
9  |         <<Q as QueryDb<'_>>::DynDb as HasQueryGroup<<Q as for<'d> QueryDb<'d>>::Group>>::group_storage(db);
   |                                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: use fully qualified syntax to disambiguate
   |
9  |         <<Q as QueryDb<'_>>::DynDb as HasQueryGroup<<Q as for<'d> QueryDb<'d>>::Group>>::group_storage(db);
   |                                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

For more information about this error, try `rustc --explain E0221`.
error: could not compile `salsa-min`

To learn more, run the command again with --verbose.

$ cargo +stable check
    Finished dev [unoptimized + debuginfo] target(s) in 0.19s

bors · 2021-02-05T15:04:26Z

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

…itance-self, r=Mark-Simulacrum" This reverts commit b4def89, reversing changes made to 7dc1e85.

An ICE happened when certain code is compiled in incremental compilation mode and there are two `Ident`s that have the same `StableHash` value but are considered different by `Eq` and `Hash`. The `Ident` issue is now fixed.

spastorino · 2021-02-08T23:53:31Z

@bors r=nikomatsakis

bors · 2021-02-08T23:53:32Z

📌 Commit 8d17c6a has been approved by nikomatsakis

Rollup of 11 pull requests Successful merges: - rust-lang#72209 (Add checking for no_mangle to unsafe_code lint) - rust-lang#80732 (Allow Trait inheritance with cycles on associated types take 2) - rust-lang#81697 (Add "every" as a doc alias for "all".) - rust-lang#81826 (Prefer match over combinators to make some Box methods inlineable) - rust-lang#81834 (Resolve typedef in HashMap lldb pretty-printer only if possible) - rust-lang#81841 ([rustbuild] Output rustdoc-json-types docs ) - rust-lang#81849 (Expand the docs for ops::ControlFlow a bit) - rust-lang#81876 (parser: Fix panic in 'const impl' recovery) - rust-lang#81882 (:arrow_up: rust-analyzer) - rust-lang#81888 (Fix pretty printer macro_rules with semicolon.) - rust-lang#81896 (Remove outdated comment in windows' mutex.rs) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

pthariensflame · 2021-04-06T10:28:45Z

Does this need relnotes?

nikomatsakis · 2021-04-09T16:42:31Z

I don't think it really needs relnotes, it's basically a minor bugfix that probably doesn't rise to that level.

rust-highfive assigned nikomatsakis Jan 5, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 5, 2021

spastorino mentioned this pull request Jan 5, 2021

Internal Compiler Error while compiling diesel #79560

Closed

spastorino force-pushed the trait-inheritance-self2 branch from 958a97b to 455a0e1 Compare January 5, 2021 20:35

camelid added A-traits Area: Trait system T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 6, 2021

spastorino mentioned this pull request Jan 11, 2021

Cross-compiling Rust to s390x yields a faulty toolchain #80810

Closed

nikomatsakis approved these changes Jan 16, 2021

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 16, 2021

m-ou-se mentioned this pull request Jan 16, 2021

Rollup of 14 pull requests #81102

Closed

lnicola mentioned this pull request Jan 17, 2021

Update rust-analyzer #80984

Closed

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 17, 2021

spastorino force-pushed the trait-inheritance-self2 branch 2 times, most recently from 149e8ab to fae509c Compare February 2, 2021 20:56

spastorino force-pushed the trait-inheritance-self2 branch from fae509c to 53cda05 Compare February 2, 2021 21:01

spastorino force-pushed the trait-inheritance-self2 branch from eb6681e to 934de88 Compare February 5, 2021 18:57

This comment has been minimized.

Sign in to view

spastorino force-pushed the trait-inheritance-self2 branch from 934de88 to 7d810b4 Compare February 5, 2021 20:46

spastorino added 3 commits February 5, 2021 18:56

Revert "Auto merge of rust-lang#79637 - spastorino:revert-trait-inher…

7aa602b

…itance-self, r=Mark-Simulacrum" This reverts commit b4def89, reversing changes made to 7dc1e85.

Add regression test for ICE that happened on incr comp

7b69987

An ICE happened when certain code is compiled in incremental compilation mode and there are two `Ident`s that have the same `StableHash` value but are considered different by `Eq` and `Hash`. The `Ident` issue is now fixed.

Adapt to latest master changes by using PredicateKind

fd09255

spastorino force-pushed the trait-inheritance-self2 branch 3 times, most recently from 6f3b0b4 to c01c135 Compare February 8, 2021 16:02

Anonymize late bound regions on transitive bounds that define assoc type

8d17c6a

spastorino force-pushed the trait-inheritance-self2 branch from c01c135 to 8d17c6a Compare February 8, 2021 19:48

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 8, 2021

Dylan-DPC-zz mentioned this pull request Feb 9, 2021

Rollup of 11 pull requests #81905

Merged

bors merged commit 44e526b into rust-lang:master Feb 9, 2021

rustbot added this to the 1.52.0 milestone Feb 9, 2021

jackh726 mentioned this pull request Mar 7, 2021

Trait inheritance gives unexpected compile error when inherited traits use associated types defined in trait #35237

Closed

hellow554 mentioned this pull request May 25, 2021

Tonic master branch builds on 1.52 but not 1.51 #85659

Closed

Shoeboxam mentioned this pull request Jul 7, 2021

specify minimum Rust version opendp/opendp#169

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Trait inheritance with cycles on associated types take 2 #80732

Allow Trait inheritance with cycles on associated types take 2 #80732

spastorino commented Jan 5, 2021

jyn514 commented Jan 5, 2021 •

edited

Loading

bors commented Jan 5, 2021

spastorino commented Jan 5, 2021

spastorino commented Jan 6, 2021 •

edited

Loading

nikomatsakis commented Jan 16, 2021 •

edited

Loading

bors commented Jan 16, 2021

spastorino commented Jan 16, 2021

spastorino commented Jan 16, 2021

bors commented Jan 16, 2021

lnicola commented Jan 17, 2021

m-ou-se commented Jan 17, 2021

lnicola commented Jan 17, 2021 •

edited

Loading

m-ou-se commented Jan 17, 2021

spastorino commented Feb 2, 2021

lnicola commented Feb 3, 2021 •

edited

Loading

bors commented Feb 5, 2021

This comment has been minimized.

spastorino commented Feb 8, 2021

bors commented Feb 8, 2021

pthariensflame commented Apr 6, 2021

nikomatsakis commented Apr 9, 2021

Allow Trait inheritance with cycles on associated types take 2 #80732

Allow Trait inheritance with cycles on associated types take 2 #80732

Conversation

spastorino commented Jan 5, 2021

jyn514 commented Jan 5, 2021 • edited Loading

bors commented Jan 5, 2021

spastorino commented Jan 5, 2021

spastorino commented Jan 6, 2021 • edited Loading

nikomatsakis commented Jan 16, 2021 • edited Loading

bors commented Jan 16, 2021

spastorino commented Jan 16, 2021

spastorino commented Jan 16, 2021

bors commented Jan 16, 2021

lnicola commented Jan 17, 2021

m-ou-se commented Jan 17, 2021

lnicola commented Jan 17, 2021 • edited Loading

m-ou-se commented Jan 17, 2021

spastorino commented Feb 2, 2021

lnicola commented Feb 3, 2021 • edited Loading

bors commented Feb 5, 2021

This comment has been minimized.

spastorino commented Feb 8, 2021

bors commented Feb 8, 2021

pthariensflame commented Apr 6, 2021

nikomatsakis commented Apr 9, 2021

jyn514 commented Jan 5, 2021 •

edited

Loading

spastorino commented Jan 6, 2021 •

edited

Loading

nikomatsakis commented Jan 16, 2021 •

edited

Loading

lnicola commented Jan 17, 2021 •

edited

Loading

lnicola commented Feb 3, 2021 •

edited

Loading