Rename ErrorReportable::report() to avoid collision with std's Termination::report()
#1561
Comments
Merged
workingjubilee
pushed a commit
that referenced
this issue
Feb 19, 2024
This set of changes allows `#[pg-extern]` functions to return `Result<composite_type!(..), E>`. To get it working, I've added another type-specific case to match statements in `UsedType::new()`. That case then calls `fn resolve_result_inner()`, which I wrote for this purpose. This is intended to resolve #1470. ### Future work * Although I've tested this behavior by hand and confirmed it's working correctly, I haven't yet written a test case for this code path. If you'd prefer me to put that in *this* PR, request changes and I'll take care of it. Otherwise, I'll open an issue for it shortly. * #1561 : While working on this implementation, I ran into a naming ambiguity. Specifically, `pgrx::pg_sys::panics::ErrorReportable::report()` looks an awful lot like `std::process::Termination::report()`, and they are both traits that are implemented on `Result<T, E>` so if you import both traits you'll have a name collision. I spoke to @workingjubilee about this and she agreed it should be renamed to `unwrap_or_report()` as part of the breaking changes in `0.12`. ### A note on something esoteric A note on something that was difficult to dig up answers to: Some concerns surrounding correctness jumped out at me while I was looking at macro expansions for this pull request. In the generated `UsedTypeEntity`, `ty_source` and `full_path` were definitely correct, but for a `Result<T, E>` the ty_id is `core::any::TypeId::of<T>`, whereas for `Option<T>` the macro expands to `core::any::TypeId::of<Option<T>>`. I asked around, and after some thinking on the part of our senior engineers it looks like this is actually correct. This type information is used to communicate to Postgres, and while `Option<T>` is used as the Rust equivalent to translate to and from nullable Postgres types, so the Option behavior is special, whereas Postgres doesn't have a concept of a Result type and so translating `Result<T>` will either give Postgres an instance of `T` or an exit code and an error message. I just thought I'd clarify that to make sure my reasoning on the correctness of this code is written down.
workingjubilee
pushed a commit
that referenced
this issue
Feb 20, 2024
As noted in #1561, `pgrx::pg_sys::panics::ErrorReportable::report()` is named identically to `std::process::Termination::report()`, and they are both traits that are implemented on `Result<T, E>` so if you import both traits you'll have a name collision. Aside from the name collision, it's very easy to see `.report()` called on a `Result` type and make the incorrect assumption this is the `Termination` trait when one is unfamiliar with PGRX's codebase. To avoid ambiguity, this set of changes renames that method to `unwrap_or_report()`, and is intended to be included in the set of breaking changes for 0.12.
|
Looks like it's all set! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
While working on the implementation for #1560, I ran into a naming ambiguity. Specifically,
pgrx::pg_sys::panics::ErrorReportable::report()looks an awful lot likestd::process::Termination::report(), and they are both traits that are implemented onResult<T, E>so if you import both traits you'll have a name collision. I spoke to @workingjubilee about this and she agreed it should be renamed tounwrap_or_report()as part of the breaking changes in0.12.The text was updated successfully, but these errors were encountered: