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

Hook order restriction #2026

Closed
mc1098 opened this issue Aug 31, 2021 · 3 comments · Fixed by #2401
Closed

Hook order restriction #2026

mc1098 opened this issue Aug 31, 2021 · 3 comments · Fixed by #2401
Labels
A-yew Area: The main yew crate bug

Comments

@mc1098
Copy link
Contributor

mc1098 commented Aug 31, 2021

Problem
Hooks must be executed in the same order otherwise the function component will panic when trying to convert the hook from hook state for the wrong hook type.

This was mentioned in #1129 and deserves it's own issue.

I have considered this a bug because it's a runtime panic and there are no guards against a user doing this by accident.

Steps To Reproduce
Minimal Code to reproduce panic:

use yew::{functional::*, html};

#[function_component(Counter)]
pub fn counter() -> Html {
    let count = use_state(|| 0);

    if *count % 2 == 0 {
        use_ref(|| 0);
    } else {
        use_effect(|| || {});
    }

    let onclick = move |_| {
        count.set(*count + 1);
    };

    html! {
        <div>
            <button {onclick}>{ "Count" }</button>
        </div>
    }
}

fn main() {
    yew::start_app::<Counter>();
}

Load this app up in the browser, it will render once without issue but once you click the "Count" button it will panic:

panicked at 'Incompatible hook type. Hooks must always be called in the same order', 

Not very useful even if it did work correctly but keeps it as simple as possible to cause the panic.

Expected behavior
To either fail at compile time when trying to use a hook conditionally or for hooks to not depend on the order in which they run to work correctly.

Environment:

  • Yew version: master

As mentioned in #1129 a possible solution is to macro-fy the hook API so that in the macro we can sneak in calls to line!/column!/file!.
Edit: The possible solution above would not solve this issue.

@mc1098 mc1098 added the bug label Aug 31, 2021
@mc1098
Copy link
Contributor Author

mc1098 commented Sep 1, 2021

Looking at this a bit more, I have realised that the panic isn't even the worst part of this ordering bug. If you use conditions with hooks in a particular way one hook can start reading and writing to another state by accident if they are the same type.

Here is a pretty contrived example:

#[function_component(Counter)]
pub fn counter() -> Html {
    let count = use_ref(|| 0);

    let rest = {
        let count = count.clone();
        move |state: UseStateHandle<usize>, action: fn(usize) -> usize| {
            let onclick = {
                let count = count.clone();
                let state = state.clone();

                move |_| {
                    *count.borrow_mut() += 1;
                    state.set(action(*state));
                }
            };

            html! {
                <div>
                    <button {onclick}>{ "Count" }</button>
                    <br />
                    { format!("count: {}", *count.borrow()) }
                    <br />
                    { format!("state count: {}", *state) }
                </div>
            }
        }
    };

    if *count.borrow() % 2 == 0 {
        rest(use_state(|| 0), add_1)
    } else {
        rest(use_state(|| 100), add_50)
    }
}

fn add_1(i: usize) -> usize {
    i + 1
}

fn add_50(i: usize) -> usize {
    i + 50
}

So if working correctly "state count" would display the following when the button was clicked: 0, 100, 1, 150, etc.
Instead both of the use_states use the same state then perform the different actions on that state so it ends up displaying: 0, 1, 51, 52, 102 etc.

The example does show that you have twist yourself in knots to get this to happen but I can see it happening out in the wild and unlike the panic this bug is insidious and could be a real headache to debug.

@lukechu10
Copy link
Contributor

As mentioned in #1129 a possible solution is to macro-fy the hook API so that in the macro we can sneak in calls to line!/column!/file!.

Unfortunately, I don't think this would work because there are plenty of edge cases. What if, for example, you use a hook inside a loop? Or calling a function that uses hooks? In fact, this will completely break the composability aspect of hooks because custom-hooks will call the primitive hooks at the same location in the source file.

@mc1098
Copy link
Contributor Author

mc1098 commented Sep 4, 2021

If you put a hook inside a loop using this technique I actually think it would work - it would call the runner for the same hook state as many times as the loop, now whether a user of the hook would think the hook should actually be unique for each invocation of the loop I don't know so this could probably lead to confusion?

Or calling a function that uses hooks?

This is the breaking point for this approach because using line/column/file only works if you call those at the call site of the hook in the function component that means anything else breaks down completely and again we have no way to stop this being abused.

I agree that this would not be the way to fix this issue, but I'm not really sure what could be the fix here and I really don't like that hooks can have this behaviour - it's good enough for JS to say "don't do this" and blow up if you do but it feels very wrong in Rust.

@mc1098 mc1098 added the A-yew Area: The main yew crate label Sep 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-yew Area: The main yew crate bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants