Generic Server Fn

[sjud]

Hello,

I know there is another generic SeverFn PR open #3008 but I decided to try my hand at it anyways since it's been something I've wanted for a while. I had needs that were not covered by the other PR. I hope I am not stepping on anyones toes @rakshith-ravi .

This PR covers implementing generic server functions without touching any non-macro code. Specifically I've added an attribute for registering the types of a particular generic function and extending the macro path logic. Instead of impl<T> ServerFn for SomeStruct<T>, for each registered type set the macro generates a specific impl ServerFn for SometStruct<SpecificT>.

My implementation is generic over server function arguments (the focus of the other PR) as well as, generic over non input types. For any type that isn't in the input of the server function, we add a _marker type with PhantomData<T,..,U> with the set of non input types.

It's also generic over the T in Result<T,ServerFnError<E>>, although not E (it'd be very easy to add that though.)

Something I also wanted which is kind of weird is a way to specify a generic server functions in the front end over a type that can only exist on the backend. This will let us (when generic components are expanded), build a component that is generic over a backend implementation. So you could have a GraphComponent which is generic over it's data source, and it uses the same server function to compile data from different data sources which are specified in the client code.

I've introduced some macros to build shims between generated front end types and the SSR only backend types, and logic within the server macro to map those client side types into their actualized server types.

I also refactored the server function macro, which I hope was OK. I wasn't able to perform any edits on it without doing a big (and yet painless) refactor.

I've added some examples to show off the the new code. This PR is based on my RFC (which got no comments lol) https://github.com/sjud/server-fn-generic-rfc , which is out of date since I wrote it before the implementation of it but covers what I was going for in more detail.

Best,
Sam

[gbj]

I have not yet looked at the implementation changes in server_fn_macro/src/lib.rs. As it's quite a big diff, I thought I'd take a look at the API (thanks for the extensive examples!) under the assumption that the implementation will probably change a bit, and I can review that once I'm ready.

It's been long enough since I've taken a look at the internals of the #[server] macro that it will probably take me a while to understand and keep track of the refactor relative to how it works.

I'll note that the CI is failing because of the unused variables etc., which also surfaces a number of todo!()s in the macro code, which should be resolved before this is ready as well.

[gbj]

#[register] seems like a reasonable way to do this.

[sjud]

:)

[gbj]

It's not clear to me that the "default generic" approach is working here — for example, if I try to call it as a function

let x = server_hello_world_generic_with_default();

The compiler errors

error[E0283]: type annotations needed
    --> src/app.rs:1077:13
     |
1077 |     let x = server_hello_world_generic_with_default();
     |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot infer type of the type parameter `S` declared on the function `server_hello_world_generic_with_default`
     |
     = note: cannot satisfy `_: SomeTrait`
     = help: the following types implement trait `SomeTrait`:
               AStruct
               AStruct2
               SomeDefault

Looking at the macro expansion, it looks like the default is not added on the function

pub async fn server_hello_world_generic_with_default<S: SomeTrait>(
) -> Result<String, ServerFnError> {
    __server_hello_world_generic_with_default::<S>().await
}

but of course, that's because you can't give default values to generics on functions

fn something<S: SomeTrait = SomeDefault>() {}

gives the error

error: defaults for type parameters are only allowed in `struct`, `enum`, `type`, or `trait` definitions
    --> src/app.rs:1075:14
     |
1075 | fn something<S: SomeTrait = SomeDefault>() {}
     |              ^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
     = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
     = note: for more information, see issue #36887 <https://github.com/rust-lang/rust/issues/36887>
     = note: `#[deny(invalid_type_param_default)]` on by default

From my perspective, this just makes it confusing: Server functions are intended as an abstraction over functions, but Rust functions don't allow default generics; so now an invalid syntax for a function (including a default generic) is accepted by the macro but stripped from the expansion.

I'll put it this way: I know Rust fairly well, and I did not know any of the above until I tested it. I think this is likely to lead to more user confusion than benefit.

[sjud]

Yes, I kind of tacked default on at the last minute as a "wouldn't this be cool" kind of feature. I came up against no default generic functions, I think I must have added it for server function structures but forget to try it against functions (I might have not gone ahead with it if I had lol). I'm not sure this feature makes sense for the reasons you stated i.e given that generics can't be default in server functions, and since the server function struct is where they'd have to be default in but that's not a given for any particular server function it is kind of confusing etc.

[gbj]

Likewise the addition of the _marker field here is confusing, I think — this name doesn't come from anywhere, so if I'm the user I don't know that it exists or how to tweak the name.

[sjud]

Should it be called _phantom instead?

We need PhantomData for specifying the generic types of the server functions when they otherwise aren't present. I could default to _phantom for field names and add an arg the ServerFnArg that let you specify an alternate _phantom field. My thinking here is that people could write

action.dispatch(ServerFnStruct{
arg1,arg2,..Default::default()
})

Maybe we should try to implement default for server fn structures with generic arguments that would otherwise require Phantomdata. But then again, you lose some of the strengths of your type system when you can then start to accidentally pass default arguments to a server function that would really prefer the real thing. Might be hard to debug...

But otherwise I think the existence of PhantomData would have to be communicated to the user via the documentation. Since I don't think there's an ergonomic way around using PhantomData here, alternatively we could propagate the generic canoncalization process to the naming convention (lol sorry) for the structure so for instance where we have

#[regsister(<String>,<u8>)]
async server_fn_name<T>() -> ...

and actually creates two structures
ServerFnName_String ServerFnName_u8, both without arguments.

But that's definitely worse.

[gbj]

I wonder if it might be better to either 1) allow the user to specify the name of the phantom struct in ssr_type_shim! or 2) rename this from ___Phantom to something more like ___FromClient.

I would prefer the first option for discoverability/"go to definition" reasons, personally.

[sjud]

Yeah I can do that, would you prefer a mixture of both or to force the user to name it.
The mixture would consume an argument and override the default of ___FromClient

[sjud]

Overall these seem like positive comments and I'm glad that the PR has passed a "vibe check" which was something I needed to know before I put more work into it. Thanks!

[sjud]

I have not yet looked at the implementation changes in server_fn_macro/src/lib.rs. As it's quite a big diff, I thought I'd take a look at the API (thanks for the extensive examples!) under the assumption that the implementation will probably change a bit, and I can review that once I'm ready.

It's been long enough since I've taken a look at the internals of the #[server] macro that it will probably take me a while to understand and keep track of the refactor relative to how it works.

I'll note that the CI is failing because of the unused variables etc., which also surfaces a number of todo!()s in the macro code, which should be resolved before this is ready as well.

For what it's worth, the refactor mostly just took the logic out of the main macro and put it in a bunch of functions but I don't think it changed the overall control flow or the logic (that much). There were some places that needed to be extended for generics. This is definitely a first pass but I basically just pulled the logic into seperate functions and then turned the macro into an if statement "if this is a generic do the new functionality" if not "do the old server function macro".