Shouldn't this be outside of the macro call?

I keep the pattern from here

I double-check with cargo expand, and the output follows. The docs are in the correct position, and the feature is in all generated code but duplicated.

# cfg inside
❯ cargo expand -F postgres pg::expression::functions::array_append
    Checking diesel v2.2.0 (/.../diesel/diesel)
    Finished `dev` profile [unoptimized   debuginfo] target(s) in 1.01s

/// Append an element to the end of an array.
/// # Example
///
/// ```rust
/// # include!("../../doctest_setup.rs");
/// #
/// # fn main() {
/// #     run_test().unwrap();
/// # }
/// #
/// # fn run_test() -> QueryResult<()> {
/// #     use diesel::dsl::array_append;
/// #     use diesel::sql_types::{Integer, Array};
/// #     let connection = &mut establish_connection();
/// let ints = diesel::select(array_append::<Integer, Array<_>, _, _>(vec![Some(1), Some(2)], Some(3)))
///     .get_result::<Vec<Option<i32>>>(connection)?;
/// assert_eq!(vec![Some(1), Some(2), Some(3)], ints);
/// #     Ok(())
/// # }
/// ```
#[cfg(feature = "postgres_backend")]
#[cfg(feature = "postgres_backend")]
#[allow(non_camel_case_types)]
pub fn array_append<
    T: SingleValue,
    Arr: ArrayOrNullableArray<Inner = Nullable<T>>   SingleValue,
    a,
    e,
>(a: a, e: e) -> array_append<T, Arr, a, e>
where
    a: ::diesel::expression::AsExpression<Arr>,
    e: ::diesel::expression::AsExpression<Nullable<T>>,
{
    array_append_utils::array_append {
        a: a.as_expression(),
        e: e.as_expression(),
        T: ::std::marker::PhantomData,
        Arr: ::std::marker::PhantomData,
    }
}
#[cfg(feature = "postgres_backend")]
#[allow(non_camel_case_types, non_snake_case)]
///The return type of [`array_append()`](super::fn_name)
pub type array_append<T, Arr, a, e> = array_append_utils::array_append<
    T,
    Arr,
    <a as ::diesel::expression::AsExpression<Arr>>::Expression,
    <e as ::diesel::expression::AsExpression<Nullable<T>>>::Expression,
>;

And with the cfg outside the macro, it removes the duplicated cfg in the generated code.

# cfg outside
❯ cargo expand -F postgres pg::expression::functions::array_append
    Blocking waiting for file lock on build directory
    Checking diesel v2.2.0 (/home/guiss/projetos/open_source/diesel/diesel)
    Finished `dev` profile [unoptimized   debuginfo] target(s) in 22.55s

/// Append an element to the end of an array.
/// # Example
///
/// ```rust
/// # include!("../../doctest_setup.rs");
/// #
/// # fn main() {
/// #     run_test().unwrap();
/// # }
/// #
/// # fn run_test() -> QueryResult<()> {
/// #     use diesel::dsl::array_append;
/// #     use diesel::sql_types::{Integer, Array};
/// #     let connection = &mut establish_connection();
/// let ints = diesel::select(array_append::<Integer, Array<_>, _, _>(vec![Some(1), Some(2)], Some(3)))
///     .get_result::<Vec<Option<i32>>>(connection)?;
/// assert_eq!(vec![Some(1), Some(2), Some(3)], ints);
/// #     Ok(())
/// # }
/// ```
#[cfg(feature = "postgres_backend")]
#[allow(non_camel_case_types)]
pub fn array_append<
    T: SingleValue,
    Arr: ArrayOrNullableArray<Inner = Nullable<T>>   SingleValue,
    a,
    e,
