Semi-automated code cleanup (pgcentralfoundation#2189)

Automated `--fix`es combined with the occasional hand-retouching, to
smooth things out after having churned large parts of cargo-pgrx.

Author: workingjubilee

cargo-pgrx/src/command/install.rs

@@ -355,16 +355,16 @@ fn copy_sql_files(
     skip_build: bool,
     output_tracking: &mut Vec<PathBuf>,
 ) -> eyre::Result<()> {
-    let (_, extname) = find_control_file(&package_manifest_path)?;
+    let (_, extname) = find_control_file(package_manifest_path)?;
     {
-        let version = get_version(&package_manifest_path)?;
+        let version = get_version(package_manifest_path)?;
         let filename = format!("{extname}--{version}.sql");
         let dest = extdir.join(filename);
 
         crate::command::schema::generate_schema(
             user_manifest_path,
             user_package,
-            package_manifest_path.as_ref(),
+            package_manifest_path,
             profile,
             is_test,
             features,
@@ -392,7 +392,7 @@ fn copy_sql_files(
                     extdir.join(filename),
                     "extension schema upgrade file",
                     true,
-                    &package_manifest_path,
+                    package_manifest_path,
                     output_tracking,
                 )?;
             }
@@ -453,10 +453,10 @@ pub(crate) fn get_version(manifest_path: &Path) -> eyre::Result<String> {
         return Ok(version.clone());
     }
 
-    let version = match get_property(&manifest_path, "default_version")? {
+    let version = match get_property(manifest_path, "default_version")? {
         Some(v) => {
             if v == "@CARGO_VERSION@" {
-                let metadata = crate::metadata::metadata(&Default::default(), Some(&manifest_path))
+                let metadata = crate::metadata::metadata(&Default::default(), Some(manifest_path))
                     .wrap_err("couldn't get cargo metadata")?;
                 crate::metadata::validate(Some(manifest_path), &metadata)?;
                 let manifest_path = crate::manifest::manifest_path(&metadata, None)
@@ -554,10 +554,10 @@ fn filter_contents(manifest_path: &Path, mut input: String) -> eyre::Result<Stri
         // avoid doing this if we don't actually have the token
         // the project might not be a git repo so running `git`
         // would fail
-        input = input.replace("@GIT_HASH@", &get_git_hash(&manifest_path)?);
+        input = input.replace("@GIT_HASH@", &get_git_hash(manifest_path)?);
     }
 
-    input = input.replace("@CARGO_VERSION@", &get_version(&manifest_path)?);
+    input = input.replace("@CARGO_VERSION@", &get_version(manifest_path)?);
 
     Ok(input)
 }

cargo-pgrx/src/command/regress.rs

@@ -82,9 +82,9 @@ impl Regress {
             return true;
         }
 
-        let sql = manifest_path_to_sql_tests_path(&manifest_path);
+        let sql = manifest_path_to_sql_tests_path(manifest_path);
         if !sql.exists() { return false; }
-        let expected = manifest_path_to_expected_tests_output_path(&manifest_path);
+        let expected = manifest_path_to_expected_tests_output_path(manifest_path);
         if !expected.exists() {return false; }
 
         let setup_sql = sql.join("setup.sql");
@@ -164,12 +164,10 @@ impl Regress {
 
         // `setup.{only}` is a special file that we handle separately
         let is_setup = |entry: &DirEntry| {
-            if let Some(filename) = entry.file_name().to_str() {
-                if filename.ends_with(&format!("setup.{only}")) {
-                    return true;
-                }
-            }
-            false
+            entry
+                .file_name()
+                .to_str()
+                .is_some_and(|filename| filename.ends_with(&format!("setup.{only}")))
         };
 
         // remove the "setup" file from the list
@@ -179,10 +177,10 @@ impl Regress {
         files.sort_unstable_by_key(|entry| entry.file_name());
 
         // if we detected a setup file and the caller wants to include it, make it the first entry
-        if let Some(setup_entry) = setup_entry {
-            if include_setup {
-                files.insert(0, setup_entry);
-            }
+        if let Some(setup_entry) = setup_entry
+            && include_setup
+        {
+            files.insert(0, setup_entry);
         }
     }
 
@@ -201,7 +199,7 @@ impl Regress {
             .to_str()
             .expect("test result output filename should be valid UTF8")
             .to_string();
-        let test_output = std::fs::read_to_string(&test_result_output)?;
+        let test_output = std::fs::read_to_string(test_result_output)?;
 
         let variant_suffix: Option<String>;
 
@@ -289,12 +287,12 @@ impl Regress {
             for new_test in new_tests {
                 if let Some(test_result_output) = create_regress_output(
                     pg_config,
-                    &manifest_path,
-                    &pgregress_path,
+                    manifest_path,
+                    pgregress_path,
                     dbname,
                     new_test,
                 )? {
-                    self.accept_new_test(&manifest_path, &test_result_output, auto)?;
+                    self.accept_new_test(manifest_path, &test_result_output, auto)?;
                 }
             }
         }
@@ -304,14 +302,14 @@ impl Regress {
 
         if !success && auto {
             // tests failed, but the user asked to `auto`matically accept their output as new output
-            let results_files = self.list_results_outputs(&manifest_path, include_setup)?;
+            let results_files = self.list_results_outputs(manifest_path, include_setup)?;
 
             println!();
             for entry in results_files {
                 let filename =
                     entry.file_name().to_str().expect("filename should be valid UTF8").to_owned();
                 let expected_path =
-                    manifest_path_to_expected_tests_output_path(&manifest_path).join(filename);
+                    manifest_path_to_expected_tests_output_path(manifest_path).join(filename);
 
                 if !expected_path.exists() {
                     // this is a file from `results/test-name.out` for which we don't have an expected file
@@ -455,7 +453,7 @@ fn create_regress_output(
         // doesn't exist, since we are creating the test output.  So if that's the case, if we have
         // a `.out` file for it in the results/ directory, then we're successful
         let out_file =
-            manifest_path_to_results_output_path(&manifest_path).join(format!("{test_name}.out"));
+            manifest_path_to_results_output_path(manifest_path).join(format!("{test_name}.out"));
         if out_file.exists() {
             return Ok(Some(out_file));
         } else {
@@ -668,10 +666,11 @@ fn manifest_path_to_results_output_path(manifest_path: &Path) -> PathBuf {
 }
 
 fn add_to_git(path: &Path) -> eyre::Result<()> {
-    if let Ok(git) = which::which("git") {
-        if is_git_repo(&git) && !Command::new(git).arg("add").arg(path).status()?.success() {
-            panic!("unable to add {} to git", path.display());
-        }
+    if let Ok(git) = which::which("git")
+        && is_git_repo(&git)
+        && !Command::new(git).arg("add").arg(path).status()?.success()
+    {
+        panic!("unable to add {} to git", path.display());
     }
     Ok(())
 }

cargo-pgrx/src/command/schema.rs

@@ -135,7 +135,7 @@ pub(crate) fn generate_schema_for_cli(
     skip_build: bool,
     output_tracking: &mut Vec<PathBuf>,
 ) -> eyre::Result<()> {
-    let manifest = Manifest::from_path(&package_manifest_path)?;
+    let manifest = Manifest::from_path(package_manifest_path)?;
     let features_arg = features.features.join(" ");
 
     let package_name = if let Some(user_package) = user_package {
@@ -180,7 +180,7 @@ pub(crate) fn generate_schema_implicit(
     output_tracking: &mut Vec<PathBuf>,
     manifest: cargo_toml::Manifest,
 ) -> eyre::Result<()> {
-    let (control_file, _extname) = find_control_file(&package_manifest_path)?;
+    let (control_file, _extname) = find_control_file(package_manifest_path)?;
     let lib_name = manifest.lib_name()?;
     let lib_filename = manifest.lib_filename()?;
 
@@ -396,7 +396,7 @@ fn compute_codegen(
         out
     };
     let build = {
-        let versioned_so = get_property(&package_manifest_path, "module_pathname")?.is_none();
+        let versioned_so = get_property(package_manifest_path, "module_pathname")?.is_none();
         quote::quote! {
             let pgrx_sql = ::pgrx::pgrx_sql_entity_graph::PgrxSql::build(
                 entities.into_iter(),
@@ -525,15 +525,15 @@ fn pgrx_embed_name(manifest: &Manifest) -> eyre::Result<String> {
 
     let package_name = name_from(&manifest.package_name()?);
     let lib_name = name_from(&manifest.lib_name()?);
-    (&manifest.bin)
-        .into_iter()
+    manifest
+        .bin
+        .iter()
         .find(|bin| {
             // As cargo_anifest autofills lib.name if it's empty, it's impossible to
             // check only against one name. Perhaps, cargo-util-schemas can help with that.
             bin.name.as_ref().is_some_and(|name| name == &package_name || name == &lib_name)
         })
-        .map(|bin| bin.name.to_owned())
-        .flatten()
+        .and_then(|bin| bin.name.to_owned())
         .ok_or_else(|| eyre!("Failed to find a pgrx_embed binary."))
 }
 

cargo-pgrx/src/command/start.rs

@@ -146,10 +146,10 @@ pub(crate) fn start_postgres(
         command.arg("--time-stamp=yes");
         command.arg("--error-markers=VALGRINDERROR-BEGIN,VALGRINDERROR-END");
         command.arg("--trace-children=yes");
-        if let Ok(path) = valgrind_suppressions_path(&pg_config) {
-            if let Ok(true) = std::fs::exists(&path) {
-                command.arg(format!("--suppressions={}", path.display()));
-            }
+        if let Ok(path) = valgrind_suppressions_path(pg_config)
+            && let Ok(true) = std::fs::exists(&path)
+        {
+            command.arg(format!("--suppressions={}", path.display()));
         }
         command.arg(pg_config.postmaster_path()?.display().to_string());
         file.write_all(format!("{command:?}").as_bytes())?;

cargo-pgrx/src/command/version.rs

@@ -79,12 +79,12 @@ mod rss {
                 let major = major.unwrap().parse::<u16>().unwrap_or_default();
                 let minor = minor.unwrap().parse::<u16>().unwrap_or_default();
 
-                if let Some(known_pgver) = versions.get_mut(&major) {
-                    if matches!(known_pgver.minor, PgMinorVersion::Latest) {
-                        // fill in the latest minor version number and its url
-                        known_pgver.minor = PgMinorVersion::Release(minor);
-                        known_pgver.url = Some(Url::parse(&download_url(major, minor))?);
-                    }
+                if let Some(known_pgver) = versions.get_mut(&major)
+                    && matches!(known_pgver.minor, PgMinorVersion::Latest)
+                {
+                    // fill in the latest minor version number and its url
+                    known_pgver.minor = PgMinorVersion::Release(minor);
+                    known_pgver.url = Some(Url::parse(&download_url(major, minor))?);
                 }
             }
 

cargo-pgrx/src/manifest.rs

@@ -75,21 +75,21 @@ pub(crate) fn modify_features_for_version(
     test: bool,
 ) {
     if let Some(features) = features {
-        if let Some(default_features) = manifest.features.get("default") {
-            if !features.no_default_features {
-                // if the user didn't specify `--no-default-features`, which would otherwise indicate
-                // they think they know what they're doing, we need to build an explicit set of features
-                // to use and turn on `--no-default-features`
-
-                features.no_default_features = true;
-                features.features.extend(
-                    default_features
-                        .iter()
-                        // only include default features that aren't known pgXX version features
-                        .filter(|flag| !pgrx.is_feature_flag(flag))
-                        .cloned(),
-                );
-            }
+        if let Some(default_features) = manifest.features.get("default")
+            && !features.no_default_features
+        {
+            // if the user didn't specify `--no-default-features`, which would otherwise indicate
+            // they think they know what they're doing, we need to build an explicit set of features
+            // to use and turn on `--no-default-features`
+
+            features.no_default_features = true;
+            features.features.extend(
+                default_features
+                    .iter()
+                    // only include default features that aren't known pgXX version features
+                    .filter(|flag| !pgrx.is_feature_flag(flag))
+                    .cloned(),
+            );
         }
 
         // if we know we're running from the `pgrx-tests/src/framework.rs`, remove any user-specified features
@@ -132,35 +132,25 @@ pub(crate) fn pg_config_and_version(
         } else if let Some(features) = user_features.as_ref() {
             // the user did not give us an explicit Postgres version, so see if there's one in the set
             // of `--feature` flags they gave us
-            for flag in &features.features {
-                if pgrx.is_feature_flag(flag) {
-                    // use the first feature flag that is a Postgres version we support
-                    return Some(PgVersionSource::FeatureFlag(flag.clone()));
-                }
+            if let Some(flag) = features.features.iter().find(|flag| pgrx.is_feature_flag(flag)) {
+                // use the first feature flag that is a Postgres version we support
+                return Some(PgVersionSource::FeatureFlag(flag.clone()));
             }
 
             // user didn't give us a feature flag that is a Postgres version
 
             // if they didn't ask for `--no-default-features` lets see if we have a default
             // postgres version feature specified in the manifest
-            if !features.no_default_features {
-                if let Some(default_features) = manifest.features.get("default") {
-                    for flag in default_features {
-                        if pgrx.is_feature_flag(flag) {
-                            return Some(PgVersionSource::DefaultFeature(flag.clone()));
-                        }
-                    }
-                }
-            }
-        } else {
-            // lets check the manifest for a default feature
-            if let Some(default_features) = manifest.features.get("default") {
-                for flag in default_features {
-                    if pgrx.is_feature_flag(flag) {
-                        return Some(PgVersionSource::DefaultFeature(flag.clone()));
-                    }
-                }
+            if !features.no_default_features
+                && let Some(default_features) = manifest.features.get("default")
+                && let Some(flag) = default_features.iter().find(|flag| pgrx.is_feature_flag(flag))
+            {
+                return Some(PgVersionSource::DefaultFeature(flag.clone()));
             }
+        } else if let Some(default_features) = manifest.features.get("default")
+            && let Some(flag) = default_features.iter().find(|flag| pgrx.is_feature_flag(flag))
+        {
+            return Some(PgVersionSource::DefaultFeature(flag.clone()));
         }
 
         // we cannot determine the Postgres version the user wants to use

pgrx-bindgen/src/build/clang.rs

@@ -40,10 +40,10 @@ pub(crate) fn detect_include_paths_for(
     // so we can only use clangs that match bindgen's libclang major version.
     if let Some(ClangSys { path, version: Some(v), c_search_paths, .. }) =
         ClangSys::find(preferred_clang, &[])
+        && Some(&*path) == preferred_clang
+        && v.Major as u32 == clang_major
     {
-        if Some(&*path) == preferred_clang && v.Major as u32 == clang_major {
-            return (false, c_search_paths.unwrap_or_default());
-        }
+        return (false, c_search_paths.unwrap_or_default());
     }
 
     // Oh no, still here?