Extend error message

[captain-yossarian]

Related to #664

[captain-yossarian]

@jstarry Error is trigered only if named props are after [with props].
Example

html! {
  <MyComponent with props value=1 />
}

I did not find any information in docs about using with props syntax.
So, I have a few questions:

1. When this error is triggered?
2. How to debug my changes ? If I use println!, I'm getting error.
3. In this case:

html! {
  <MyComponent value=1 with props  />
}

I'm receiving unexpected token. It is looks like this error going from syn::parse_macro_input .
How can I handle it? Do you have any suggestions?

Thank you

[jstarry]

1. Not possible to get that error

2. Debugging is tough, I'm not sure of a great way unfortunately. You could try unit testing the macro code

3. Check out this line: https://github.com/captain-yossarian/yew-1/blob/3a196c3c7d6509ec74c5900b55da789727543361/crates/macro/src/html_tree/html_component.rs#L396

[captain-yossarian]

@jstarry It is looks like that error handling from L 401 to to L410 does not work at all
Try:

html! {
  <MyComponent type  />
}

You will see unexpected token instead of "expected identifier"

[jstarry]

It should work. Your example doesnt work because you need an equals sign, see here https://github.com/captain-yossarian/yew-1/blob/3a196c3c7d6509ec74c5900b55da789727543361/crates/macro/src/html_tree/html_prop.rs#L16. That said, we should improve the error for your example as well!

[captain-yossarian]

@jstarry Ready for review

As for error handling for

html! {
  <MyComponent type  />
}

/// OR

html! {
  <MyComponent type=  />
}

I wold like to make it in separate PR, because for me, this condition

if prop.label.to_string() == "type" {
     return Err(syn::Error::new_spanned(&prop.label, "expected identifier"));
}

does not work at all.

[jstarry]

@captain-yossarian great! Unfortunately, the macro tests were not running on CI for your PR. I just merged #965 to fix that. Could you please rebase this PR and fix the tests? It just involves:

1. removing the old stderr file
2. running the tests
3. and moving the new stderr from the wip directory

[captain-yossarian]

@jstarry Done

What do you think about updating contribution guide?

For example:

When you add the tests, please make sure you have updated stderr appropriate file, which you can find in ./tests/macro.
To generate new stderr file you should run ./ci/run_tests.sh and follow instructions.
You can be asked to set env variable TRYBUILD=overwrite to automatically overwrite old stder file.
Once you have done with it, your PR can be accepted.

[jstarry]

This could catch errors not related to the with syntax like so:

error: Using special syntax `with props` along with named prop is not allowed
  --> $DIR/html-component-fail.rs:63:26
   |
63 |     html! { <Child int=1 "with" /> };
   |                          ^^^^^^

I'd prefer this approach:

if let Some(ident) = input.cursor().ident() {
  if ident.0 == "with" {
    // ... 
  }
}

[jstarry]

@captain-yossarian the contributing guide changes sound great to me. I created an issue here: #967

[captain-yossarian]

@jstarry please take a look on my changes

[jstarry]

I just realized that this error is misleading... It's not clear from this error message that the following is valid:

html! { <Child with props ref=() /> } 

This is because ref=.. is a special prop and can be used together with with props as long as it comes after. Note, I'm open to html! { <Child ref=() with props /> } being valid as well.

[jstarry]

Sorry, I got confused which test case that error was for.

In any case, I think that for

html! { <Child with props ref=() ref=() /> };

we should use this error: "too many refs set"

[captain-yossarian]

@jstarry What do you think about refactor of this function ?
It looks like I have to apply same conditions for both WithProps and ListProps logic.

So, I created a HashMap with all edge cases.
For example: if you want apply only ref edge case, you can pass a slice with one element "ref" as a second argument to apply_edge_cases. This let you use same logic for both ListProps and WithProps.

If in the future, you would like to add more adge cases, you can only insert new [key, valye] to the hash map.
I just don't want to copy/paste logic from one function to another.

Also, here you can concatenate all errors in one string and output it to the user.

If you think it is not Ok, you can check this 23bdf72 commit.
Here I just copied logic from one function to another, it covers all edge cases and it is not DRY

P.S. The code you see is a draft, it is not my final PR

[jstarry]

@captain-yossarian nice! Here are my thoughts:

It looks like I have to apply same conditions for both WithProps and ListProps logic.

Good point, how about we pull that common logic out of WithProps and ListProps and handle it Props instead? I'm imagining something like:

1. Parse list of props
2. Parse with props
3. Parse list of props
4. Check for double refs from 1. and 3.

So, I created a HashMap with all edge cases

I'm not convinced this is really helping much, I think the for loop and if statements that we had before were clearer. We don't need to generalize this too much.

I just don't want to copy/paste logic from one function to another.

Yeah, generally it's good to avoid copy/paste logic. I think in this case, we can avoid it by handling common paths at the Props level

you can concatenate all errors in one string and output it to the user.

I think one error at a time is fine. Also, you can collect a list of results into one: https://doc.rust-lang.org/std/result/enum.Result.html#impl-FromIterator%3CResult%3CA%2C%20E%3E%3E

[jstarry]

With this approach, I think we can allow ref to come before with props

[captain-yossarian]

@jstarry I have a problem with props type.
return_type function can catch all kind of errors. It works perfectly.
But it works only with let _ = Props::collect_props(input)?; 433 line
If I remove this line, app throw an error in this place: return Err(input.error("MY")); 464 line.

Do you have any thoughts why does it is happening?

[jstarry]

@captain-yossarian yeah the issue is that you are consuming the ParseStream in get_type and so there are no tokens left to parse. Look at how Peek implementations work. They use a cursor which can read without consuming any tokens from the ParseStream.

[captain-yossarian]

@jstarry not quite. Here I making a fork. If you log input.cursor().token_stream().to_string() at the end of get_type you will get all initial stream

[jstarry]

This test doesn't look right

[captain-yossarian]

@jstarry It is looks like currently on master if you use this approach, you will receive an error

[jstarry]

Yes, currently on master this will cause an error. My concern is that the test error message is confusing for this test case:

"error: Using special syntax `with props` along with named prop is not allowed. This rule does not apply to special `ref` prop"

[captain-yossarian]

@jstarry Thank you for the help! :)

[captain-yossarian]

@jstarry Unfortunately, after resolving all merge conflicts, yew build failing. Maybe due to web-sys . I dont know what I can else do

 --> /home/travis/build/yewstack/yew/src/virtual_dom/vtext.rs:113:9
    |
60  |       ) -> Option<Node> {
    |            ------------ expected `std::option::Option<web_sys::features::gen_Node::Node>` because of return type
...
113 | /         self.reference.as_ref().map(|t| {
114 | |             let node = cfg_match! {
115 | |                 feature = "std_web" => t.as_node(),
116 | |                 feature = "web_sys" => t.deref(),
117 | |             };
118 | |             node.to_owned()
119 | |         })
    | |__________^ expected struct `web_sys::features::gen_Node::Node`, found struct `web_sys::features::gen_CharacterData::CharacterData`
    |
    = note: expected type `std::option::Option<web_sys::features::gen_Node::Node>`
               found type `std::option::Option<web_sys::features::gen_CharacterData::CharacterData>`

[jstarry]

@captain-yossarian can you please rebase on (or merge) master and fix the cargo fmt errors?