Skip to content
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 2024 expression overflowing #6260

Merged

Conversation

calebcartwright
Copy link
Member

@calebcartwright calebcartwright commented Aug 2, 2024

This PR does two things:

These two items were combined in the same PR because it's much, much easier to test and demonstrate the former by the latter, and the latter cannot be done without the former.

The vast majority of the diff consists of test snippets taken from existing tests, so shouldn't be as difficult a review as one might infer from the diff numbers. Commits structured in a way that's hopefully logical and conducive to commit-by-commit review

There's a few things we can probably bikeshed on (names, inclusion of helper functions to minimize number of unnecessary callsite changes, etc.) that I'll add inline comments on later

r? @ytmimi

Copy link
Contributor

@ytmimi ytmimi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went through each commit one by one. They were organized very logically and that made the PR much easier to review. I've very happy to move forward with the PR without any bikesheding!

@calebcartwright
Copy link
Member Author

From a design perspective, there were a few ways I could see that we could potentially utilize for having configuration defaults that could vary by style edition (ordered only for reference, not in any preference/priority):

  1. "on_set" event inline overrides
  2. load 2015 & then merge with detected style edition version defaults
  3. load 2015 & reload with detected style edition
  4. load based on inputted/calculated style edition

All of these options have drawbacks, but I opted for option (d) as I think it's the lesser of the evils in the grand scheme of things.

The issue with (a) is that while it works and would likely be maintainable for the 2024 edition, it is a tad hacky and overtime would likely grow to be rather unwieldy.

The issue with (b) is that there are various configuration options who have cascading effects on other options (e.g. use_small_heuristics) so attempting to merge two config structs while maintaining the user specified values starts to get much more complex than one would first anticipate (e.g. imagine edition 2027 needs to change the default value of array_width but not the value of use_small_heuristics). Trying to merge in a scenario where the user has specified a non-default value for use_small_heuristics and/or max_width (or any other combination of cascading options) is technically possible, but I couldn't see a straightforward way of doing it without breaking internal encapsulation and duplicating responsibilities within the codebase

The issue with (c) is that it's inherently inefficient; going through the entire configuration loading process just to determine the derived style_edition value only to then do the same thing all over again with the now known style edition value

The downside to the proposed path here, option (d), is that I feel it does bleed some CLI specifics a bit into the configuration loading process. The CliOptions struct is actually already defined within the config module so there's been some level of familiarity there from the outset, but I did have to extend this trait so that in addition to having a getter-like function for the inline config, there's now similar functions for grabbing the style edition, edition, and (temporarily) the version value. I don't love it, but as I said at the outset, lesser of the evils

@@ -1,3 +1,4 @@
error_on_line_overflow = true
error_on_unformatted = true
style_edition = "2024"
overflow_delimited_expr = false
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this to reduce churn, as the 2024 default (true) will require some updates to the formatting of the rustfmt codebase itself

@@ -210,7 +208,7 @@ macro_rules! create_config {
)+

