Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unbox types properly from arrays #1476

Merged
Show file tree
Hide file tree
Changes from 63 commits
Commits
Show all changes
68 commits
Select commit Hold shift + click to select a range
2141edd
Lift impl of FunctionMetadata to F: Fn(A) -> R
workingjubilee Dec 5, 2023
44eaa4a
Allow fn pointers to need hrtb
workingjubilee Dec 5, 2023
55c4867
preserve some code
workingjubilee Dec 5, 2023
22de98e
Stop rewriting types to have zero spaces
workingjubilee Jan 3, 2024
3f781b6
Anonymize lts in used types
workingjubilee Jan 3, 2024
497cb5f
Use the named lifetime for composite_type!
workingjubilee Jan 5, 2024
0d6fd48
Autofixes
workingjubilee Jan 6, 2024
ca2e321
rustfmt
workingjubilee Jan 6, 2024
00168cd
Remove staticize_lifetimes
workingjubilee Jan 6, 2024
ef2c081
Remove broken staticize_lifetimes code
workingjubilee Jan 6, 2024
f4ee20f
Remove inline staticization in used_type.rs
workingjubilee Jan 6, 2024
7de394a
Simplify FunctionMetadata impls for fn() -> R
workingjubilee Jan 6, 2024
e153bda
Allow using FnMut
workingjubilee Jan 6, 2024
60e3d11
Commit the TypeId crime
workingjubilee Jan 9, 2024
4cf2d41
well apparently that works
workingjubilee Jan 9, 2024
641ab92
Erase unused code and fmt
workingjubilee Jan 9, 2024
4e28358
Unify split_for_impl since no staticization
workingjubilee Jan 9, 2024
a0184f4
Comment on reasons
workingjubilee Jan 9, 2024
e8ead32
Add `Datum<'dat>`
workingjubilee Nov 2, 2023
1704cd0
Add UnboxDatum
workingjubilee Nov 2, 2023
b244355
Add array leakage test
workingjubilee Jan 12, 2024
7cf8fdc
Rampage over Array
workingjubilee Nov 2, 2023
6106a14
Make arrays work with lifetimes
workingjubilee Jan 11, 2024
5c2277c
Revert "Make arrays work with lifetimes"
workingjubilee Jan 12, 2024
73930fb
Sorta-mostly fix arrays kinda
workingjubilee Nov 4, 2023
c8bf1ea
Make another bad example into a ui test
workingjubilee Jan 12, 2024
2a574e9
Add unboxing for VariadicArray
workingjubilee Jan 11, 2024
d1e9ec3
Disable unsoundness tests
workingjubilee Nov 2, 2023
31c41a0
Fix lifetime on json test
workingjubilee Nov 2, 2023
99ca6d8
Make arbitrary PostgresEnums unboxable
workingjubilee Nov 2, 2023
a315e0f
Adding bounds to heap_tuple unboxing
workingjubilee Jan 11, 2024
c9e11da
Add notes on work-to-do
workingjubilee Jan 11, 2024
4be285e
Docs and inlining hints for UnboxDatum
workingjubilee Jan 11, 2024
f13940f
Document more
workingjubilee Jan 11, 2024
67062cc
Comment out the broken tests
workingjubilee Jan 12, 2024
b2cf030
Try to fix array iteration
workingjubilee Jan 13, 2024
aa2274d
Futz with lifetimes
workingjubilee Jan 13, 2024
e4118f8
Remove spare file
workingjubilee Jan 13, 2024
6727d4c
Revert "Remove spare file"
workingjubilee Jan 14, 2024
70c7d5d
Revert "Futz with lifetimes"
workingjubilee Jan 14, 2024
a6e1c42
Revert "Try to fix array iteration"
workingjubilee Jan 14, 2024
9021dcf
Try to fix htup lifetimes
workingjubilee Jan 14, 2024
f496c76
Add unboxing for PostgresTypes
workingjubilee Jan 14, 2024
44d431f
Prevent leaking lifetimes from HeapTuple
workingjubilee Jan 15, 2024
33f8011
Fix postgresenum impl
workingjubilee Jan 15, 2024
a46dc31
Mark currently-unusable tests
workingjubilee Jan 15, 2024
3707a21
Get over the line with some failing tests
workingjubilee Jan 15, 2024
5bbc504
Infest arrays with pedantically correct lifetimes
workingjubilee Jan 15, 2024
4c311b8
Fix last doctest
workingjubilee Jan 15, 2024
54d3954
Erase redundant lifetime
workingjubilee Jan 15, 2024
39f273f
fmt
workingjubilee Jan 15, 2024
af60efa
More lifetime pedantry
workingjubilee Jan 15, 2024
816d464
More cleanup and strictly static
workingjubilee Jan 15, 2024
f0db83e
Fix test more properly
workingjubilee Jan 15, 2024
acaede9
Fix doc comment
workingjubilee Jan 15, 2024
8270065
Minor cleanup
workingjubilee Jan 15, 2024
c71ec91
Fix most LT testing in roundtrip tests
workingjubilee Jan 15, 2024
1740ad5
Add some TODOs
workingjubilee Jan 17, 2024
1941050
Add todo tests as ui tests
workingjubilee Jan 17, 2024
eaee180
Mv compile-fail tests to a dir
workingjubilee Jan 17, 2024
9d50280
Add serde test to todo tests
workingjubilee Jan 17, 2024
91112f4
Add roundtrip tests to todo
workingjubilee Jan 17, 2024
159a3a7
Fixup todo tests
workingjubilee Jan 17, 2024
ace035c
Add newlines because GH's EOF symbol annoys me
workingjubilee Jan 17, 2024
9131c12
More test fixup
workingjubilee Jan 17, 2024
d6796d6
More fixes to test todos
workingjubilee Jan 17, 2024
a7b910d
Rename 'dat to 'src
workingjubilee Jan 17, 2024
3b6cf39
fmt
workingjubilee Jan 17, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pgrx-examples/bad_ideas/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ fn warning(s: &str) -> bool {
#[pg_extern]
fn exec<'a>(
command: &'a str,
args: default!(Vec<Option<&'a str>>, "ARRAY[]::text[]"),
args: default!(Vec<Option<String>>, "ARRAY[]::text[]"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a question... why this change?

Could it not be Vec<Option<&'a str>>?

This was the only example you touched, so either that means this example is the only one that accepts a &str and that pattern is no longer valid, or you changed this for some other reason?

Copy link
Member Author

@workingjubilee workingjubilee Jan 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because this changes FromDatum in the impls in pgrx/src/datum/array.rs to be in terms of T: UnboxDatum with certain bounds, this PR currently breaks FromDatum specifically for Vec<Option<T>> where there is any doubt as to whether T implements UnboxDatum. Essentially, the trait resolver can't figure it out at that depth. This can be solved by involving additional traits which work differently and either allow Vec<Option<T>> to not depend directly on Array<'_, T> for its unboxing or to move pg_getarg (or its replacement) off T: FromDatum.

) -> TableIterator<'static, (name!(status, Option<i32>), name!(stdout, String))> {
let mut command = &mut Command::new(command);

Expand Down
16 changes: 16 additions & 0 deletions pgrx-macros/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -618,6 +618,7 @@ pub fn postgres_enum(input: TokenStream) -> TokenStream {
fn impl_postgres_enum(ast: DeriveInput) -> syn::Result<proc_macro2::TokenStream> {
let mut stream = proc_macro2::TokenStream::new();
let sql_graph_entity_ast = ast.clone();
let generics = &ast.generics;
let enum_ident = &ast.ident;
let enum_name = enum_ident.to_string();

Expand Down Expand Up @@ -660,6 +661,14 @@ fn impl_postgres_enum(ast: DeriveInput) -> syn::Result<proc_macro2::TokenStream>
}
}

unsafe impl #generics ::pgrx::datum::UnboxDatum for #enum_ident #generics {
type As<'dat> = #enum_ident #generics where Self: 'dat;
#[inline]
unsafe fn unbox<'dat>(d: ::pgrx::datum::Datum<'dat>) -> Self::As<'dat> where Self: 'dat {
Self::from_datum(::core::mem::transmute(d), false).unwrap()
}
}

