yewstack/yew_router

Code generated by `Switch` derive macro depends on evaluation order of tuple constructor arguments

Closed this issue · 4 comments

See https://rust-lang.github.io/rust-clippy/master/index.html#eval_order_dependence for why this is bad.

yew_router currently generates code like the following:

impl ::yew_router::Switch for OtherSingle {
    fn from_route_part<__T>(
        route: String,
        mut state: Option<__T>,
    ) -> (::std::option::Option<Self>, ::std::option::Option<__T>) {
        // ...
        if let Some(mut captures) = matcher
            .capture_route_into_vec(&route_string)
            .ok()
            .map(|x| x.1)
        {
            let mut drain = captures.drain(..);
            return (
                ::std::option::Option::Some(OtherSingle({
                    let (v, s) = match drain.next() {
                        ::std::option::Option::Some(value) => {
                            <u32 as ::yew_router::Switch>::from_route_part(value, state)
                        }
                        ::std::option::Option::None => {
                            (<u32 as ::yew_router::Switch>::key_not_available(), state)
                        }
                    };
                    match v {
                        ::std::option::Option::Some(val) => {
                            state = s; // state is written as part of the returned tuple's construction
                            val
                        }
                        ::std::option::Option::None => return (::std::option::Option::None, s),
                    }
                })),
                state, // state is also read as part of the returned tuple's construction
            );
        };
        return (::std::option::Option::None, state);
    }

    // fn build_route_section<__T>(...) -> ::std::option::Option<__T> { ... }
}

Am I assuming correctly that the read of state should happen before the write? If that's the case I can open a PR that moves creation of the first tuple element before the tuple's construction to fix this clippy warning (or I can add that to #238).

Sorry I didn't get to this this weekend; was busy.

I can see the readability problem represented by this code now.

I think lifting the expression that makes up the first element in the returned tuple out into a variable would be advisable.

State behavior isn't super well documented (its a feature that I assume no one is using at the moment, and will be changed significantly soon), so I don't think tests would catch breakage here.

This is all fine, because this behavior is changing significantly in the near future. Readability of the macro output needs to be improved anyways and this is a good step in that direction.

Since I'm going to try to push out a new version soon, and I'm going to be messing around with code that might interfere with this change with one of its planned features #126 , I'm going to push out a new version without further significant changes and defer to allowing you to implement this change and then I'll see if I can finish #126 after.

I've started work on this and should be able to finish it this evening (CET).