refactor: unify #[pg_extern] attribute parsing via syn

pgrx-examples/errors/src/lib.rs

@@ -71,6 +71,14 @@ fn throw_pg_fatal(message: &str) {
 mod tests {
     use pgrx::prelude::*;
 
+    /// Raises an error whose message contains a literal double-quote: foo "bar"
+    ///
+    /// Used by the regression test for `#[pg_test(error = r#"..."#)]` — the old hand-rolled attribute walker corrupted raw string literals, so this exact message used to silently mismatch the `expected` value at runtime.
+    #[pg_extern]
+    fn raise_quoted_error() {
+        error!(r#"foo "bar""#);
+    }
+
     #[pg_test]
     fn test_it() {
         // do testing here.
@@ -81,6 +89,12 @@ mod tests {
         //
         // In either case, they all run in parallel
     }
+
+    /// Regression test for the raw-string corruption bug in `#[pg_test]`'s attribute parser. The expected message contains embedded double-quotes expressed via a Rust raw string literal. Prior to switching `pg_test` to syn-based parsing this would have been stored as `#"foo "bar""#` and never matched the real error.
+    #[pg_test(error = r#"foo "bar""#)]
+    fn raw_string_expected_matches_quoted_error() {
+        Spi::run("SELECT tests.raise_quoted_error()").unwrap();
+    }
 }
 
 #[cfg(test)]

pgrx-macros/src/lib.rs

@@ -15,14 +15,15 @@ use std::ffi::CString;
 
 use proc_macro2::Ident;
 use quote::{ToTokens, format_ident, quote};
+use syn::parse::Parser;
 use syn::spanned::Spanned;
 use syn::{Attribute, Data, DeriveInput, Item, ItemImpl, parse_macro_input};
 
 use operators::{deriving_postgres_eq, deriving_postgres_hash, deriving_postgres_ord};
 use pgrx_sql_entity_graph as sql_gen;
 use sql_gen::{
-    CodeEnrichment, ExtensionSql, ExtensionSqlFile, ExternArgs, PgAggregate, PgCast, PgExtern,
-    PostgresEnum, Schema, parse_extern_attributes,
+    Attribute as SqlGenAttribute, CodeEnrichment, ExtensionSql, ExtensionSqlFile, PgAggregate,
+    PgCast, PgExtern, PostgresEnum, Schema,
 };
 
 mod operators;
@@ -60,13 +61,18 @@ pub fn pg_guard(attr: TokenStream, item: TokenStream) -> TokenStream {
 #[proc_macro_attribute]
 pub fn pg_test(attr: TokenStream, item: TokenStream) -> TokenStream {
     let mut stream = proc_macro2::TokenStream::new();
-    let args = parse_extern_attributes(proc_macro2::TokenStream::from(attr.clone()));
 
-    let mut expected_error = None;
-    args.into_iter().for_each(|v| {
-        if let ExternArgs::ShouldPanic(message) = v {
-            expected_error = Some(message)
-        }
+    let parsed_attrs =
+        match syn::punctuated::Punctuated::<SqlGenAttribute, syn::Token![,]>::parse_terminated
+            .parse(attr.clone())
+        {
+            Ok(p) => p,
+            Err(e) => return e.into_compile_error().into(),
+        };
+
+    let expected_error = parsed_attrs.iter().find_map(|a| match a {
+        SqlGenAttribute::ShouldPanic(lit) => Some(lit.value()),
+        _ => None,
     });
 
     let ast = parse_macro_input!(item as syn::Item);

pgrx-sql-entity-graph/src/lib.rs

@@ -31,6 +31,7 @@ pub use enrich::CodeEnrichment;
 pub use extension_sql::entity::{ExtensionSqlEntity, SqlDeclaredEntity};
 pub use extension_sql::{ExtensionSql, ExtensionSqlFile, SqlDeclared};
 pub use extern_args::{ExternArgs, parse_extern_attributes};
+pub use pg_extern::attribute::Attribute;
 pub use pg_extern::entity::{
     PgCastEntity, PgExternArgumentEntity, PgExternEntity, PgExternReturnEntity,
     PgExternReturnEntityIteratedItem, PgOperatorEntity,

pgrx-sql-entity-graph/src/pg_extern/attribute.rs

@@ -15,6 +15,7 @@
 > to the `pgrx` framework and very subject to change between versions. While you may use this, please do it with caution.
 
 */
+use crate::extern_args::ExternArgs;
 use crate::positioning_ref::PositioningRef;
 use crate::to_sql::ToSqlConfig;
 use proc_macro2::TokenStream as TokenStream2;
@@ -100,6 +101,35 @@ impl ToTokens for Attribute {
     }
 }
 
+impl Attribute {
+    /// Convert this attribute into an [`ExternArgs`] for SQL emission.
+    ///
+    /// Returns `None` for attributes (currently only [`Attribute::Sql`]) that are handled outside the extern-args pipeline.
+    pub fn as_extern_arg(&self) -> Option<ExternArgs> {
+        Some(match self {
+            Self::CreateOrReplace => ExternArgs::CreateOrReplace,
+            Self::Immutable => ExternArgs::Immutable,
+            Self::Strict => ExternArgs::Strict,
+            Self::Stable => ExternArgs::Stable,
+            Self::Volatile => ExternArgs::Volatile,
+            Self::Raw => ExternArgs::Raw,
+            Self::NoGuard => ExternArgs::NoGuard,
+            Self::SecurityDefiner => ExternArgs::SecurityDefiner,
+            Self::SecurityInvoker => ExternArgs::SecurityInvoker,
+            Self::ParallelSafe => ExternArgs::ParallelSafe,
+            Self::ParallelUnsafe => ExternArgs::ParallelUnsafe,
+            Self::ParallelRestricted => ExternArgs::ParallelRestricted,
+            Self::ShouldPanic(v) => ExternArgs::ShouldPanic(v.value()),
+            Self::Schema(v) => ExternArgs::Schema(v.value()),
+            Self::Support(v) => ExternArgs::Support(v.clone()),
+            Self::Name(v) => ExternArgs::Name(v.value()),
+            Self::Cost(v) => ExternArgs::Cost(v.to_token_stream().to_string()),
+            Self::Requires(items) => ExternArgs::Requires(items.iter().cloned().collect()),
+            Self::Sql(_) => return None,
+        })
+    }
+}
+
 impl Parse for Attribute {
     fn parse(input: ParseStream) -> Result<Self, syn::Error> {
         let ident: syn::Ident = input.parse()?;
@@ -181,3 +211,70 @@ impl Parse for Attribute {
         Ok(found)
     }
 }
+
+#[cfg(test)]
+mod tests {
+
+    use super::Attribute;
+    use std::str::FromStr;
+    use syn::parse::Parser;
+    use syn::punctuated::Punctuated;
+
+    fn parse(src: &str) -> Punctuated<Attribute, syn::Token![,]> {
+        let ts = proc_macro2::TokenStream::from_str(src).expect("tokenize");
+        Punctuated::<Attribute, syn::Token![,]>::parse_terminated.parse2(ts).expect("parse")
+    }
+
+    fn expected_value(attrs: &Punctuated<Attribute, syn::Token![,]>) -> Option<String> {
+        attrs.iter().find_map(|a| match a {
+            Attribute::ShouldPanic(lit) => Some(lit.value()),
+            _ => None,
+        })
+    }
+
+    #[test]
+    fn plain_string_expected() {
+        let attrs = parse(r#"expected = "syntax error""#);
+        assert_eq!(expected_value(&attrs).as_deref(), Some("syntax error"));
+    }
+
+    #[test]
+    fn escaped_quotes_in_plain_string() {
+        let attrs = parse(r#"expected = "syntax error at or near \"THIS\"""#);
+        assert_eq!(expected_value(&attrs).as_deref(), Some(r#"syntax error at or near "THIS""#),);
+    }
+
+    #[test]
+    fn raw_string_with_embedded_quotes() {
+        // The bug we are pinning: the old walker would have produced `#"foo "bar""#` (raw-string delimiters leaking into the value).
+        let attrs = parse(r###"expected = r#"foo "bar""#"###);
+        assert_eq!(expected_value(&attrs).as_deref(), Some(r#"foo "bar""#));
+    }
+
+    #[test]
+    fn raw_string_with_nested_hashes() {
+        let attrs = parse(r####"expected = r##"weird"#text"##"####);
+        assert_eq!(expected_value(&attrs).as_deref(), Some(r##"weird"#text"##));
+    }
+
+    #[test]
+    fn error_alias_works_like_expected() {
+        let attrs = parse(r#"error = "boom""#);
+        assert_eq!(expected_value(&attrs).as_deref(), Some("boom"));
+    }
+
+    #[test]
+    fn other_attrs_alongside_expected_do_not_interfere() {
+        let attrs = parse(r#"immutable, expected = "ok", strict"#);
+        assert_eq!(expected_value(&attrs).as_deref(), Some("ok"));
+        assert!(attrs.iter().any(|a| matches!(a, Attribute::Immutable)));
+        assert!(attrs.iter().any(|a| matches!(a, Attribute::Strict)));
+    }
+
+    #[test]
+    fn malformed_input_is_a_syn_error_not_a_panic() {
+        let ts = proc_macro2::TokenStream::from_str("expected").expect("tokenize");
+        let result = Punctuated::<Attribute, syn::Token![,]>::parse_terminated.parse2(ts);
+        assert!(result.is_err(), "expected = is required");
+    }
+}

pgrx-sql-entity-graph/src/pg_extern/mod.rs

@@ -16,7 +16,7 @@
 
 */
 mod argument;
-mod attribute;
+pub mod attribute;
 mod cast;
 pub mod entity;
 mod operator;
@@ -40,7 +40,7 @@ use operator::{PgrxOperatorAttributeWithIdent, PgrxOperatorOpName};
 use search_path::SearchPathList;
 
 use proc_macro2::{Ident, Span, TokenStream as TokenStream2};
-use quote::{ToTokens, quote};
+use quote::quote;
 use syn::parse::{Parse, ParseStream, Parser};
 use syn::punctuated::Punctuated;
 use syn::spanned::Spanned;
@@ -293,7 +293,9 @@ impl PgExtern {
         };
         let sql_graph_entity_fn_name = format_ident!("__pgrx_schema_fn_{}", ident);
         let input_count = self.inputs.len();
-        let extern_attr_count = self.attrs.len();
+        let extern_args: Vec<ExternArgs> =
+            self.attrs.iter().filter_map(Attribute::as_extern_arg).collect();
+        let extern_attr_count = extern_args.len();
         let args_len = inputs.iter().map(PgExternArgument::section_len_tokens);
         let return_len = returns.section_len_tokens();
         let schema_len = schema
@@ -305,30 +307,7 @@ impl PgExtern {
                 }
             })
             .unwrap_or_else(|| quote! { ::pgrx::pgrx_sql_entity_graph::section::bool_len() });
-        let extern_attr_lens = self.attrs.iter().map(|attr| {
-            let extern_arg = match attr {
-                Attribute::CreateOrReplace => ExternArgs::CreateOrReplace,
-                Attribute::Immutable => ExternArgs::Immutable,
-                Attribute::Strict => ExternArgs::Strict,
-                Attribute::Stable => ExternArgs::Stable,
-                Attribute::Volatile => ExternArgs::Volatile,
-                Attribute::Raw => ExternArgs::Raw,
-                Attribute::NoGuard => ExternArgs::NoGuard,
-                Attribute::SecurityDefiner => ExternArgs::SecurityDefiner,
-                Attribute::SecurityInvoker => ExternArgs::SecurityInvoker,
-                Attribute::ParallelSafe => ExternArgs::ParallelSafe,
-                Attribute::ParallelUnsafe => ExternArgs::ParallelUnsafe,
-                Attribute::ParallelRestricted => ExternArgs::ParallelRestricted,
-                Attribute::ShouldPanic(value) => ExternArgs::ShouldPanic(value.value()),
-                Attribute::Schema(value) => ExternArgs::Schema(value.value()),
-                Attribute::Support(value) => ExternArgs::Support(value.clone()),
-                Attribute::Name(value) => ExternArgs::Name(value.value()),
-                Attribute::Cost(value) => ExternArgs::Cost(value.to_token_stream().to_string()),
-                Attribute::Requires(items) => ExternArgs::Requires(items.iter().cloned().collect()),
-                Attribute::Sql(_) => unreachable!("sql attributes are handled separately"),
-            };
-            extern_arg.section_len_tokens()
-        });
+        let extern_attr_lens = extern_args.iter().map(ExternArgs::section_len_tokens);
         let search_path_len = self
             .search_path
             .as_ref()
@@ -388,30 +367,9 @@ impl PgExtern {
             .as_ref()
             .map(|schema| quote! { .bool(true).str(#schema) })
             .unwrap_or_else(|| quote! { .bool(false) });
-        let extern_attr_writers = self.attrs.iter().map(|attr| {
-            let extern_arg = match attr {
-                Attribute::CreateOrReplace => ExternArgs::CreateOrReplace,
-                Attribute::Immutable => ExternArgs::Immutable,
-                Attribute::Strict => ExternArgs::Strict,
-                Attribute::Stable => ExternArgs::Stable,
-                Attribute::Volatile => ExternArgs::Volatile,
-                Attribute::Raw => ExternArgs::Raw,
-                Attribute::NoGuard => ExternArgs::NoGuard,
-                Attribute::SecurityDefiner => ExternArgs::SecurityDefiner,
-                Attribute::SecurityInvoker => ExternArgs::SecurityInvoker,
-                Attribute::ParallelSafe => ExternArgs::ParallelSafe,
-                Attribute::ParallelUnsafe => ExternArgs::ParallelUnsafe,
-                Attribute::ParallelRestricted => ExternArgs::ParallelRestricted,
-                Attribute::ShouldPanic(value) => ExternArgs::ShouldPanic(value.value()),
-                Attribute::Schema(value) => ExternArgs::Schema(value.value()),
-                Attribute::Support(value) => ExternArgs::Support(value.clone()),
-                Attribute::Name(value) => ExternArgs::Name(value.value()),
-                Attribute::Cost(value) => ExternArgs::Cost(value.to_token_stream().to_string()),
-                Attribute::Requires(items) => ExternArgs::Requires(items.iter().cloned().collect()),
-                Attribute::Sql(_) => unreachable!("sql attributes are handled separately"),
-            };
-            extern_arg.section_writer_tokens(quote! { #writer_ident })
-        });
+        let extern_attr_writers = extern_args
+            .iter()
+            .map(|extern_arg| extern_arg.section_writer_tokens(quote! { #writer_ident }));
         let search_path_writer = self
             .search_path
             .as_ref()