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

Incorrect error output for mistakes in HTML fragments #1444

Closed
1 of 3 tasks
ilyvion opened this issue Jul 24, 2020 · 3 comments · Fixed by #1445
Closed
1 of 3 tasks

Incorrect error output for mistakes in HTML fragments #1444

ilyvion opened this issue Jul 24, 2020 · 3 comments · Fixed by #1445
Labels

Comments

@ilyvion
Copy link
Contributor

ilyvion commented Jul 24, 2020

Problem
The macro parser doesn't correctly handle errors in HTML fragments.

Steps To Reproduce
Steps to reproduce the behavior:

  1. Put the following into a html! invocation:
        html! {
            <key="key" key="key">
                <div>{ "Hello World!" }</div>
            </>
        };
  2. Attempt to compile the code
  3. See that the error you get is "error: unexpected token"

Expected behavior
If you look at the code in yew-macro\src\html_tree\html_list.rs,

impl Parse for ParseKey {
fn parse(input: ParseStream) -> ParseResult<Self> {
let key = input.parse::<HtmlProp>()?;
if !input.is_empty() {
input.error("Only a single key element is allowed on a <></>");
}
Ok(ParseKey { key })
}
}

the intended error message here is clearly meant to be

Only a single key element is allowed on a <></>

Environment:

  • Yew version: master
  • Rust version: 1.44

Questionnaire

  • I'm interested in fixing this myself but don't know where to start
  • I would like to fix and I have a solution
  • I don't have time to fix this right now, but maybe later
@ilyvion ilyvion added the bug label Jul 24, 2020
@ilyvion
Copy link
Contributor Author

ilyvion commented Jul 24, 2020

My guess is that this happens because something in here says "I'll handle this one" from their peek() calls, but then shouldn't actually have done so.

impl Parse for HtmlTree {
fn parse(input: ParseStream) -> Result<Self> {
let html_type = HtmlTree::peek(input.cursor())
.ok_or_else(|| input.error("expected a valid html element"))?;
let html_tree = match html_type {
HtmlType::Empty => HtmlTree::Empty,
HtmlType::Component => HtmlTree::Component(Box::new(input.parse()?)),
HtmlType::Tag => HtmlTree::Tag(Box::new(input.parse()?)),
HtmlType::Block => HtmlTree::Block(Box::new(input.parse()?)),
HtmlType::List => HtmlTree::List(Box::new(input.parse()?)),
};
Ok(html_tree)
}
}
impl PeekValue<HtmlType> for HtmlTree {
fn peek(cursor: Cursor) -> Option<HtmlType> {
if cursor.eof() {
Some(HtmlType::Empty)
} else if HtmlComponent::peek(cursor).is_some() {
Some(HtmlType::Component)
} else if HtmlTag::peek(cursor).is_some() {
Some(HtmlType::Tag)
} else if HtmlBlock::peek(cursor).is_some() {
Some(HtmlType::Block)
} else if HtmlList::peek(cursor).is_some() {
Some(HtmlType::List)
} else {
None
}
}
}

@ilyvion
Copy link
Contributor Author

ilyvion commented Jul 24, 2020

Also, while it's only bad for compilation times, due to all the peeking, this code effectively parses everything twice, doesn't it? Plus it may also partially parse things that aren't even the thing in need of parsing.

@siku2
Copy link
Member

siku2 commented Jul 24, 2020

You're right, unfortunately Yew performs a lot of duplicate parsing. This is something I've thought about a lot and I think our best bet is to get rid of the Peek traits entirely. It should be possible implement the parsing using only a few lookaheads to distinguish between multiple variants.
This is a huge endeavour though and deserves its own issue.

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

Successfully merging a pull request may close this issue.

2 participants