Implement RFC 2532 – Associated Type Defaults #61812

jonas-schievink · 2019-06-13T19:40:19Z

This is a partial implementation that is still missing the changes to object types, since I ran into some trouble while implementing that. I'm opening this part already to get feedback on the implementation and the unexpected test fallout (see my comments below). The remaining changes can be done in a later PR.

Blockers before this can land:

Resolve unsoundness around interaction with specialization (Implement RFC 2532 – Associated Type Defaults #61812 (comment)) - Deny specializing items not in the parent impl #64564

cc #29661
Fixes #53907
Fixes #54182
Fixes #62211
Fixes #41868
Fixes #63593
Fixes #47385
Fixes #43924
Fixes #32350
Fixes #26681
Fixes #67187

jonas-schievink · 2019-06-13T19:41:20Z

src/librustc/traits/project.rs

@@ -1104,27 +1104,41 @@ fn assemble_candidates_from_impls<'cx, 'gcx, 'tcx>(
                    // an error when we confirm the candidate
                    // (which will ultimately lead to `normalize_to_error`
                    // being invoked).
-                    node_item.item.defaultness.has_value()
+                    false


This looks very wrong to me, since it sounds like it'd allow assuming defaults inside the trait. For some reason that doesn't happen (the tests pass).

@jonas-schievink

Inside the trait, you won't be having a VtableImpl (which indicates a specific impl was selected), but a VtableParam (because of an "implicit where-clause").

However, this does not correctly handle the case where an associated type default is overridden in a derived impl:

#![feature(associated_type_defaults, specialization)] trait Foo { type Xyz = u32; fn foobar() -> Self::Xyz; } // 1) impl<T> Foo for T { fn foobar() -> Self::Xyz { 0u32 } } // 2) impl Foo for u32 { type Xyz = &'static u32; } fn main() { u32::foobar(); }

However, according to the spec, the problem is somewhere else, because of

To permit this is not a problem because Foo for Vec<T> is not further specializable since Bar in the implementation has not been marked as default.

However, according to the spec, the problem is somewhere else, because of

So the RFC says:

If an implementation does make the associated type available for further specialization, then other definitions in the implementation may not assume the given underlying specified type of the associated type and may only assume that it is Self::TheAssociatedType.

which the snippet above clearly violates... but the more interesting question is "how do we ban it?"...

...For the sake of separate compilation (i.e. imagine 2) is in a different crate), I think we need to say that 1) is legitimate but 2) is not. Why? Because it overrides type Xyz which should not be up for specialization since it was not marked as default in 1) (if it did then fn foobar could not assume type Xyz's definition). Moreover fn foobar here assumes type Xyz's definition.

Sure. If we go by the RFC, the default associated type type Xyz = u32; should only be specializable by the first impl that matches, not by any specializing impls (this can be checked at 2) time because of the orphan rules), so 2) should give an E0520 (Xyz specializes an item from a parent impl, but that item is not marked default).

I'll note that this contradicts the way associated fns work:

#![feature(specialization)] trait Foo { fn foobar() -> bool { false } } impl<T> Foo for T { } impl Foo for u32 { fn foobar() -> bool { true } } fn main() { println!("{:?}", u32::foobar()); // prints `true` }

also, if we do that, we have to decide whether associated consts should work like associated types or like associated fns.

also, if we do that, we have to decide whether associated consts should work like associated types or like associated fns.

The RFC applies to associated fns as well so how they work will need to be changed as well but not necessarily in this PR. The RFC says:

This applies generally to any item inside a trait. [...]

which includes fns and consts also.