>(a: a, e: e) -> array_append<T, Arr, a, e>
where
    a: ::diesel::expression::AsExpression<Arr>,
    e: ::diesel::expression::AsExpression<Nullable<T>>,
{
    array_append_utils::array_append {
        a: a.as_expression(),
        e: e.as_expression(),
        T: ::std::marker::PhantomData,
        Arr: ::std::marker::PhantomData,
    }
}
#[cfg(feature = "postgres_backend")]
#[allow(non_camel_case_types, non_snake_case)]
///The return type of [`array_append()`](super::fn_name)
pub type array_append<T, Arr, a, e> = array_append_utils::array_append<
    T,
    Arr,
    <a as ::diesel::expression::AsExpression<Arr>>::Expression,
    <e as ::diesel::expression::AsExpression<Nullable<T>>>::Expression,
>;

You are right, thanks!
I can open a PR later to fix the others

It doesn't look like there should be Nullable here : the fact that pg arrays are always Nullable is already expressed to the Rust type system by the fact that we export them to the schema.rs as Array<Nullable<T>>, and if the schema was patched to express that the type of the array is in fact Array<T> we should still be able to use this function by providing an expression of sql type T.

It looks like this can be achieved by having Nullable<T> as ElementType for ArrayOrNullableArray

We should have the following two properties (unless I'm mistaken on how postgresql behaves)

1. The SQL type of the resulting expression is Nullable depending on whether Arr is Nullable (assuming postgres returns NULL if passing a NULL array)
2. The SQL type of the resulting expression is not Nullable depending on whether T is Nullable (assuming that postgres would append a NULL to the array, not make the whole resulting expression NULL)

Can you add tests that showcase that both these things work as expected?

This should be doable by duplicating the existing test and explicitly specifying the types corresponding to each scenario.

Thanks 😊🙏

Thanks for the review!

The pg behavior for this functions with NULL is listed bellow:

1. SELECT array_append(ARRAY[1,2], 3) returns ARRAY[1,2,3]
2. SELECT array_append(ARRAY[1,2], NULL) returns ARRAY[1, 2, NULL]
3. SELECT array_append(NULL, 3) returns ARRAY[3]
4. SELECT array_append(NULL, NULL) returns ARRAY[NULL]

1. The SQL type of the resulting expression is Nullable depending on whether Arr is Nullable (assuming postgres returns NULL if passing a NULL array)

If passing NULL, postgres apparently handles it as an empty array, so I can't think of a way that this function returns NULL.

2. The SQL type of the resulting expression is not Nullable depending on whether T is Nullable (assuming that postgres would append a NULL to the array, not make the whole resulting expression NULL)

I see, thanks for the clarification. Can you help me on how to do that? That's why I did with Nullable<T> before, I don't know how to express this with types.

Can you add tests that showcase that both these things work as expected?

Sure! Should I create doc tests or test in diesel_test?

Thanks!

Ah! Thanks for investigating the behavior! 😊

I thought that we may have to involve the nullability propagation helpers traits here to construct the return type, which would maybe have involved either declaring it without the macro or adding a feature to do so in the macro, however given what you're saying (3 and 4) it seems that we only ever want to return an array, regardless of original nullability of the array, so it seems that the current signature is correct. 😊
(Which is furtunate because that turns out to be significantly simpler.)

It's fine as a first version if a user needs the input array to already have its elements be Nullable to be able to append potentially null values to it. That may be incomplete (although I doubt there would be many use cases), but most importantly it's correct 😊.
What wouldn't have been fine is if it could return nulls but that wasn't expressed in the signature (1), or if by fixing (1) we accidentally made (2) wrong (by considering the arguments in the same way, in a mode where if any argument is Nullable resulting value is Nullable, which IIRC is already our default behavior for operators but apparently not for functions - what I meant is in that case we couldn't just "enable" that behavior for functions as well).

OK so overall, current implementation looks good, we can probably just add tests to make sure that we get the correct type for all 4 scenarios, with somewhat explicit sql types. 🙂

I have no strong opinion on where each test for the 4 scenarios you mentioned should live, but maybe another reviewer will. By default unless too verbose I'd probably just have them all in the one doctest.

Thanks! I added the test as a docstring