Impl `StyleEditionDefault` trait for all configs #5937

ytmimi · 2023-10-08T23:31:07Z

This PR is a follow up to #5933, ~~but currently includes all the changes from that PR since it hasn't been merged yet~~.

This PR makes internal changes to define each configuration in terms of the StyleEditionDefault trait, and hard codes StyleEdition::Edition2015 anywhere the new style_edition is needed.

Note users are still unable to configure style edition.

r? @calebcartwright

Edit: #5933 was merged and I rebased this PR.

ytmimi · 2023-10-09T00:14:43Z

src/config/mod.rs

+    max_width: MaxWidth, true, "Maximum width of each line";
+    hard_tabs: HardTabs, true, "Use tab characters for indentation, spaces for alignment";
+    tab_spaces: TabSpaces, true, "Number of spaces per tab";
+    newline_style: NewlineStyleConfig, true, "Unix or Windows line endings";
+    indent_style: IndentStyleConfig, false, "How do we indent expressions or items";


Not a change we need to make now (or ever), but as I was working on this I wondered if we could generalize StyleEditionDefault and rename it to something like RustfmtConfigOption (happy to bikeshed on the name). That trait could also be used to define each configuration option's description, stability, and default value for different style editions.

For example:

use crate::config::StyleEdition; use crate::config::config_type::ConfigType; pub trait RustfmtConfigOption { type ConfigType: ConfigType; fn default(style_edition: StyleEdition) -> Self::ConfigType; fn description() -> &'static str; fn stability() -> bool; }

That way, create_config! could focus on setting field names and tys that impl RustfmtConfigOption.

For example if we implemented RustfmtConfigOption for MaxWidth it might look something like this:

impl RustfmtConfigOption for MaxWidth { type ConfigType = usize; fn default(_: StyleEdition) -> Self::ConfigType { 100 } fn description() -> &'static str { "Maximum width of each line" } fn stability() -> bool { true } }

And then the call to create_config! would be simplified to:

create_config! { max_width: MaxWidth, }

This would also eliminate some of the duplication for the test config we create in mod test

I like the sound of this, but agree it's something worth deferring

ytmimi · 2023-10-09T00:15:58Z

src/config/options.rs

+config_option_with_style_edition_default!(
+    // Fundamental stuff
+    MaxWidth, usize, 100;
+    HardTabs, bool, false;
+    TabSpaces, usize, 4;
+    NewlineStyleConfig, NewlineStyle, NewlineStyle::Auto;
+    IndentStyleConfig, IndentStyle, IndentStyle::Block;


Defines a unit struct for each config option that implements StyleEditionDefault

ytmimi · 2023-12-16T18:11:46Z

src/config/mod.rs

-pub use crate::config::lists::*;
-#[allow(unreachable_pub)]
-pub use crate::config::macro_names::{MacroSelector, MacroSelectors};
+pub use crate::config::macro_names::MacroSelector;


The unused import lint was triggering for crate::config::lists::*; and MacroSelectors so I removed the import for crate::config::lists::*;, and I moved the MacroSelectors import down to the test module.

ytmimi · 2023-12-16T18:12:17Z

src/config/mod.rs

-    use crate::config::macro_names::MacroName;
+    use crate::config::macro_names::{MacroName, MacroSelectors};


MacroSelectros import moved here: https://github.com/rust-lang/rustfmt/pull/5937/files#r1428877853

calebcartwright · 2024-04-07T18:56:08Z

@ytmimi do you think this would be better to merge before or after the next sync? (no promises but I'm going to try to do one today)

ytmimi · 2024-04-07T19:01:57Z

@calebcartwright If we're going to do a sync we should update the changelog. I've started that on #6004, but there are a few other fixes that I merged that don't have entries yet.

ytmimi · 2024-04-07T19:02:17Z

That said, we can probably wait to merge this until after the sync

calebcartwright

Still have a bit more to get through but leaving some initial thoughts now

calebcartwright · 2024-07-01T04:44:12Z

src/config/config_type.rs

@@ -317,14 +340,17 @@ macro_rules! create_config {
                            }
                            name_out.push_str(name_raw);
                            name_out.push(' ');
-                            let mut default_str = format!("{}", $def);
+                            let default = <$ty as StyleEditionDefault>::style_edition_default(


probably best to name this as something other than default given keyword overlap

This was renamed as default_value

calebcartwright · 2024-07-01T04:44:16Z

src/config/config_type.rs

                $(
+                    let default = <$ty as StyleEditionDefault>::style_edition_default(


calebcartwright · 2024-07-01T04:45:08Z

src/config/config_type.rs

                self.$i.0.set(true);
                self.$i.2.clone()
            }
            )+

+            #[allow(unreachable_pub)]
+            pub fn deafult_with_style_edition(style_edition: StyleEdition) -> Config {


typo, would need to be updated at call sites too (though I won't try to leave inline comments for those)

Suggested change

pub fn deafult_with_style_edition(style_edition: StyleEdition) -> Config {

pub fn default_with_style_edition(style_edition: StyleEdition) -> Config {

Thanks for catching this. Luckily this was a pretty easy fix with find and replace.

calebcartwright · 2024-07-01T04:46:02Z

src/config/mod.rs

+    max_width: MaxWidth, true, "Maximum width of each line";
+    hard_tabs: HardTabs, true, "Use tab characters for indentation, spaces for alignment";
+    tab_spaces: TabSpaces, true, "Number of spaces per tab";
+    newline_style: NewlineStyleConfig, true, "Unix or Windows line endings";
+    indent_style: IndentStyleConfig, false, "How do we indent expressions or items";


I like the sound of this, but agree it's something worth deferring

calebcartwright · 2024-07-01T04:49:22Z

src/config/mod.rs

-            verbose: Verbosity, Verbosity::Normal, false,
-                "How much to information to emit to the user";
-            file_lines: FileLines, FileLines::all(), false,
+            max_width: MaxWidth, true, "Maximum width of each line";


just leaving this as a thought, not requesting/suggesting a pivot, but I found myself having to dart back and forth between the two sites to get a "full picture" of a given option.

that's fine for moving ahead with 2024 edition support, but it's definitely something I want to address sooner rather than later, as it's going to be prone for errors IMO (perhaps you're earlier suggestion on future refactoring would help as well)

Agreed that there's plenty of room for improvement, and I'm open to suggestions on how to make this better

Makes internal changes to define each configuration in terms of the `StyleEditionDefault` trait, and hard codes `StyleEdition::Edition2015` anywhere the new `style_edition` is needed. **Note** users are still unable to configure `style edition`.

rustbot added the pr-not-reviewed label Oct 8, 2023

ytmimi commented Oct 9, 2023

View reviewed changes

ytmimi force-pushed the impl_style_edition_default_trait_for_all_configs branch from 09eb6ff to 37b0505 Compare December 16, 2023 18:10

ytmimi commented Dec 16, 2023

View reviewed changes

ytmimi requested a review from calebcartwright December 16, 2023 18:15

ytmimi force-pushed the impl_style_edition_default_trait_for_all_configs branch from 37b0505 to e9da818 Compare March 18, 2024 22:56

ytmimi mentioned this pull request Apr 3, 2024

Add StyleEditionDefault trait, StyleEdition enum, and #[style_edition] proc macro #5854

Closed

calebcartwright added the p-high label Apr 7, 2024

calebcartwright self-assigned this Apr 7, 2024

traviscross mentioned this pull request Apr 11, 2024

Tracking Issue for Rust 2024: rustfmt style editions rust-lang/rust#123799

Open

4 tasks

ytmimi added the a-2024-edition Style Edition 2024 label Jun 27, 2024

ytmimi force-pushed the impl_style_edition_default_trait_for_all_configs branch 2 times, most recently from d1d1f41 to 3e03c8d Compare June 27, 2024 05:05

calebcartwright force-pushed the impl_style_edition_default_trait_for_all_configs branch from 3e03c8d to 2adf879 Compare July 1, 2024 04:39

calebcartwright reviewed Jul 1, 2024

View reviewed changes

ytmimi force-pushed the impl_style_edition_default_trait_for_all_configs branch from 2adf879 to 0e612d5 Compare July 6, 2024 00:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Impl `StyleEditionDefault` trait for all configs #5937

Impl `StyleEditionDefault` trait for all configs #5937

ytmimi commented Oct 8, 2023 •

edited

Loading

ytmimi Oct 9, 2023 •

edited

Loading

calebcartwright Jul 1, 2024

ytmimi Oct 9, 2023

ytmimi Dec 16, 2023 •

edited

Loading

ytmimi Dec 16, 2023

calebcartwright commented Apr 7, 2024

ytmimi commented Apr 7, 2024 •

edited

Loading

ytmimi commented Apr 7, 2024

calebcartwright left a comment

calebcartwright Jul 1, 2024

ytmimi Jul 6, 2024

calebcartwright Jul 1, 2024

calebcartwright Jul 1, 2024

ytmimi Jul 6, 2024

calebcartwright Jul 1, 2024

calebcartwright Jul 1, 2024

ytmimi Jul 6, 2024

		use crate::config::macro_names::MacroName;
		use crate::config::macro_names::{MacroName, MacroSelectors};

		$(
		let default = <$ty as StyleEditionDefault>::style_edition_default(

	pub fn deafult_with_style_edition(style_edition: StyleEdition) -> Config {
	pub fn default_with_style_edition(style_edition: StyleEdition) -> Config {

Impl StyleEditionDefault trait for all configs #5937

Are you sure you want to change the base?

Impl StyleEditionDefault trait for all configs #5937

Conversation

ytmimi commented Oct 8, 2023 • edited Loading

ytmimi Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ytmimi Dec 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebcartwright commented Apr 7, 2024

ytmimi commented Apr 7, 2024 • edited Loading

ytmimi commented Apr 7, 2024

calebcartwright left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Impl `StyleEditionDefault` trait for all configs #5937

Impl `StyleEditionDefault` trait for all configs #5937

ytmimi commented Oct 8, 2023 •

edited

Loading

ytmimi Oct 9, 2023 •

edited

Loading

ytmimi Dec 16, 2023 •

edited

Loading

ytmimi commented Apr 7, 2024 •

edited

Loading