Permit evaluation of assoc items on `Self` by avoiding cycle error #74130

estebank · 2020-07-07T17:24:31Z

Skip the recursive evaluation of super-traits that reference associated
types on Self directly (without using a fully-qualified path) in order
to allow its use.

The following code would previously emit a cycle evaluation error and
will now successfuly compile:

trait Bar<T> {
    fn foo() -> T;
}
trait Foo: Bar<Self::Qux> {
    type Qux;
}

The following will also work:

trait Bar<T> {
    fn foo() -> T;
}
trait Baz {
    type Qux;
}
trait Foo: Bar<Self::Qux> + Baz {}

Fix #35237.

rust-highfive · 2020-07-07T17:24:35Z

r? @davidtwco

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

estebank · 2020-07-07T17:48:19Z

cc @nikomatsakis @joshtriplett

src/librustc_typeck/astconv.rs

joshtriplett · 2020-07-07T18:17:51Z

Very nice work!

estebank · 2020-07-07T18:53:25Z

@bors try @rust-timer queue

rust-timer · 2020-07-07T18:53:27Z

Awaiting bors try build completion

bors · 2020-07-07T18:53:37Z

⌛ Trying commit 759a56dcc21e49eac014e195d579f5a26effa251 with merge 6024cfef3a82f9a7688f07054f1650081bad3af5...

estebank · 2020-07-07T20:54:40Z

@rust-lang/lang this PR introduces new behavior letting us refer to associated types in super-trait arguments without using a fully qualified path. Only edge case I've been able to come up with is the following:

trait Foo<T> {
    type B;
}
trait Bar<T> {}
trait Baz {
    type A;
}
trait Qux: Foo<Self::A> + Bar<Self::B> + Baz {}

which causes the compile to fail because we proactively skip all bounds that reference Self when evaluating associated types:

error[E0220]: associated type `B` not found for `Self`
 --> fil12.rs:8:37
  |
8 | trait Qux: Foo<Self::A> + Bar<Self::B> + Baz {}
  |                                     ^ help: there is an associated type with a similar name: `A`

You can still make it work with fully qualified paths:

trait Foo<T> {
    type B;
}
trait Bar<T> {}
trait Baz {
    type A;
}
trait Qux: Foo<<Self as Baz>::A> + Bar<<Self as Foo<<Self as Baz>::A>>::B> + Baz {}

or even rely on the new behavior to reduce the verbosity:

trait Foo<T> {
    type B;
}
trait Bar<T> {}
trait Baz {
    type A;
}
trait Qux: Foo<<Self as Baz>::A> + Bar<Self::B> + Baz {}

but I don't know if we want such a "special case" in the language. I think it is worth it (particularly if we at least improve the diagnostics to suggest the appropriate syntax).

bors · 2020-07-07T21:57:32Z

☀️ Try build successful - checks-actions, checks-azure
Build commit: 6024cfef3a82f9a7688f07054f1650081bad3af5 (6024cfef3a82f9a7688f07054f1650081bad3af5)

rust-timer · 2020-07-07T21:57:33Z

Queued 6024cfef3a82f9a7688f07054f1650081bad3af5 with parent e1beee4, future comparison URL.

rust-timer · 2020-07-08T01:53:11Z

Finished benchmarking try commit (6024cfef3a82f9a7688f07054f1650081bad3af5): comparison url.

estebank · 2020-07-08T03:34:59Z

@craterbot check

craterbot · 2020-07-08T03:35:04Z

👌 Experiment pr-74130 created and queued.
🤖 Automatically detected try build 6024cfef3a82f9a7688f07054f1650081bad3af5
🔍 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

davidtwco

Implementation looks reasonable to me, but someone else should have a look.

estebank · 2020-07-08T23:11:13Z

src/librustc_typeck/astconv.rs

