Replace impl AsRef<T> with &T#2182
Merged
workingjubilee merged 12 commits intoOct 30, 2025
Merged
Conversation
A pattern that was briefly in vogue some years ago, when impl Trait args 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. Clear the codebase of it, starting with schema.rs in cargo-pgrx
This reverts commit 77f577b.
Member
Author
|
My mistake: it is semi-publicly exposed in a way that I do not care to fix in one place. |
workingjubilee
commented
Oct 30, 2025
| target, | ||
| Some(&dest), | ||
| Option::<String>::None, | ||
| None, |
Member
Author
There was a problem hiding this comment.
This is a perfect example of what I mean by "using impl AsRef everywhere weakens type inference". It can't even infer the type of an Option, here!
|
|
||
| // 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/")) { |
Member
Author
There was a problem hiding this comment.
A lot of the churn is in cases like this, where we just are straight-up deleting intermediate function calls.
Comment on lines
+102
to
+108
| &package_manifest_path, | ||
| &profile, | ||
| self.test, | ||
| &self.features, | ||
| self.target.as_deref(), | ||
| self.out.as_ref(), | ||
| self.dot, | ||
| self.out.as_deref(), | ||
| self.dot.as_deref(), |
Member
Author
There was a problem hiding this comment.
there's the occasional straight loss, but they don't justify the rest.
Member
Author
|
All this was done because it made it excessively painful to split up functions, which is probably why this codebase has so many huge ones. |
daamien
pushed a commit
to daamien/pgrx
that referenced
this pull request
Dec 15, 2025
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.
eeeebbbbrrrr
added a commit
that referenced
this pull request
Feb 9, 2026
Welcome to pgrx v0.17.0. This is a new minor release that brings a bunch of internal code refactoring, cleanup, new Postgres headers, and additional `cargo-pgrx regress` CLI options. As always, please install the latest `cargo-pgrx` with `cargo install cargo-pgrx --version 0.17.0 --locked` and also update your extension crate dependencies with `cargo pgrx upgrade`. ## What's Changed ### New Headers/Symbols * include `access/tsmapi.h` by @usamoi in #2155 * Include access/subtrans.h & access/commit_ts.h by @isdaniel in #2147 * include `utils/guc_tables.h` by @usamoi in #2243 * Include `scanner.h` in Rust bindings by @piki in #2163 * Add `commands/publicationcmds.h` to generated `pg_sys` bindings by @Sinjo in #2221 * Add the ClientAuthentication_hook by @daamien in #2231 * add storage/dsm_*.h headers by @eeeebbbbrrrr in #2244 * pg-sys: add catalog/heap.h bindings for PG18 by @charmitro in #2150 * Add back `peer_dn` field to `Port` on `pg17..` by @workingjubilee in #2200 * Add `repr(C)` to `struct Port` on `pg13..=16` by @workingjubilee in #2201 ### `cargo-pgrx` improvements * Add a psql_verbosity parameter for cargo pgrx regress by @daamien in #2230 ### Code Cleanup * Replace with let-chains in pgrx-bindgen by @workingjubilee in #2168 * Replace with let-chains in pgrx-sql-entity-graph by @workingjubilee in #2169 * Restore so-called needless lifetimes by @workingjubilee in #2167 * Drop stray Postgres 12 support in cargo-pgrx by @workingjubilee in #2173 * Remove deprecated enum##member constants by @workingjubilee in #2170 * Remove `variadic!` macro by @workingjubilee in #2171 * Semi-automated code cleanup by @workingjubilee in #2189 * Deref instead of borrowing `Option<&String>` by @workingjubilee in #2185 * Refactor schema.rs using a cargo command builder by @workingjubilee in #2187 * Replace `impl AsRef<T>` with `&T` by @workingjubilee in #2182 * Make `PgRelation::heap_relation` unsafe by @workingjubilee in #2176 * Move `cargo_pgrx::env` to `cargo_pgrx::cargo` by @workingjubilee in #2186 * Move `CargoProfile` into `cargo_pgrx::cargo` by @workingjubilee in #2188 * Add Scalar trait for generic handling of primitive arrays by @workingjubilee in #2153 * Set BGWORKER_SHMEM_ACCESS when building background workers by @cbandy in #2161 * Stop referencing `pg_config` in schema generation by @workingjubilee in #2174 * Show line number of failed asserts by @piki in #2175 * Ghostbust `used_type.rs` a bit by @workingjubilee in #2190 * Manually bind `static MyProcPort` and `struct Port` by @workingjubilee in #2162 * Make datetime types import each other via `use super` by @workingjubilee in #2198 * Migrate datetime types into `pgrx::datetime` by @workingjubilee in #2199 * Use diagnostic attrs for ABI/SQL-related traits by @workingjubilee in #2203 * Make `Scalar` provide `pg_sys::Oid` constant by @workingjubilee in #2209 * Hoist varlena encoding fn by @workingjubilee in #2208 * Recurse through `T` while anonymizing `&T` by @workingjubilee in #2212 * `impl BorrowDatum for TimeTz` by @workingjubilee in #2213 * Change `MemCx::alloc_bytes` to return `NonNull` by @workingjubilee in #2214 * Add `MemCx<'current, T>` arguments and `PBox<'current, T>` returns by @workingjubilee in #2210 * Further improve diagnostics for signatures with no SQL form by @workingjubilee in #2215 * Add `array::Element` and `callconv::DatumPass` by @workingjubilee in #2218 * Remove unused line in `memcxt_tests.rs` by @workingjubilee in #2220 * Add `FlatArray<'_, T>` by @workingjubilee in #2207 * Allow explicit handling of allocation errors by @workingjubilee in #2226 * Place lbound before updating array len product by @workingjubilee in #2236 * introduce `#[pg_guard(unsafe_entry_thread)]` by @eeeebbbbrrrr in #2242 ### Project Administrativa * Use Rust Edition 2024 by @workingjubilee in #2165 * Remove testing for deprecated Intel Macs by @workingjubilee in #2156 * Remove unused Cargo.lock for version-updater by @workingjubilee in #2180 * Add rust-analyzer.toml by @workingjubilee in #2179 * Note pgrx requires an unknown minimum Xcode version by @workingjubilee in #2164 ## New Contributors * @isdaniel made their first contribution in #2147 * @cbandy made their first contribution in #2161 * @piki made their first contribution in #2163 * @Sinjo made their first contribution in #2221 **Full Changelog**: v0.16.1...v0.17.0
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A pattern that was popular some years ago, when
impl Traitarguments were first introduced, was to make everything animpl Traitarg. 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 usingOption::as_derefa 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.NoneAlmost 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 withPathorstr, and replace them with&Pathor&stras 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 removingclones of data!The remaining instances linger with a note about their shame.