This is an open question in the design of specialization (#31844):

Should default trait items be considered default (i.e. specializable)?

Specialization is unstable, so I don't think there's a real reason not to forbid respecialization of associated types and associated fns together. However, I might want to do this in an earlier PR, and assess crater impact.

That change that has a fair bit of breaking impact, so I'm for implementing it fairly quickly. However, I'll still be OK with opening an issue for the specialization rules change and linking it from the OP of #29661.

If we continue on this question: can you have default provided trait items that can be re-overriden?

#![feature(specialization)] trait Foo { default fn foobar() -> bool { false } } impl<T> Foo for T { } impl Foo for u32 { fn foobar() -> bool { true } // OK } fn main() { println!("{:?}", u32::foobar()); // prints `true` }

I think either option would be worth writing clearly in a "changes to specialization" section in RFC 2532, along with examples.

This is an open question in the design of specialization (#31844):

Should default trait items be considered default (i.e. specializable)?

This is a bit of an orthogonal question; the main question is instead whether re-overriding is allowed as you point out later. I think it flows from the reference of the RFC and the spirit of it that re-overriding is banned. Instead, the idea is that 1) in both of your snippets "lock in" what they do not explicitly allow for further specialization (with default $item in impls). Whether trait items must be prefixed with default to be specializable is not a particularly interesting question from this perspective and especially not from the POV of specialization groups as per in the "future possibilities" section of the RFC.

I think either option would be worth writing clearly in a "changes to specialization" section in RFC 2532, along with examples.

It's a good idea to write down interesting points and decisions somewhere be it in the RFC or in the tracking issue. I think we can link to this discussion in a bullet point in the tracking issue description and then consider amending the RFC with new information at a later point (when things have slowed down and the pieces are in place). These notes will be useful when I (or someone else, but most likely not...) writes the stabilization report.

jonas-schievink · 2019-06-13T19:43:20Z

src/test/ui/privacy/associated-item-privacy-trait.rs

-        let _: <Pub as PubTr>::AssocTy;
-        //~^ ERROR type `priv_parent_substs::Priv` is private
-        //~| ERROR type `priv_parent_substs::Priv` is private
+        let _: <Pub as PubTr>::AssocTy;  // TODO no longer an error?!


This also looks extremely wrong to me since the only difference to the code below is the omission of <_>, but I can't imagine how this patch caused this, so it might be preexisting?

jonas-schievink · 2019-06-13T19:44:15Z

src/test/ui/ufcs/ufcs-partially-resolved.stderr

@@ -184,19 +184,19 @@ error[E0223]: ambiguous associated type
  --> $DIR/ufcs-partially-resolved.rs:36:12
   |
 LL |     let _: <u8 as Tr>::Y::NN;
-   |            ^^^^^^^^^^^^^^^^^ help: use fully-qualified syntax: `<<u8 as Tr>::Y as Trait>::NN`
+   |            ^^^^^^^^^^^^^^^^^ help: use fully-qualified syntax: `<u16 as Trait>::NN`


These suggestions are now worse due to the projection succeeding

Centril · 2019-06-13T20:07:02Z

r? @nikomatsakis

src/test/ui/associated-types/defaults-in-other-trait-items.rs

src/test/ui/associated-types/issue-54182-1.rs

src/test/ui/associated-types/issue-54182-2.rs

src/test/ui/associated-types/issue-54182-1.rs

src/test/ui/associated-const/defaults-cyclic-pass.rs

src/test/ui/associated-const/defaults-not-assumed-fail.rs

src/test/ui/associated-types/defaults-specialization.rs

Centril · 2019-06-14T19:50:03Z

src/test/ui/associated-types/defaults-specialization.stderr

+   |                  ^^ expected associated type, found u8
+   |
+   = note: expected type `fn() -> <A<T> as Tr>::Ty`
+              found type `fn() -> u8`


I wonder if it would be feasible in a not-too-hacky a manner to tell the user that defaults are the cause of the failure (both when using specialization and not). Otherwise the user might be reasonably confused as to why they don't get to assume the default since they are unlikely to be aware of the type theoretical reasons.

This could also be left as future work; if you prefer that, please leave a checkbox in the tracking issue :)

Also cc @estebank

Yeah I'll do this. This is challenging enough as is ;)

For that future work... One thing you might try in the unhappy path is to check whether there is a default that would have allowed it to succeed if not for our rules.

src/test/ui/associated-types/defaults-suitability.rs

Centril · 2019-06-14T22:09:23Z

src/librustc_typeck/collect.rs

