From 0882f8cdd087187f61d4fadf6fd0f7f7dfe73a94 Mon Sep 17 00:00:00 2001 From: Jubilee <46493976+workingjubilee@users.noreply.github.com> Date: Mon, 28 Aug 2023 22:19:55 -0700 Subject: [PATCH] Make `PostgresHash` also need `Eq` (#1264) Latest in series of fixing the derive macros to require the correct bounds. This time, basically, "The hash operators are used for hashmappy things, so they should have the same bounds as hashmaps." Moves away from the briefly-adopted `#[doc(hidden)]` trait pattern to instead using a "trivial bound" for these. It didn't actually occur to me, first go-around, that such was correct in Rust and would emit a similar, reasonably-nice error. --- pgrx-macros/src/lib.rs | 2 +- pgrx-macros/src/operators.rs | 27 +++++++++++++++---- pgrx-tests/tests/ui/eq_for_postgres_hash.rs | 9 +++++++ .../tests/ui/eq_for_postgres_hash.stderr | 27 +++++++++++++++++++ .../tests/ui/total_eq_for_postgres_eq.stderr | 13 ++++----- pgrx/src/deriving.rs | 3 --- pgrx/src/lib.rs | 1 - 7 files changed, 64 insertions(+), 18 deletions(-) create mode 100644 pgrx-tests/tests/ui/eq_for_postgres_hash.rs create mode 100644 pgrx-tests/tests/ui/eq_for_postgres_hash.stderr delete mode 100644 pgrx/src/deriving.rs diff --git a/pgrx-macros/src/lib.rs b/pgrx-macros/src/lib.rs index b7c87350ad..3b0cd07ad7 100644 --- a/pgrx-macros/src/lib.rs +++ b/pgrx-macros/src/lib.rs @@ -1039,7 +1039,7 @@ Unlike some derives, this does not implement a "real" Rust trait, thus PostgresHash cannot be used in trait bounds, nor can it be manually implemented. */ #[proc_macro_derive(PostgresHash, attributes(pgrx))] -pub fn postgres_hash(input: TokenStream) -> TokenStream { +pub fn derive_postgres_hash(input: TokenStream) -> TokenStream { let ast = parse_macro_input!(input as syn::DeriveInput); deriving_postgres_hash(ast).unwrap_or_else(syn::Error::into_compile_error).into() } diff --git a/pgrx-macros/src/operators.rs b/pgrx-macros/src/operators.rs index 07c5f08864..f0c3834075 100644 --- a/pgrx-macros/src/operators.rs +++ b/pgrx-macros/src/operators.rs @@ -95,9 +95,6 @@ pub(crate) fn deriving_postgres_hash(ast: DeriveInput) -> syn::Result proc_macro2::TokenStream { let pg_name = Ident::new(&format!("{}_eq", name).to_lowercase(), name.span()); quote! { - #[doc(hidden)] - impl ::pgrx::deriving::PostgresEqRequiresTotalEq for #name {} - #[allow(non_snake_case)] #[::pgrx::pgrx_macros::pg_operator(immutable, parallel_safe)] #[::pgrx::pgrx_macros::opname(=)] @@ -107,7 +104,10 @@ pub fn derive_pg_eq(name: &Ident, path: &proc_macro2::TokenStream) -> proc_macro #[::pgrx::pgrx_macros::join(eqjoinsel)] #[::pgrx::pgrx_macros::merges] #[::pgrx::pgrx_macros::hashes] - fn #pg_name(left: #path, right: #path) -> bool { + fn #pg_name(left: #path, right: #path) -> bool + where + #path: ::core::cmp::Eq, + { left == right } } @@ -213,12 +213,29 @@ pub fn derive_pg_cmp(name: &Ident, path: &proc_macro2::TokenStream) -> proc_macr } } +/// Derive a Postgres hash operator using a provided hash function +/// +/// # HashEq? +/// +/// To quote the std documentation: +/// +/// "When implementing both Hash and Eq, it is important that the following property holds: +/// ```text +/// k1 == k2 -> hash(k1) == hash(k2) +/// ``` +/// In other words, if two keys are equal, their hashes must also be equal. HashMap and HashSet both rely on this behavior." +/// +/// Postgres is no different: this hashing is for the explicit purpose of equality checks, +/// and it also needs to be able to reason from hash equality to actual equality. pub fn derive_pg_hash(name: &Ident, path: &proc_macro2::TokenStream) -> proc_macro2::TokenStream { let pg_name = Ident::new(&format!("{}_hash", name).to_lowercase(), name.span()); quote! { #[allow(non_snake_case)] #[::pgrx::pgrx_macros::pg_extern(immutable, parallel_safe)] - fn #pg_name(value: #path) -> i32 { + fn #pg_name(value: #path) -> i32 + where + #path: ::core::hash::Hash + ::core::cmp::Eq, + { ::pgrx::misc::pgrx_seahash(&value) as i32 } } diff --git a/pgrx-tests/tests/ui/eq_for_postgres_hash.rs b/pgrx-tests/tests/ui/eq_for_postgres_hash.rs new file mode 100644 index 0000000000..34a2c735c3 --- /dev/null +++ b/pgrx-tests/tests/ui/eq_for_postgres_hash.rs @@ -0,0 +1,9 @@ +use pgrx::prelude::*; +use serde::{Serialize, Deserialize}; + +#[derive(Serialize, Deserialize, PostgresType, PostgresHash)] +pub struct BrokenType { + int: i32, +} + +fn main() {} diff --git a/pgrx-tests/tests/ui/eq_for_postgres_hash.stderr b/pgrx-tests/tests/ui/eq_for_postgres_hash.stderr new file mode 100644 index 0000000000..34894d1d4c --- /dev/null +++ b/pgrx-tests/tests/ui/eq_for_postgres_hash.stderr @@ -0,0 +1,27 @@ +error[E0277]: the trait bound `BrokenType: std::cmp::Eq` is not satisfied + --> tests/ui/eq_for_postgres_hash.rs:4:48 + | +4 | #[derive(Serialize, Deserialize, PostgresType, PostgresHash)] + | ^^^^^^^^^^^^ the trait `std::cmp::Eq` is not implemented for `BrokenType` + | + = help: see issue #48214 + = note: this error originates in the derive macro `PostgresHash` (in Nightly builds, run with -Z macro-backtrace for more info) +help: consider annotating `BrokenType` with `#[derive(Eq)]` + | +5 + #[derive(Eq)] +6 | pub struct BrokenType { + | + +error[E0277]: the trait bound `BrokenType: std::hash::Hash` is not satisfied + --> tests/ui/eq_for_postgres_hash.rs:4:48 + | +4 | #[derive(Serialize, Deserialize, PostgresType, PostgresHash)] + | ^^^^^^^^^^^^ the trait `std::hash::Hash` is not implemented for `BrokenType` + | + = help: see issue #48214 + = note: this error originates in the derive macro `PostgresHash` (in Nightly builds, run with -Z macro-backtrace for more info) +help: consider annotating `BrokenType` with `#[derive(Hash)]` + | +5 + #[derive(Hash)] +6 | pub struct BrokenType { + | diff --git a/pgrx-tests/tests/ui/total_eq_for_postgres_eq.stderr b/pgrx-tests/tests/ui/total_eq_for_postgres_eq.stderr index 4743611920..5f49237565 100644 --- a/pgrx-tests/tests/ui/total_eq_for_postgres_eq.stderr +++ b/pgrx-tests/tests/ui/total_eq_for_postgres_eq.stderr @@ -1,14 +1,11 @@ error[E0277]: the trait bound `BrokenType: std::cmp::Eq` is not satisfied - --> tests/ui/total_eq_for_postgres_eq.rs:5:12 + --> tests/ui/total_eq_for_postgres_eq.rs:4:59 | -5 | pub struct BrokenType { - | ^^^^^^^^^^ the trait `std::cmp::Eq` is not implemented for `BrokenType` +4 | #[derive(Serialize, Deserialize, PartialEq, PostgresType, PostgresEq)] + | ^^^^^^^^^^ the trait `std::cmp::Eq` is not implemented for `BrokenType` | -note: required by a bound in `PostgresEqRequiresTotalEq` - --> $WORKSPACE/pgrx/src/deriving.rs - | - | pub trait PostgresEqRequiresTotalEq: Eq {} - | ^^ required by this bound in `PostgresEqRequiresTotalEq` + = help: see issue #48214 + = note: this error originates in the derive macro `PostgresEq` (in Nightly builds, run with -Z macro-backtrace for more info) help: consider annotating `BrokenType` with `#[derive(Eq)]` | 5 + #[derive(Eq)] diff --git a/pgrx/src/deriving.rs b/pgrx/src/deriving.rs deleted file mode 100644 index 9754fff16f..0000000000 --- a/pgrx/src/deriving.rs +++ /dev/null @@ -1,3 +0,0 @@ -#![doc(hidden)] - -pub trait PostgresEqRequiresTotalEq: Eq {} diff --git a/pgrx/src/lib.rs b/pgrx/src/lib.rs index db850df14b..881a328278 100644 --- a/pgrx/src/lib.rs +++ b/pgrx/src/lib.rs @@ -43,7 +43,6 @@ pub mod atomics; pub mod bgworkers; pub mod callbacks; pub mod datum; -pub mod deriving; pub mod enum_helper; #[cfg(feature = "cshim")] pub mod fcall;