stabilize dyn Trait in Rust 2015

[nikomatsakis]

This is a sub-issue of #44662 proposing the Rust 2015 stabilization of dyn Trait.

Status

The decision is made, we just need a stabilization PR!

Details

The new syntactic form dyn <relative-path> is added. It is an error to use dyn <relative-path> if path does not name a trait. The result is a so-called dynamic trait type.

For backwards compatibility, dyn can still be used as an identifier -- in other words, it is a contextual keyword. Note also that dyn :: foo parses as a path dyn::foo and not a use of the dyn keyword. You can however write dyn (::foo) to clarify your meaning.

There is a "allow by default" lint that suggests uses of Trait be rewritten to dyn Trait. Much code in rustc was converted using rustfix in conjunction with this lint. I believe the current plan is to keep this lint as "allow by default" for the time being, but make it as part of the "idiom shift" lints for Rust 2018.

Tests

Here are some tests documenting the current behavior:

• Positive uses of dyn Trait, including a use of dyn (::Path).
• dyn by itself is an identifier
• dyn::foo is a path

Other details

Once this is stable, we should adjust the output from ppaux to use it! (#49277)

[nikomatsakis]

cc @petrochenkov, who implemented, and @Manishearth, who has been working on lint etc

[nikomatsakis]

@rfcbot fcp merge

I propose that we stabilize the Rust 2015 behavior of dyn Trait. Details found in the issue header.

[rfcbot]

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

• @aturon
• @cramertj
• @eddyb
• @nikomatsakis
• @nrc
• @pnkfelix
• @withoutboats

No concerns currently listed.

Once a majority of reviewers approve (and none object), 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.

[nikomatsakis]

(Here is a question to ponder, that does not I think block stabilization: for Rust 2018, is it working making dyn a true keyword? If we opt towards crate::foo paths, as I currently prefer, then I am leaning towards "no", since the only weird behavior is around dyn :: foo, and ::foo paths are likely deprecated, so there is then only one reasonable interpretation of such a path. This allows for a gentler transition into Rust 2018.)

[Manishearth]

we opt towards crate::foo paths, as I currently prefer, then I am leaning towards "no", since the only weird behavior is around dyn :: foo, and ::foo paths are likely deprecated, so there is then only one reasonable interpretation of such a path. This allows for a gentler transition into Rust 2018.

This isn't consistent -- crate::foo is an idiom and not a reqirement; however making dyn::foo parse differently is a breaking change; we can't rely on an idiom change to make a breakage "non breaking".

Though it seems like you're suggesting that we could scope the epoch breakage so that dyn stays a contextual keyword, but now dyn::foo parses as dyn Trait -- i.e. instead of breaking all dyn idents we only break it in this one case. That could work, I think.

I do think that contextual keywords should eventually become true keywords and we're well poised to just make it a keyword with the epoch, so idk.

[nikomatsakis]

@Manishearth

however making dyn::foo parse differently is a breaking change;

I am proposing not making it parse differently. It currently parses as a path dyn::foo, and I am claiming that that is the only sensible interpretation in Rust 2018, since ::foo would not be a path on its own (well, it's a deprecated form of path). Though we might want to lint against it.

Though it seems like you're suggesting that we could scope the epoch breakage so that dyn stays a contextual keyword, but now dyn::foo parses as dyn Trait -- i.e. instead of breaking all dyn idents we only break it in this one case. That could work, I think.

As I wrote above, I am proposing that dyn::foo continues to parse as a path, not as a usage of dyn Trait (though perhaps it would be linted against).

I do think that contextual keywords should eventually become true keywords and we're well poised to just make it a keyword with the epoch, so idk.

This is an interesting policy question. Some cases, like catch, really must change. But other cases, like union and -- I think -- dyn, don't necessarily have to. So should we change them to keywords in Rust 2018? I feel divided. I sort of like the idea of 'cleaning up' and being consistent, but I also sort of like the idea of 'make it a lint error and maybe make it a hard error in the next epoch'.

[nikomatsakis]

I noticed something: in our error messages, we don't use dyn (of course). Once it's stable, we should fix that! (Filed #49277)

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[leoyvens]

I wonder if by stabilizing the new syntax with the exact same semantics as the old one we are not missing an opportunity to make small semantic changes without breaking existing code? I don't have any such changes in mind, it's just a thought.

[rfcbot]

The final comment period is now complete.

[nikomatsakis]

@leodasvacas I have wondered the same thing. Occasionally I dream about making dyn Trait be equal to Box<Trait>, so as to remove a major source of unsized types, for example. Still, that's obviously not a viable plan, and I don't really know of any major proposals for making changes here, so I feel ok about making this move -- there are real "code comprehension" costs to the current syntax, I find.

[nikomatsakis]

The final comment period is up, let's do this! There are general instructions on how to stabilize a feature given here:

https://forge.rust-lang.org/stabilization-guide.html

In this case, the feature name in question is dyn_trait.

[pvdrz]

I'd like to take care of the stabilization PR :)

[nikomatsakis]

@christianpoveda encountered a question (we were discussing on IRC) that I didn't know the answer to. @Manishearth maybe you can help. Right now, the dyn_trait feature also enables some future compatibility lints:

rust/src/librustc/hir/lowering.rs

Lines 4090 to 4100 in eec3208

	fn maybe_lint_bare_trait(&self, span: Span, id: NodeId, is_global: bool) {
	if self.sess.features_untracked().dyn_trait {
	self.sess.buffer_lint_with_diagnostic(
	builtin::BARE_TRAIT_OBJECT,
	id,
	span,
	"trait objects without an explicit `dyn` are deprecated",
	builtin::BuiltinLintDiagnostics::BareTraitObject(span, is_global),
	)
	}
	}

However, I believe that we would prefer this lint to be gated on the edition that is in use. @Manishearth how should we tweak this line?

[Manishearth]

What's the reasoning behind that? If dyn trait is going to exist on both epochs, no need to gate the lint, yes?.

(Also, this isn't a future compat lint anymore)

[nikomatsakis]

@Manishearth

What's the reasoning behind that? If dyn trait is going to exist on both epochs, no need to gate the lint, yes?

True, we could just enable the lint (but make it not future-compat). My assumption though was that we didn't want to be quite so forceful in terms of nagging people to convert their code, at least until Edition 2018 (or until they opt into the 2018 transition lints). Maybe others feel differently.

[Manishearth]

Ah. Well, in that case I can make it such that the lint always exists but is Allow on 2015? We have EpochDeny, I can add EpochWarn.

[nikomatsakis]

cc @rust-lang/lang -- I'd like to hear from others. Do we think that we ought to warn by default in Edition 2015 regarding the dyn keyword?

My inclination for now is to take it slow. We can always add in such warnings later. But maybe I'm being too cautious. Among other things, though, I am reluctant to unleash an avalanche of warnings before the cargo rustfix story is very strong.

[Manishearth]

Come to think of it, the original plan was that we enable dyn for both epochs, and then turn on this warning a couple of months into the epoch (i.e. well after this feature stabilizes). We still have that option -- both as a 2018-only lint or a both lint.

[nikomatsakis]

@Manishearth right -- so what are the concrete steps that @christianpoveda should take? It seems like we should always emit the lint, but we should modify its definition to be "allow by default", right? (Poking at the code, I'm finding it a bit confusing actually -- I can't tell where/how the "priority" of this lint gets upgraded from Allow...)

[Manishearth]

The lint is currently always-allow and it should stay that way for now. Christian just needs to remove the feature gate in the lint.

Making it EpochWarn or EpochDeny can be done later as y'all decide what we actually want to do here.

[joshtriplett]

I had assumed that we would just always leave dyn contextual, and leave the lint as allow-by-default in 2015.

[Manishearth]

rust/src/librustc/lint/builtin.rs

Line 253 in eec3208

Allow,

is where it's Allow, FWIW. We can switch that to Warn, but if we want it to be EpochWarn I have to add functionality for that (willing to mentor, but we should do this in a later step)

[nikomatsakis]

Closed by #49968

[dhardy]

cc @rust-lang/lang -- I'd like to hear from others. Do we think that we ought to warn by default in Edition 2015 regarding the dyn keyword?

I realise this decision was made a long time ago, but do me &Trait (without dyn) is perfectly valid Edition 2015 code, and I don't appreciate being nagged about it (especially since I often see the warning in other people's code and in old code not receiving updates excepting for bug fixes).

In the future could we please be more cautious about deprecating previously-valid code in an existing epoch?

[cramertj]

It's just as valid of code in 2018 as it is in 2018-- the warning and the new syntax aren't edition-related.

[dhardy]

The syntax is valid, but the deprecation is another matter. If &Trait is to become illegal in future Edition 2015 code, then this will be a breaking change (defeating the point of editions). But if not, then I see no use to the deprecation warning — it is only of use when migrating to Edition 2018 (because lets face it, there is little reason to continue using E2015 with in code being actively maintained).

[cramertj]

it is only of use when migrating to Edition 2018

I don't agree with this. The syntax is not a hard error in either edition. It is a warning in both editions. It is not an edition-related feature or change.

[dhardy]

It is edition-related since only E2018 implemented this properly (with a new keyword). Since this discussion was already happening way before E2018 was a thing, I don't understand why &Trait isn't a hard error there, but that's a different topic.

But my point is more that breaking changes are not expected in an existing edition, and the purpose of default lints is to point out real issues (e.g. unused code is often the result of an unfinished job). Breaking changes to the standard library API have been accepted within E2015 in a few cases, but this change affects far more code.

So I ask again: is there any plan to make this a hard error within Edition 2015 (bearing in mind that any code still using E2015 is obviously not being updated for some reason)? Because if not, this deprecation warning has no raison d'être (though it would not be out of place in Clippy, phrased as a "recommended style" or some such).

[petrochenkov]

This warning seems to affect the majority of 2015 crates, e.g. when looking through crater logs the walls of dyn Trait warnings is the primary thing I see.
Enabling warnings of this scale is not reasonable, so would be better to make it edition-dependent even if it's not right now.

[Manishearth]

I don't agree with the reasoning that we should never add new lints unless things are being deprecated to the point of being removed.

This is a form of deprecation, just one where we don't intend to remove support for the syntax.

I don't see an issue with the scale of the warnings. These are easy to fix with rustfix. If lots of people see them it's no big deal, this happens with other kinds of new warnings reasonably often as well (we had a bunch with ..=, and some more with the float match changes)

[dhardy]

These are easy to fix with rustfix.

This assumes one intends to modify the code in question, and do so immediately.

At the end of the day we can live with this, but from my POV the nuisance out-weighs the benefit.

[Manishearth]

No, it doesn't, it's a warning, you can also choose to not fix it.

[whitequark]

No, it doesn't, it's a warning, you can also choose to not fix it.

This is technically true but does not account for the fact that deprecation warnings carry a lot of psychological pressure to fix them (which is also why they work in the first place).

[cramertj]

If someone is the type of programmer who feels compelled to fix all warnings, then I believe they should also want new warnings to be enabled that would help improve their code. If they don't believe following the warning improves their code, it's easy to disable the warning.

[whitequark]

If they don't believe following the warning improves their code, it's easy to disable the warning.

Indeed, and the key part is "their code". To quote @dhardy:

[...] especially since I often see the warning in other people's code and in old code not receiving updates excepting for bug fixes

[Ixrec]

I think most of this was already settled by the Editions RFC and the dyn Trait RFCs, and we're almost reiterating discussions that have already happened, but I also think there's a valid point buried under the confusion.

First, the relevant part of the Editions RFC:

There are only two things a new edition can do that a normal release cannot:

• Change an existing deprecation into a hard error.
  • This option is only available when the deprecation is expected to hit a relatively small percentage of code.
• Change an existing deprecation to deny by default, and leverage the corresponding lint setting to produce error messages as if the feature were removed entirely.

The second option is to be preferred whenever possible. Note that warning-free code in one edition might produce warnings in the next edition, but it should still compile successfully.

The relevant part of the dyn Trait RFC:

On the current edition:

• The dyn keyword will be added, and will be a contextual keyword
• A lint against bare trait syntax will be added

In the next edition:

• dyn becomes a real keyword, uses of it as an identifier become hard errors
• The bare trait syntax lint is raised to deny-by-default

This follows the policy laid out in the editions RFC, where a hard error is "only available when the deprecation is expected to hit a relatively small percentage of code." Adding the dyn keyword is unlikely to affect much code, but removing bare trait syntax will clearly affect a lot of code, so only the latter change is implemented as a deny-by-default lint.

In particular, some of the comments above seem to be simply confused about these points:

• while the status of dyn as a keyword does depend on the edition, the &dyn Trait syntax is available on both editions and is strongly preferred to the &Trait syntax on both editions, so debating whether the lint is "edition-related" is at best completely ambiguous and unhelpful
• the wording above makes it pretty clear that when some syntax is "deprecated", it is "deprecated" on both editions, even though it may never be fully removed from either edition

Non-obvious historical context: I wrote the dyn Trait RFC before the big discussion surrounding the Epochs Editions RFC. The text I just quoted from it is the part which I had to update after the Editions RFC achieved consensus. That's why my arguments in the Motivation section are talking about the then-upcoming impl Trait stabilization as a source of impending churn rather than the then-upcoming 2018 edition, yet this one part quotes the Editions RFC verbatim.

Now, with all that out of the way, the semi-buried but valid novel point I see in this discussion is:

When I updated the dyn Trait RFC, I did not notice that the argument for a warn-by-default lint against bare trait syntax on Rust 2015 becomes much weaker when you realize dyn Trait came out around the same time as a new Edition, and I don't think anybody else noticed this either. In all fairness, it is pretty minor compared to the "wait, can we actually get away with this?" question that correctly dominated the discussion.

So even though the explicit consensus of that RFC was to warn against it on 2015, in hindsight I have no objection to softening that now. Especially since we're also considering making the 2015 edition be a warning unto itself, it does seem a bit silly to have more than one warning that effectively says "this is 2015 code" on by default.