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

Extend error message #960

Merged
merged 40 commits into from
Mar 5, 2020
Merged
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
a0b2e96
Initial
captain-yossarian Feb 22, 2020
ad35203
formatting
captain-yossarian Feb 23, 2020
d41d466
wip
captain-yossarian Feb 23, 2020
5ca1685
add util fn
captain-yossarian Feb 23, 2020
0538813
message update
captain-yossarian Feb 23, 2020
166e4b4
refactor
captain-yossarian Feb 23, 2020
1516923
update error logic
captain-yossarian Feb 24, 2020
1cefeae
message refactor
captain-yossarian Feb 24, 2020
a056c89
remove debug flag
captain-yossarian Feb 24, 2020
e0b0684
update
captain-yossarian Feb 24, 2020
bc3b08c
add stderr
captain-yossarian Feb 25, 2020
69f0e0a
update condition
captain-yossarian Feb 26, 2020
8ea567c
CR fix
captain-yossarian Feb 27, 2020
81edf1b
remove empty line
captain-yossarian Feb 27, 2020
ae76a38
update
captain-yossarian Feb 27, 2020
3546d81
update error message
captain-yossarian Feb 27, 2020
b0cef02
wip
captain-yossarian Feb 27, 2020
9c69526
work
captain-yossarian Feb 27, 2020
145393a
unchange clippy
captain-yossarian Feb 27, 2020
6f307f9
update
captain-yossarian Feb 27, 2020
f7681e5
remove logs
captain-yossarian Feb 27, 2020
990437e
println
captain-yossarian Feb 27, 2020
f421174
format
captain-yossarian Feb 27, 2020
23bdf72
clippy
captain-yossarian Feb 27, 2020
62bb054
VERY WIP
captain-yossarian Feb 27, 2020
8fdbc69
error collect
captain-yossarian Feb 28, 2020
bf42a1a
refactor
captain-yossarian Feb 28, 2020
b9b2a6d
add second argument
captain-yossarian Feb 28, 2020
ee82df4
format
captain-yossarian Feb 28, 2020
6ee8610
wip
captain-yossarian Feb 29, 2020
0ff9648
wip with main parsser
captain-yossarian Feb 29, 2020
53bd8ce
wip
captain-yossarian Mar 1, 2020
a619b5b
receive unexpected error
captain-yossarian Mar 1, 2020
8a4f067
Fix with props checks
jstarry Mar 3, 2020
b55d572
merge
captain-yossarian Mar 4, 2020
748c62d
remove comments
captain-yossarian Mar 4, 2020
b8c8a38
update
captain-yossarian Mar 4, 2020
5b64ce8
format
captain-yossarian Mar 5, 2020
114cb16
Merge remote-tracking branch 'origin' into issue/664
captain-yossarian Mar 5, 2020
7fd2382
Fix test
jstarry Mar 5, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 94 additions & 13 deletions crates/macro/src/html_tree/html_component.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use boolinator::Boolinator;
use proc_macro2::Span;
use quote::{quote, quote_spanned, ToTokens};
use std::cmp::Ordering;
use std::collections::HashMap;
use syn::buffer::Cursor;
use syn::parse;
use syn::parse::{Parse, ParseStream, Result as ParseResult};
Expand Down Expand Up @@ -359,6 +360,9 @@ impl Props {
Props::None => None,
}
}
fn collision_message() -> &'static str {
"Using special syntax `with props` along with named prop is not allowed. This rule does not apply to special `ref` prop"
}
}

