Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Attributes of function arguments should be applied to the function or to properties? #11

Open
cecton opened this issue Nov 13, 2023 · 6 comments

Comments

@cecton
Copy link
Member

cecton commented Nov 13, 2023

It is debatable of whether attributes of function arguments should be applied to the function itself or to properties struct definition. #[props_...] should be transferred to struct definition, however, I am not sure about other attributes.

All of the following order of attribute macros would result in different code generation behaviour to #[foreign_trait] if #[foreign_trait] will generate code based on #[foreign_trait_attr].

#[foreign_trait]
#[autoprops]
#[function_component]
fn Comp(#[foreign_trait_attr] arg: Arg) -> Html {}
#[autoprops]
#[foreign_trait]
#[function_component]
fn Comp(#[foreign_trait_attr] arg: Arg) -> Html {}
#[autoprops]
#[function_component]
#[foreign_trait]
fn Comp(#[foreign_trait_attr] arg: Arg) -> Html {}

(However, since this is not part of Yew, I am OK with leaving the implementation as-is and revisiting this in the future.)

Originally posted by @futursolo in #10 (comment)

@kirillsemyonkin
Copy link
Collaborator

kirillsemyonkin commented Nov 13, 2023

I said everything in the original discussion.

External

Given following:

#[foreign_trait]
#[autoprops]
#[function_component]
fn Comp(#[foreign_trait_attr] arg: Arg) -> Html { ... }

The proc macro named foreign_trait should strip the resulting output of any #[foreign_trait_attr] as following:

#[autoprops]
#[function_component]
fn Comp(arg: Arg /* possibly transformed fn args */) -> Html /* possibly transformed return type */ {
    /* possibly transformed fn body */
    ...
}

/* possibly generated external items */

After this, #[autoprops] sees only the output of #[foreign_trait], thus, it has no idea #[foreign_trait_attr] exists at the point it runs.

Internal for #[autoprops]

Given following:

#[autoprops]
#[foreign_trait]
#[function_component]
fn Comp(#[foreign_trait_attr] arg: Arg) -> Html {
    ...
}

The code generated in #10 would look something like this:

#[derive(Properties)]
struct CompProps {
    #[foreign_trait_attr] arg: Arg,
}

#[foreign_trait]
#[function_component]
fn Comp(CompProps { arg }: &CompProps) -> Html {
    let arg = ImplicitClone::implicit_clone(arg);
    ...
}

As you can see, the #[foreign_trait_attr] is detached from #[foreign_trait], and the generated struct contains no #[derive(ForeignTrait)] to accomodate the newly inserted #[foreign_trait_attr] on the struct field.

Even if #[foreign_trait] is moved toward #[foreign_trait_attr], it might be not where it is supposed to be: fn proc macros most likely are supposed to be there to transform the fn body, rather than do something to the struct.

If you want to move the #[foreign_trait_attr] toward #[foreign_trait], where would it be applied? There are no individual fn args in sight in the generated code, as it has been transformed into either a destructure (CompProps { #[foreign_trait_attr] arg }?) or into a clone assignment (#[foreign_trait_attr] let arg = arg.clone();?). The #[foreign_trait_attr] most likely is not supposed to be in either of those two positions, thus is not a good idea to do either of them.

Internal for #[function_component]

Just for the completeness sake, given following:

#[autoprops]
#[function_component]
#[foreign_trait]
fn Comp(#[foreign_trait_attr] arg: Arg) -> Html {
    ...
}

The code after autoprops would look like this:

#[derive(Properties)]
struct CompProps {
    #[foreign_trait_attr] arg: Arg,
}

#[function_component]
#[foreign_trait]
fn Comp(CompProps { arg }: &CompProps) -> Html {
    let arg = ImplicitClone::implicit_clone(arg);
    ...
}

First of all, current solution still does not solve the problem. Second, handling #[foreign_trait] has now been delegated to the #[function_component], and it is Yew's problem to handle, rather than yew-autoprops's (if #[function_component] passes fn attributes as-is, good for them).

Solution

The solution should be banning any usages #[foreign_trait_attr] (i.e. showing a user-readable error like could not use function argument attributes as they no longer exist as-is after #[autoprops]), simply for the reason that the fn args it was attached to no longer exist in the generated code (as fn args, which it is supposed to attach to).

#[foreign_trait_attr] is anything except #[prop_or_default], #[prop_or(...)], #[prop_or_else(...)], but also in the future may support more attributes (like #[cfg(...)], which would dictate generation in all 3 places, where you can see ident arg in examples above: CompProps { ... }, let ..., struct CompProps { ... }).

Mind that banning #[foreign_trait_attr] is not banning #[foreign_trait], as it may still just transform the fn body without any damage:

// source code
#[autoprops]
#[styled]
#[function_component]
fn Comp(arg: Arg) -> Html {
    ...
}

// generated after `#[autoprops]`
#[styled]
#[function_component]
fn Comp(CompProps { arg }: &CompProps) -> Html {
    let arg = ImplicitClone::implicit_clone(arg);
    ...
}

// generated after `#[styled]`
#[function_component]
fn Comp(CompProps { arg }: &CompProps) -> Html {
    // possible additional lines generated by `#[styled]` here before `let`s
    // shouldn't matter I don't think, since `#[styled]` sees fn declaration as `&CompProps`
    let arg = ImplicitClone::implicit_clone(arg);
    ... /* body transformed by `#[styled]` here */
}

Alternative

Generate code that retains the fn args somehow:

#[function_component]
fn Comp(CompProps { arg }: &CompProps) -> Html {
    let arg = ImplicitClone::implicit_clone(arg);
    
    #[foreign_trait]
    fn __Comp(#[foreign_trait_attr] arg: Arg) -> Html {
        ... /* this body will need hooks to be handled?! */
    }
    
    __Comp(arg)
}

This solution is worse performance wise and will have the burden of #[function_component]'s hooks upon #[autoprops].

@Madoshakalaka
Copy link

Just reporting a use case I actually bumped into in my code here.

I have #![feature(stmt_expr_attributes, lint_reasons)] turned on, and this is the code:

#[auto_props]
#[function_component]
pub fn Inner(
    graph: &Graph<Node>,
    #[expect(unused_variables, reason="todo: display the name somewhere")]
    name: &String     
) -> Html {
	//...
}

where name is not used in the body. The #[expect] attribute is stripped and rendered useless (I still get warnings about name being unused.

I can raised the attribute to the function level but that defeats the purpose of stmt_expr_attributes and might accidently silence other warnings.

@cecton
Copy link
Member Author

cecton commented Jan 16, 2024

Maybe there is a way to differentiate which goes where @kirillsemyonkin? For example:

#[auto_props]
#[function_component]
pub fn SomeComponent(
    #[function(expect(...))]
    name: &String,
    #[prop(prop_or_default)]
    is_loading: bool,
) -> Html {
	//...
}

Anything in #[function(...)] would be passed to the functions while anything in #[prop(...)] would be passed to the function itself. We could add a few exceptions for yew's standard ones (prop_or, prop_or_default, ...) and builtins (cfg should go everywhere).

I think it's doable, those attributes are parsed after all.

@kirillsemyonkin
Copy link
Collaborator

kirillsemyonkin commented Jan 17, 2024

The #[expect] attribute is stripped and rendered useless

@Madoshakalaka I wonder if there are proc-macros that can be made outside of Rust's std that can be applied to places like this. If not, I guess we could just keep adding things like this to our macro (preferrably before they release, which is your case).

Anything in #[function(...)] would be passed to the functions

@cecton If by "passed to function" you mean to function parameters, that is, again, not possible, since they no longer exist.

With your solution, our current options could be:

  • Apply to generated Properties's struct field, which can be #[prop(...)] or #[field(...)].
  • Apply to generated variables, e.g. #[attr] let prop = prop; // or prop.clone();. This can be #[var(...)] or #[function(...)].
  • Apply to both, with #[both(...)], #[everywhere(...)], #[all(...)], #[prop_and_var(...)], etc.

Also consider making it a bit more clear (and thus less prone to collisions with other proc macros) with #[for_prop(...)], #[for_var(...)], #[for_both(...)], etc, or #[to_prop(...)], etc.

With "both" option being a thing, it might be possible to stuff the cfg into it, as opposed to making it an exception, which could be more welcome I believe, as it is less burden for us to keep adding those exceptions, and it makes it a bit more consistent.

The only issue is that this behavior seems to be quite unintuitive, and if futursolo were to be more involved in this project, I'm sure they would slap us on our wrists for such solutions.

@cecton
Copy link
Member Author

cecton commented Jan 17, 2024

It's actually more complicated for the expect because if the lint does not trigger, it yields a warning "this lint expectation is unfulfilled" and I'm pretty sure that unused fields in struct are dead_code while unused variables are unused_variables.

You would need to write something like:

#[for_var(expect(unused_variables, reason="todo"))]
#[for_prop(expect(dead_code, reason="todo"))]
name: &AttrValue,

(Also you should use AttrValue and not String to avoid unnecessary allocations @Madoshakalaka)

In the meantime it might just be easier to drop the value when entering the function. That will clear the warning:

#[auto_props]
#[function_component]
pub fn SomeComponent(
    name: &AttrValue,
    #[prop(prop_or_default)]
    is_loading: bool,
) -> Html {
    // TODO: display the name somewhere
    drop(name);
	//...
}

@kirillsemyonkin
Copy link
Collaborator

kirillsemyonkin commented Jan 17, 2024

    drop(name);

I would use let _ = name; or _ = name;

The only place where I would use drop is when I want to explicitly convert to () in a succint way:

match x {
    A => do_something(),
    B => drop(get_and_do_something()),
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants