dsl::max doesn't work with dsl::auto_type

[Elrendio]

Setup

Versions

• Rust: rustc --version = rustc 1.70.0 (90c541806 2023-05-31)
• Diesel: master
• Database: _
• Operating System Archlinux

Feature Flags

• diesel: "postgres", "r2d2", "serde_json", "chrono",

Problem Description

FYI @Ten0 knows and says it's a blocker for the release of auto_type so he'll try to fix.

When deriving on a struct Selectable with one field having a select_expression containing dsl::max compilation breaks with error:

error[E0573]: expected type, found function `diesel::dsl::max`
   --> mod.rs:463:6
    |
463 |                     diesel::dsl::max<table::column>,
    |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not a type
    |
help: consider importing this type alias instead
    |
8   + use diesel::helper_types::max;
    |
help: if you import `max`, refer to it directly
    |
463 -                     diesel::dsl::max<table::column>,
463 +                     max<table::column>,
    |

What are you trying to accomplish?

Use Selectable derive

What is the expected output?

Code compiles

What is the actual output?

Compilation fails

Are you seeing any additional errors?

No

Steps to reproduce

mod schema {

    diesel::table! {
	    table (id) {
		    id -> Int4,
                    other_id -> Int4,,
		    column -> Int4,
	    }
    }
    diesel::table! {
	    other_table (id) {
		    id -> Int4,
		    column -> Int4,
	    }
    }
}

#[derive(Queryable, Serialize, Selectable)]
#[diesel(table_name = table)]
pub(crate) struct Struct {
	#[diesel(
		select_expression = schema::table::table
			.filter(schema::table::other_id.eq(schema::other_table::id))
			.select(dsl::max(table::column))
			.single_value(),
	)]
	pub(crate) max_val: Option<i32>,
}

Checklist

• I have already looked over the issue tracker and the discussion forum for similar possible closed issues.

• This issue can be reproduced on Rust's stable channel. (Your issue will be
  closed if this is not the case)
• This issue can be reproduced without requiring a third party crate

[Ten0]

@weiznich this might be an issue with the "type alias at the same path as function" approach (might require breaking change) so I think it's a blocker for the release.
I'll try to fix it this week. 🙂

[weiznich]

I think we can handle this without breaking change. We can just have a ”private" type dsl module in diesel::internals::derive::auto_type that is essentially

pub use diesel::dsl::*;
// overwrite the type provided by diesel::dsl wit the correct one
pub use diesel::expressions::helper_types::max;

#[auto_type] can then internally use that dsl module by default. Additionally we likely should mention these exceptions somehow in the documentation of the custom dsl prelude for user defined extension.

[Ten0]

#[auto_type] can then internally use that dsl module by default

if it did then custom functions couldn't be referred directly anymore. That's notably why it uses "always same path as function" by default (otherwise I guess it could just use diesel::helper_types).

That would be a breaking change for this reason.

I suspect max is actually not the only such case.

[Ten0]

So re-defining the dsl module in the exact same way as within Diesel in the local crate works. (I did not expect name resolving would work differently on external modules than on local modules...)

Alternately, explicitly specifying helper_types::max in addition to helper_types::* in the definition of diesel::dsl works.
However, in addition to being very verbose, that also breaks previously compiling code such as:

diesel/diesel_tests/tests/select_by.rs

Lines 161 to 162 in c99ebda

	use diesel::dsl::{count_star, max, CountStar};
	use diesel::helper_types::max;

(which did need the second line to compile for the same reason)

As far as I can tell that makes the best solution to keep both local type aliases generated by dsl::auto_type and diesel types working to leave them at the ~same path, but change the name. Either prefix/suffix it with something, or put it PascalCase.
Unfortunately that would imply:

1. Duplicating all our type aliases for SQL functions within Diesel to preserve backwards-compatibility
2. If PascalCase, method names can now collide with function names. Fortunately I don't think there are such cases currently.

[Ten0]

Wait not and exists both work. Not sure yet what the difference with min is.

[Ten0]

Ok so the issue comes from the fact that sql_function! generates a module with the same name as the function.
When that happens, Rust gets confused with the import of the module colliding with that of the type, and eventually stops resolving anything but the function.

Re-defining in the local crate works because the module generated by sql_function! is pub(crate) so it doesn't collide anymore.

[Ten0]

The best compromise is probably to release a new sql_function macro that generates the helper type at that path instead of the module.
This would also enable removing this doc:

diesel/diesel_derives/src/lib.rs

Lines 1029 to 1054 in c99ebda

	/// If you are using this macro for part of a library, where the function is
	/// part of your public API, it is highly recommended that you re-export this
	/// helper type with the same name as your function. This is the standard
	/// structure:
	///
	/// ```ignore
	/// pub mod functions {
	/// use super::types::*;
	/// use diesel::sql_types::*;
	///
	/// sql_function! {
	/// /// Represents the Pg `LENGTH` function used with `tsvector`s.
	/// fn length(x: TsVector) -> Integer;
	/// }
	/// }
	///
	/// pub mod helper_types {
	/// /// The return type of `length(expr)`
	/// pub type Length<Expr> = functions::length::HelperType<Expr>;
	/// }
	///
	/// pub mod dsl {
	/// pub use functions::*;
	/// pub use helper_types::*;
	/// }
	/// ```

Any opinion with regards to choice between:

