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

Allow arbitrary syntax in KeyedAttributeValue with braces #54

Closed
blorbb opened this issue Jul 10, 2024 · 8 comments
Closed

Allow arbitrary syntax in KeyedAttributeValue with braces #54

blorbb opened this issue Jul 10, 2024 · 8 comments

Comments

@blorbb
Copy link

blorbb commented Jul 10, 2024

Kind of a follow up to #48

There is some interest in requiring braces on attribute values for better rust-analyzer support in the leptos view! macro. See leptos-rs/leptos#2196 (point 1). However, it seems like the issue comes from https://github.com/rs-tml/rstml/blob/main/src/node/attribute.rs#L228, where attribute values are always parsed as a syn::Expr, so this change cannot be made in leptos only.

(note: the better rust-analyzer support would come from simply parsing a brace and the TokenStream stored inside, rather than trying to parse a syn::Expr. This would allow invalid expressions to still expand in the macro, which lets rust-analyzer do its thing on the expanded but invalid syntax.)

This braced value requirement may be too strict for rstml (and is of course a massive breaking change). I propose something more lenient: if the attribute value is in braces, it is parsed as a new variant KeyedAttributeValue::Block(syn::token::Brace, proc_macro2::TokenStream) (or something similar). The idea is to just grab whatever is inside the block as the attribute value, and move on. If a brace is not found, syn::Expr is parsed instead, giving the existing KeyedAttributeValue::Value variant.

This allows users to expand the inner bits as an expression, or implement some custom syntax too like #48 proposed. Leptos could enforce the braced value requirement by rejecting the KeyedAttributeValue::Value variant.

This is still a breaking change, but (probably?) less likely to occur in user code. Expressions like key={my_value}.into() would not be parsed properly, as only the {my_value} would be parsed as the value, and the .into() would try to be parsed as the next attribute key.

The new variant can of course come in many forms, like (syn::token::Brace, proc_macro2::TokenStream) to separate the brace and token stream, (proc_macro2::TokenStream) which discards the span provided by the brace, (proc_macro2::TokenTree) or (proc_macro2::Group) with less type safety about the delimiter being a brace, and probably more (is there a syn type for something that is braced but not necessarily an ExprBlock?).

If you like the idea, I can make a PR for this!

@vldm
Copy link
Collaborator

vldm commented Jul 24, 2024

Sorry, I haven't updated the project for a while.
I'll try to release the unreleased commits over this weekend.

I'm not sure we need to make any breaking changes in order to make r-a happy.

I thing a sufficient requirement would be to allow library users to choose how they want to handle attribute values.

In order to reach it without breaking change we can introduce one additional configuration enum:
AttributeValues::Simple|Expr(default)|Braced

Expr - repeat current parsing
Braced - only parses syn::Expr started with Brace - simple but not beautiful for simple attribute values like "foo" or ""
Simple - is some kind of trade-off (check inspiration from: #46)

@tachibanayui
Copy link

@vldm
Sorry my #49 is kinda stale, I was busy with schoolwork. Anyway, I think this structure should address a few things you commented on in my pr and delegate the custom syntax parsing for downstream users.

Expr - repeat current parsing
Braced - only parses syn::Expr started with Brace - simple but not beautiful for simple attribute values like "foo" or ""

I think we should also consider Braced as Expr if it is a valid Block so that existing usages would still work the same way. However, for invalid Block we introduce a new variant Braced to store the Brace and TokenStream so that consumers of this API can do further parsing if needed.

pub struct KeyedAttributeValue {
    Binding(FnBinding),
    Value(AttributeValue)
    None,
}

pub struct AttributeValue {
    pub token_eq: Token![=],
    pub value: Value,
}

pub enum Value {
    Expr(Expr), // For any valid expr syntax, including blocks 
    Braced(Braced, TokenStream) // For any invalid blocks
}

I'm currently modifying my PR to use the this structure, let me know if you have any suggestions

@blorbb
Copy link
Author

blorbb commented Jul 24, 2024

Thanks for the comments!

Braced - only parses syn::Expr started with Brace - simple but not beautiful for simple attribute values like "foo" or ""

The requirements proposed in Leptos allow literals without braces. Anything other than literals will require braces.

What @tachibanayui proposed with braced expressions still being parsed as an Expr with the Braced fallback only if parsing fails sounds good! Seems like it would be best, as it isn't breaking, allows unbraced literals, it's just a more lenient parsing method. (minor: maybe naming it Fallback would make it more obvious that it's the result of failed parsing, rather than just having braces)

some conversion methods to help handle the difference between expr/fallback would be useful too.

@blorbb
Copy link
Author

blorbb commented Jul 24, 2024

just having the fallback wouldn't help with #8 / #46 though. Seems like another config option that fully implements the brace requirement for exprs (must have braces excl. literals) would help with those.

@tachibanayui
Copy link

IMO #8 and #46 is tricky to implement in general because the method / field / index access suffix syntax is valid on any expr. We can restrict key-value attributes that must be quoted or braced to solve this issue though

@vldm
Copy link
Collaborator

vldm commented Jul 25, 2024

@tachibanayui

pub enum Value {

    Expr(Expr), // For any valid expr syntax, including blocks 

    Braced(Braced, TokenStream) // For any invalid blocks

}

If the main motivation is to keep "invalid token streams for future parsing" maybe Braced should be named as "Invalid" (or InvalidBlock) analogous to NodeBlock::Invalid variant.

But overall it's looks great to me.

#8 and #46 can be addressed separately.

@tachibanayui
Copy link

Yeah InvalidBraced is better, I renamed it my pr

@vldm
Copy link
Collaborator

vldm commented Jul 28, 2024

Fixed in #49
Released in 0.12.0

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