Composite type result #1560
Merged
Composite type result #1560
Conversation
This file contains 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
Comment on lines
632
to
651
| if let Some(first_ty) = path_arg.args.first() { | ||
| // Since `pub type Result<T> = std::error::Result<T, OurError> | ||
| // is a common pattern, | ||
| // we should support single-argument Result<T> style. | ||
| if path_arg.args.len() == 1 { | ||
| (first_ty.clone(), None) | ||
| } | ||
| else if path_arg.args.len() == 2 { | ||
| (first_ty.clone(), Some(path_arg.args[1].clone())) | ||
| } | ||
| else { | ||
| return Err(syn::Error::new( | ||
| last.arguments.span(), | ||
| "Result<..> with more than two type arguments not supported.", | ||
| )); | ||
| } | ||
| } | ||
| else { | ||
| // Return early, Result<> with no type args. | ||
| return Err(syn::Error::new( | ||
| last.arguments.span(), | ||
| "Cannot return a Result without type generic arguments.", | ||
| )); | ||
| } |
There was a problem hiding this comment.
Can this just be
let mut iter = args.iter();
match (iter.next(), iter.next(), iter.next()) {So that we can have the compiler check that we're exhaustively handling all cases we want to handle, and we don't have the problem of trying to track whether the indexing we do later is correct? Seeing .first() and then first_ty and then [1] is lexically confusing.
There was a problem hiding this comment.
Sadly, syn doesn't expose a slice type here, so we can't just
match &slice_of_generic_args {
[a, b, c, ..] => return Err(..),
[a, b] => { /* code */ },
[a] => { /* code */ },
[] => return Err(..),
}There was a problem hiding this comment.
This should be cleaner now, let me know if the new implementation fits.
|
Also, can you rebase this on |
NotGyro
force-pushed
the
composite-type-result
branch
from
b2555d0
to
04bd34e
Compare
I've rebased it now, the diff should be cleaner. |
workingjubilee
added a commit
that referenced
this pull request
Mar 1, 2024
Welcome to pgrx 0.12.0-alpha.1! Say the magic words with me! ```shell cargo install cargo-pgrx --locked --version 0.12.0-alpha.1 ``` # Breaking Changes ## No more dlopen! Perhaps the most exciting change this round is @usamoi's contribution in #1468 which means that we no longer perform a `dlopen` in order to generate the schema. The cost, such as it is, is that your pgrx extensions now require a `src/bin/pgrx_embed.rs`, which will be used to generate the schema. This has much less cross-platform issues and will enable supporting things like `cargo binstall` down the line. It may be a bit touchy on first-time setup for transitioning older repos. If necessary, you may have to directly add a `src/bin/pgrx_embed.rs` and add the following code (which should be the only code in the file, though you can add comments if you like?): ```rust ::pgrx::pgrx_embed!(); ``` Your Cargo.toml will also want to update its crate-type key for the library: ```toml [lib] crate-type = ["cdylib", "lib"] ``` ## Library Code - pgrx-pg-sys will now use `ManuallyDropUnion` thanks to @NotGyro in #1547 - VARHDRSZ `const`s are no longer `fn`, thanks to @workingjubilee in #1584 - We no longer have `Interval::is_finite` since #1594 - We translate more `*_tree_walker` functions to the same signature their `*_impl` version in Postgres 16 has: #1596 - Thanks to @eeeebbbbrrrr in #1591 we no longer have the `pg_sql_graph_magic!()` macro, which should help with more things in the future! # What's New We have quite a lot of useful additions to our API: - `SpiClient::prepare_mut` was added thanks to @XeniaLu in #1275 - @usamoi also contributed bindings subscripting code in #1562 - For `#[pg_test]`, you have been able to use `#[should_panic(expected = "string")]` to anticipate a panic that contains that string in that test. For various reasons, `#[pg_test(error = "string")]` is much the same. Now, you can also use `#[pg_test(expected = "string")]`, in the hopes that is easier to stumble across, as of #1570 ## `Result<composite_type!("..."), E>` support - In #1560 @NotGyro contributed support for using `Result<composite_type!("Name"), E>`, as a case that had not been handled before. ## Significantly expanded docs Thanks to @rjuju, @NotGyro, and @workingjubilee, we now have significantly expanded docs for cargo-pgrx and pgrx in general. Some of these are in the API docs on https://docs.rs or the READMEs, but there's also a guide, now! It's not currently published, but is available as an [mdbook](https://github.com/rust-lang/mdBook) in the repo. Some diagnostic information that is also arguably documentation, like comments and the suggestion to `cargo install`, have also been improved, thanks to @workingjubilee in - #1579 - #1573 ## `#[pg_cast]` An experimental macro for a `CREATE CAST` was contributed by @xwkuang5 in #1445! ## Legal Stuff Thanks to @the-kenny in #1490 and @workingjubilee in #1504, it was brought to our attention that some dependencies had unusual legal requirements. So we fixed this with CI! We now check our code included into pgrx-using binaries is MIT/Apache 2.0 licensed, as is common across crates.io, using `cargo deny`!. The build tools will have more flexible legal requirements (partly due to the use of Mozilla Public License code in rustls). # Internal Changes Many internal cleanups were done thanks to - @workingjubilee in too many PRs to count! - @thomcc found a needless condition in #1501 - @nyurik in too many PRs to count! In particular: - we now actually `pfree` our `Array`s we detoasted as-of #1571 - creating a `RawArray` is now low-overhead due to #1587 ## Soundness Fixes We had a number of soundness issues uncovered or have added more tests to catch them. - Bounds-checking debug assertions for array access by @NotGyro in #1514 - Fix unsound `&` and `&mut` in `fcinfo.rs` by @workingjubilee in #1595 ## Less Deps Part of the cleanup by @workingjubilee was reducing the number of deps we compile: * cargo-pgrx: reduce trivial dep usages in #1499 * Update 2 syn in #1557 Hopefully it will reduce compile time and disk usage! ## New Contributors * @the-kenny made their first contribution in #1490 * @xwkuang5 made their first contribution in #1445 * @rjuju made their first contribution in #1516 * @nyurik made their first contribution in #1533 * @NotGyro made their first contribution in #1514 * @XeniaLu made their first contribution in #1275 **Full Changelog**: v0.12.0-alpha.0...v0.12.0-alpha.1
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.
This set of changes allows
#[pg-extern]functions to returnResult<composite_type!(..), E>. To get it working, I've added another type-specific case to match statements inUsedType::new(). That case then callsfn resolve_result_inner(), which I wrote for this purpose.This is intended to resolve #1470.
Future work
ErrorReportable::report()to avoid collision withstd'sTermination::report()#1561 : While working on this implementation, 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.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_sourceandfull_pathwere definitely correct, but for aResult<T, E>the ty_id iscore::any::TypeId::of<T>, whereas forOption<T>the macro expands tocore::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 whileOption<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 translatingResult<T>will either give Postgres an instance ofTor 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.