html!: fix mixed self-closing and non-sc tags #523

totorigolo · 2019-07-09T10:58:26Z

Before, self-closing tags where considered as open tags in verify_end, causing codes like this to not compile:

html! {
    <div>
        <div/> // <- considered as open tag
    </div>
}

error: this open tag has no corresponding close tag
   --> src/lib.rs:264:17
    |
... | <div>
    | ^^^^^

However, this fix isn't ideal because it peeks the buffer twice for non self-closing tags. I did it that way in order to keep the peek thing. An alternative would be to turn HtmlSelfClosingTag::peek into a standard function returning (ident, is_self_closing).

Fixes: #522

totorigolo · 2019-07-09T15:39:58Z

I checked if the problem is actually allowed by the HTML specification, and it happens that it's not: https://stackoverflow.com/a/3558200/2209243

jstarry · 2019-07-21T19:51:32Z

Hey @totorigolo I think this is actually worth supporting. JSX supports self-closing tags so I think developers will be accustomed to this short-hand even if it's not valid HTML. See here: https://reactjs.org/docs/jsx-in-depth.html

crates/macro-impl/src/html_tree/html_tag/mod.rs

jstarry

This looks awesome! I think your approach looks good, could you add one more test and fix the merge conflicts? Thanks!

jstarry · 2019-07-24T15:23:53Z

crates/shared/tests/vtag_test.rs

+
+    let a: VNode<Comp> = html! {
+        <div>
+            <div data-val={ 2 / 1 }/>


Can you also add a test for an expression with a >? Thanks!

This works:

let a: VNode<Comp> = html! { <div> <div data-val={ 2 > 1 }/> </div> }; let b: VNode<Comp> = html! { <div> <div data-val={ true }></div> </div> }; assert_eq!(a, b);

But this doesn't:

let a: VNode<CompInt> = html! { <div> <div onblur=|_| 2 > 1/> </div> }; let b: VNode<CompInt> = html! { <div> <div onblur=|_| false></div> </div> }; assert_eq!(a, b); // NB: assert_eq! doesn't (cannot) compare the closures

It wasn't working before this PR, is now breaking with a less nice message. This should however happen very rarely, and the easy workaround is to add brackets around 2 > 1. React's JSX doesn't have this problem AFAIK because it enforces putting brackets around expressions. I think that I'll take the pragmatic approach here and try to add a meaningful error message.

The aforementioned failing test on master:

diff --git a/tests/vtag_test.rs b/tests/vtag_test.rs index 5ca5d6d..312888c 100644 --- a/tests/vtag_test.rs +++ b/tests/vtag_test.rs @@ -28,6 +28,27 @@ impl Renderable<Comp> for Comp { } } +struct CompBool; + +impl Component for CompBool { + type Message = bool; + type Properties = (); + + fn create(_: Self::Properties, _: ComponentLink<Self>) -> Self { + CompBool + } + + fn update(&mut self, _: Self::Message) -> ShouldRender { + unimplemented!(); + } +} + +impl Renderable<CompBool> for CompBool { + fn view(&self) -> Html<Self> { + unimplemented!(); + } +} + #[test] fn it_compares_tags() { let a: VNode<Comp> = html! { @@ -258,3 +279,11 @@ fn it_allows_aria_attributes() { panic!("vtag expected"); } } + +#[test] +fn this_fails() { + let a: VNode<CompBool> = html! { <div> <a onblur=|_| 2 > 1 /> </div> }; + let b: VNode<CompBool> = html! { <div> <a onblur=|_| false></div> </div> }; + assert_eq!(a, b); +} +

error: this open tag has no corresponding close tag --> tests/vtag_test.rs:285:44 | 285 | let a: VNode<CompBool> = html! { <div> <a onblur=|_| 2 > 1 /> </div> }; | ^^^^^^^^^^^^^^^^^ error: this open tag has no corresponding close tag --> tests/vtag_test.rs:286:44 | 286 | let b: VNode<CompBool> = html! { <div> <a onblur=|_| false></div> </div> }; | ^^^^^^^^^^^^^^^^^^^^ error: aborting due to 2 previous errors error: Could not compile `yew`.

I had a feeling that was broken in the current macro implementation ;) thanks for adding tests and a better error message!

I see three choices in order to provide better error messages:

wait for proc-macro diagnostics, which are currently nightly-only, and just use eprintln! in the meantime (this error should occur quite rarely).

mess around the actual code in order to return a result from verify_end.

use global variables.

I implemented the first one for now. What do you think?

totorigolo · 2019-07-25T13:16:20Z

@jstarry How do you run failing tests like tests/macro/html-tag-fail.rs?

jstarry · 2019-07-25T13:33:07Z

@totorigolo see here: https://github.com/DenisKolodin/yew/blob/master/ci/run_tests.sh#L30

jstarry

This is great, just a few small changes needed

jstarry · 2019-07-26T12:09:02Z

