Add a test which calls a user-defined function with NULL argument #668
Conversation
willmurnane
changed the title
There was a problem hiding this comment.
Apologies for the delay in review. I find the content of this segment of the code hard to follow in general as I am not as experienced in it, and as I start working on attempting to update and land #615, it's likely to induce conflicts with this set of changes. So my initial remarks are just from a relatively cursory pass... in general this has proven to be an extremely touchy segment of the code, in the past, especially around SRFs, so any changes touching it could stand to add some comments and focus, a lot, on legibility.
pgx-utils/src/lib.rs
Outdated
| Iterator { | ||
| types: Vec<String>, | ||
| is_optional: bool, | ||
| }, |
There was a problem hiding this comment.
I'm not sure I understand why we're now adding a bool into the same variant, as opposed to using a different variant, as previously done?
There was a problem hiding this comment.
The different variant was translated into "call the same code, but with a boolean flag" anyways. I don't mind reverting this change, but it collapsed the code in item_fn_with_rewrite quite a bit. Combined with factoring out the if types.len() == 1 check, that collapsed all 4 cases down to one code path.
There was a problem hiding this comment.
I would actually expect it to be such an... "overly ritualistic" form? It does suggest this code needs to be refactored, but there's something in the back of my mind that suggests this particular refactoring will reach a local maxima faster and thus lower than other ones which try to address the code factoring from a "one-level up" perspective.
pgx-utils/src/rewriter.rs
Outdated
| } | ||
| } else if type_matches(&type_, "pg_sys :: FunctionCallInfo") | ||
| || type_matches(&type_, "pgx :: pg_sys :: FunctionCallInfo") | ||
| { | ||
| quote_spanned! {ident.span()=> | ||
| let #name = #fcinfo_ident; | ||
| let #name: pgx::pg_sys::FunctionCallInfo = #fcinfo_ident; |
There was a problem hiding this comment.
rustfmt often leaves formatting in macro contexts untouched. Here, #name: is used, elsewhere, #name : is used, this must be consistent, and preferably consistent with the default type annotation formatting that rustfmt emits.
There was a problem hiding this comment.
Good catch. I'll update.
pgx-utils/src/rewriter.rs
Outdated
| if type_matches(ty.ty.as_ref(), "()") | ||
| || extract_type_from_option(ty.ty.as_ref()).is_some() | ||
| || type_matches(&ty.ty, "pg_sys :: FunctionCallInfo") | ||
| || type_matches(&ty.ty, "pgx :: pg_sys :: FunctionCallInfo") |
There was a problem hiding this comment.
Why the spaces in pgx :: pg_sys :: FunctionCallInfo?
There was a problem hiding this comment.
I think that's due to how format!("{}", quote! works. I was just matching the style seen elsewhere.
pgx-utils/src/rewriter.rs
Outdated
| @@ -482,7 +449,7 @@ impl PgGuardRewriter { | |||
| func.sig.ident.span(), | |||
| ); | |||
|
|
|||
| let arg_list = PgGuardRewriter::build_arg_list(&sig, false); | |||
| let (arg_list, _matchable) = PgGuardRewriter::build_arg_list(&sig, false); | |||
There was a problem hiding this comment.
This function had its return signature altered but the latter half of the tuple is used in only one location? Is there another way to do this?
There was a problem hiding this comment.
I'm not sure it's entirely correct that I'm ignoring it in this case. I'm not totally clear on how PgGuardRewriter .item_fn is called and for what reasons. It looks like it's usually called from here, where rewrite_args is always supplied as false, but is also called from here, where it's also hard-coded to a value of false. So based on I think it might be okay to entirely remove item_fn_with_rewrite, the argument rewrite_args, and so on. I didn't want to do that surgery without understanding more, though.
There was a problem hiding this comment.
Oops - I misread. In the first case, it's got a value of true hard-coded, not false. Not sure how I managed that one...
|
I agree with the overall challenging nature of this code, especially given the behavior of Rust macros ("macro expansion to text is a lossy process"). I could try a couple of things to make this easier to review:
|
|
I think this is fine as one PR, but comments comments comments. I don't think you want to rebase onto #615 yet. Soon maybe. If you want to give it a quick experimental pass on your local to see if there are any conflicts you think are particularly noteworthy, sure, but that PR itself needs to be updated. |
There was a problem hiding this comment.
I want to land this before 0.5.0, truly I do, but this PR too is also dashed upon the rocks of #615.
|
#615 has landed. |
|
#615 fixed the problem that this PR was addressing. I've winnowed it down to just add the test that previously failed. |
willmurnane
changed the title
I found this incorrect behavior, but I haven't figured out how to fix it.