-
Notifications
You must be signed in to change notification settings - Fork 1.6k
RFC: 2021 Edition #3085
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RFC: 2021 Edition #3085
Conversation
Thanks to simulacrum, rylev, ehuss, and others for feedback.
I've generally been against having an edition 2021 so far, but this assuages my main concerns. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A huge step forward in terms of the previous proposal, imo. My main concerns were resolved, and I like this new lean approach for editions. Great work! 👍
(Thanks all for the comments! I'm a bit busy today but I'll go through and incorporate feedback later this week, hopefully tomorrow.) |
Co-authored-by: Ibraheem Ahmed <[email protected]> Co-authored-by: bstrie <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I like this approach. I have some concerns and I hope they are not hard to fix.
Some examples of changes the editions can make: | ||
|
||
* Introducing a new keyword, as with `async-await`. | ||
* This change comes with automated tooling that rewrites identifiers using that keyword to use `r#` form. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is great. Sure the code works but it's ugly. Yes, the proper fix requires developer interaction but it will require it anyway unless the developer is fine with ugly marker.
Having an option to replace it with some other specific identifier would be much nicer. I realize that it may need to be different in different contexts.
If this is too complicated (but maybe the code can be reused in a general-purpose refactoring tool?), then at least a message saying "Keyword foo was replaced in 5 scopes" would be helpful reminder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be nice if a preference list could be given, e.g. I've seen the pattern that variables which wanted to use internal keywords as a name, were given the name {name}_
(e.g. with an appended underscore at the end) instead of r#{name}
(which I've also seen, but less often).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant having the option configurable. E.g. if you try to apply cargo fix
it will fail "the code uses new keywords as identifiers" message and then the user would supply either:
--raw-identifiers
--replace-ident async=asynchronous --replace-ident await=awaiting
This would probably cover all major use cases. The only thing it can't do is replacing based on context (maybe the different replacement is better in different functions), but this seems like an acceptable limitation to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That does sound like a nice option, but I would prefer not to add it as required by this RFC. We've added keywords in the past using the simpler system and it did seem adequate; one can also do a search-and-replace with ripgrep
readily enough as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My primary concern is people running cargo fix, they'll see it "works" and don't notice changed identifier. So I believe at least warning should be strongly encouraged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. I think one of our key goals is that editions can be migrated to piecemeal, and ultimately in rarely read code, this is fine; asking users to add an allow for working code is not too typical in Rust. Regardless, I think this could be added without changes to this rfc, per usual lang team processes (as an extension to the existing lints around identifier naming); I am not inclined to specify a particular behavior here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant one-time warning emitted from cargo fix
. (Something like "The identifier x, which is now a keyword, was converted to raw form 5 times, you may wish to rename it to something else.")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to prescribe such specifics in this RFC, but I think it may make sense for migrations to have the ability to give some kind of report - I know the closure changes for example could tell you how much the closures in your code changed size, etc. which might be difficult otherwise. I'd say this feels like something you could file an issue for on rust-lang/rust, though, just like any other diagnostic improvement, and we can discuss there.
* The expectation is that `cargo fix` should be able to fix the majority of crates out there, but it is not required that the tooling is able to handle every case. As long as code does not occur frequently in the wild, it is acceptable for the automated suggestions to be inapplicable to edge cases. The metrics section in this RFC includes some guidelines for how to measure this. | ||
* **Upgrade:** Edit your cargo.toml to include `edition = "2021"` instead of the older edition. | ||
* The code should still build, but it may be necessary to make some fixes or other changes. | ||
* **Cleanup:** After upgrading, you should run `cargo fix` again. This second step will apply any remaining changes that are only possible in the new edition. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if it is worth pointing out, but I think the intent here is to use the --edition-idioms
flag at this step.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something that @Manishearth and I have been discussing a bit on Zulip. I am not entirely convinced whether 'edition idioms' as a distinct category makes sense, or whether those lints are simply "style lints" that trigger on Rust 2021 code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be good to discuss this a bit more on Zulip, I'd like to go over some of the examples of idiom lints I guess and think about it.
Co-authored-by: Eric Huss <[email protected]>
Co-authored-by: Eric Huss <[email protected]>
Overall this looks good to me, so let's move to merge. I just left some comments in response to some of the unresolved questions, but I think they are not blockers to merging, and box checking is likely to take some time. @rustbot fcp merge |
Er and I did the same thing Niko was telling me about just today... @rfcbot fcp merge |
Team member @Mark-Simulacrum has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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. |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. The RFC will be merged soon. |
This RFC describes the plan for the 2021 Edition. It supercedes RFC 2052. The proposed 2021 Edition is intentionally more limited than the 2018 Edition. Rather than representing a major marketing push, the 2021 Edition represents a chance for us to introduce changes that would otherwise be backwards incompatible. It is meant to be marketed in many ways as something closer to "just another release".
Rendered
Key points include:
This RFC is meant to establish the high-level purpose of an edition and to describe how the RFC feels to end users. It intentionally avoids detailed policy discussions, which are deferred to the appropriate subteams (compiler, lang, dev-tools, etc) to figure out.
Thanks
Thanks to @Mark-Simulacrum, @rylev, @ehuss and others for feedback.