Missing prop error is not user-friendly

[Boscop]

(I'm using Yew 0.13. https://github.com/yewstack/yew/pull/975, https://github.com/yewstack/yew/issues/928) When the prop allow_zoom is missing from these props:

#[derive(Clone, Properties)]
pub struct Props {
	pub weak_link: WeakComponentLink<Model>,
	pub clicked_pos: Callback<i32>,
	pub allow_zoom: bool,
}

I get this error:

error: frontend\src\app.rs:588: no method named `clicked_pos` found for struct `deck_view::PropsBuilder<deck_view::PropsBuilderStep_missing_required_prop_allow_zoom>` in the current scope
error: frontend\src\app.rs:588: method not found in `deck_view::PropsBuilder<deck_view::PropsBuilderStep_missing_required_prop_allow_zoom>`
error: frontend\src\deck_view.rs:20: method `clicked_pos` not found for this

Ideally, the error should not mention other props that ARE passed, such as clicked_pos. It should only mention the missing prop, but not encoded in code, but in clear text, like

error: missing required prop allow_zoom

[jstarry]

Yeah not super ideal, the hope is that devs notice this part: PropsBuilderStep_missing_required_prop_allow_zoom.

If anyone comes up with a better approach, I'd be very happy!

[TheNeikos]

So I've thought about this, and have created a possible alternative to the current system. It would work by creating a macro for each property, that is then used by the html! macro.

A simplified example looks like this:

struct Foo;

struct Testing {
    f: Option<i32>,
    b: Foo,
}

macro_rules! CreateTesting {
    (@match_b b: $expr:expr , $($rest:tt)*) => {{
        $expr
    }};
    (@match_b $first:ident : $expr:expr , $($rest:tt)*) => {
        CreateTesting! { @match_b $($rest)* }
    };
    (@match_b) => {
        compile_error!("Field 'b' is required");
    };
    
    (@match_f f: $expr:expr , $($rest:tt)*) => {
        $expr
    };
    (@match_f $first:ident : $expr:expr , $($rest:tt)*) => {
        CreateTesting! { @match_f $($rest)* }
    };
    (@match_f) => {
        Default::default()
    };
    
    ($($fields:tt)*) => {{
        let f = CreateTesting! { @match_f $($fields)* };
        let b = CreateTesting! { @match_b $($fields)* };
        
        Testing {
            f, b
        }
    }};
}

fn main() {

    let t : Testing = CreateTesting! { f: None, b: Foo, };
}

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=0d463e4120f84d6f051764ea066d7876

This can be a bit odd in non-2018 editions, but in 2018 edition crates, macros can be imported and don't pollute the global name space.

It works by recursing through all the fields, and trying to match the 'correct' one. If it can't find it, it will output either the default (which doesn't have to be Default::default at the end) or it will output a specific compiler_error which will tell the user exactly what was missing. This will also tell them all the missing props from the get-go, so it has nice UX already. If it finds the field it just uses the expression.

If this sounds promising I'd go and implement a proof of concept rewrite @jstarry

---

Edit: I've looked deeper into this, and it's sadly not that easy to add as I hoped it would be. Since macros are hygienic, but proc-macros are not this creates very odd situations. Additionally, you can't associate a macro to a struct, so one needs to have more workarounds in total. Maybe a more ergonomic way to implement this comes to my mind later.

[jstarry]

This looks promising! The answer is always more macros 😆

I've looked deeper into this, and it's sadly not that easy to add as I hoped it would be. Since macros are hygienic, but proc-macros are not this creates very odd situations. Additionally, you can't associate a macro to a struct, so one needs to have more workarounds in total. Maybe a more ergonomic way to implement this comes to my mind later.

Can you elaborate a bit more on the hygiene issues and what you mean by "can't associate a macro to a struct"?

[TheNeikos]

There’s multiple issues:

• Macros can’t be added to traits or structs. So we need a way to link props to their model. I did this by adding a new attribute “#[prop_for(Model)]” which then generates the macro “ModelPropCreator!”
• You can’t associate a Macro to a type. So if someone imports “foo” and uses “foo::Model” we need to be smart and derive from that path the correct “macro” invocation. It works okayisch for the simplest cases. But if you don’t know the type of the Model and use a generic it won’t work, since we don’t know the name of it. There is some reflection in Rust to possibly get the name but they don’t include the path
• The macro expands where the html! Macro is invoked. This is where the hygiene comes biting you, because if you use local paths in your Prop struct definition, they will expand as is right now and thus not point to the right thing. This could be “fixed” with some more macro generation.

