create rustfmt configs when compiling each rustfmt binary

Before we would allocate a new string for the rustfmt configs  every
time we invoked rustfmt. This was unnecessary since the configs won't
change during the runtime of `check_diff`.

Now we'll only create the configs for each binary once. Also, we'll
only need to allocate a new string if the user passed a non-empty
`--rustfmt-config` option  when invoking `check_diff`.

Author: ytmimi

check_diff/src/lib.rs

@@ -1,3 +1,4 @@
+use std::borrow::Cow;
 use std::env;
 use std::fmt::{Debug, Display};
 use std::io::{self, Write};
@@ -181,19 +182,11 @@ pub struct CheckDiffRunners<F, S> {
 }
 
 pub trait CodeFormatter {
-    fn format_code<T: AsRef<str>>(
-        &self,
-        code: &str,
-        config: Option<&[T]>,
-    ) -> Result<String, FormatCodeError>;
-
-    fn format_code_from_path<T: AsRef<str>, P: AsRef<Path>>(
-        &self,
-        path: P,
-        config: Option<&[T]>,
-    ) -> Result<String, FormatCodeError> {
+    fn format_code(&self, code: &str) -> Result<String, FormatCodeError>;
+
+    fn format_code_from_path<P: AsRef<Path>>(&self, path: P) -> Result<String, FormatCodeError> {
         let code = std::fs::read_to_string(path)?;
-        self.format_code(&code, config)
+        self.format_code(&code)
     }
 }
 
@@ -202,6 +195,7 @@ pub struct RustfmtRunner {
     binary_path: PathBuf,
     edition: Edition,
     style_edition: StyleEdition,
+    config: Cow<'static, str>,
 }
 
 impl<F, S> CheckDiffRunners<F, S> {
@@ -219,17 +213,9 @@ where
     S: CodeFormatter,
 {
     /// Creates a diff generated by running the source and feature binaries on the same file path
-    pub fn create_diff<T: AsRef<str>, P: AsRef<Path>>(
-        &self,
-        path: P,
-        additional_configs: Option<&[T]>,
-    ) -> Result<Diff, CreateDiffError> {
-        let src_format = self
-            .src_runner
-            .format_code_from_path(&path, additional_configs);
-        let feature_format = self
-            .feature_runner
-            .format_code_from_path(&path, additional_configs);
+    pub fn create_diff<P: AsRef<Path>>(&self, path: P) -> Result<Diff, CreateDiffError> {
+        let src_format = self.src_runner.format_code_from_path(&path);
+        let feature_format = self.feature_runner.format_code_from_path(&path);
 
         match (src_format, feature_format) {
             (Ok(s), Ok(f)) => Ok(Diff {
@@ -272,6 +258,10 @@ impl RustfmtRunner {
 
         Ok(buffer_into_utf8_lossy(command.stdout))
     }
+
+    fn command_line_configs(&self) -> &str {
+        &self.config
+    }
 }
 
 /// Convert a buffer of u8 into a String.
@@ -303,12 +293,7 @@ impl CodeFormatter for RustfmtRunner {
     // rustfmt.toml `ignore` list. For example, this helps us skip files in r-l/rust that have
     // been explicitly skipped because trying to format them causes rustfmt to hang or rustfmt.
     // doesn't do a good job at formatting those files.
-    fn format_code_from_path<T: AsRef<str>, P: AsRef<Path>>(
-        &self,
-        path: P,
-        config: Option<&[T]>,
-    ) -> Result<String, FormatCodeError> {
-        let config = create_config_arg(config);
+    fn format_code_from_path<P: AsRef<Path>>(&self, path: P) -> Result<String, FormatCodeError> {
         let command = Command::new(&self.binary_path)
             .env(
                 dynamic_library_path_env_var_name(),
@@ -322,7 +307,7 @@ impl CodeFormatter for RustfmtRunner {
                 "--unstable-features",
                 "--skip-children",
                 "--emit=stdout",
-                config.as_str(),
+                self.command_line_configs(),
             ])
             .arg(path.as_ref())
             .stdin(Stdio::piped())
@@ -351,12 +336,7 @@ impl CodeFormatter for RustfmtRunner {
     // code: Code to run the binary on
     // config: Any additional configuration options to pass to rustfmt
     //
-    fn format_code<T: AsRef<str>>(
-        &self,
-        code: &str,
-        config: Option<&[T]>,
-    ) -> Result<String, FormatCodeError> {
-        let config = create_config_arg(config);
+    fn format_code(&self, code: &str) -> Result<String, FormatCodeError> {
         let mut command = Command::new(&self.binary_path)
             .env(
                 dynamic_library_path_env_var_name(),
@@ -370,7 +350,7 @@ impl CodeFormatter for RustfmtRunner {
                 "--unstable-features",
                 "--skip-children",
                 "--emit=stdout",
-                config.as_str(),
+                self.command_line_configs(),
             ])
             .stdin(Stdio::piped())
             .stdout(Stdio::piped())
@@ -394,25 +374,42 @@ impl CodeFormatter for RustfmtRunner {
     }
 }
 
+const DEFAULT_CONFIG: &str = "--config=error_on_line_overflow=false,error_on_unformatted=false";
+
 /// Creates a configuration in the following form:
 /// <config_name>=<config_val>, <config_name>=<config_val>, ...
-fn create_config_arg<T: AsRef<str>>(config: Option<&[T]>) -> String {
-    let config_arg: String = match config {
-        Some(configs) => {
-            let mut result = String::new();
-            for arg in configs.iter() {
-                result.push(',');
-                result.push_str(arg.as_ref());
-            }
-            result
-        }
-        None => String::new(),
+fn create_config_arg<T: AsRef<str>>(configs: Option<&[T]>) -> Cow<'static, str> {
+    let Some(configs) = configs else {
+        return Cow::Borrowed(DEFAULT_CONFIG);
     };
-    let config = format!(
-        "--config=error_on_line_overflow=false,error_on_unformatted=false{}",
-        config_arg.as_str()
-    );
-    config
+
+    let mut configs_len = 0;
+    let mut num_configs = 0;
+
+    // Determine how many non empty configs we've got
+    for c in configs.iter().map(AsRef::as_ref) {
+        if c.is_empty() {
+            continue;
+        }
+
+        configs_len += c.len();
+        num_configs += 1;
+    }
+
+    if num_configs == 0 {
+        // All configs were empty so return the default.
+        return Cow::Borrowed(DEFAULT_CONFIG);
+    }
+
+    // We need capacity for the default configs len + one ',' per config + total config element len
+    let mut result = String::with_capacity(DEFAULT_CONFIG.len() + num_configs + configs_len);
+
+    for c in configs.iter().map(AsRef::as_ref) {
+        result.push(',');
+        result.push_str(c);
+    }
+
+    Cow::Owned(result)
 }
 /// Clone a git repository
 ///
@@ -537,11 +534,12 @@ pub fn get_cargo_version() -> Result<String, CheckDiffError> {
 
 /// Obtains the ld_lib path and then builds rustfmt from source
 /// If that operation succeeds, the source is then copied to the output path specified
-pub fn build_rustfmt_from_src(
+pub fn build_rustfmt_from_src<T: AsRef<str>>(
     binary_path: PathBuf,
     dir: &Path,
     edition: Edition,
     style_edition: StyleEdition,
+    config: Option<&[T]>,
 ) -> Result<RustfmtRunner, CheckDiffError> {
     // Because we're building standalone binaries we need to set the dynamic library path
     // so each rustfmt binary can find it's runtime dependencies.
@@ -565,20 +563,22 @@ pub fn build_rustfmt_from_src(
         binary_path,
         edition,
         style_edition,
+        config: create_config_arg(config),
     })
 }
 
 // Compiles and produces two rustfmt binaries.
 // One for the current main branch, and another for the feature branch
 // Parameters:
 // dest: Directory where rustfmt will be cloned
-pub fn compile_rustfmt(
+pub fn compile_rustfmt<T: AsRef<str>>(
     dest: &Path,
     remote_repo_url: String,
     feature_branch: String,
     edition: Edition,
     style_edition: StyleEdition,
     commit_hash: Option<String>,
+    config: Option<&[T]>,
 ) -> Result<CheckDiffRunners<RustfmtRunner, RustfmtRunner>, CheckDiffError> {
     const RUSTFMT_REPO: &str = "https://github.com/rust-lang/rustfmt.git";
 
@@ -589,16 +589,26 @@ pub fn compile_rustfmt(
 
     let cargo_version = get_cargo_version()?;
     info!("Compiling with {}", cargo_version);
-    let src_runner =
-        build_rustfmt_from_src(dest.join("src_rustfmt"), dest, edition, style_edition)?;
+    let src_runner = build_rustfmt_from_src(
+        dest.join("src_rustfmt"),
+        dest,
+        edition,
+        style_edition,
+        config,
+    )?;
     let should_detach = commit_hash.is_some();
     git_switch(
         commit_hash.as_ref().unwrap_or(&feature_branch),
         should_detach,
     )?;
 
-    let feature_runner =
-        build_rustfmt_from_src(dest.join("feature_rustfmt"), dest, edition, style_edition)?;
+    let feature_runner = build_rustfmt_from_src(
+        dest.join("feature_rustfmt"),
+        dest,
+        edition,
+        style_edition,
+        config,
+    )?;
     info!("RUSFMT_BIN {}", src_runner.get_binary_version()?);
     let dynamic_library_path_env_var = dynamic_library_path_env_var_name();
     info!(
@@ -633,8 +643,7 @@ pub fn search_for_rs_files(repo: &Path) -> impl Iterator<Item = PathBuf> {
 
 /// Calculates the number of errors when running the compiled binary and the feature binary on the
 /// repo specified with the specific configs.
-pub fn check_diff<T: AsRef<str>, P: AsRef<Path>>(
-    config: Option<&[T]>,
+pub fn check_diff<P: AsRef<Path>>(
     runners: &CheckDiffRunners<impl CodeFormatter, impl CodeFormatter>,
     repo: P,
     repo_url: &str,
@@ -652,7 +661,7 @@ pub fn check_diff<T: AsRef<str>, P: AsRef<Path>>(
             relative_path.display()
         );
 
-        match runners.create_diff(file.as_path(), config) {
+        match runners.create_diff(file.as_path()) {
             Ok(diff) => {
                 if !diff.is_empty() {
                     error!(

check_diff/src/main.rs

@@ -77,6 +77,7 @@ fn main() -> Result<ExitCode, Error> {
         args.edition,
         args.style_edition,
         args.commit_hash,
+        args.rustfmt_config.as_deref(),
     );
 
     let check_diff_runners = match compilation_result {
@@ -88,13 +89,11 @@ fn main() -> Result<ExitCode, Error> {
     };
 
     let errors = Arc::new(AtomicUsize::new(0));
-    let rustfmt_config = Arc::new(args.rustfmt_config);
     let check_diff_runners = Arc::new(check_diff_runners);
 
     thread::scope(|s| {
         for url in REPOS {
             let errors = Arc::clone(&errors);
-            let rustfmt_config = Arc::clone(&rustfmt_config);
             let check_diff_runners = Arc::clone(&check_diff_runners);
             s.spawn(move || {
                 let repo_name = get_repo_name(url);
@@ -115,12 +114,7 @@ fn main() -> Result<ExitCode, Error> {
                     return;
                 };
 
-                let error_count = check_diff(
-                    rustfmt_config.as_deref(),
-                    &check_diff_runners,
-                    tmp_dir.path(),
-                    url,
-                );
+                let error_count = check_diff(&check_diff_runners, tmp_dir.path(), url);
 
                 errors.fetch_add(error_count as usize, Ordering::Relaxed);
             });

check_diff/tests/check_diff.rs

@@ -8,11 +8,7 @@ use tempfile::Builder;
 struct DoNothingFormatter;
 
 impl CodeFormatter for DoNothingFormatter {
-    fn format_code<T: AsRef<str>>(
-        &self,
-        _code: &str,
-        _config: Option<&[T]>,
-    ) -> Result<String, FormatCodeError> {
+    fn format_code(&self, _code: &str) -> Result<String, FormatCodeError> {
         Ok(String::new())
     }
 }
@@ -21,11 +17,7 @@ impl CodeFormatter for DoNothingFormatter {
 struct AddWhiteSpaceFormatter;
 
 impl CodeFormatter for AddWhiteSpaceFormatter {
-    fn format_code<T: AsRef<str>>(
-        &self,
-        code: &str,
-        _config: Option<&[T]>,
-    ) -> Result<String, FormatCodeError> {
+    fn format_code(&self, code: &str) -> Result<String, FormatCodeError> {
         let result = code.to_string() + " ";
         Ok(result)
     }
@@ -80,7 +72,7 @@ fn check_diff_test_no_formatting_difference() -> Result<(), CheckDiffError> {
     let _tmp_file = File::create(file_path)?;
     let repo_url = "https://github.com/rust-lang/rustfmt.git";
 
-    let errors = check_diff(None::<&[&str]>, &runners, dir.path(), repo_url);
+    let errors = check_diff(&runners, dir.path(), repo_url);
     assert_eq!(errors, 0);
     Ok(())
 }
@@ -93,7 +85,7 @@ fn check_diff_test_formatting_difference() -> Result<(), CheckDiffError> {
     let _tmp_file = File::create(file_path)?;
     let repo_url = "https://github.com/rust-lang/rustfmt.git";
 
-    let errors = check_diff(None::<&[&str]>, &runners, dir.path(), repo_url);
+    let errors = check_diff(&runners, dir.path(), repo_url);
     assert_ne!(errors, 0);
     Ok(())
 }