crates/macro/src/html_tree/html_tag/mod.rs

        loop {
            if let Some(next_close_ident) = HtmlTagClose::peek(input.cursor()) {
-                if open.ident.to_string() == next_close_ident.to_string() {
+                if open_ident_str == next_close_ident.to_string() {


I recently learned that Ident implements PartialEq so open.ident == next_close_ident should be fine

jstarry · 2019-07-26T12:12:43Z

crates/macro/src/html_tree/html_tag/mod.rs

+                        // only '<' or '{ ... }' can follow. (that should be
+                        // enough for reasonable cases)
+                        //
+                        let next_punct = next_cursor.punct().is_some();


This should also check if punct.as_char() == '<'

crates/macro/src/html_tree/html_tag/mod.rs

#### Problem Self-closing tags are more convenient when there are no children. For instance, React's JSX allows it. Before, self-closing tags where considered as open tags in `verify_end`, causing codes like this to not compile: ```rust html! { <div> <div/> // <- considered as open tag </div> } ``` ``` error: this open tag has no corresponding close tag --> src/lib.rs:264:17 | ... | <div> | ^^^^^ ``` #### Solution Add a new `Peek`-able `HtmlSelfClosingTag`, used in `verify_end`. However, this fix isn't ideal because it peeks the buffer twice for non self-closing tags. I did it that way in order to keep the peek thing. An alternative would be to turn HtmlSelfClosingTag::peek into a function returning (ident, is_self_closing). #### Notes I added a TODO for when proc-macro::Diagnostic gets stabilized, in order to replace the clunky `eprintln!` with proper diagnostics. I didn't try to return a Result to get a nice error right now because this error should be fairly rare so we can just wait IMO. Fixes: yewstack#522

totorigolo · 2019-07-26T13:11:17Z

Fixed 🙂

jstarry · 2019-07-26T14:01:10Z

bors r+

523: html!: fix mixed self-closing and non-sc tags r=jstarry a=totorigolo Before, self-closing tags where considered as open tags in `verify_end`, causing codes like this to not compile: ```rust html! { <div> <div/> // <- considered as open tag </div> } ``` ``` error: this open tag has no corresponding close tag --> src/lib.rs:264:17 | ... | <div> | ^^^^^ ``` However, this fix isn't ideal because it peeks the buffer twice for non self-closing tags. I did it that way in order to keep the peek thing. An alternative would be to turn `HtmlSelfClosingTag::peek` into a standard function returning `(ident, is_self_closing)`. Fixes: #522 Co-authored-by: Thomas Lacroix <toto.rigolo@free.fr>

jstarry · 2019-07-26T14:01:42Z

Thanks for fixing this!

bors · 2019-07-26T14:31:41Z

Build succeeded

continuous-integration/travis-ci/push

totorigolo closed this Jul 9, 2019

jstarry reopened this Jul 21, 2019

jstarry requested changes Jul 21, 2019

View reviewed changes

crates/macro-impl/src/html_tree/html_tag/mod.rs Outdated Show resolved Hide resolved

totorigolo force-pushed the fix-mixed-tags branch 2 times, most recently from 59e90a9 to 57ffe3e Compare July 23, 2019 16:51

jstarry reviewed Jul 24, 2019

View reviewed changes

totorigolo force-pushed the fix-mixed-tags branch 2 times, most recently from 892856d to dd54b18 Compare July 25, 2019 15:40

jstarry requested changes Jul 26, 2019

View reviewed changes

totorigolo force-pushed the fix-mixed-tags branch from dd54b18 to 5908d33 Compare July 26, 2019 13:09

jstarry approved these changes Jul 26, 2019

View reviewed changes

bors bot merged commit 5908d33 into yewstack:master Jul 26, 2019

jstarry mentioned this pull request Aug 3, 2019

Tracking issue for yew 0.8.0 #553

Closed

totorigolo deleted the fix-mixed-tags branch August 4, 2019 18:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

html!: fix mixed self-closing and non-sc tags #523

html!: fix mixed self-closing and non-sc tags #523

totorigolo commented Jul 9, 2019

totorigolo commented Jul 9, 2019

jstarry commented Jul 21, 2019

jstarry left a comment

jstarry Jul 24, 2019

totorigolo Jul 24, 2019

totorigolo Jul 24, 2019

jstarry Jul 25, 2019

totorigolo Jul 25, 2019

totorigolo commented Jul 25, 2019

jstarry commented Jul 25, 2019

jstarry left a comment

jstarry Jul 26, 2019

jstarry Jul 26, 2019

totorigolo commented Jul 26, 2019

jstarry commented Jul 26, 2019

jstarry commented Jul 26, 2019

bors bot commented Jul 26, 2019

html!: fix mixed self-closing and non-sc tags #523

html!: fix mixed self-closing and non-sc tags #523

Conversation

totorigolo commented Jul 9, 2019

totorigolo commented Jul 9, 2019

jstarry commented Jul 21, 2019

jstarry left a comment

Choose a reason for hiding this comment

jstarry Jul 24, 2019

Choose a reason for hiding this comment

totorigolo Jul 24, 2019

Choose a reason for hiding this comment

totorigolo Jul 24, 2019

Choose a reason for hiding this comment

jstarry Jul 25, 2019

Choose a reason for hiding this comment

totorigolo Jul 25, 2019

Choose a reason for hiding this comment

totorigolo commented Jul 25, 2019

jstarry commented Jul 25, 2019

jstarry left a comment

Choose a reason for hiding this comment

jstarry Jul 26, 2019

Choose a reason for hiding this comment

jstarry Jul 26, 2019

Choose a reason for hiding this comment

totorigolo commented Jul 26, 2019

jstarry commented Jul 26, 2019

jstarry commented Jul 26, 2019

bors bot commented Jul 26, 2019

Build succeeded