impl PeekValue<PropType> for Props {
Expand Down Expand Up @@ -388,28 +392,94 @@ struct ListProps {
props: Vec<HtmlProp>,
node_ref: Option<Expr>,
}

impl Parse for ListProps {
fn parse(input: ParseStream) -> ParseResult<Self> {
impl ListProps {
fn collect_props(input: ParseStream) -> ParseResult<Vec<HtmlProp>> {
let mut props: Vec<HtmlProp> = Vec::new();
while HtmlProp::peek(input.cursor()).is_some() {
props.push(input.parse::<HtmlProp>()?);
}
Ok(props)
}

fn remove_refs(mut props: Vec<HtmlProp>) -> ListProps {
let ref_position = props.iter().position(|p| p.label.to_string() == "ref");
let node_ref = ref_position.map(|i| props.remove(i).value);
for prop in &props {
ListProps { props, node_ref }
}

fn apply_edge_cases(props: &Vec<HtmlProp>) -> Result<(), syn::Error> {
Copy link
Contributor Author

@captain-yossarian captain-yossarian Feb 28, 2020

Choose a reason for hiding this comment

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

@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

Copy link
Member

Choose a reason for hiding this comment

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

@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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@captain-yossarian captain-yossarian Mar 1, 2020

Choose a reason for hiding this comment

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

@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?

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

let mut map: HashMap<&str, Box<dyn Fn(&HtmlProp) -> Result<_, syn::Error>>> =
HashMap::new();

let ref_handler = |prop: &HtmlProp| -> Result<_, syn::Error> {
if prop.label.to_string() == "ref" {
return Err(syn::Error::new_spanned(&prop.label, "too many refs set"));
Err(syn::Error::new_spanned(&prop.label, "too many refs set"))
} else {
Ok(())
}
};

let type_handler = |prop: &HtmlProp| -> Result<_, syn::Error> {
if prop.label.to_string() == "type" {
return Err(syn::Error::new_spanned(&prop.label, "expected identifier"));
Err(syn::Error::new_spanned(&prop.label, "expected identifier"))
} else {
Ok(())
}
};

let unexpected_handler = |prop: &HtmlProp| -> Result<_, syn::Error> {
if !prop.label.extended.is_empty() {
return Err(syn::Error::new_spanned(&prop.label, "expected identifier"));
Err(syn::Error::new_spanned(&prop.label, "expected identifier"))
} else {
Ok(())
}
};

map.insert("ref", Box::new(ref_handler));
map.insert("type", Box::new(type_handler));
map.insert("unexpected", Box::new(unexpected_handler));

let errors = props.iter().fold(vec![], |acc, prop: &HtmlProp| {
[
acc,
["ref", "type", "unexpected"]
.iter()
.map(|elem| match map.get(elem) {
Some(handler) => handler(prop),
None => Err(syn::Error::new_spanned(&prop.label, "something went wrong")),
})
.filter(Result::is_err)
.collect::<Vec<Result<_, syn::Error>>>(),
]
.concat()
});


for error in errors {
return error;
}

Ok(())
}
}

impl Parse for ListProps {
fn parse(input: ParseStream) -> ParseResult<Self> {
let props = ListProps::collect_props(input)?;

if let Some(ident) = input.cursor().ident() {
if ident.0 == "with" {
return Err(input.error(Props::collision_message()));
}
}

let ListProps {
mut props,
node_ref,
} = ListProps::remove_refs(props);

ListProps::apply_edge_cases(&props)?;

// alphabetize
props.sort_by(|a, b| {
if a.label == b.label {
Expand Down Expand Up @@ -442,16 +512,27 @@ impl Parse for WithProps {
return Err(input.error("expected to find `with` token"));
}
let props = input.parse::<Ident>()?;

let _ = input.parse::<Token![,]>();

// Check for the ref tag after `with`
let mut node_ref = None;
if let Some(ident) = input.cursor().ident() {
let prop = input.parse::<HtmlProp>()?;
if ident.0 == "ref" {
node_ref = Some(prop.value);
} else {
return Err(syn::Error::new_spanned(&prop.label, "unexpected token"));
if input.cursor().ident().is_some() {
let ListProps {
props: list_props,
node_ref: reference,
} = ListProps::remove_refs(ListProps::collect_props(input)?);
node_ref = reference;

for prop in &list_props {
if prop.label.to_string() == "ref" {
return Err(syn::Error::new_spanned(&prop.label, "too many refs set"));
} else {
return Err(syn::Error::new_spanned(
&prop.label,
Props::collision_message(),
));
}
}
}

Expand Down
8 changes: 8 additions & 0 deletions tests/macro/html-component-fail.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,17 @@ fn compile_fail() {
html! { <Child props /> };
html! { <Child with props > };
html! { <Child with props ref=() ref=() /> };
html! { <Child with props ref=() ref=() value=1 /> };
html! { <Child with props ref=() value=1 ref=() /> };
html! { <Child with props value=1 ref=() ref=() /> };
html! { <Child value=1 with props ref=() ref=() /> };
html! { <Child value=1 ref=() with props ref=() /> };
html! { <Child ref=() ref=() value=1 with props /> };
html! { <Child ref=() with props /> };
html! { <Child with blah /> };
html! { <Child with props () /> };
html! { <Child value=1 with props /> };
html! { <Child with props value=1 /> };
html! { <Child type=0 /> };
html! { <Child invalid-prop-name=0 /> };
html! { <Child unknown="unknown" /> };
Expand Down
Loading