Introduce a #[declare_sql_function] attribute macro

[weiznich]

This commit introduces a new #[declare_sql_function] attribute macro that can be applied to extern "SQL" blocks. This is essentially the same as the existing define_sql_function! function like macro in terms of functionality.

I see the following advantages of using an attribute macro + an extern "SQL" block instead:

• This is closer to rust syntax, so rustfmt will understand that and work correctly inside these blocks
• This allows to put several functions into the same block
• Maybe in the future this also allows to apply attributes to the whole block instead of to each item (that's something for a followup PR)

The downside of this change is that we then have three variants to declare sql functions:

• sql_function!() (deprectated)
• define_sql_function!() (introduced in 2.2, we might want to deprecate that as well?)
• The new attribute macro

For now this is more a request for comments if that's a desirable change. If we decide to go that way I would like to:

• Deprecating define_sql_function!()
• Reexport this new macro from the crate root
• Convert all existing usages in diesel
• Add a changelog entry

[Ten0]

TL;DR: Not initially super enthusiastic, but no strong opinion and open 🙂

Currently in the examples and tests we write things such as:

define_sql_function!(fn nextval(a: sql_types::VarChar) -> sql_types::BigInt);

Indeed this feels harder to read than

#[declare_sql_function]
extern "SQL" {
    fn nextval(a: sql_types::VarChar) -> sql_types::BigInt;
}

However I actually almost never use the first writing, I rather use this one:

define_sql_function! {
    fn nextval(a: sql_types::VarChar) -> sql_types::BigInt;
}

and that one feels to me like it's as readable as the proposed syntax.
(We'd just need to add ability to declare several functions in there like is done with declare_sql_function.)

My first feeling is that the new syntax may feel more magical and more compiler-integrated than it actually is.
It also seems more verbose. (In particular "SQL" as a string literal rubs me the wrong way, maybe because there's already #[declare_sql_function] above 😬)
But that's no strong opinion though. Maybe I'd actually change my mind with usage.

Also I wouldn't want to prevent anyone from having this writing if they prefer. :)
It feels like a nice compromise would be to allow the two writings (and not deprecate define_sql_function)? (in a similar spirit as we did with alias)?

This is closer to rust syntax, so rustfmt will understand that and work correctly inside these blocks

I recall seeing rustfmt being able to format code that is used in macros (specifically with format_macro_bodies enabled) (in particular with lazy_static!), maybe specifically when used through rust-analyzer, maybe because it may be able to understand how items end up being parsed (expression, ...).
Currently we seem to be parsing each element manually. I wonder what would be the extent to which we could instead parse it directly as ForeignItemFn, which seems to be what the new code does, and that may solve this issue?
(But then maybe that doesn't work and rust-anlayzer/rustfmt uses some magic that's related to spans of expanded macros instead, I'm not sure.)

[weiznich]

(In particular "SQL" as a string literal rubs me the wrong way, maybe because there's already #[declare_sql_function] above 😬)

It shouldn't be too hard to just not have an API there at all given that it's not to specify one in valid rust syntax.

It feels like a nice compromise would be to allow the two writings (and not deprecate define_sql_function)? (in a similar spirit as we did with alias)?

That would be an variant, although I would prefer to have only one variant if possible.

I recall seeing rustfmt being able to format code that is used in macros (specifically with format_macro_bodies enabled) (in particular with lazy_static!), maybe specifically when used through rust-analyzer, maybe because it may be able to understand how items end up being parsed (expression, ...).
Currently we seem to be parsing each element manually. I wonder what would be the extent to which we could instead parse it directly as ForeignItemFn, which seems to be what the new code does, and that may solve this issue?
(But then maybe that doesn't work and rust-anlayzer/rustfmt uses some magic that's related to spans of expanded macros instead, I'm not sure.)

I don't think rustfmt does any macro expansion at all. As far as I know there is just some heuristics that checks if the code is ~rust code and then formats it. For whatever reason that doesn't seem to work with the function like macro, but it works with the marco attribute variant.

---

That written: The main advantage of the new variant is that you can easily declare multiple functions in one block. It becomes much shorter than the older variant in these cases. You can see that in src/pg/expressions/functions.rs where this change removes quite a lot of lines.

The other motivation for this that while working on #4322 I noticed that we currently don't restrict sql functions to specific backends. That's something we should likely do for many cases (like for all postgres specific functions, etc). If all these functions are placed into a single extern block we can easily place the relevant attributes on the block itself, instead of having to repeat them for each function definition.

Finally it might help to speedup the compilation time of diesel while working on diesel. Rustc doesn't cache proc-macro expansions and I think having one large expension is "cheaper" than repeatably calling the same macro again and again, but I've not measured that yet.

[weiznich]

As for the formatting argument: It seems like you can configure rustfmt to format macro calls: https://github.com/rust-lang/rustfmt/blob/master/Configurations.md#skip_macro_invocations

That setting is false by default and currently unstable.

[Ten0]

I have no strong preference about this.
My gut feeling is that this writing feels more magical than it really is but if you feel strongly about not having two writings that's fine with me 🙂