impl ::pgrx::datum::IntoDatum for #enum_ident {
#[inline]
fn into_datum(self) -> Option<::pgrx::pg_sys::Datum> {
Expand Down Expand Up @@ -808,6 +817,13 @@ fn impl_postgres_type(ast: DeriveInput) -> syn::Result<proc_macro2::TokenStream>
}
}
}

unsafe impl #generics ::pgrx::datum::UnboxDatum for #name #generics {
type As<'dat> = Self where Self: 'dat;
unsafe fn unbox<'dat>(datum: ::pgrx::datum::Datum<'dat>) -> Self::As<'dat> where Self: 'dat {
<Self as ::pgrx::datum::FromDatum>::from_datum(::core::mem::transmute(datum), false).unwrap()
}
}
}
)
}
Expand Down
2 changes: 1 addition & 1 deletion pgrx-macros/src/rewriter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ pub fn item_fn_without_rewrite(mut func: ItemFn) -> syn::Result<proc_macro2::Tok
#[allow(unused_unsafe)]
unsafe {
// NB: this is purposely not spelled `::pgrx` as pgrx itself uses #[pg_guard]
pgrx::pg_sys::submodules::panic::pgrx_extern_c_guard( || #body )
pgrx::pg_sys::submodules::panic::pgrx_extern_c_guard(move || #body )
}
}
})
Expand Down
70 changes: 0 additions & 70 deletions pgrx-sql-entity-graph/src/lifetimes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,76 +7,6 @@
//LICENSE All rights reserved.
//LICENSE
//LICENSE Use of this source code is governed by the MIT license that can be found in the LICENSE file.
use crate::NameMacro;
use proc_macro2::TokenStream;

