Fix support of sql functions with #[auto_type]

[Ten0]

Fixes #3745

The proposed implementation is not strictly backwards-compatible in that it breaks code like this:

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;

where with dsl::max now importing the type alias as well and diesel::helper_types::max also importing that type alias, rust would say there is a conflict between the two.
(Note that this resolves an inconsistency though: in the current version of Diesel some type aliases are directly provided by dsl, and for some you have to go through HelperTypes.)

[weiznich]

I had finally some time to look at this again and try some other things I had in mind. One of the ideas was to basically migrate all of the breaking changes by just using associated types/functions on the type def instead of having a module. Unfortunately that does not work as for the associated types we run into rust-lang/rust#8995. Although it should work for the register_* functions.

That written: I agree with you that changing the generated is fundamentally the right approach and it seems like we cannot avoid the breaking change. Therefore I would propose the following way forward:

• Create a macro with a new name (as you've already done, although I would prefer having another name than sql_function_v2!. Maybe just define_sql_function! instead?)
• Move the free register_ function as associated functions onto the struct. (That should allow us to call them on the type def as well)
• Mark the internal module as #[doc(hidden)] to exclude it from the public API
• Deprecate the old macro and point to the new one

[Ten0]

• Maybe just define_sql_function! instead

Love that name! 😃

Move the free register_ function as associated functions onto the struct.

Not sure what struct you're thinking of - they are mostly all generic, but that function shouldn't be, no?

[weiznich]

Not sure what struct you're thinking of - they are mostly all generic, but that function shouldn't be, no?

I'm thinking about the struct generated by the macro. As the functions don't care about the generic parameter we can just use a fixed type in place of it. So essentially we would have a impl foo<()> block for the foo function. After that it should be possible to skip that generic to call the then associated functions.

[Ten0]

I'm thinking about the struct generated by the macro

Somewhat raw thoughts: I'm not sure that works through the type alias that remaps the type arguments through an AsExpression... Not completely sure though.
If it does work, that might still block evolutions where we'd want the type alias to not point directly to the generated type, like we had to do with Grouped a while back. But that seems reasonably unlikely with functions, and worse case scenario I guess we could embed the grouping (or similar) inside the implementations generated by the macro, so it's probably not that big a deal.

[weiznich]

You are right, that sadly also doesn't work: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=d1a03dc070d8e6595c52b6255f4911b5

The other alternative that might work to hide the *_impl module is to move the register functions to SqliteConnection. Essentially have something like that there:

impl SqliteFunction {
    fn register_function<T: SqliteFunctionTrait>(&mut self, callback: impl Fn*()) -> Result<()>;
}

although that likely requires again that the user specify a type for the generic argument there :( So that is also not great …

[Ten0]

TBH I think I am reasonably happy with the extra module. It's not particularly elegant but I feel like it's probably the most programmatically appropriate representation, and we don't necessarily need to make it more private than it used to be - but I have no strong conviction on this.

[weiznich]

My reasoning for not wanting to expose that module (again) is that it would prevent breaking anything again due to that module. But given that I do not see a good way to make this happen it's likely the best to expose that module anyway. Maybe we can change the name to {function_name}_implementation? That seems to be more fitting than {function_name}_internals, which might imply that users shouldn't touch these functions.

With these two names changed (module + macro) I'm fine with this change. I will update my other PR to pull in these changes so that we get all the fixes in the end.

[Ten0]

{function_name}_implementation

I'm not super fond of that name either but {function_name}_internals doesn't feel better indeed. 😅
Overall both convey that this is where stuff is implemented but AFAICT we don't need to actually expose the types that are in there to limit the public surface, so it's not particularly relevant that they are there.
I think what we actually want to convey is that this is where the user would find the rest of the relevant stuff tied to this function (besides the function itself and the type alias).
How about {function_name}_utils ?

[weiznich]

{function_name}_utils sounds great. Let's go with that.

[Ten0]

closing in favor of #3783