Replace impl AsRef<T> with &T (pgcentralfoundation#2182)

A pattern that was popular some years ago, when `impl Trait` arguments
were first introduced, was to make everything an `impl Trait` arg. This
can be undesirable because it weakens type inference and spreads
whatever indirection is used throughout the codebase, demanding repeat
invocations of `.as_ref()` or whatever. Unfortunately, pgrx was first
made in the period of that being en vogue.

The alternative, of requiring things like `Option<&T>`, does demand
using `Option::as_deref` a bit, but fortunately that convenience exists
nowadays. It also means that the indirection is handled once at the top
level and not addressed after that, which is more appropriate for
internals. ~~None~~ **Almost** none of the relevant functions in pgrx
are considered public API, after all, as they are found throughout
cargo-pgrx and pgrx-sql-entity-graph.

Remove almost all instances of `impl AsRef`, usually with `Path` or
`str`, and replace them with `&Path` or `&str` as appropriate. This
deletes a few lines from the codebase outright, and in those that
remain, it usually simplifies the existing code. In a couple cases, the
benefit involves actually removing `clone`s of data!

The remaining instances linger with a note about their shame.

Author: workingjubilee

cargo-pgrx/src/command/connect.rs

@@ -51,7 +51,7 @@ impl CommandExecute for Connect {
         let (package_manifest, package_manifest_path) = get_package_manifest(
             &Features::default(),
             self.package.as_ref(),
-            self.manifest_path.as_ref(),
+            self.manifest_path.as_deref(),
         )?;
         let (pg_config, _pg_version) = match pg_config_and_version(
             &pgrx,
@@ -75,7 +75,7 @@ impl CommandExecute for Connect {
             Some(dbname) => dbname,
             None => {
                 // We should infer from package
-                get_property(package_manifest_path, "extname")
+                get_property(&package_manifest_path, "extname")
                     .wrap_err("could not determine extension name")?
                     .ok_or(eyre!("extname not found in control file"))?
             }

cargo-pgrx/src/command/get.rs

@@ -33,14 +33,15 @@ pub(crate) struct Get {
 impl CommandExecute for Get {
     #[tracing::instrument(level = "error", skip(self))]
     fn execute(self) -> eyre::Result<()> {
-        let metadata = crate::metadata::metadata(&Default::default(), self.manifest_path.as_ref())
-            .wrap_err("couldn't get cargo metadata")?;
-        crate::metadata::validate(self.manifest_path.as_ref(), &metadata)?;
+        let metadata =
+            crate::metadata::metadata(&Default::default(), self.manifest_path.as_deref())
+                .wrap_err("couldn't get cargo metadata")?;
+        crate::metadata::validate(self.manifest_path.as_deref(), &metadata)?;
         let package_manifest_path =
             crate::manifest::manifest_path(&metadata, self.package.as_ref())
                 .wrap_err("Couldn't get manifest path")?;
 
-        if let Some(value) = get_property(package_manifest_path, &self.name)? {
+        if let Some(value) = get_property(&package_manifest_path, &self.name)? {
             println!("{value}");
         }
         Ok(())
@@ -49,9 +50,9 @@ impl CommandExecute for Get {
 
 #[tracing::instrument(level = "error", skip_all, fields(
     %name,
-    manifest_path = %manifest_path.as_ref().display(),
+    manifest_path = %manifest_path.display(),
 ))]
-pub fn get_property(manifest_path: impl AsRef<Path>, name: &str) -> eyre::Result<Option<String>> {
+pub fn get_property(manifest_path: &Path, name: &str) -> eyre::Result<Option<String>> {
     let (control_file, extname) = find_control_file(manifest_path)?;
 
     if name == "extname" {
@@ -84,13 +85,10 @@ pub fn get_property(manifest_path: impl AsRef<Path>, name: &str) -> eyre::Result
     Ok(None)
 }
 
-pub(crate) fn find_control_file(
-    manifest_path: impl AsRef<Path>,
-) -> eyre::Result<(PathBuf, String)> {
+pub(crate) fn find_control_file(manifest_path: &Path) -> eyre::Result<(PathBuf, String)> {
     let parent = manifest_path
-        .as_ref()
         .parent()
-        .ok_or_else(|| eyre!("could not get parent of `{}`", manifest_path.as_ref().display()))?;
+        .ok_or_else(|| eyre!("could not get parent of `{}`", manifest_path.display()))?;
 
     for f in (std::fs::read_dir(parent).wrap_err_with(|| {
         eyre!("cannot open current directory `{}` for reading", parent.display())
@@ -112,7 +110,7 @@ pub(crate) fn find_control_file(
         }
     }
 
-    Err(eyre!("control file not found in `{}`", manifest_path.as_ref().display()))
+    Err(eyre!("control file not found in `{}`", manifest_path.display()))
 }
 
 fn determine_git_hash() -> eyre::Result<Option<String>> {

cargo-pgrx/src/command/install.rs

@@ -71,9 +71,9 @@ impl CommandExecute for Install {
             return sudo_install.execute();
         }
 
-        let metadata = crate::metadata::metadata(&self.features, self.manifest_path.as_ref())
+        let metadata = crate::metadata::metadata(&self.features, self.manifest_path.as_deref())
             .wrap_err("couldn't get cargo metadata")?;
-        crate::metadata::validate(self.manifest_path.as_ref(), &metadata)?;
+        crate::metadata::validate(self.manifest_path.as_deref(), &metadata)?;
         let package_manifest_path =
             crate::manifest::manifest_path(&metadata, self.package.as_ref())
                 .wrap_err("Couldn't get manifest path")?;
@@ -100,7 +100,7 @@ impl CommandExecute for Install {
 
         display_version_info(&pg_config, &PgVersionSource::PgConfig(pg_config.label()?));
         install_extension(
-            self.manifest_path.as_ref(),
+            self.manifest_path.as_deref(),
             self.package.as_ref(),
             &package_manifest_path,
             &pg_config,
@@ -122,7 +122,7 @@ impl CommandExecute for Install {
     features = ?features.features,
 ))]
 pub(crate) fn install_extension(
-    user_manifest_path: Option<impl AsRef<Path>>,
+    user_manifest_path: Option<&Path>,
     user_package: Option<&String>,
     package_manifest_path: &Path,
     pg_config: &PgConfig,
@@ -140,7 +140,7 @@ pub(crate) fn install_extension(
     let versioned_so = get_property(package_manifest_path, "module_pathname")?.is_none();
 
     let build_command_output =
-        build_extension(user_manifest_path.as_ref(), user_package, profile, features, target)?;
+        build_extension(user_manifest_path, user_package, profile, features, target)?;
     let build_command_bytes = build_command_output.stdout;
     let build_command_reader = BufReader::new(build_command_bytes.as_slice());
     let build_command_stream = CargoMessage::parse_stream(build_command_reader);
@@ -241,7 +241,7 @@ fn copy_file(
     dest: PathBuf,
     msg: &str,
     do_filter: bool,
-    package_manifest_path: impl AsRef<Path>,
+    package_manifest_path: &Path,
     output_tracking: &mut Vec<PathBuf>,
 ) -> eyre::Result<()> {
     let Some(dest_dir) = dest.parent() else {
@@ -281,7 +281,7 @@ fn copy_file(
 }
 
 pub(crate) fn build_extension(
-    user_manifest_path: Option<impl AsRef<Path>>,
+    user_manifest_path: Option<&Path>,
     user_package: Option<&String>,
     profile: &CargoProfile,
     features: &clap_cargo::Features,
@@ -295,7 +295,7 @@ pub(crate) fn build_extension(
 
     if let Some(user_manifest_path) = user_manifest_path {
         command.arg("--manifest-path");
-        command.arg(user_manifest_path.as_ref());
+        command.arg(user_manifest_path);
     }
 
     if let Some(user_package) = user_package {
@@ -344,9 +344,9 @@ pub(crate) fn build_extension(
 }
 
 fn copy_sql_files(
-    user_manifest_path: Option<impl AsRef<Path>>,
+    user_manifest_path: Option<&Path>,
     user_package: Option<&String>,
-    package_manifest_path: impl AsRef<Path>,
+    package_manifest_path: &Path,
     profile: &CargoProfile,
     is_test: bool,
     features: &clap_cargo::Features,
@@ -364,21 +364,21 @@ fn copy_sql_files(
         crate::command::schema::generate_schema(
             user_manifest_path,
             user_package,
-            &package_manifest_path,
+            package_manifest_path.as_ref(),
             profile,
             is_test,
             features,
             target,
             Some(&dest),
-            Option::<String>::None,
+            None,
             None,
             skip_build,
             output_tracking,
         )?;
     }
 
     // now copy all the version upgrade files too
-    if let Ok(dir) = fs::read_dir(package_manifest_path.as_ref().parent().unwrap().join("sql/")) {
+    if let Ok(dir) = fs::read_dir(package_manifest_path.parent().unwrap().join("sql/")) {
         for sql in dir.flatten() {
             let filename = sql.file_name().into_string().unwrap();
 
@@ -444,8 +444,8 @@ pub(crate) fn find_library_file(
 
 static CARGO_VERSION: OnceLock<MemoizeKeyValue> = OnceLock::new();
 
-pub(crate) fn get_version(manifest_path: impl AsRef<Path>) -> eyre::Result<String> {
-    let path_string = manifest_path.as_ref().to_owned();
+pub(crate) fn get_version(manifest_path: &Path) -> eyre::Result<String> {
+    let path_string = manifest_path.to_owned();
 
     if let Some(version) =
         CARGO_VERSION.get_or_init(Default::default).lock().unwrap().get(&path_string)
@@ -486,8 +486,8 @@ pub(crate) fn get_version(manifest_path: impl AsRef<Path>) -> eyre::Result<Strin
 
 static GIT_HASH: OnceLock<MemoizeKeyValue> = OnceLock::new();
 
-fn get_git_hash(manifest_path: impl AsRef<Path>) -> eyre::Result<String> {
-    let path_string = manifest_path.as_ref().to_owned();
+fn get_git_hash(manifest_path: &Path) -> eyre::Result<String> {
+    let path_string = manifest_path.to_owned();
 
     let mut mutex = GIT_HASH.get_or_init(Default::default).lock().unwrap();
     if let Some(hash) = mutex.get(&path_string) {
@@ -540,8 +540,7 @@ fn make_relative_extdir(_: PathBuf) -> PathBuf {
     "share/extension".into()
 }
 
-pub(crate) fn format_display_path(path: impl AsRef<Path>) -> eyre::Result<String> {
-    let path = path.as_ref();
+pub(crate) fn format_display_path(path: &Path) -> eyre::Result<String> {
     let out = path
         .strip_prefix(get_target_dir()?.parent().unwrap())
         .unwrap_or(path)
@@ -550,7 +549,7 @@ pub(crate) fn format_display_path(path: impl AsRef<Path>) -> eyre::Result<String
     Ok(out)
 }
 
-fn filter_contents(manifest_path: impl AsRef<Path>, mut input: String) -> eyre::Result<String> {
+fn filter_contents(manifest_path: &Path, mut input: String) -> eyre::Result<String> {
     if input.contains("@GIT_HASH@") {
         // avoid doing this if we don't actually have the token
         // the project might not be a git repo so running `git`

cargo-pgrx/src/command/package.rs

@@ -51,9 +51,9 @@ pub(crate) struct Package {
 
 impl Package {
     pub(crate) fn perform(mut self) -> eyre::Result<(PathBuf, Vec<PathBuf>)> {
-        let metadata = crate::metadata::metadata(&self.features, self.manifest_path.as_ref())
+        let metadata = crate::metadata::metadata(&self.features, self.manifest_path.as_deref())
             .wrap_err("couldn't get cargo metadata")?;
-        crate::metadata::validate(self.manifest_path.as_ref(), &metadata)?;
+        crate::metadata::validate(self.manifest_path.as_deref(), &metadata)?;
         let package_manifest_path =
             crate::manifest::manifest_path(&metadata, self.package.as_ref())
                 .wrap_err("Couldn't get manifest path")?;
@@ -85,7 +85,7 @@ impl Package {
         };
 
         let output_files = package_extension(
-            self.manifest_path.as_ref(),
+            self.manifest_path.as_deref(),
             self.package.as_ref(),
             &package_manifest_path,
             &pg_config,
@@ -114,7 +114,7 @@ impl CommandExecute for Package {
     test = is_test,
 ))]
 pub(crate) fn package_extension(
-    user_manifest_path: Option<impl AsRef<Path>>,
+    user_manifest_path: Option<&Path>,
     user_package: Option<&String>,
     package_manifest_path: &Path,
     pg_config: &PgConfig,
@@ -147,7 +147,7 @@ pub(crate) fn package_extension(
 
 pub(crate) fn build_base_path(
     pg_config: &PgConfig,
-    manifest_path: impl AsRef<Path>,
+    manifest_path: &Path,
     profile: &CargoProfile,
     target: Option<&str>,
 ) -> eyre::Result<PathBuf> {