-
Notifications
You must be signed in to change notification settings - Fork 898
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
implement Style Edition support #6247
implement Style Edition support #6247
Conversation
@@ -1,4 +1,4 @@ | |||
// rustfmt-version: Two | |||
// rustfmt-style_edition: 2024 |
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 opted to do these as direct mappings, version Two become edition 2024 and version One edition 2015.
There's a reasonable case to be made that we should fan these out across all style editions, but (1) I don't think we should do that in this PR and (2) if we go that route I think we need to make that standard practice and apply it consistently moving forward 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.
Mapping version: Two
-> style_edition: 2024
and version: One
-> style_edition: 2015
seems reasonable to me, and I agree that we don't need to worry about style_edition
2018
and 2021
in this PR.
Brainstorming a bit for a future PR, I wonder if we could establish a file naming convention for style_edition
overrides. The basic idea is that we'd run our idempotency tests once for every style_edition. For example, if we had tests/source/test_file.rs
and tests/target/test_file.rs
, then the assumption would be that formatting wouldn't change between any of the editions, but if we had both tests/target/test_file.rs
and tests/target/test_file.2024.rs
then we'd make sure to test style_edition: 2024
output against the test_file.2024.rs
file, and test all other edition output against the test_file.rs
file.
@@ -0,0 +1,9 @@ | |||
// rustfmt-version: Two |
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.
Just a small test capturing that the the legacy version
option is still supported
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 it might make sense to leave a comment explaining that leaving version=Two
here is intentional to test the legacy version=Two
setting. I could see us forgetting about why we added this in the future.
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.
Here's my first pass at reviewing the PR. Adding style_edition
config and --style-edition
CLI options mostly look good to me, though there are a few things that I think are missing from this PR.
I think we need to make tweaks to load_config
to ensure that we're loading the Config
correctly (based on rules described here), and it would be great to see some (ideally extensive) unit tests that validate all the different ways users might set style_edition
(--style-edition
, --config=style_edition
, style_edition
in rustfmt.toml, --edition
, --config=edition
, edition
in rustfmt.toml).
Calls to Config::default
outside of test code should probably be replaced with calls to Config::default_with_style_edition
, so that we know we're setting the correct defaults based on the style edition before we start applying any overrides.
I also think Config::print_docs
and Config::is_default
need to be updated to not hard code the style_edition
.
lastly, I saw that you added set_version
and set_edition
methods on the Config
, and that each adds logic to override the value of style_edition
. I'm personally not 100% sure if that logic is necessary, and I'll need to think through that a little more.
src/overflow.rs
Outdated
let additional_cases = match (self, config.style_edition()) { | ||
(OverflowableItem::MacroArg(..), StyleEdition::Edition2024) => SPECIAL_CASE_MACROS_V2, |
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 could see this being a subtle bug when the next edition comes out. Probably makes sense to change this to:
let additional_cases = match (self, config.style_edition()) { | |
(OverflowableItem::MacroArg(..), StyleEdition::Edition2024) => SPECIAL_CASE_MACROS_V2, | |
let additional_cases = match self { | |
OverflowableItem::MacroArg(..) if config.style_edition >= StyleEdition::Edition2024 => { | |
SPECIAL_CASE_MACROS_V2 | |
} |
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.
good idea - 3a6eb5c
@@ -1,4 +1,4 @@ | |||
// rustfmt-version: Two | |||
// rustfmt-style_edition: 2024 |
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.
Mapping version: Two
-> style_edition: 2024
and version: One
-> style_edition: 2015
seems reasonable to me, and I agree that we don't need to worry about style_edition
2018
and 2021
in this PR.
Brainstorming a bit for a future PR, I wonder if we could establish a file naming convention for style_edition
overrides. The basic idea is that we'd run our idempotency tests once for every style_edition. For example, if we had tests/source/test_file.rs
and tests/target/test_file.rs
, then the assumption would be that formatting wouldn't change between any of the editions, but if we had both tests/target/test_file.rs
and tests/target/test_file.2024.rs
then we'd make sure to test style_edition: 2024
output against the test_file.2024.rs
file, and test all other edition output against the test_file.rs
file.
@@ -0,0 +1,9 @@ | |||
// rustfmt-version: Two |
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 it might make sense to leave a comment explaining that leaving version=Two
here is intentional to test the legacy version=Two
setting. I could see us forgetting about why we added this in the future.
src/config/config_type.rs
Outdated
if self.was_set().style_edition() { | ||
eprintln!( | ||
"Warning: the deprecated `version` option was \ | ||
used in conjunction with the `edition` or \ | ||
`style_edition` options which take precedence. \ | ||
The value of the `version` option will be ignored." | ||
); | ||
} else if matches!(self.version(), Version::Two) { | ||
self.style_edition.2 = StyleEdition::Edition2024; | ||
} else { | ||
self.style_edition.2 = StyleEdition::Edition2015; | ||
} | ||
} | ||
|
||
fn set_edition(&mut self) { | ||
if self.was_set().style_edition() || self.was_set().version() { | ||
return; | ||
} | ||
|
||
// User has explicitly specified an Edition value without | ||
// explicitly specifying a Style Edition value, so the Style Edition | ||
// must default to whatever value was provided for Edition | ||
// as per: https://rust-lang.github.io/rfcs/3338-style-evolution.html#explanation | ||
self.style_edition.2 = self.edition().into(); | ||
} |
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 need to think on this a little more. I'm not 100% sure that it makes sense to set style_edition
in set_edition
or set_version
, and part of my thinking is that style_edition
has to be known before we call override_value
.
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 has to be done. won't belabor the point any more on this PR, but I think it'll be clearer once you see the next one
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 we should add a set of unit tests that validate the config loading rules by calling load_config
directly instead of the style_edition/default.rs
, style_edition/follows_edition.rs
and style_edition/overrides_edition_when_set.rs
tests. That way we can test out various scenarios like what happens when you specify style_edition
in your rustfmt.toml, but also pass it as --config=style_edition={StyleEdition}
.
I think we'll also need to make some tweaks to the CliOptions
trait to make sure that configs are loaded properly based on the rules outlined here.
At the very least I think we need tests for load_config::<GetOptsOptions>()
, since that's the main implementation called by the rustfmt binary.
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.
as noted down in #6247 (comment) i think an inaccuracy in my comment in Zulip may have been a source of confusion.
I did add quite a lot of testing in 85059ee and from my perspective I think that level of testing was above and beyond the scope of this PR. however, it's a worthwhile set of tests to have anyway and we can build on it for various use cases moving forward
Thank you for the review! Some of the feedback I'd anticipated (I tested heavily locally but knew there were other tests that could be added), but other parts of the feedback I found puzzling or even contradicting which tells me I probably didn't accurately understand what you were trying to convey. If you could elaborate that would be helpful, but given the mix of top level commentary and inline feedback left in the initial review, I'd suggest we start high level here to try to get on the same page before proceeding to inline items so I'll ask a few questions. (a) style_edition value precedence taken from https://rust-lang.github.io/rfcs/3338-style-evolution.html#explanation emphasis mine
this, plus rustfmt's existing, standard process for configuration priority (cli > config file) is what I described in my comment on Zulip that you linked. from my perspective, there's nothing novel here other than the fact that does that align with your understanding, or did you have a different interpretation? (b) I did not intend to hard-deprecate the so here again do you feel differently? were you anticipating that version would be deprecated? (c) style_edition value default the default value of so any invocation of does this align with your understanding or did you have a different interpretation? |
(a) style_edition value precedence (Agreement)Yeah, I'm 100% in agreement on this. (b) version option (Mixed Agreement)I'm not trying to suggest that we hard deprecate I think some of the disagreement is that I think Assuming we replace calls to I agree that the gating mechanism between The main concern I have with As a concrete example, take For overflow_delimited_expr=false
version = "Two" For overflow_delimited_expr=false
version = "Two"
style_edition = "2024" For overflow_delimited_expr=false
version = "One"
style_edition = "2024" For overflow_delimited_expr=true
version = "Two"
style_edition = "2024" A lot of this is speculation on my part, but having read through the config loading code I'm a little worried that things aren't going to be configured correctly based on the PR as currently implemented. (c) style_edition value default (Mixed Agreement)My understanding is that defaults are now dynamic based on the style edition, and I think pinning the default style edition to That said, I agree that the default value should be I would also expect that Taking the PR as currently implemented, if you run
My expectation is that this would output:
As is, setting I know there's a lot here, and I hope the explanations haven't added to any confusion / misunderstanding. Let me know if there are still points that I should further elaborate on. As I mentioned, some of my reasoning is based on how I think the default config values are set, and ultimately I think more extensive unit tests for |
To be explicitly clear, this PR does not finish all the 2024 style edition work, it never intended to. There's of course going to have to be additional PRs that wire in any relevant configuration options that align to 2024 style edition defaults. This PR does nothing more and nothing less than expose
I'm puzzled by this. The first step in this process is essentially transitioning/renaming If we didn't do that, if we printed a warning when people had set
This is predicated on your perspective that this PR should functionally eliminate
I disagree with your expected output. How do you view the differences today between the existing We have to start from a baseline. Every configuration option has to have a default value. Every option, and that includes |
Perhaps another way to think about this.. in whatever version of stable Rust ships the 2024 edition (and edition of the style guide):
I'm not trying to do all of that in this PR. My desired outcome for this PR landing and getting out on nightly is for nightly users to be able to start adding/using |
To state some things more explicitly, this PR:
and I feel like the review and discussion so far has centered on the perspective that the PR lacks those things, or speculation that it wouldn't directly support a specific implementation of those things, which is where I'm feeling confused
There are multiple paths for the future implementation of this feature. Some are more "tactical" in that they can be done quickly and easily but aren't necessarily the more "strategic" options that would provide the more optimal internal code implementation. The same is true in my opinion of #5937 in that it's not perfect across the board and there's plans for future refactoring (e.g. #5937 (comment)) but we press on because the external behavior is unaffected and it allows our edition 2024 work to progress
To expand:
The current behavior of prints:
That's the way This PR doesn't modify those behaviors, and we haven't had any discussions about modifying any of those behaviors. Any changes to those behaviors and any extensions to those behaviors are outside the scope of this PR (though I'll go ahead and note that I think it would be nice to add the ability to to print defaults for a given style edition, but I'd prefer we open a new issue to discuss and decide on how to implement, e.g. perhaps a new It is absolutely possible that certain approaches to implementing rust-lang/rust#123751 could very well require making additional modifications to the config loading process. However, rust-lang/rust#123751 and implementation discussions are not part of the scope of this PR, and there's nothing in this PR that I can see which would prevent any of those implementation options, nor which would make those implementations/reviews more difficult.
Again this is something that I'm puzzled by because of the consistent and oft reiterated message that we would not deactivate the option and that it would be mapped to rustfmt's There are separate, individual tracking issues that cover the "implementation" work to address that gap, but that's not part of the scope of the PR. In #6247 (comment) you said:
That's technically an option, but it's not the one I took here and it's not one that I think we should take. If we do that we'd be adding We're going to map |
Adds the 'style_edition' configuration option along with documentation, 'version' option deprecation, and mapping of 'edition' and 'version' values for backwards compatibility and adherence to the style evolution RFC
Updates the relevant formatting logic to utilize the new 'style_edition' option directly instead of the now deprecated 'version' option. 'version' has only been soft deprecated and has auto mapping in place so there should be zero formatting impact to current 'version' users.
Adds a few tests that validate the various scenarios of precendence, overrides, and defaults to ensure the correct 'style_edition' value is selected even when other options like 'edition' and/or 'version' are included.
Updates all the various files utilized in the system and idempotence tests to directly use the corresponding 'style_edition' configuration as opposed to keeping the original 'version' values and relying on the mapping.
4c8ddb6
to
ec7b298
Compare
ec7b298
to
9375b5a
Compare
9375b5a
to
e362297
Compare
Additional changes made, offline discussion with reviewer who's away for an extended period of time, and intent communicated to proceed with follow up discussion
e362297
to
85059ee
Compare
Summarizing for posterity:
|
This fixes the remaining CI failures. The `rustfmt.toml` change was prompted by @Firestar99's formatting changes in #2, and the subsequent local rebasing, with `version = "Two"` causing warnings due to: - rust-lang/rustfmt#6247 So the perma-unstable `rustfmt.toml` `version` config is now `style_edition = "2024"`, and hopefully that gets stabilized at the same time as Rust 2024 becoming stable.
This PR finishes the implementation of support for Style Editions in rustfmt as laid out in RFC 3338 and required for the 2024 Edition (implementation of rust-lang/rust#123799 will be complete once this is landed).
A few notes for review:
version
->style_edition
that resulted in simple changes in multiple files, and many cases of parts of the rustfmt codebase having to change from single line formatting to multiple lines due to the longer namer? @ytmimi