[Syntax] Implement auto trait syntax #45247

leoyvens · 2017-10-12T22:34:02Z

Implements auto trait Send {} as a substitute for trait Send {} impl Send for .. {}.

See the internals thread for motivation. Part of #13231.

The first commit is just a rename moving from "default trait" to "auto trait". The rest is parser->AST->HIR work and making it the same as the current syntax for everything below HIR. It's under the optin_builtin_traits feature gate.

When can we remove the old syntax? Do we need to wait for a new stage0? We also need to formally decide for the new form (even if the keyword is not settled yet).

Observations:

If you auto trait Auto {} and then impl Auto for .. {} that's accepted even if it's redundant.
The new syntax is simpler internally which will allow for a net removal of code, for example well-formedness checks are effectively moved to the parser.
Rustfmt and clippy are broken, need to fix those.
Rustdoc just ignores it for now.

ping @petrochenkov @nikomatsakis

rust-highfive · 2017-10-12T22:34:18Z

r? @pnkfelix

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

kennytm · 2017-10-13T05:01:05Z

Some code are still using the old names.

[00:07:46] error[E0531]: cannot find tuple struct/variant `VtableDefaultImpl` in this scope
[00:07:46]    --> /checkout/src/librustc/ich/impls_ty.rs:859:14
[00:07:46]     |
[00:07:46] 859 |             &VtableDefaultImpl(ref table_def_impl) => table_def_impl.hash_stable(hcx, hasher),
[00:07:46]     |              ^^^^^^^^^^^^^^^^^ not found in this scope
[00:07:46] 
[00:07:46] error[E0412]: cannot find type `VtableDefaultImplData` in module `traits`
[00:07:46]    --> /checkout/src/librustc/ich/impls_ty.rs:887:13
[00:07:46]     |
[00:07:46] 887 | for traits::VtableDefaultImplData<N> where N: HashStable<StableHashingContext<'gcx>> {
[00:07:46]     |             ^^^^^^^^^^^^^^^^^^^^^ did you mean `VtableAutoImplData`?
[00:07:46] 
[00:07:46] error[E0422]: cannot find struct, variant or union type `VtableDefaultImplData` in module `traits`
[00:07:46]    --> /checkout/src/librustc/ich/impls_ty.rs:891:21
[00:07:46]     |
[00:07:46] 891 |         let traits::VtableDefaultImplData {
[00:07:46]     |                     ^^^^^^^^^^^^^^^^^^^^^ did you mean `VtableAutoImplData`?
[00:07:46] 
[00:07:49] error[E0207]: the type parameter `N` is not constrained by the impl trait, self type, or predicates
[00:07:49]    --> /checkout/src/librustc/ich/impls_ty.rs:886:12
[00:07:49]     |
[00:07:49] 886 | impl<'gcx, N> HashStable<StableHashingContext<'gcx>>
[00:07:49]     |            ^ unconstrained type parameter

leoyvens · 2017-10-13T17:31:23Z

@kennytm Thanks, fixed it!

nikomatsakis · 2017-10-13T20:42:42Z

When can we remove the old syntax? Do we need to wait for a new stage0? We also need to formally decide for the new form (even if the keyword is not settled yet).

We could remove it ASAP, but it may be kind to offer a grace period. In the compiler itself, we can just use #[cfg(stage0)] impls for the time being.

nikomatsakis

What do you think about simplifying the HIR representation to align better with the new syntax? Would you need some tips for how to work that through?

Also, we may want to start issuing a future compatibility warning if we are going to continue to accept the old syntax.

nikomatsakis · 2017-10-13T20:39:30Z

src/librustc_typeck/collect.rs

-    let has_default_impl = tcx.hir.trait_is_auto(def_id);
+    let is_auto = match item.node {
+        hir::ItemTrait(hir::IsAuto::Yes, ..) => true,
+        _ => tcx.hir.trait_is_auto(def_id),


Once we make hir::IsAuto::Yes be the canonical way to encode hir traits, this code can be simplified, as can quite a bit of other code I imagine.

nikomatsakis · 2017-10-13T20:45:59Z

src/librustc/hir/lowering.rs

@@ -1479,14 +1479,14 @@ impl<'a> LoweringContext<'a> {
                let vdata = self.lower_variant_data(vdata);
                hir::ItemUnion(vdata, self.lower_generics(generics))
            }
-            ItemKind::DefaultImpl(unsafety, ref trait_ref) => {
+            ItemKind::AutoImpl(unsafety, ref trait_ref) => {


I'd prefer to see all evidence of impl Trait for .. go away by the time HIR lowering is done. That is, I think the HIR should not include impl Trait for ..-style impls, and instead we should lower any trait that contains such an impl to have hir::IsAuto set to true.

This will presumably require one of two things:

A pre-pass that identifies the set of traits targeted by auto-impls, so that later, when we lower traits, we can set hir::IsAuto to true.

Or, alternatively, we could collect the set as we walk, and update the lowered form of traits after the fact. I suspect we still have unique access and can mutate them in place?

nikomatsakis · 2017-10-13T20:50:51Z

That said, I'm not convinced this is a widely used feature. At least I sort of hope it's not. So maybe it's better to just make the change immediately.

nikomatsakis · 2017-10-13T20:50:58Z

Probably worth testing. =)

nikomatsakis · 2017-10-13T20:56:41Z

To work around the limitations of #[cfg(stage0)] and parsing, you could do something like this:

#[cfg(stage0)]
macro_rules! conditional {
    (stage0 => { $($stage0:tt)* } stageN => { $($stageN:tt)* }) => $($stage0)*
}

#[cfg(not(stage0))]
macro_rules! conditional {
    (stage0 => { $($stage0:tt)* } stageN => { $($stageN:tt)* }) => $($stageN)*
}

now you can do:

conditional {
    stage0 => {
        trait Foo { }
        impl trait Foo for .. { }
    }
    stageN => {
        auto trait Foo { }
    }
}

but even better might be to make:

auto_trait!(Foo);

that expands to either auto trait Foo { } or trait Foo { } impl Foo for .. { } as appropriate.

leoyvens · 2017-10-13T21:00:46Z

@nikomatsakis Thanks, I'll work on killing off the old syntax then!

nikomatsakis · 2017-10-13T21:01:36Z

@rfcbot fcp merge

This PR changes the syntax of auto traits to be auto trait Foo { } instead of trait Foo { } impl Foo for .. { }. Regardless of the specific keyword that we choose, I think it's clear by now that co-induction (and auto-ness) is a propery of the trait, not an impl, and so it would be better to declare it at the trait level (e.g., the work on co-logic programming also talks of mixing inductive and co-inductive predicates, not of making co-inductive clauses). auto trait seems like it is a reasonable choice.

This FCP is specifically for making the shift, but not for stabilization (of course) of any part of auto traits that are not already stabilized.

rfcbot · 2017-10-13T21:55:28Z

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

eddyb · 2017-10-13T22:02:51Z

I'm not sold on the syntax but if we can't come up with anything more interesting/descriptive 🤷‍♂️

cramertj · 2017-10-13T22:25:27Z

@rfcbot reviewed

leoyvens · 2017-10-14T12:13:34Z

If we kill the old syntax now by abusing cfg(stage0) then when this becomes the stage0 it won't be able to boostrap itself. Better to wait for a stage0 that accepts both forms.

Since we must accept the old syntax for a few more weeks, I think an error level future compatibility lint would be appropiate. In January there were 15 users of this feature in crates.io.

@nikomatsakis

What do you think about simplifying the HIR representation to align better with the new syntax?

Coherence needs to check orphan rules, overlap and unsafety for hir::ItemAutoImpl which makes no sense for the new syntax. Refactorings that will just get removed in six weeks don't seem worth pursuing. But improvements to the representation of auto trait itself would make sense.

bors · 2017-10-14T18:51:23Z

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

Now we can do the well formedness checks in the parser, yay!

Forgot this ones.

This moves the well formedness checks to the AST validation pass. Tests were adjusted. The auto keyword should be back-compat now.

It was being printed wrong as auto unsafe trait

kennytm · 2017-11-03T18:32:44Z

@bors r=nikomatsakis

bors · 2017-11-03T18:32:45Z

📌 Commit 5190abb has been approved by nikomatsakis

bors · 2017-11-03T19:07:52Z

⌛ Testing commit 5190abb with merge 2278506...

@petrochenkov

…omatsakis [Syntax] Implement auto trait syntax Implements `auto trait Send {}` as a substitute for `trait Send {} impl Send for .. {}`. See the [internals thread](https://internals.rust-lang.org/t/pre-rfc-renaming-oibits-and-changing-their-declaration-syntax/3086) for motivation. Part of #13231. The first commit is just a rename moving from "default trait" to "auto trait". The rest is parser->AST->HIR work and making it the same as the current syntax for everything below HIR. It's under the `optin_builtin_traits` feature gate. When can we remove the old syntax? Do we need to wait for a new `stage0`? We also need to formally decide for the new form (even if the keyword is not settled yet). Observations: - If you `auto trait Auto {}` and then `impl Auto for .. {}` that's accepted even if it's redundant. - The new syntax is simpler internally which will allow for a net removal of code, for example well-formedness checks are effectively moved to the parser. - Rustfmt and clippy are broken, need to fix those. - Rustdoc just ignores it for now. ping @petrochenkov @nikomatsakis

bors · 2017-11-04T00:09:10Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 2278506 to master...

Breakage came from rust-lang/rust#45247

rust-highfive assigned pnkfelix Oct 12, 2017

leoyvens force-pushed the implement-auto-trait-syntax branch from 917cb34 to 90c80f1 Compare October 13, 2017 00:11

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Oct 13, 2017

kennytm 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 Oct 13, 2017

petrochenkov assigned nikomatsakis and petrochenkov and unassigned pnkfelix Oct 13, 2017

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

nikomatsakis requested changes Oct 13, 2017

View reviewed changes

rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Oct 13, 2017

leoyvens force-pushed the implement-auto-trait-syntax branch 3 times, most recently from 04f4c4e to 7dc5d57 Compare October 14, 2017 22:15

leoyvens added 11 commits November 3, 2017 16:13

Add tests for auto trait, fix parsing bug

acf50ee

Now we can do the well formedness checks in the parser, yay!

Finish DefaultImpl -> AutoImpl rename.

0d1b79a

Forgot this ones.

auto trait future compatibility lint

8b586e6

Fix tests and rebase conflict

9d181ac

Parse auto traits the same as traits.

97de8ca

This moves the well formedness checks to the AST validation pass. Tests were adjusted. The auto keyword should be back-compat now.

update unstable book and error example

94b07a9

Rename trait_has_auto_impl to trait_is_auto

27efe12

fix rebase conflict

5e74353

rustfmt is broken which makes rls broken

3241f45

Update pretty test for auto trait syntax.

ca26f01

Fix unsafe auto trait pretty print.

5190abb

It was being printed wrong as auto unsafe trait

leoyvens force-pushed the implement-auto-trait-syntax branch from 9e7a5eb to 5190abb Compare November 3, 2017 18:13

kennytm 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 Nov 3, 2017

bors merged commit 5190abb into rust-lang:master Nov 4, 2017

bors mentioned this pull request Nov 4, 2017

Move collector to librustc_mir::monomorphize #45525

Merged

killercup added a commit to killercup/rustfmt that referenced this pull request Nov 5, 2017

Fix compile on 2017-11-04 nightly

5365266

Breakage came from rust-lang/rust#45247

killercup mentioned this pull request Nov 5, 2017

Fix compile on 2017-11-04 nightly rust-lang/rustfmt#2118

Merged

bstrie mentioned this pull request Nov 7, 2017

Tracking Issue for auto traits (auto_traits) -- formerly called opt-in built-in traits (optin_builtin_traits) #13231

Open

24 tasks

dtolnay mentioned this pull request Nov 18, 2017

Change syntax of auto traits dtolnay/syn#238

Closed

leoyvens mentioned this pull request Dec 3, 2017

Remove impl Foo for .. {} in favor auto trait Foo {} #46480

Closed

petrochenkov mentioned this pull request Dec 6, 2017

syntax: Rewrite parsing of impls #46455

Merged

leoyvens deleted the implement-auto-trait-syntax branch March 7, 2018 10:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Syntax] Implement auto trait syntax #45247

[Syntax] Implement auto trait syntax #45247

leoyvens commented Oct 12, 2017 •

edited

Loading

rust-highfive commented Oct 12, 2017

kennytm commented Oct 13, 2017

leoyvens commented Oct 13, 2017

nikomatsakis commented Oct 13, 2017

nikomatsakis left a comment

nikomatsakis Oct 13, 2017

nikomatsakis Oct 13, 2017

nikomatsakis commented Oct 13, 2017

nikomatsakis commented Oct 13, 2017

nikomatsakis commented Oct 13, 2017 •

edited

Loading

leoyvens commented Oct 13, 2017

nikomatsakis commented Oct 13, 2017

rfcbot commented Oct 13, 2017 •

edited by aturon

Loading

eddyb commented Oct 13, 2017

cramertj commented Oct 13, 2017

leoyvens commented Oct 14, 2017

bors commented Oct 14, 2017

kennytm commented Nov 3, 2017

bors commented Nov 3, 2017

bors commented Nov 3, 2017

bors commented Nov 4, 2017

[Syntax] Implement auto trait syntax #45247

[Syntax] Implement auto trait syntax #45247

Conversation

leoyvens commented Oct 12, 2017 • edited Loading

rust-highfive commented Oct 12, 2017

kennytm commented Oct 13, 2017

leoyvens commented Oct 13, 2017

nikomatsakis commented Oct 13, 2017

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis Oct 13, 2017

Choose a reason for hiding this comment

nikomatsakis Oct 13, 2017

Choose a reason for hiding this comment

nikomatsakis commented Oct 13, 2017

nikomatsakis commented Oct 13, 2017

nikomatsakis commented Oct 13, 2017 • edited Loading

leoyvens commented Oct 13, 2017

nikomatsakis commented Oct 13, 2017

rfcbot commented Oct 13, 2017 • edited by aturon Loading

eddyb commented Oct 13, 2017

cramertj commented Oct 13, 2017

leoyvens commented Oct 14, 2017

bors commented Oct 14, 2017

kennytm commented Nov 3, 2017

bors commented Nov 3, 2017

bors commented Nov 3, 2017

bors commented Nov 4, 2017

leoyvens commented Oct 12, 2017 •

edited

Loading

nikomatsakis commented Oct 13, 2017 •

edited

Loading

rfcbot commented Oct 13, 2017 •

edited by aturon

Loading