+
+            if let Some(default_ty) = default {
+                let default_ty = AstConv::ast_ty_to_ty(&ItemCtxt::new(tcx, def_id), default_ty);
+                let preds = AstConv::compute_bounds(


Can you add a test to also make sure that bounds are respected when not written directly on the associated type itself?

trait Foo<T> where // Same bound as `type Bar: Clone` but written indirectly: <Self as Foo<T>>::Bar: Clone, { type Bar = Vec<T>; }

Another case to consider is when you have multiple defaults:

trait Foo<T> where <Self as Foo<T>>::Bar: Clone, { type Bar = Vec<Self::Baz>; type Baz = T; }

Yeah this doesn't work at all right now, this check needs to be implemented differently.

This gets particularly tricky when you have a where-clause like where Self::Ty: Con<Self> on the trait. You can't really know if this holds for the defaulted type because Self isn't yet known.

In general, when faced with a situation "you can't really know yet" this should be treated as if it wasn't so. In other words, we want constructive proof that something holds. So in a situation with where Self::Ty: Con<Self>, because Self isn't yet known, the answer is to treat it as a type variable (which it is...) and then error.

I think the check should be something along the lines of substituting the associated types for the defaults and then try to prove the normalized where clauses. It seems to me that rejecting where Self::Ty: Con<Self>, should just "fall out".

Just pushed a better implementation of the check and a comprehensive test

Looks great!

Can you also add my last snippet to that so we check for scenarios involving more than one associated type?

Done. Right now the behavior is to do a shallow replacement of associated types with their defaults, so <Self as Foo<T>>::Bar: Clone is turned into Vec<Self::Baz>, not Vec<T>. ~~I guess this needs to be fixed.~~

Ah, I think the shallow substitution is actually beneficial here, since it forces the trait to work with any combination of defaults and non-defaults: If we did the deep substitution and had a T: Clone bound, we'd prove that Vec<T>: Clone and the trait would be accepted. If a user then implements it and overrides only Baz, suddenly Vec<Self::Baz>: Clone might no longer hold, and the user would be forced to also override Bar. With the shallow substitution, the trait would have to add a Self::Baz: Clone bound instead/in addition to the T: Clone bound, which means that an implementation could replace either assoc. type without making others not work anymore.

Interesting; it seems to me a shallow substitution is strictly more conservative than the deeper one, right? So we can relax it later if we want.

I think it would be good to demonstrate the difference in a special test file and then also leave a description of the distinction (with examples) on the tracking issue so that these interesting aspects aren't lost in comments here.

src/librustc_typeck/check/wfcheck.rs

nikomatsakis · 2020-02-24T22:56:06Z

I read over the code here -- but then I was looking back over the list of tests that I assembled earlier and I felt like I didn't see (e.g.) tests that were looking at the interaction of dyn Trait and associated type defaults. Did I overlook that functionality, or is it not included in the PR?

Centril · 2020-02-24T22:57:34Z

I believe that part is not included per PR OP:

This is a partial implementation that is still missing the changes to object types, since I ran into some trouble while implementing that. [...]

nikomatsakis · 2020-02-24T23:22:02Z

OK, great, I thought that might be the case, given the code diffs I saw.

nikomatsakis · 2020-02-25T18:57:22Z

@bors r+

I also added the list of tests to the tracking issue #29661

bors · 2020-02-25T18:57:23Z

📌 Commit 6cc268b has been approved by nikomatsakis

nikomatsakis · 2020-02-25T18:57:33Z

@jonas-schievink sorry for taking so unforgiveably long here ❤️

Centril · 2020-02-25T22:17:51Z

I'm so excited to finally see this land! Thanks so much @jonas-schievink for your hard work and thanks @nikomatsakis for diligently assembling all those test cases.

bors · 2020-02-26T09:27:13Z

⌛ Testing commit 6cc268b with merge 3a0d106...

bors · 2020-02-26T12:48:45Z

☀️ Test successful - checks-azure
Approved by: nikomatsakis
Pushing 3a0d106 to master...

rust-highfive · 2020-02-26T12:49:55Z

📣 Toolstate changed by #61812!

Tested on commit 3a0d106.
Direct link to PR: #61812

💔 clippy-driver on windows: test-pass → test-fail (cc @mcarton @oli-obk @Manishearth @flip1995 @yaahc @phansch @llogiq).
💔 clippy-driver on linux: test-pass → test-fail (cc @mcarton @oli-obk @Manishearth @flip1995 @yaahc @phansch @llogiq).

@mcarton

Tested on commit rust-lang/rust@3a0d106. Direct link to PR: <rust-lang/rust#61812> 💔 clippy-driver on windows: test-pass → test-fail (cc @mcarton @oli-obk @Manishearth @flip1995 @yaahc @phansch @llogiq). 💔 clippy-driver on linux: test-pass → test-fail (cc @mcarton @oli-obk @Manishearth @flip1995 @yaahc @phansch @llogiq).

Rustup to rust-lang/rust#61812 changelog: none

Update cargo, clippy Closes #69601 ## cargo 16 commits in e57bd02999c9f40d52116e0beca7d1dccb0643de..bda50510d1daf6e9c53ad6ccf603da6e0fa8103f 2020-02-21 20:20:10 +0000 to 2020-03-02 18:05:34 +0000 - Fix rare failure in collision_export test. (rust-lang/cargo#7956) - Ignore broken Cargo.toml in git sources (rust-lang/cargo#7947) - Add more fingerprint mtime debug logging. (rust-lang/cargo#7952) - Fix plugin tests for latest nightly. (rust-lang/cargo#7955) - Simplified usage code of SipHasher (rust-lang/cargo#7945) - Add a special case for git config discovery inside tests (rust-lang/cargo#7944) - Fixes issue rust-lang/cargo#7543 (rust-lang/cargo#7946) - Filter out cfgs which should not be used during build (rust-lang/cargo#7943) - Provide extra context on a query failure. (rust-lang/cargo#7934) - Try to clarify `cargo metadata`'s relationship with the workspace. (rust-lang/cargo#7927) - Update libgit2 dependency (rust-lang/cargo#7939) - Fix link in comment (rust-lang/cargo#7936) - Enable `cargo doc --open` tests on macos. (rust-lang/cargo#7932) - build-std: remove sysroot probe (rust-lang/cargo#7931) - Try to clarify how feature flags work on the "current" package. (rust-lang/cargo#7928) - Add extra details in the new feature resolver doc comment. (rust-lang/cargo#7918) ## clippy 6 commits in fc5d0cc..8b7f7e6 2020-02-24 05:58:17 +0000 to 2020-03-02 20:00:31 +0000 - Rustup to #69469 (rust-lang/rust-clippy#5254) - Some rustups (rust-lang/rust-clippy#5247) - Update git2 to 0.12 (rust-lang/rust-clippy#5232) - Rustup to #61812 (rust-lang/rust-clippy#5231) - Add lint to improve floating-point expressions (rust-lang/rust-clippy#4897) - Do not run deploy action on other repos (rust-lang/rust-clippy#5222)

rust-highfive assigned cramertj Jun 13, 2019

This comment has been minimized.

Sign in to view

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

jonas-schievink commented Jun 13, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

rust-highfive assigned nikomatsakis and unassigned cramertj Jun 13, 2019

Centril reviewed Jun 13, 2019

View reviewed changes

src/test/ui/associated-types/defaults-in-other-trait-items.rs Show resolved Hide resolved

This comment has been minimized.

Sign in to view

Centril reviewed Jun 13, 2019

View reviewed changes

src/test/ui/associated-types/issue-54182-1.rs Show resolved Hide resolved

src/test/ui/associated-types/issue-54182-2.rs Outdated Show resolved Hide resolved

src/test/ui/associated-types/issue-54182-1.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

jonas-schievink mentioned this pull request Jun 13, 2019

Omitting return type of trait method to use () doesn't work with associated_type_defaults when default is () #54182

Closed

Centril reviewed Jun 13, 2019

View reviewed changes

src/test/ui/associated-types/issue-54182-1.rs Show resolved Hide resolved

Centril reviewed Jun 13, 2019

View reviewed changes

src/test/ui/associated-const/defaults-cyclic-pass.rs Show resolved Hide resolved

Centril mentioned this pull request Jun 13, 2019

Projecting to an associated constant fails but with wrong reason #61815

Open

Centril reviewed Jun 13, 2019

View reviewed changes

src/test/ui/associated-const/defaults-not-assumed-fail.rs Show resolved Hide resolved

This comment has been minimized.

Sign in to view

Centril reviewed Jun 14, 2019

View reviewed changes

jonas-schievink mentioned this pull request Jun 14, 2019

Tracking issue for RFC 2532, "Associated type defaults" #29661

Open

85 tasks

jonas-schievink commented Jun 14, 2019

View reviewed changes

src/test/ui/associated-types/defaults-suitability.rs Outdated Show resolved Hide resolved

Centril reviewed Jun 14, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

Centril reviewed Jun 15, 2019

View reviewed changes

src/librustc_typeck/check/wfcheck.rs Outdated Show resolved Hide resolved

Centril reviewed Jun 15, 2019

View reviewed changes

src/librustc_typeck/check/wfcheck.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

jonas-schievink added 5 commits February 21, 2020 19:42

Fix tests that fail with --emit metadata

9930e1f

Fix rebase damage

4d4da92

Mark E0399 test as obsolete

af2931b

Fix rebase fallout

c605831

Mark E0399.md as obsolete

6cc268b

nikomatsakis approved these changes Feb 24, 2020

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 25, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 26, 2020

bors merged commit 3a0d106 into rust-lang:master Feb 26, 2020

This was referenced Feb 26, 2020

Make error codes into strings #67086

Closed

Split librustc::{traits,infer} to their respective crates #69076

Merged

JohnTitor added a commit to JohnTitor/rust-clippy that referenced this pull request Feb 27, 2020

Rustup to rust-lang/rust#61812

7c760e5

bors added a commit to rust-lang/rust-clippy that referenced this pull request Feb 27, 2020

Auto merge of #5231 - JohnTitor:associated-type-defaults, r=phansch

384593c

Rustup to rust-lang/rust#61812 changelog: none

ehuss mentioned this pull request Mar 2, 2020

Update cargo, clippy #69627

Merged

nikomatsakis mentioned this pull request Mar 26, 2020

blank specializing impls don't "lock in" defaults #70442

Closed

dvdplm mentioned this pull request May 4, 2021

[http server] Batch requests paritytech/jsonrpsee#292

Merged

Implement RFC 2532 – Associated Type Defaults #61812

Implement RFC 2532 – Associated Type Defaults #61812

Conversation

jonas-schievink commented Jun 13, 2019 • edited Loading

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielb1 Jul 4, 2019 • edited by Centril Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Centril Jul 6, 2019 • edited Loading

Choose a reason for hiding this comment

arielb1 Jul 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielb1 Jul 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Centril Jul 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

Centril commented Jun 13, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Centril Jun 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonas-schievink Jun 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nikomatsakis commented Feb 24, 2020

Centril commented Feb 24, 2020

nikomatsakis commented Feb 24, 2020

nikomatsakis commented Feb 25, 2020

bors commented Feb 25, 2020

nikomatsakis commented Feb 25, 2020

Centril commented Feb 25, 2020

bors commented Feb 26, 2020

bors commented Feb 26, 2020

rust-highfive commented Feb 26, 2020

jonas-schievink commented Jun 13, 2019 •

edited

Loading

arielb1 Jul 4, 2019 •

edited by Centril

Loading

Centril Jul 6, 2019 •

edited

Loading

arielb1 Jul 6, 2019 •

edited

Loading

arielb1 Jul 6, 2019 •

edited

Loading

Centril Jul 6, 2019 •

edited

Loading

Centril Jun 14, 2019 •

edited

Loading

jonas-schievink Jun 15, 2019 •

edited

Loading