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

Rsx parser with recovery after errors, and unquoted text #1054

Merged
merged 4 commits into from
May 21, 2023

Conversation

vldm
Copy link
Contributor

@vldm vldm commented May 17, 2023

Hi.
Original author of syn-rsx is absent for a while.
I tried to reach him through the e-mail but have no luck with that.

I have created a fork of https://github.com/rs-tml/rstml
With aim to better IDE support.
Fork contain few refactors which breaks the API, but during refactor all types became to be self-describing (every type contain all tokens that was parsed), which, i hope, will reduce future breaking changes.

In this PR i move from syn-rsx to rstml fork.
Which also bump minimum syn version to 2.0 (it was already in dependencies before)

In summary, this pr introduce or changes :

  1. Add support of unquoted text support.
  2. Introduce recovery of parser after invalid closing tags, unclosed tags or invalid expression ( can be checked by semantic syntax highlight)
  3. Simplified Node::Text ( no expressions, or other literals except string)
  4. Attribute refactoring - not a node anymore, seperated Block and key=value attributes.
  5. As part of 2. NodeBlock::Invalid was introduced, which represent block with invalid syn::Block syntax.

To discuss:

  • Component macro refactor.
    Currently if inside fn body any invalid expression was found, then macro will fail to expand.
    To fix this, we can omit parsing of fn body.

  • #[allow(unused_braces)], vec![] and other macro usage inside expansion.
    I have removed this macro usage, because with them rust-analyzer failed to give completion for invalid expression inside braces <div> {v.<COMPLETION>} </div>. Probably i can return it for NodeBlock::Valid, and keep it removed only for InvalidBlock.

@@ -387,9 +387,13 @@ fn child_to_tokens(
))
}
}
//TODO Should we ewerywhere use syn::expr?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you say a little more about what you mean here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am just wasn't sure what block_to_tokens is doing. And what it expect for different syn::Expr.

There also was str_value match that looks like value_to_string to string body.

So my thought about block_to_tokens was:

  1. should it reuse value_to_string?
  2. if it tries to handle primitive literals differently maybe this handling should be done on higher lever (split this function into two (text_to_tokens/dyn_block_to_tokens), and use them in place where needed?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay. No, I think it was just legacy code. Yes str_value is same as value_to_string.

Copy link
Collaborator

@gbj gbj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very exciting! I have tested some examples locally and the unquoted text appears to work well on both stable and nightly, although of course for edge cases whitespace-trimming means that quoted strings are sometimes useful. I really appreciate the thought you've put into this.

The CI issues are

  1. cargo fmt
  2. some unrelated issues I've fixed on main that should be fixed if you rebase

If you want to respond to my comments on some of the todos, rebase, and run cargo fmt, I'll be very happy to merge this.

Thanks for all your work on it.

This would close #833. I think rstml also provides an opportunity to do #766.

leptos_macro/src/view.rs Outdated Show resolved Hide resolved
leptos_macro/src/view.rs Outdated Show resolved Hide resolved
leptos_macro/src/view.rs Outdated Show resolved Hide resolved
leptos_macro/src/view.rs Outdated Show resolved Hide resolved
vldm added 4 commits May 19, 2023 22:34
1. Add quotation to RawText node.
2. Replace vec! macro with [].to_vec().
Cons:
1. Temporary remove allow(unused_braces) from expressions, to allow completion after dot in rust-analyzer.
@vldm
Copy link
Contributor Author

vldm commented May 19, 2023

I have fixed fmt, regenerate test stderr for macro ui.

Also for value_to_string, added conversion for primitive Node::Block - now it should explicitly convert
{"string literal"} to "string literal" and use it view::SsrElementChunks::String or in block_to_tokens.

I'm not sure if this pr can move forward on #766 - because the syntax that was declared in the first post can significantly complicate attribute parsing logic. I also left comment in #766 , with imaginary syntax that could be a bit easier to implement (at least on parsing level), but also solve same problem.

I also could mention, that my fork is also closes stoically/syn-rsx#51
But i am not sure how you bind close_tag with name (you should probably emit some dummy tokens?), so if you can share artifacts of your previous work on it, i can update it.

@gbj
Copy link
Collaborator

gbj commented May 19, 2023

Agreed, #766 should be entirely separate from this PR. I just meant that having you maintaining rstml more actively than syn-rsx is maintained would mean we might have some more syntactic flexibility! (I see your issue list for example includes a few things Leptos is shipping workarounds for right now)

Re: stoically/syn-rsx#51, I essentially added this to leptos_dom/src/html.rs after HtmlElement::with_view_marker

#[cfg(debug_assertions)]
    /// Marks the closing of this HTML element.
    #[inline(always)]
    pub fn closing_tag(self) -> Self {
        self
    }

and then added a call to closing_tag(), spanned to the closing tag, in element_to_tokens, for elements that had closing tags. This caused the syntax highlighting for the open and closing tags to match, but didn't do anything more interesting like try to link them to one another somehow. I'm not sure if there's a way to indicate to rust-analyzer that two different spans refer to the same thing. Conceptually, for elements and components both the opening and closing tags are linked to the name variable in element_to_tokens, but at present only the opening tag name token is connected to the element/component function invocation from rust-analyzer's perspective.

@vldm
Copy link
Contributor Author

vldm commented May 19, 2023

I'm not sure if there's a way to indicate to rust-analyzer that two different spans

I didn't find a way either.

and then added a call to closing_tag(), spanned to the closing tag, in element_to_tokens, for elements that had closing tags

Theoretically macro also can emit dummy token before dom builder, example that i use in html-to-string-macro:

enum x {}; // for each html, head, body (open and close tag)
let _ = crate::docs::element; // for rest tags

I done this as an example, how one can colorize its output, and add docs.

I think some hacky way to keep close tag colorized and linked to the valid documentation would be emiting tokens inside if fasle, so rust will not execute it, but it still be possible to bould span to valid component Type.

{
if false {
  let _ = leptos::leptos_dom::html::dom; // for close dom tag
  let _ = Option::<Component>::Some; // for close component
}

leptos::leptos_dom::html::section().child(..)

}

@gbj
Copy link
Collaborator

gbj commented May 20, 2023

I think some hacky way to keep close tag colorized and linked to the valid documentation would be emiting tokens inside if fasle, so rust will not execute it, but it still be possible to bould span to valid component Type.

{
if false {
  let _ = leptos::leptos_dom::html::dom; // for close dom tag
  let _ = Option::<Component>::Some; // for close component
}

leptos::leptos_dom::html::section().child(..)

}

Oh I think this is very clever.

I think I'll merge this, for now, and then add the closing tag piece in a separate PR. Does that sound good to you? I think this is complete otherwise, right?

@vldm
Copy link
Contributor Author

vldm commented May 20, 2023

I think some hacky way to keep close tag colorized and linked to the valid documentation would be emiting tokens inside if fasle, so rust will not execute it, but it still be possible to bould span to valid component Type.

{

if false {

let _ = leptos::leptos_dom::html::dom; // for close dom tag

let _ = Option::::Some; // for close component

}

leptos::leptos_dom::html::section().child(..)

}

Oh I think this is very clever.

I think I'll merge this, for now, and then add the closing tag piece in a separate PR. Does that sound good to you? I think this is complete otherwise, right?

Yes, thanks. I will open separate PR.
This pr was simple refactoring, but for closing tags I want to understand more about view and template macro.

@gbj gbj merged commit 5a71ca7 into leptos-rs:main May 21, 2023
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

Successfully merging this pull request may close these issues.

2 participants