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

[aturon]

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)

[nikomatsakis]

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.

[petrochenkov]

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.

[nikomatsakis]

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

[tomhoule]

I'm interested in tackling this :)

[nikomatsakis]

@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.)

[nikomatsakis]

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

[petrochenkov]

@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.