pub fn staticize_lifetimes_in_type_path(value: syn::TypePath) -> syn::TypePath {
let mut ty = syn::Type::Path(value);
staticize_lifetimes(&mut ty);
match ty {
syn::Type::Path(type_path) => type_path,

// shouldn't happen
_ => panic!("not a TypePath"),
}
}

pub fn staticize_lifetimes(value: &mut syn::Type) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

goodby, staticize_lifetimes... you will not be missed!

match value {
syn::Type::Path(syn::TypePath { path: syn::Path { segments, .. }, .. }) => segments
.iter_mut()
.filter_map(|segment| match &mut segment.arguments {
syn::PathArguments::AngleBracketed(bracketed) => Some(bracketed),
_ => None,
})
.flat_map(|bracketed| &mut bracketed.args)
.for_each(|arg| match arg {
// rename lifetimes to the static lifetime so the TypeIds match.
syn::GenericArgument::Lifetime(lifetime) => {
lifetime.ident = syn::Ident::new("static", lifetime.ident.span());
}
// recurse
syn::GenericArgument::Type(ty) => staticize_lifetimes(ty),
syn::GenericArgument::Binding(binding) => staticize_lifetimes(&mut binding.ty),
syn::GenericArgument::Constraint(constraint) => {
constraint.bounds.iter_mut().for_each(|bound| {
if let syn::TypeParamBound::Lifetime(lifetime) = bound {
lifetime.ident = syn::Ident::new("static", lifetime.ident.span())
}
})
}
// nothing to do otherwise
_ => {}
}),

syn::Type::Reference(type_ref) => {
if let Some(lifetime) = &mut type_ref.lifetime {
lifetime.ident = syn::Ident::new("static", lifetime.ident.span());
} else {
type_ref.lifetime = Some(syn::parse_quote!('static));
}
}

syn::Type::Tuple(type_tuple) => type_tuple.elems.iter_mut().for_each(staticize_lifetimes),

syn::Type::Macro(syn::TypeMacro { mac })
if mac.path.segments.last().is_some_and(|seg| seg.ident == "name") =>
{
// We don't particularly care what the identifier is, so we parse a
// raw TokenStream. Specifically, it's okay for the identifier String,
// which we end up using as a Postgres column name, to be nearly any
// string, which can include Rust reserved words such as "type" or "match"
let Ok(out) = mac.parse_body::<NameMacro>() else { return };
let Ok(ident) = syn::parse_str::<TokenStream>(&out.ident) else { return };
let mut ty = out.used_ty.resolved_ty;

// rewrite the name!() macro's type so that it has a static lifetime, if any
staticize_lifetimes(&mut ty);
*mac = syn::parse_quote! {::pgrx::name!(#ident, #ty)};
}
_ => {}
}
}

pub fn anonymize_lifetimes_in_type_path(value: syn::TypePath) -> syn::TypePath {
let mut ty = syn::Type::Path(value);
Expand Down
54 changes: 16 additions & 38 deletions pgrx-sql-entity-graph/src/metadata/function_metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,61 +42,39 @@ pub trait FunctionMetadata<A> {
fn entity(&self) -> FunctionMetadataEntity;
}

impl<R> FunctionMetadata<()> for fn() -> R
where
R: SqlTranslatable,
{
fn entity(&self) -> FunctionMetadataEntity {
FunctionMetadataEntity {
arguments: vec![],
retval: {
let marker: PhantomData<R> = PhantomData;
marker.entity()
},
path: self.path(),
}
}
}

impl<R> FunctionMetadata<()> for unsafe fn() -> R
where
R: SqlTranslatable,
{
fn entity(&self) -> FunctionMetadataEntity {
FunctionMetadataEntity {
arguments: vec![],
retval: {
let marker: PhantomData<R> = PhantomData;
marker.entity()
},
path: self.path(),
}
}
}

macro_rules! impl_fn {
($($T:ident),* $(,)?) => {
impl<$($T: SqlTranslatable,)* R: SqlTranslatable> FunctionMetadata<($($T,)*)> for fn($($T,)*) -> R {
($($A:ident),* $(,)?) => {
impl<$($A,)* R, F> FunctionMetadata<($($A,)*)> for F
where
$($A: SqlTranslatable,)*
R: SqlTranslatable,
F: FnMut($($A,)*) -> R,
{
fn entity(&self) -> FunctionMetadataEntity {
FunctionMetadataEntity {
arguments: vec![$(PhantomData::<$T>.entity()),+],
arguments: vec![$(PhantomData::<$A>.entity()),*],
retval: PhantomData::<R>.entity(),
path: self.path(),
}
}
}
impl<$($T: SqlTranslatable,)* R: SqlTranslatable> FunctionMetadata<($($T,)*)> for unsafe fn($($T,)*) -> R {
impl<$($A,)* R> FunctionMetadata<($($A,)*)> for unsafe fn($($A,)*) -> R
where
$($A: SqlTranslatable,)*
R: SqlTranslatable,
{
fn entity(&self) -> FunctionMetadataEntity {
FunctionMetadataEntity {
arguments: vec![$(PhantomData::<$T>.entity()),+],
arguments: vec![$(PhantomData::<$A>.entity()),*],
retval: PhantomData::<R>.entity(),
path: self.path(),
}
}
}
};
}
// empty tuples are above

impl_fn!();
impl_fn!(T0);
impl_fn!(T0, T1);
impl_fn!(T0, T1, T2);
Expand Down
27 changes: 19 additions & 8 deletions pgrx-sql-entity-graph/src/pg_extern/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ use search_path::SearchPathList;
use crate::enrich::CodeEnrichment;
use crate::enrich::ToEntityGraphTokens;
use crate::enrich::ToRustCodeTokens;
use crate::lifetimes::staticize_lifetimes;
use proc_macro2::{Ident, Span, TokenStream as TokenStream2};
use quote::{quote, quote_spanned, ToTokens};
use syn::parse::{Parse, ParseStream, Parser};
Expand Down Expand Up @@ -133,12 +132,11 @@ impl PgExtern {
match v {
syn::FnArg::Receiver(_) => None,
syn::FnArg::Typed(pat_ty) => {
let mut static_ty = match UsedType::new(*pat_ty.ty.clone()) {
let ty = match UsedType::new(*pat_ty.ty.clone()) {
Ok(v) => v.resolved_ty,
Err(e) => return Some(Err(e)),
};
staticize_lifetimes(&mut static_ty);
Some(Ok(static_ty))
Some(Ok(ty))
}
}
})
Expand Down Expand Up @@ -275,9 +273,8 @@ impl PgExtern {
let return_type = match &self.func.sig.output {
syn::ReturnType::Default => None,
syn::ReturnType::Type(arrow, ty) => {
let mut static_ty = ty.clone();
staticize_lifetimes(&mut static_ty);
Some(syn::ReturnType::Type(*arrow, static_ty))
let ty = ty.clone();
Some(syn::ReturnType::Type(*arrow, ty))
}
};

Expand All @@ -287,6 +284,20 @@ impl PgExtern {
Some(content) => ToSqlConfig { content: Some(content), ..self.to_sql_config.clone() },
};

let lifetimes = self
.func
.sig
.generics
.params
.iter()
.filter_map(|generic| match generic {
// syn::GenericParam::Type(_) => todo!(),
syn::GenericParam::Lifetime(lt) => Some(lt),
_ => None, // syn::GenericParam::Const(_) => todo!(),
})
.collect::<Vec<_>>();
let hrtb = if lifetimes.is_empty() { None } else { Some(quote! { for<#(#lifetimes),*> }) };

let sql_graph_entity_fn_name =
syn::Ident::new(&format!("__pgrx_internals_fn_{}", ident), Span::call_site());
quote_spanned! { self.func.sig.span() =>
Expand All @@ -297,7 +308,7 @@ impl PgExtern {
extern crate alloc;
#[allow(unused_imports)]
use alloc::{vec, vec::Vec};
type FunctionPointer = #unsafety fn(#( #input_types ),*) #return_type;
type FunctionPointer = #hrtb #unsafety fn (#( #input_types ),*) #return_type;
let metadata: FunctionPointer = #ident;
let submission = ::pgrx::pgrx_sql_entity_graph::PgExternEntity {
name: #name,
Expand Down
47 changes: 17 additions & 30 deletions pgrx-sql-entity-graph/src/postgres_type/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use crate::enrich::{ToEntityGraphTokens, ToRustCodeTokens};
use proc_macro2::{Ident, Span, TokenStream as TokenStream2};
use quote::quote;
use syn::parse::{Parse, ParseStream};
use syn::{DeriveInput, Generics, ItemStruct};
use syn::{DeriveInput, Generics, ItemStruct, Lifetime, LifetimeDef};

use crate::{CodeEnrichment, ToSqlConfig};

Expand Down Expand Up @@ -103,37 +103,24 @@ impl PostgresTypeDerive {
impl ToEntityGraphTokens for PostgresTypeDerive {
fn to_entity_graph_tokens(&self) -> TokenStream2 {
let name = &self.name;
let mut static_generics = self.generics.clone();
static_generics.params = static_generics
.params
.clone()
.into_iter()
.flat_map(|param| match param {
item @ syn::GenericParam::Type(_) | item @ syn::GenericParam::Const(_) => {
Some(item)
}
syn::GenericParam::Lifetime(mut lifetime) => {
lifetime.lifetime.ident = Ident::new("static", Span::call_site());
Some(syn::GenericParam::Lifetime(lifetime))
}
})
.collect();
let mut staticless_generics = self.generics.clone();
staticless_generics.params = static_generics
let generics = self.generics.clone();
let (impl_generics, ty_generics, where_clauses) = generics.split_for_impl();

// We need some generics we can use inside a fn without a lifetime for qualified paths.
let mut anon_generics = generics.clone();
anon_generics.params = anon_generics
.params
.clone()
.into_iter()
.flat_map(|param| match param {
item @ syn::GenericParam::Type(_) | item @ syn::GenericParam::Const(_) => {
Some(item)
}
syn::GenericParam::Lifetime(_) => None,
syn::GenericParam::Lifetime(lt_def) => Some(syn::GenericParam::Lifetime(
LifetimeDef::new(Lifetime::new("'_", lt_def.lifetime.span())),
)),
})
.collect();
let (staticless_impl_generics, _staticless_ty_generics, _staticless_where_clauses) =
staticless_generics.split_for_impl();
let (_static_impl_generics, static_ty_generics, static_where_clauses) =
static_generics.split_for_impl();
let (_, anon_ty_gen, _) = anon_generics.split_for_impl();

let in_fn = &self.in_fn;
let out_fn = &self.out_fn;
Expand All @@ -144,7 +131,7 @@ impl ToEntityGraphTokens for PostgresTypeDerive {
let to_sql_config = &self.to_sql_config;

quote! {
unsafe impl #staticless_impl_generics ::pgrx::pgrx_sql_entity_graph::metadata::SqlTranslatable for #name #static_ty_generics #static_where_clauses {
unsafe impl #impl_generics ::pgrx::pgrx_sql_entity_graph::metadata::SqlTranslatable for #name #ty_generics #where_clauses {
fn argument_sql() -> core::result::Result<::pgrx::pgrx_sql_entity_graph::metadata::SqlMapping, ::pgrx::pgrx_sql_entity_graph::metadata::ArgumentError> {
Ok(::pgrx::pgrx_sql_entity_graph::metadata::SqlMapping::As(String::from(stringify!(#name))))
}
Expand All @@ -166,19 +153,19 @@ impl ToEntityGraphTokens for PostgresTypeDerive {
use ::pgrx::datum::WithTypeIds;

let mut mappings = Default::default();
<#name #static_ty_generics as ::pgrx::datum::WithTypeIds>::register_with_refs(
<#name #anon_ty_gen as ::pgrx::datum::WithTypeIds>::register_with_refs(
&mut mappings,
stringify!(#name).to_string()
);
::pgrx::datum::WithSizedTypeIds::<#name #static_ty_generics>::register_sized_with_refs(
::pgrx::datum::WithSizedTypeIds::<#name #anon_ty_gen>::register_sized_with_refs(
&mut mappings,
stringify!(#name).to_string()
);
::pgrx::datum::WithArrayTypeIds::<#name #static_ty_generics>::register_array_with_refs(
::pgrx::datum::WithArrayTypeIds::<#name #anon_ty_gen>::register_array_with_refs(
&mut mappings,
stringify!(#name).to_string()
);
::pgrx::datum::WithVarlenaTypeIds::<#name #static_ty_generics>::register_varlena_with_refs(
::pgrx::datum::WithVarlenaTypeIds::<#name #anon_ty_gen>::register_varlena_with_refs(
&mut mappings,
stringify!(#name).to_string()
);
Expand All @@ -187,7 +174,7 @@ impl ToEntityGraphTokens for PostgresTypeDerive {
file: file!(),
line: line!(),
module_path: module_path!(),
full_path: core::any::type_name::<#name #static_ty_generics>(),
full_path: core::any::type_name::<#name #anon_ty_gen>(),
mappings: mappings.into_iter().collect(),
in_fn: stringify!(#in_fn),
in_fn_module_path: {
Expand Down
Loading
Loading