1. sql_function_v2! + deprecate sql_function!
2. sql_function_v2! and don't deprecate
3. Breaking change on sql_function!

?

EDIT: Ah but that also breaks

diesel/diesel_tests/tests/select_by.rs

Lines 161 to 162 in c99ebda

	use diesel::dsl::{count_star, max, CountStar};
	use diesel::helper_types::max;

the name `max` is defined multiple times

Well alternately that leaves us with the "duplicate everything to PascalCase" option...

[Ten0]

@weiznich I'd like an opinion on whether this particular breaking change could be considered a bug fix, vs the loss of ergonomics and duplicated types of pascal-casing

[weiznich]

Well alternately that leaves us with the "duplicate everything to PascalCase" option...

To explore that option a bit more: We only talk about these 6 functions here, right? (If that includes now, today and date as well we are talking about 9 functions…)

diesel/diesel/src/expression/functions/helper_types.rs

Lines 8 to 23 in 409585e

	pub type not<Expr> = operators::Not<Grouped<Expr>>;
	/// The return type of [`max(expr)`](crate::dsl::max())
	pub type max<Expr> = super::aggregate_ordering::max::HelperType<SqlTypeOf<Expr>, Expr>;
	/// The return type of [`min(expr)`](crate::dsl::min())
	pub type min<Expr> = super::aggregate_ordering::min::HelperType<SqlTypeOf<Expr>, Expr>;
	/// The return type of [`sum(expr)`](crate::dsl::sum())
	pub type sum<Expr> = super::aggregate_folding::sum::HelperType<SqlTypeOf<Expr>, Expr>;
	/// The return type of [`avg(expr)`](crate::dsl::avg())
	pub type avg<Expr> = super::aggregate_folding::avg::HelperType<SqlTypeOf<Expr>, Expr>;
	/// The return type of [`exists(expr)`](crate::dsl::exists())
	pub type exists<Expr> = crate::expression::exists::Exists<Expr>;

If that only affects that number of function I think it might be the easiest solution to:

• Deprecate the old aliases
• Add new ones in in PascalCase
• Update the documentation of sql_function! to show that these type aliases should be named in PascalCase, although it seems like that's even already the case as shown by the link in your comment above.

[Ten0]

We only talk about these 6 functions here, right?

The issue is that we want the types to all be in the same case so that dsl::auto_type can look for the types in a consistent manner, so instead that would be updating the aliases for all the functions and deprecating all the old aliases.
Another advantage of having the types hold the same exact name as the functions is that a use statement will import both the function and its type, making it easy to declare query fragments/selectable using other dsl::auto_type factored query fragments.

although it seems like that's even already the case as shown by the link in your comment above

Although that is the official convention proposed by this doc, it's not the current convention within Diesel itself (and the user is invited to do it themselves for the particular case of sql_function!), whereas we'd want the function to always generate that type.

[weiznich]

I did not find the time today to look into this issue, hopefully I have capacity for that next week. I've started to play a bit around, but I did not come to a conclusion yet. As part of the playing around I've discovered that QueryDsl::distinct, QueryDsl::distinct_on, QueryDsl::limit and QueryDsl::offset do not work with #[auto_type]. For at least the later two the issue is that the corresponding types have only one generic argument instead of two. I think it would be useful to write some code that checks for all existing QueryDsl, *ExpressionMethod and other dsl functions whether or not the corresponding type defs are what #[auto_type] expects.

[Ten0]

Cool, thanks.

As far as I'm concerned I'd favor the compromise of considering the solution at #3745 (comment) as a bugfix of the types not being available in the dsl module, which is inconsistent and seems like it wasn't intended.

[weiznich]

I've continued to put together a list of all expressions that might occur. It still misses some items like free standing functions (which are the original problem reported by this issue.) See here for the result. Every commented out expression seems to be not supported right now. I think we want to classify each of the items and see what's the problem for each one and how we might solve it. Help for this classification would be greatly appreciated.

[Ten0]

Help for this classification would be greatly appreciated.

I've implemented the fix for sql_function in the slightly-breaking manner in #3773, which should fix the issue with the sql functions.

Using that (and #3774) it seems that all the commented code still doesn't compile, so the issue is just missing type aliases for the corresponding expression methods (e.g. fn contains_or_eq<T>(self, other: T) -> dsl::ContainsNetLoose<Self, T> the names don't match, we should make them match and deprecate the old type alias).

(All these examples seem to be expression methods though, not expression functions, which is why they don't seem to encounter the issue with functions described earlier)

The only two particular cases that I see are:

• fn concat<T>(self, other: T) -> dsl::ConcatBinary<Self, T>
• fn is_contained_by<T>(self, other: T) -> dsl::IsContainedByJsonb<Self, T>

because there seems to already be type aliases called resp. Concat and IsContainedBy, but they represent different things.

That being said I think if these require manual annotations for now:

#[auto_type]
fn qf() -> _ {
    let x: dsl::ConcatBinary<_, _> = a.concat(b);
    some_expression_that_uses(x)
}

that's probably fine given how uncommon they are.

[weiznich]

I've filled #3783 to address most of the broken cases in a hopefully not-breacking manner. This is an alternative approach to #3773 which does not require to update all code that already uses sql_function!. It also addresses most of the cases regarding to existing aliases with the same name and adds some more aliases for free standing functions like insert and delete.