I’m trying to think of ways to circumvent these issues. But macros are annoying to debug ^^’

All in all, this is highlighting a hole in Rusts features, which is partially default structs. Because in that case it would be trivial to implement.

[jstarry]

Thanks for the context, makes sense. Might be worth hacking it out because macros would give us more flexibility.

All in all, this is highlighting a hole in Rusts features, which is partially default structs. Because in that case it would be trivial to implement.

I came across the "Init struct pattern" on r/rust recently and I think it should work actually.

If we call YakShaverInit -> OptionalProps...

• [x] impl Default for OptionalProps is trivial using the #[prop_or...] attributes
• [x] OptionalProps can then be created using passed props and filled with ..Default::default()
• [x] OptionalProps::init method can be generated from the passed required props and the props from OptionalProps

Am I missing anything? I think the compiler error messages will be better too because Rust would just complain that a certain field is not set (i.e. missing required prop) which is more intuitive.

---

EDIT: There may be some trickiness creating OptionalProps because it would need to be associated with the Properties trait

[TheNeikos]

That was my initial try, but the issue is creating the actual final version. The html! Macro itself doesn’t know which fields are optional or not. So it won’t know what goes into OptionalProps or not. And you will also need to create the actual Props struct, but that can only be done with the struct constructor, which requires all fields front the get go. And you can only interpolate structs from the same type.

One thing that would work is Mark the optional fields as “overrides” or so. By for example prepending with an “@“, this way one could make it work. But I tried to make it work without it.

[jstarry]

Ah, that's right. Bummer. I guess the state builder approach lives another day.

[TheNeikos]

What do you think of a syntax that looks like this:

html! {
    <my::Model req_prop=123 @optional_prop=123 />
}

It would solve the big issue of "which" field is missing. The only downside I see is that going from required to optional is a breaking change, whereas right now it is not. To me that doesn't sound like a big price to pay for nicer errors.

[jstarry]

I'm open to it, but yeah I would have the same concern over the pain of switching properties from required to optional and vice versa. We would need nice error messages for that 😝

[jstarry]

marking as blocked because there's no known alternative approach that makes this better without a syntax change to html!

[ta32]

This is the workaround I use, It feels a bit weird though...

#[derive(Clone, PartialEq)]
pub struct GridPaginationBarProps {
    pub page: i32,
    pub page_size: i32,
    pub total_rows: usize
}

#[derive(Properties, Clone, PartialEq)]
pub struct Props {
    pub props: GridPaginationBarProps
}

#[function_component(GridPaginationBar)]
pub fn grid_pagination_bar(i: &Props) -> Html {}

but then caller has to pass a struct into a single property <GridPaginationBar props={GridPaginationBarProps{}}/>

fixed

          <div class="yew-data-grid-footer-container">
              <GridPaginationBar props={GridPaginationBarProps{ page: 1, page_size: 100, total_rows: rows_total}}/>
          </div>

[WorldSEnder]

Note the error has changed with yew 0.20 and is hopefully a bit more readable now. It should say something along the lines of

error[E0277]: the trait bound `AssertAllProps: HasProp<_GridPaginationBarProperties::page, _>` is not satisfied
   --> tests/html_macro/component-fail.rs:115:14
    |
115 |     html! { <GridPaginationBar /> };
    |              ^^^^^^^^^^^^^^^^^ the trait `HasProp<_GridPaginationBarProperties::page, _>` is not implemented for `AssertAllProps`
    |

While I still think the mentioned names should be explained somewhere in the docs (can't find AssertAllProps or HasProp at the moment), at least the name of the missing property is now clearly visible. The work-around mentioned by @ta32 does not support default properties, which are automatically filled in by the macro otherwise. Another possible syntax to write this, if you do not care about default properties, would be using a base expression

#[function_component]
pub fn GridPaginationBar(i: &GridPaginationBarProps) -> Html {}
html! {
    <GridPaginationBar ..GridPaginationBarProps{ page: 1, page_size: 100, total_rows: rows_total}/>
}