+                        ty::Param(param) if param.name == kw::SelfUpper => true,
+                        _ => false,
+                    };
+                    if !(visitor.0 && skip_self_param_evaluation && is_self_param) {


Ideally instead of just skipping super traits we would synthesize a new PolyTraitRef with a TraitRef with params set to some placeholder to avoid the weird edge cases I mentioned before, but couldn't see a good way to do that to a hir node.

craterbot · 2020-07-09T08:57:49Z

🚧 Experiment pr-74130 is now running

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

craterbot · 2020-07-14T23:43:21Z

🎉 Experiment pr-74130 is completed!
📊 7 regressed and 8 fixed (112019 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

estebank · 2020-07-15T23:56:47Z

r? @nikomatsakis ?

nikomatsakis · 2020-07-17T18:46:43Z

So, I'm not sure yet what I think about this specific change, but I agree this is a common pain point that we should try to address.

In short, the current code that handles "short-hands" like T::Foo is already based on some heuristics that sometimes falldown and also are occasionally inaccurate. So I guess that in principle extending these heuristics to cover this case in a semi ad-hoc fashion feels ok.

I think one thing that might make me feel better about it overall is trying to describe the algorithm in some form that would be suitable for the Rust reference, or at least rustc-dev-guide or a nice comment, so that people can understand what's going on. Might be a useful exercise either for you @estebank or for me as a reviewer. =)

(Longer term, I had toyed with ideas of integrating this selection into associated type projection; chalk had a way to represent an "unspecific projection" like T::Iter where the trait was not yet known, and it would figure out what trait the projection might be from, but we kind of canned that out for later.)

estebank · 2020-07-31T17:32:45Z

@nikomatsakis expanded a little bit on the documentation with some extra doc comments. Let me know if it seems like it goes into enough detail.

bors · 2020-08-25T03:12:19Z

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

bors · 2020-08-30T17:44:52Z

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

Skip the recursive evaluation of super-traits that reference associated types on `Self` directly (without using a fully-qualified path) in order to allow its use. The following code would previously emit a cycle evaluation error and will now successfuly compile: ```rust trait Bar<T> { fn foo() -> T; } trait Foo: Bar<Self::Qux> { type Qux; } ``` The following will also work: ```rust trait Bar<T> { fn foo() -> T; } trait Baz { type Qux; } trait Foo: Bar<Self::Qux> + Baz {} ``` Fix rust-lang#35237.

bors · 2020-09-22T10:28:37Z

☔ The latest upstream changes (presumably #76906) 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

crlf0710 · 2020-10-08T12:40:21Z

Triage: there's merge conflicts now.

nikomatsakis

OK, I read this through and I understand it now. I am feeling positively inclined. The question, I guess, is whether it introduces any kinds of backwards compatibility hazards. Let me post separately on this.

nikomatsakis · 2020-10-15T18:54:11Z

compiler/rustc_typeck/src/astconv/mod.rs

@@ -1446,6 +1531,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
        qself_res: Res,
        assoc_segment: &hir::PathSegment<'_>,
        permit_variants: bool,
+        dont_recurse: bool,


Pre-existing, sort of, but could we introduce struct PermitVariants(bool) and struct DontRecurse(bool) or something? I found it hard to read calls like self.associated_path_to_ty(..., true, false) etc and remember what those booleans were meant to represent.

nikomatsakis · 2020-10-15T18:56:25Z

compiler/rustc_typeck/src/astconv/mod.rs

-                    trait_bounds.push((b, constness))
+                hir::GenericBound::Trait(ref b, modif @ hir::TraitBoundModifier::None) => {
+                    let mut visitor = SelfFinder(false);
+                    visitor.visit_poly_trait_ref(b, modif);


It would be better to avoid the visitor if !skip_self_param_evaluation or !is_self_param, right?

I would also maybe find that if below a little easier to read if it were restructured.

nikomatsakis · 2020-10-19T21:48:39Z

I was thinking about what kind of backwards compatibility hazard this code might introduce. I guess that the case we might worry about it something like this

trait Foo<T> {
    type A;
}
trait Baz {
    type A;
}
trait Qux: Foo<Self::A> + Baz {}

@estebank I think that what will happen here is that, to resolve Self::A, we'll skip the first bound and just look at the second one, does that sound correct?

nikomatsakis · 2020-10-19T21:50:29Z

I suspect we might be able to make all the cases that we want to accept work without accepting negative cases with a different approach. The idea would be to make a query that walks a given trait and extracts the names of all associated types found in that trait or any supertrait -- but it does this without any sort of substitution or creation of types. We can then leverage this query to skip "elaborating" things that don't potentially reference an associated type with the given name. Does this make sense, @estebank? I would find this approach a bit more elegant, I think.

nikomatsakis · 2020-11-07T11:26:40Z

So @spastorino is currently exploring the alternative idea I proposed. I'm going to close this PR for the time being because I think that the back compat hazard I mentioned here is kind of a blocker to this approach. We can always re-open. Thanks @estebank for poking at this though, it'd be great to fix this problem.

rust-highfive assigned davidtwco Jul 7, 2020

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

estebank mentioned this pull request Jul 7, 2020

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

Closed

estebank force-pushed the self-param-assoc-item branch 4 times, most recently from a4a675c to 54c0927 Compare July 7, 2020 17:47

joshtriplett reviewed Jul 7, 2020

View reviewed changes

src/librustc_typeck/astconv.rs Outdated Show resolved Hide resolved

estebank force-pushed the self-param-assoc-item branch from 54c0927 to 759a56d Compare July 7, 2020 18:27

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 8, 2020

davidtwco approved these changes Jul 8, 2020

View reviewed changes

estebank commented Jul 8, 2020

View reviewed changes

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Jul 14, 2020

rust-highfive assigned nikomatsakis and unassigned davidtwco Jul 15, 2020

estebank force-pushed the self-param-assoc-item branch from 759a56d to d745f92 Compare July 31, 2020 17:32

estebank force-pushed the self-param-assoc-item branch 3 times, most recently from 16ce81a to 369c2b8 Compare July 31, 2020 18:24

estebank force-pushed the self-param-assoc-item branch from 369c2b8 to ac20dee Compare August 12, 2020 01:30

This comment has been minimized.

Sign in to view

estebank force-pushed the self-param-assoc-item branch 2 times, most recently from 23e943a to 3380862 Compare August 23, 2020 01:57

estebank force-pushed the self-param-assoc-item branch from 3380862 to 304235b Compare August 27, 2020 02:46

estebank force-pushed the self-param-assoc-item branch from 304235b to 53bacac Compare August 31, 2020 17:57

nikomatsakis approved these changes Oct 19, 2020

View reviewed changes

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 Nov 6, 2020

nikomatsakis closed this Nov 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Permit evaluation of assoc items on `Self` by avoiding cycle error #74130

Permit evaluation of assoc items on `Self` by avoiding cycle error #74130

estebank commented Jul 7, 2020

rust-highfive commented Jul 7, 2020

estebank commented Jul 7, 2020

joshtriplett commented Jul 7, 2020

estebank commented Jul 7, 2020

rust-timer commented Jul 7, 2020

bors commented Jul 7, 2020

estebank commented Jul 7, 2020 •

edited

Loading

bors commented Jul 7, 2020

rust-timer commented Jul 7, 2020

rust-timer commented Jul 8, 2020

estebank commented Jul 8, 2020

craterbot commented Jul 8, 2020

davidtwco left a comment

estebank Jul 8, 2020 •

edited

Loading

craterbot commented Jul 9, 2020

craterbot commented Jul 14, 2020

estebank commented Jul 15, 2020

nikomatsakis commented Jul 17, 2020

estebank commented Jul 31, 2020

This comment has been minimized.

bors commented Aug 25, 2020

bors commented Aug 30, 2020

bors commented Sep 22, 2020

crlf0710 commented Oct 8, 2020

nikomatsakis left a comment

nikomatsakis Oct 15, 2020

nikomatsakis Oct 15, 2020

nikomatsakis commented Oct 19, 2020

nikomatsakis commented Oct 19, 2020

nikomatsakis commented Nov 7, 2020

Permit evaluation of assoc items on Self by avoiding cycle error #74130

Permit evaluation of assoc items on Self by avoiding cycle error #74130

Conversation

estebank commented Jul 7, 2020

rust-highfive commented Jul 7, 2020

estebank commented Jul 7, 2020

joshtriplett commented Jul 7, 2020

estebank commented Jul 7, 2020

rust-timer commented Jul 7, 2020

bors commented Jul 7, 2020

estebank commented Jul 7, 2020 • edited Loading

bors commented Jul 7, 2020

rust-timer commented Jul 7, 2020

rust-timer commented Jul 8, 2020

estebank commented Jul 8, 2020

craterbot commented Jul 8, 2020

davidtwco left a comment

Choose a reason for hiding this comment

estebank Jul 8, 2020 • edited Loading

Choose a reason for hiding this comment

craterbot commented Jul 9, 2020

craterbot commented Jul 14, 2020

estebank commented Jul 15, 2020

nikomatsakis commented Jul 17, 2020

estebank commented Jul 31, 2020

This comment has been minimized.

bors commented Aug 25, 2020

bors commented Aug 30, 2020

bors commented Sep 22, 2020

crlf0710 commented Oct 8, 2020

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis Oct 15, 2020

Choose a reason for hiding this comment

nikomatsakis Oct 15, 2020

Choose a reason for hiding this comment

nikomatsakis commented Oct 19, 2020

nikomatsakis commented Oct 19, 2020

nikomatsakis commented Nov 7, 2020

Permit evaluation of assoc items on `Self` by avoiding cycle error #74130

Permit evaluation of assoc items on `Self` by avoiding cycle error #74130

estebank commented Jul 7, 2020 •

edited

Loading

estebank Jul 8, 2020 •

edited

Loading