#[allow(unreachable_pub)]
pub fn default_with_style_edition(style_edition: StyleEdition) -> Config {
pub(super) fn default_with_style_edition(style_edition: StyleEdition) -> Config {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may have just been caught up in the difficulty of naming things, but I did feel odd having this function as well as default_for_possible_style_edition. While the function names imply a lot of overlap, I do feel they are doing very different things.

This function is very much instantiating a config with the provided style edition, whereas default_for_possible_style_edition is more of a "figure out what style edition we should be using, and then give me the default config for that style edition"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth adding doc comments to explain what each function's purpose is within the codebase.

Comment on lines +373 to +375
pub(super) fn from_toml(toml: &str, dir: &Path) -> Result<Config, String> {
Self::from_toml_for_style_edition(toml, dir, None, None, None)
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I debated with myself whether this was necessary, but I opted to put it in for now so as to reduce the amount of diff in the PR. I think we can consider removing it later and just updating the various call sites (which were all in tests if I recall correctly)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I didn't see any issue with from_toml, and if it's only used in test code then we don't have to make any changes right now

Ok(parsed_config) => {
if !err.is_empty() {
eprint!("{err}");
}
Ok(Config::default().fill_from_parsed_config(parsed_config, dir))
Ok(parsed_config.to_parsed_config(style_edition, edition, version, dir))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also didn't love the parsed_config.to_parsed_config piece here, as the idents make for a confusing combination. I think this could be avoided with a simple rename but as always naming things is hard 🤷

Copy link
Contributor

@ytmimi ytmimi Aug 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I saw that and thought the wording was a bit confusing, but nothing too bad to stop us from moving forward with these changes.

@ytmimi
Copy link
Contributor

ytmimi commented Aug 16, 2024

From a design perspective, there were a few ways I could see that we could potentially utilize for having configuration defaults that could vary by style edition (ordered only for reference, not in any preference/priority):

  1. "on_set" event inline overrides
  2. load 2015 & then merge with detected style edition version defaults
  3. load 2015 & reload with detected style edition
  4. load based on inputted/calculated style edition

All of these options have drawbacks, but I opted for option (d) as I think it's the lesser of the evils in the grand scheme of things.

The issue with (a) is that while it works and would likely be maintainable for the 2024 edition, it is a tad hacky and overtime would likely grow to be rather unwieldy.

The issue with (b) is that there are various configuration options who have cascading effects on other options (e.g. use_small_heuristics) so attempting to merge two config structs while maintaining the user specified values starts to get much more complex than one would first anticipate (e.g. imagine edition 2027 needs to change the default value of array_width but not the value of use_small_heuristics). Trying to merge in a scenario where the user has specified a non-default value for use_small_heuristics and/or max_width (or any other combination of cascading options) is technically possible, but I couldn't see a straightforward way of doing it without breaking internal encapsulation and duplicating responsibilities within the codebase

The issue with (c) is that it's inherently inefficient; going through the entire configuration loading process just to determine the derived style_edition value only to then do the same thing all over again with the now known style edition value

The downside to the proposed path here, option (d), is that I feel it does bleed some CLI specifics a bit into the configuration loading process. The CliOptions struct is actually already defined within the config module so there's been some level of familiarity there from the outset, but I did have to extend this trait so that in addition to having a getter-like function for the inline config, there's now similar functions for grabbing the style edition, edition, and (temporarily) the version value. I don't love it, but as I said at the outset, lesser of the evils

I really appreciate you taking the time to outline your thoughts and all the tradeoffs that we could have made here. (d) feels like the best option in my opinion, and it's the one that I likely would have come up with if I was working to implement this. I'm sure we can rework this in the future if it becomes an issue.

@calebcartwright calebcartwright merged commit 46cb7d3 into rust-lang:master Aug 28, 2024
26 checks passed
@calebcartwright calebcartwright deleted the 2024-overflow-delim-expr branch August 28, 2024 01:49
@ytmimi ytmimi added the release-notes Needs an associated changelog entry label Sep 3, 2024
@ytmimi ytmimi removed the release-notes Needs an associated changelog entry label Sep 20, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 23, 2024
Bump stage0 to beta-2024-09-22 and rustfmt to nightly-2024-09-22

I'm doing this to apply the changes to version sorting (rust-lang/rustfmt#6284) that have occurred since rustfmt last upgraded (and a few other miscellaneous changes, like changes to expression overflowing: rust-lang/rustfmt#6260). Eagerly updating rustfmt and formatting-the-world will ideally move some of the pressure off of the beta bump which will happen at the beginning of the next release cycle.

You can verify this is correct by checking out the changes, reverting the last commit, reapplying them, and diffing the changes:

```
git fetch [email protected]:compiler-errors/rust.git bump
git checkout -b bump FETCH_HEAD
git reset --hard HEAD~5
./x.py fmt --all
git diff FETCH_HEAD
# ignore the changes to stage0, and rustfmt.toml,
# and test file changes in rustdoc-js-std, run-make.
```

Or just take my word for it? Up to the reviewer.

r? release
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-2024-edition Style Edition 2024 p-high
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants