Tracking issue for RFC 2113: dyn Trait Syntax for Trait Objects: Take 2

@Manishearth

This is a tracking issue for the RFC "dyn Trait Syntax for Trait Objects: Take 2 " (rust-lang/rfcs#2113).

Implementation plan:

In Rust 2015, accept dyn Foo but we parse dyn ::Foo as dyn::Foo
We lint against uses of dyn as an identifier
- This is a "breakage" lint, so it warns by default in Rust 2015 (and is inapplicable in Rust 2018)
In Rust 2018, we parse dyn only as a dyn Trait, and hence dyn ::Foo
We have a rustfix-compatible lint that suggests rewriting Trait to dyn Trait; if the path Trait is absolute (::Trait), then we suggest dyn (::Trait)
- This is an "idiom shift" lint, in @Manishearth's terminology from Provide a way to distinguish between "breakage" and "idiom" epoch lints? #48796
- It is opt-in via the use of #[warn(rust_2018_idioms)] (Tracking issue for warning for rust_2018_idioms by default #54910)
Use dyn Trait in error messages (use dyn syntax in error messages #49277)

Steps to stabilize:

Adjust documentation (dyn Trait (RFC 2113) reference#279) (see instructions on forge)
Stabilization PR (see instructions on forge)

@petrochenkov

A lot of the interesting question marks here arise around backwards compatibility. Here are some observations:

We could almost introduce dyn completely backwards compatibility. Things like dyn Foo do not parse today, after all. However, there is this one corner case of absolute paths: dyn::Foo parses today as a path, and after this change it would presumably be parsed as dyn (::Foo) rather than (dyn::Foo).
Therefore, I think that ultimately the opt-in (i.e., the new epoch) will be controlling precisely the behavior in this ambiguous cases. In all other cases, code should work in either epoch.
- Moreover, in the 2015 Epoch, we will want to give warnings for paths like dyn::Foo, but I think we should hold off on that until all the other pieces are in place (including likely some RFC for how to "escape" a keyword).

It turns out that we already have support for "paths that must be trait objects" in the AST, which is almost exactly what we need. This is the TraitObject variant of the AST's TyKind enum. This is currently used for paths with a +, like Debug + Sized, which are already known to be trait objects.

We can use this same AST variant to support dyn Foo traits. We will however need to remember (for pretty-printing) whether the dyn keyword was used. We can do this by adding a enum TraitObjectStyle { Dyn, Bare } and including that with the TraitObject variant.

So probably the initial work items are something like this:

In the AST definition, add the TraitObjectStyle enum and include it in the variant.
Modify pretty-printer code to respect TraitObjectStyle.
Modify the parser_ty_common() routine in the parser to accept dyn Foo and create the TraitObject variant.
- For now, dyn::Foo should parse as it does today.
- @petrochenkov -- can you update this with a few notes on how to properly handle dyn as a onditional keyword, and/or point to some code in the parser that can be used as a model? [EDIT: Sure, see the next item.]I think that something like this line or like the handling of impl Trait is roughly what we want.
- dyn needs to be added to the list of weak keywords in libsyntax_pos/symbol.rs. When starting to parse a type, if dyn is encountered we need to look at the next token. If it's something that can start a bound (is_path_start, 'a, ?, for, ( - see fn parse_ty_param_bounds_common), but can't continue a type (::, <, <<, (), then parse it with parse_ty_param_bounds, otherwise continue as if dyn were a usual identifier.

At this point, we should be able to use dyn Foo, and we should get errors if we use it incorrectly, so it'd be good to have tests for:

Using dyn Foo where Foo is a trait (should work).
- This should require a feature gate!
Using dyn Foo where Foo is not a trait (should error...but otherwise act like Foo?).
Using dyn::Foo to refer to dyn::Foo, which is not a trait (should work)

After this point, we need to start thinking about the transition. Hopefully by then more of the epoch machinery is in place.

This can be a parser-only change - "TyKind::DynTrait" already exists and is called TyKind::TraitObject, so if dyn is encountered, bounds following it can be just stored into TyKind::TraitObject without any further changes in resolution/lowering.

Additionally, TyKind::TraitObject can get a flag "dyn was used" to pretty-print faithfully.

@petrochenkov

@petrochenkov ah, nice, I forgot about that! I will update the mentoring instructions accordingly (or feel free to do so yourself...).

I'm interested in tackling this :)

@petrochenkov

@petrochenkov I updated the mentoring instructions. I left one "TODO" in there that maybe you can fill-in.

--

@tomhoule great! let me or @petrochenkov know if anything is unclear from the mentoring instructions. If you're not getting any response, I will warn you that I at least have a hard time keeping up with notifications, so a ping on gitter may be faster. =) (Also, others in that room can likely help.)

Also, we really should resolve the precedence of impl trait, which is directly relevant here. Not sure what's the best way to force that question. cc @rust-lang/lang

@nikomatsakis

@nikomatsakis

Also, we really should resolve the precedence of impl trait, which is directly relevant here.

I think I made my peace with the existing rules, i.e. bounds being parsed greedily after impl (or dyn) in all contexts, i.e.
x as dyn Send + Sync - y is (x as (dyn Send + Sync)) - y, not (x as (dyn Send)) + Sync - y.

@petrochenkov

@petrochenkov

I still find it hard to square that into an actual grammar. In particular, something like this parses:

fn foo<F>(f: F)
    where F: Fn() -> Foo + Send
{ }

parses as F: (Fn() -> Foo) + Send. But if you had where F: Fn() -> dyn Foo + Send, it would parse as F: Fn() -> dyn (Foo + Send). I suppose it can be made to work but it's going to be an awfully tortured setup. (Similarly, it seems pretty inconsistent that you have to write &(Iterator + Send), but you can write &dyn Iterator + Send, but I guess the former is deprecated, so maybe it's a moot point.)

It's times like these that I wish we had made more progress on a formal grammar! I think there are some projects that have done so, though, so we could maybe get some sense for how awful it is.

(Now, perhaps it was ill-advised to make the grammar work the way it does with +... but water under the bridge I suppose.)

I've marked this as due for RC2; I think we need to make dyn< an error per the recent discussion to keep our options open going forward.

Optionally, if it turns out to not be used anywhere (pending crater run), we can also make dyn a proper keyword in 2015. If not, it may be prudent to make it a keyword on 2018.

@eddyb

Me and @eddyb discussed this briefly on Discord.
We came to the conclusion that the cleanest and most uniform way to solve this would be to make dyn a proper keyword on Rust 2018. EDIT: This was also what the original RFC proposed.

While there is a single root crate that uses this, https://crates.io/crates/goblin -- we have done 2018-only keywords for 3 other words (async, try, await), so it makes sense to do this for dyn as well.

So...

@rfcbot poll lang Can we make dyn a proper keyword on Rust 2018?

@Centril

Team member @Centril has asked teams: T-lang, for consensus on:

Can we make dyn a proper keyword on Rust 2018?

AIUI the language team made a decision on this specific point months ago: https://paper.dropbox.com/doc/Keyword-policy--AM1CL55t0gF_Zoh7VeYN~HXSAg-SmIMziXBzoQOEQmRgjJPm

Maybe the circumstances have changed? Back then we didn't have the implementation clarity on keyword edition hygiene we do now. Worth explicitly mentioning what has changed since then.

@Manishearth

@Manishearth see discussion about dyn<'a> above.

While we wouldn't, strictly speaking, need to make dyn a real keyword to enable where dyn<'a> Fn(&'a T) -> &'a U, it is much simpler to make it a real keyword in 2018 to handle that. (also because we are short on time...).

On sourcegraph I found one crate making actual use of dyn as an identifier, so the breakage is extremely small (much smaller than try and friends...). If we wish to revert this after Rust 2018, we can do that.

IIRC the way contextual keywords are implemented makes this not necessarily easy to do, at least this was the case in March. So I'm not sure if "short on time" is valid here. In March one of the concerns was implementation complexity and it's not clear to me that has changed. The breakage is very likely small, yeah, but is it something we can easily implement?

…

On Sat, Sep 15, 2018, 10:32 PM Mazdak Farrokhzad ***@***.***> wrote: @Manishearth <https://github.com/Manishearth> see discussion about dyn<'a> above. While we wouldn't, strictly speaking, need to make dyn a real keyword to enable where dyn<'a> Fn(&'a T) -> &'a U, it is much simpler to make it a real keyword in 2018 to handle that. (also because we are short on time...). On sourcegraph I found one actual use of dyn as an identifier, so the breakage is extremely small (much smaller than try and friends...). If we wish to revert this after Rust 2018, we can do that. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#44662 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABivSHNAxi5YUqAm0ceq_SLJ6shn7Rgeks5ubTKsgaJpZM4PaRse> .

the way contextual keywords are implemented makes this not necessarily easy to do

What exactly, making dyn a keyword on 2018 and a context sensitive identifier on 2015?
This should be simple now after the general strategy of treating edition-specific keywords (per-span editions) is established.

@varkor

@varkor is looking into this :)

We discussed this on this weeks lang team meeting and the general consensus seemed to be that given that we now have a good mechanism to make this a real keyword, we should do so given the added motivation.

Rollup merge of rust-lang#54261 - varkor:dyn-keyword-2018, r=petroche…

I went through the boxes and looks like it's fully done now.

Navigation Menu

Repository navigation

Tracking issue for RFC 2113: `dyn Trait` Syntax for Trait Objects: Take 2 #44662

39 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Tracking issue for RFC 2113: dyn Trait Syntax for Trait Objects: Take 2 #44662

Description

Activity

nikomatsakis commented on Sep 18, 2017

petrochenkov commented on Sep 18, 2017

nikomatsakis commented on Sep 19, 2017

tomhoule commented on Sep 19, 2017

nikomatsakis commented on Sep 19, 2017

nikomatsakis commented on Sep 19, 2017

petrochenkov commented on Sep 19, 2017

nikomatsakis commented on Sep 20, 2017

39 remaining items

Centril commented on Sep 15, 2018

Centril commented on Sep 15, 2018

rfcbot commented on Sep 15, 2018

Manishearth commented on Sep 15, 2018

Centril commented on Sep 15, 2018

Manishearth commented on Sep 15, 2018

petrochenkov commented on Sep 15, 2018

Centril commented on Sep 15, 2018

Centril commented on Sep 20, 2018

petrochenkov commented on Oct 13, 2018

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Tracking issue for RFC 2113: `dyn Trait` Syntax for Trait Objects: Take 2 #44662