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.

[nikomatsakis]

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

[nikomatsakis]

@tomhoule just checking in -- any progress? Any questions I can help with?

(BTW, with respect to the open question about the precedence of dyn, I think it makes sense (for now) to implement it precisely the same way that impl Trait works when parsing. They should behave the same, however they work.)

[tomhoule]

Since it's my first attempt at working on the compiler I'm taking off slowly, I haven't got much further than setting things up and making sure I completely understand the RFC. I expect I'll have more time this week to start actually writing code and come back to you with questions. Thanks for the clarification on precedence :)

[gaurikholkar-zz]

@tomhoule are you still working on this?

[tomhoule]

Yes, I've made some progress and currently working on the parsing part and starting to write tests. I'll work on it more over the week-end and make sure to open a PR or report back here if it doesn't look doable by monday at the latest :)

[tomhoule]

@gaurikholkar It doesn't look like I'll be able to finish this today, and I won't have free time before next week-end so you or someone else should feel free to take the issue. Sorry for delaying this feature :/

[petrochenkov]

Implemented in #45175

[cramertj]

The box for the lint at the top of this thread is checked, but I can't seem to get the dyn Trait lint to fire on playground. What am I doing wrong?

[est31]

@cramertj adding #![deny(rust_2018_idioms)] works.

[Manishearth]

The lint is opt in basically

[eddyb]

#![deny(rust_2018_idioms)]

Has anyone pointed out how this seems somewhat flipped? Shouldn't it be rust_2018_unidiomatic or something similar, evoking "code that should be changed in Rust 2018"?

EDIT: OTOH, this is only because of the use of verbs like allow and deny, making the lint name appears to be the object that the verb acts on, as opposed to "just a name".

[eddyb]

We need to make sure that we handle dyn< correctly: currently it's treated as a generic type named dyn but in the new edition it should be a syntax error.

I have a suggestion for the Rust 2018 side of this, that's maybe a bit late:
We can replace the wordy dyn for<'a> Fn(&'a T) -> &'a U with dyn<'a> Fn(&'a T) -> &'a U.
(and then turn the former into a syntax error)
I don't know why we didn't do this for impl Trait either.

@rust-lang/lang What do you think?

[alexreg]

I like the explicitness of dyn for<'a> ... personally.

[Centril]

With respect to @eddyb's comment:

• I agree that dyn< should be a syntax error in Rust 2018.
• I think dyn<'a> Fn(&'a T) -> &'a U is nice as a shorthand and may lead us to dyn<T: Bar> long term.
• I don't think we need to make dyn for<'a> Fn(&'a T) -> &'a U illegal. Having it, even tho redundant, is good for consistency and preserving the property that dyn $bound works as much as possible improves uniformity. This may also make macros happier.

[scottmcm]

@eddyb I don't think dyn<'a> is important enough to try to rush it into the edition (without prejudice).

Maybe start an IRLO thread or RFC issue to discuss it? I'm not convinced here is the best place.

[eddyb]

As long as dyn< is an error and we don't plan to deprecate for<'a> then it's a non-issue.

[petrochenkov]

Note, that for<...> applies to a single bound, but impl<...>/dyn<...> apply to all bounds, so it's not just syntactic sugar.
I don't think we can express something like impl<'a> Trait1<'a> + Trait2<'a> right now outside of where clauses that also have this "widely applied" for<...> (where for<'a> Type<'a>: Trait1<'a> + Trait2<'a>).

[eddyb]

@petrochenkov Huh, I have assumed that it spanned all bounds, oops!
cc @nikomatsakis What's actually intended here?

[Centril]

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.

[Centril]

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?

[rfcbot]

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

Can we make dyn a proper keyword on Rust 2018?

• @Centril
• @aturon
• @cramertj
• @eddyb
• @joshtriplett
• @nikomatsakis
• @nrc
• @pnkfelix
• @scottmcm
• @withoutboats

[Manishearth]

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.

[Centril]

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

[Manishearth]

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

[petrochenkov]

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.

[Centril]

@varkor is looking into this :)

[Centril]

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.

[petrochenkov]

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