Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Edition 2021 and beyond #2966
Edition 2021 and beyond #2966
Changes from 8 commits
7464420
f30e7fe
09bc9a3
7ef2476
b1fc149
ebf2b49
235bd82
3b57d66
8cded7b
012dd80
8c12c54
3cc771e
0a4b1ed
abe46e7
329cbe0
178e646
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
"expect", while I also expect this, we may want to explain why we expect this and how we intend to maintain this expectation over time as the "we" changes.
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 think there are 2 very important points here that deserve their own sections:
they are related (as suggested here) but i think there are several ways in which they may be at odds with each other. for example: if we judge the changes individually, by what mechanism do we decide that the edition is not achievable or "too big" or too many changes, or too many similar/conflicting changes that are hard to explain.
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.
To me the paragraphs here don't really answer the question "What is a Rust edition?".
It is briefly touched in the last pragraph but not in detail. So it is just that year's version that allows us to introduce features that would otherwise be impossible (so are these breaking changes I assume or just big features e.g. should async go here? -- this isn't clear).
Then the question remains, if there are no breaking changes, why do we want a new edition? Wouldn't that cause confusion? For example, do we need to still set the edition number in my
cargo.toml
if there are 0 breaking changes?Would crates that don't change the edition number miss on potential improvements that are tagged with an edition 201X in that case (in the 0 breakage case)?
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.
So that you can always know to set your edition value to 2015+3x and you'll get a valid number.
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.
We could should probably start here with: "A rust edition is ..." and maybe also mention what it is not, and the other paragraphs regarding name, cadence and opportunity to look back and celebrate can follow.
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 guess my main worry (besides the lack of a definition) is the dual meaning of an edition, it can be a breaking change or can be a non-breaking changes at the same time (depending on the year).
This is the main reason that makes me ask all the questions above and I am sure that other users of rust may have the same dificulties.
I am more of a fan where things have a clean meaning in all situations:
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 posted this in the PR which I should ask here. I am thinking that if there are no breaking change, means that people may not change to the newer edition since it is the same. Questions
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 think a tl;dr here that says "Edition are how Rust introduces changes to the language that may be breaking in a non breaking way."
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.
s/easy/low effort
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.
"You may have to do a bit of cleanup after the tool runs, but it shouldn't be much."
This feels like it doesn't match the language of the rest of the RFC, i might reword
"It is not possible to automate all changes. As a result, some manual changes may be required in addition to the tool. The goal is to keep these as minimal as possible."
questions:
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 thing worth highlighting somehow:
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.
how would "but may no longer compile on edition X" work? Code shouldn't suddenly "no longer compile on <specification from the past>"
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 had to read that twice too. I think what @Manishearth meant is that -- once you've applied the idiom lint, the resulting code (in the new edition) may not compile in the old edition anymore.
To be honest, I had forgotten that migrations are supposed to target the "intersection" of editions, so this is perhaps a good addition -- but I'm not sure if it's always true. I remember thinking that we might have to have cases were the migration required you to use the new edition. But I guess we avoided those for the module transition? I'd be ok writing the above as the general rules -- kind of a "unless otherwise stated" sort of thing.
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.
@nikomatsakis The migration/idiom split is deliberately designed to deal with this specific problem. Idiom lints do not necessarily produce code that does not compile on older editions, but typically do. That's more central to their working than "will become on by default in the next edition" actually.
I'm talking about the migration present within those lints.
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.
The reasoning here is that it is not always possible to migrate wholesale, so the edition lints migrate it to be compatible with the new edition (while still compiling on the old one so that you can perform any manual fixes). The idiom lints finish off any parts of the migration and apply potentially incompatible changes that make the code more idiomatic (but are not necessary to make the code compile).
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.
Yeah, this is sounding quite familiar. I will add some text to that effect.
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.
However, I believe that idiom lints can also be used to change the default severity from warning (older editions) to error (newer editions) correct? I'll have to review the code, but I'm pretty sure that's something we also want.
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.
OK, I pushed some changes -- @Manishearth please take a look and see if this sounds right to you.