refactor: improve mapping of legacy 'version' to 'style_edition'

calebcartwright · calebcartwright · commit 7869b0c26c2c · 2024-08-14T16:05:24.000-05:00
diff --git a/src/bin/main.rs b/src/bin/main.rs
@@ -19,7 +19,7 @@ use getopts::{Matches, Options};
 
 use crate::rustfmt::{
     load_config, CliOptions, Color, Config, Edition, EmitMode, FileLines, FileName,
-    FormatReportFormatterBuilder, Input, Session, StyleEdition, Verbosity,
+    FormatReportFormatterBuilder, Input, Session, StyleEdition, Verbosity, Version,
 };
 
 const BUG_REPORT_URL: &str = "https://github.com/rust-lang/rustfmt/issues/new?labels=bug";
@@ -745,6 +745,13 @@ impl CliOptions for GetOptsOptions {
             .get("style_edition")
             .map_or(self.style_edition, |se| StyleEdition::from_str(se).ok())
     }
+
+    fn version(&self) -> Option<Version> {
+        self.inline_config
+            .get("version")
+            .map(|version| Version::from_str(version).ok())
+            .flatten()
+    }
 }
 
 fn edition_from_edition_str(edition_str: &str) -> Result<Edition> {
@@ -813,6 +820,17 @@ mod test {
         options.inline_config = HashMap::from([("version".to_owned(), "Two".to_owned())]);
         let config = get_config(None, Some(options));
         assert_eq!(config.style_edition(), StyleEdition::Edition2024);
+        assert_eq!(config.overflow_delimited_expr(), true);
+    }
+
+    #[nightly_only_test]
+    #[test]
+    fn version_config_file_sets_style_edition_override_correctly() {
+        let options = GetOptsOptions::default();
+        let config_file = Some(Path::new("tests/config/style-edition/just-version"));
+        let config = get_config(config_file, Some(options));
+        assert_eq!(config.style_edition(), StyleEdition::Edition2024);
+        assert_eq!(config.overflow_delimited_expr(), true);
     }
 
     #[nightly_only_test]
@@ -857,6 +875,7 @@ mod test {
         ]);
         let config = get_config(None, Some(options));
         assert_eq!(config.style_edition(), StyleEdition::Edition2024);
+        assert_eq!(config.overflow_delimited_expr(), true);
     }
 
     #[nightly_only_test]
diff --git a/src/config/config_type.rs b/src/config/config_type.rs
@@ -134,7 +134,6 @@ macro_rules! create_config {
                     "fn_args_layout" => self.0.set_fn_args_layout(),
                     "hide_parse_errors" => self.0.set_hide_parse_errors(),
                     "version" => self.0.set_version(),
-                    "edition" => self.0.set_edition(),
                     &_ => (),
                 }
             }
@@ -165,7 +164,6 @@ macro_rules! create_config {
                     "fn_args_layout" => self.0.set_fn_args_layout(),
                     "hide_parse_errors" => self.0.set_hide_parse_errors(),
                     "version" => self.0.set_version(),
-                    "edition" => self.0.set_edition(),
                     &_ => (),
                 }
             }
@@ -264,7 +262,6 @@ macro_rules! create_config {
                 self.set_fn_args_layout();
                 self.set_hide_parse_errors();
                 self.set_version();
-                self.set_edition();
                 self
             }
 
@@ -367,7 +364,6 @@ macro_rules! create_config {
                     "fn_args_layout" => self.set_fn_args_layout(),
                     "hide_parse_errors" => self.set_hide_parse_errors(),
                     "version" => self.set_version(),
-                    "edition" => self.set_edition(),
                     &_ => (),
                 }
             }
@@ -585,30 +581,9 @@ macro_rules! create_config {
                         option which takes 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) {
-                let style_edition_set = self.was_set().style_edition()
-                    || self.was_set_cli().style_edition();
-
-                if style_edition_set || 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();
-            }
-
-
-
             #[allow(unreachable_pub)]
             /// Returns `true` if the config key was explicitly set and is the default value.
             pub fn is_default(&self, key: &str) -> bool {
diff --git a/src/config/mod.rs b/src/config/mod.rs
@@ -222,11 +222,13 @@ impl PartialConfig {
         self,
         style_edition_override: Option<StyleEdition>,
         edition_override: Option<Edition>,
+        version_override: Option<Version>,
         dir: &Path,
     ) -> Config {
         Config::default_for_possible_style_edition(
             style_edition_override.or(self.style_edition),
             edition_override.or(self.edition),
+            version_override.or(self.version),
         )
         .fill_from_parsed_config(self, dir)
     }
@@ -236,16 +238,25 @@ impl Config {
     pub fn default_for_possible_style_edition(
         style_edition: Option<StyleEdition>,
         edition: Option<Edition>,
+        version: Option<Version>,
     ) -> Config {
-        style_edition.map_or_else(
-            || {
-                edition.map_or_else(
-                    || Config::default(),
-                    |e| Self::default_with_style_edition(e.into()),
-                )
-            },
-            |se| Self::default_with_style_edition(se),
-        )
+        // Ensures the configuration defaults associated with Style Editions
+        // follow the precedence set in
+        // https://rust-lang.github.io/rfcs/3338-style-evolution.html
+        // 'version' is a legacy alias for 'style_edition' that we'll support
+        // for some period of time
+        // FIXME(calebcartwright) - remove 'version' at some point
+        match (style_edition, version, edition) {
+            (Some(se), _, _) => Self::default_with_style_edition(se),
+            (None, Some(Version::Two), _) => {
+                Self::default_with_style_edition(StyleEdition::Edition2024)
+            }
+            (None, Some(Version::One), _) => {
+                Self::default_with_style_edition(StyleEdition::Edition2015)
+            }
+            (None, None, Some(e)) => Self::default_with_style_edition(e.into()),
+            (None, None, None) => Config::default(),
+        }
     }
 
     pub(crate) fn version_meets_requirement(&self) -> bool {
@@ -275,6 +286,7 @@ impl Config {
         file_path: &Path,
         edition: Option<Edition>,
         style_edition: Option<StyleEdition>,
+        version: Option<Version>,
     ) -> Result<Config, Error> {
         let mut file = File::open(&file_path)?;
         let mut toml = String::new();
@@ -284,6 +296,7 @@ impl Config {
             file_path.parent().unwrap(),
             edition,
             style_edition,
+            version,
         )
         .map_err(|err| Error::new(ErrorKind::InvalidData, err))
     }
@@ -301,6 +314,7 @@ impl Config {
         dir: &Path,
         edition: Option<Edition>,
         style_edition: Option<StyleEdition>,
+        version: Option<Version>,
     ) -> Result<(Config, Option<PathBuf>), Error> {
         /// Try to find a project file in the given directory and its parents.
         /// Returns the path of the nearest project file if one exists,
@@ -347,24 +361,25 @@ impl Config {
 
         match resolve_project_file(dir)? {
             None => Ok((
-                Config::default_for_possible_style_edition(style_edition, edition),
+                Config::default_for_possible_style_edition(style_edition, edition, version),
                 None,
             )),
-            Some(path) => Config::from_toml_path(&path, edition, style_edition)
+            Some(path) => Config::from_toml_path(&path, edition, style_edition, version)
                 .map(|config| (config, Some(path))),
         }
     }
 
     #[allow(dead_code)]
     pub(super) fn from_toml(toml: &str, dir: &Path) -> Result<Config, String> {
-        Self::from_toml_for_style_edition(toml, dir, None, None)
+        Self::from_toml_for_style_edition(toml, dir, None, None, None)
     }
 
     pub(crate) fn from_toml_for_style_edition(
         toml: &str,
         dir: &Path,
         edition: Option<Edition>,
         style_edition: Option<StyleEdition>,
+        version: Option<Version>,
     ) -> Result<Config, String> {
         let parsed: ::toml::Value = toml
             .parse()
@@ -385,7 +400,7 @@ impl Config {
                 if !err.is_empty() {
                     eprint!("{err}");
                 }
-                Ok(parsed_config.to_parsed_config(style_edition, edition, dir))
+                Ok(parsed_config.to_parsed_config(style_edition, edition, version, dir))
             }
             Err(e) => {
                 err.push_str("Error: Decoding config file failed:\n");
@@ -403,19 +418,24 @@ pub fn load_config<O: CliOptions>(
     file_path: Option<&Path>,
     options: Option<O>,
 ) -> Result<(Config, Option<PathBuf>), Error> {
-    let (over_ride, edition, style_edition) = match options {
-        Some(ref opts) => (config_path(opts)?, opts.edition(), opts.style_edition()),
-        None => (None, None, None),
+    let (over_ride, edition, style_edition, version) = match options {
+        Some(ref opts) => (
+            config_path(opts)?,
+            opts.edition(),
+            opts.style_edition(),
+            opts.version(),
+        ),
+        None => (None, None, None, None),
     };
 
     let result = if let Some(over_ride) = over_ride {
-        Config::from_toml_path(over_ride.as_ref(), edition, style_edition)
+        Config::from_toml_path(over_ride.as_ref(), edition, style_edition, version)
             .map(|p| (p, Some(over_ride.to_owned())))
     } else if let Some(file_path) = file_path {
-        Config::from_resolved_toml_path(file_path, edition, style_edition)
+        Config::from_resolved_toml_path(file_path, edition, style_edition, version)
     } else {
         Ok((
-            Config::default_for_possible_style_edition(style_edition, edition),
+            Config::default_for_possible_style_edition(style_edition, edition, version),
             None,
         ))
     };
diff --git a/src/config/options.rs b/src/config/options.rs
@@ -418,6 +418,7 @@ pub trait CliOptions {
     fn config_path(&self) -> Option<&Path>;
     fn edition(&self) -> Option<Edition>;
     fn style_edition(&self) -> Option<StyleEdition>;
+    fn version(&self) -> Option<Version>;
 }
 
 /// The edition of the syntax and semantics of code (RFC 2052).
diff --git a/src/git-rustfmt/main.rs b/src/git-rustfmt/main.rs
@@ -11,7 +11,9 @@ use getopts::{Matches, Options};
 use rustfmt_nightly as rustfmt;
 use tracing_subscriber::EnvFilter;
 
-use crate::rustfmt::{load_config, CliOptions, FormatReportFormatterBuilder, Input, Session};
+use crate::rustfmt::{
+    load_config, CliOptions, FormatReportFormatterBuilder, Input, Session, Version,
+};
 
 fn prune_files(files: Vec<&str>) -> Vec<&str> {
     let prefixes: Vec<_> = files
@@ -91,6 +93,9 @@ impl CliOptions for NullOptions {
     fn style_edition(&self) -> Option<rustfmt_nightly::StyleEdition> {
         unreachable!();
     }
+    fn version(&self) -> Option<Version> {
+        unreachable!();
+    }
 }
 
 fn uncommitted_files() -> Vec<String> {
diff --git a/src/lib.rs b/src/lib.rs
@@ -48,7 +48,7 @@ use crate::utils::indent_next_line;
 
 pub use crate::config::{
     load_config, CliOptions, Color, Config, Edition, EmitMode, FileLines, FileName, NewlineStyle,
-    Range, StyleEdition, Verbosity,
+    Range, StyleEdition, Verbosity, Version,
 };
 
 pub use crate::format_report_formatter::{FormatReportFormatter, FormatReportFormatterBuilder};
diff --git a/src/test/mod.rs b/src/test/mod.rs
@@ -15,7 +15,7 @@ use crate::rustfmt_diff::{make_diff, print_diff, DiffLine, Mismatch, ModifiedChu
 use crate::source_file;
 use crate::{
     is_nightly_channel, Edition, FormatReport, FormatReportFormatterBuilder, Input, Session,
-    StyleEdition,
+    StyleEdition, Version,
 };
 
 use rustfmt_config_proc_macro::nightly_only_test;
@@ -713,7 +713,7 @@ fn print_mismatches<T: Fn(u32) -> String>(
 
 fn read_config(filename: &Path) -> Config {
     let sig_comments = read_significant_comments(filename);
-    let (edition, style_edition) = get_editions_from_comments(&sig_comments);
+    let (edition, style_edition, version) = get_editions_from_comments(&sig_comments);
     // Look for a config file. If there is a 'config' property in the significant comments, use
     // that. Otherwise, if there are no significant comments at all, look for a config file with
     // the same name as the test file.
@@ -722,12 +722,14 @@ fn read_config(filename: &Path) -> Config {
             sig_comments.get("config").map(Path::new),
             edition,
             style_edition,
+            version,
         )
     } else {
         get_config(
             filename.with_extension("toml").file_name().map(Path::new),
             edition,
             style_edition,
+            version,
         )
     };
 
@@ -761,14 +763,17 @@ enum IdempotentCheckError {
 
 fn get_editions_from_comments(
     comments: &HashMap<String, String>,
-) -> (Option<Edition>, Option<StyleEdition>) {
+) -> (Option<Edition>, Option<StyleEdition>, Option<Version>) {
     (
         comments
             .get("edition")
             .map(|e| Edition::from_str(e).expect(&format!("invalid edition value: '{}'", e))),
         comments.get("style_edition").map(|se| {
             StyleEdition::from_str(se).expect(&format!("invalid style_edition value: '{}'", se))
         }),
+        comments
+            .get("version")
+            .map(|v| Version::from_str(v).expect(&format!("invalid version value: '{}'", v))),
     )
 }
 
@@ -778,8 +783,8 @@ fn idempotent_check(
 ) -> Result<FormatReport, IdempotentCheckError> {
     let sig_comments = read_significant_comments(filename);
     let config = if let Some(ref config_file_path) = opt_config {
-        let (edition, style_edition) = get_editions_from_comments(&sig_comments);
-        Config::from_toml_path(config_file_path, edition, style_edition)
+        let (edition, style_edition, version) = get_editions_from_comments(&sig_comments);
+        Config::from_toml_path(config_file_path, edition, style_edition, version)
             .expect("`rustfmt.toml` not found")
     } else {
         read_config(filename)
@@ -808,14 +813,15 @@ fn get_config(
     config_file: Option<&Path>,
     edition: Option<Edition>,
     style_edition: Option<StyleEdition>,
+    version: Option<Version>,
 ) -> Config {
     let config_file_name = match config_file {
-        None => return Config::default_for_possible_style_edition(style_edition, edition),
+        None => return Config::default_for_possible_style_edition(style_edition, edition, version),
         Some(file_name) => {
             let mut full_path = PathBuf::from("tests/config/");
             full_path.push(file_name);
             if !full_path.exists() {
-                return Config::default_for_possible_style_edition(style_edition, edition);
+                return Config::default_for_possible_style_edition(style_edition, edition, version);
             };
             full_path
         }
@@ -832,6 +838,7 @@ fn get_config(
         Path::new("tests/config/"),
         edition,
         style_edition,
+        version,
     )
     .expect("invalid TOML")
 }
diff --git a/tests/config/style-edition/just-version/rustfmt.toml b/tests/config/style-edition/just-version/rustfmt.toml
@@ -0,0 +1 @@
+version = "Two"

Original file line number	Diff line number	Diff line change
`@@ -418,6 +418,7 @@ pub trait CliOptions {`
`418`	`418`	`fn config_path(&self) -> Option<&Path>;`
`419`	`419`	`fn edition(&self) -> Option<Edition>;`
`420`	`420`	`fn style_edition(&self) -> Option<StyleEdition>;`
	`421`	`+ fn version(&self) -> Option<Version>;`
`421`	`422`	`}`
`422`	`423`
`423`	`424`	`/// The edition of the syntax and semantics of code (RFC 2052).`