widgetti / reacton

A pure Python port of React for ipywidgets
https://reacton.solara.dev/
MIT License
289 stars 19 forks source link

Compile-time detection of `@component` functions with inconsistent state #37

Closed ntjess closed 2 months ago

ntjess commented 2 months ago

Consider the following simple component:

import solara as sl

@sl.component
def Page():
    initial = sl.use_reactive(1)

    def make_invalid():
        initial.set(-initial.value)

    sl.Button("Toggle invalid", on_click=make_invalid)

    if initial.value < 0:
        sl.Markdown("Invalid value")
        return
    another_reactive = sl.use_reactive(0)

After clicking the button, we get the following error:

Traceback (most recent call last):
  File "/Users/ntjess/miniconda3/envs/py312/lib/python3.12/site-packages/reacton/core.py", line 1751, in _render
    raise RuntimeError(
RuntimeError: Previously render had 4 effects, this run 3 (in element/component: Page()/react.component(__main__.Page)). Are you using conditional hooks?

In this case, it is clear how to fix the issue. But if the condition to trigger conditional hooks rarely appears, debugging is especially difficult.

I wrote the following function which attempts to detect these cases at compile time:

```python DEFAULT_USE_FUNCTIONS = ( "use_state", "use_reactive", "use_thread", "use_task", "use_effect", "use_memo", ) def error_on_early_return(component: t.Callable, use_functions=DEFAULT_USE_FUNCTIONS): nodes = list(ast.walk(ast.parse(inspect.getsource(component)))) earliest_return_node: ast.Return | None = None latest_use_node: ast.expr | None = None latest_use_node_id = "" for node in nodes: if isinstance(node, ast.Return): if ( earliest_return_node is None or node.lineno > earliest_return_node.lineno ): earliest_return_node = node elif isinstance(node, ast.Call): func = node.func if isinstance(func, ast.Call): # Nested function, it will appear in another node later continue if isinstance(func, ast.Name): id_ = func.id elif isinstance(func, ast.Attribute): id_ = func.attr else: raise ValueError( f"Unexpected function node type: {func}, {func.lineno=}" ) if id_ in use_functions and ( latest_use_node is None or node.lineno > latest_use_node.lineno ): latest_use_node = node latest_use_node_id = id_ if ( earliest_return_node and latest_use_node and earliest_return_node.lineno <= latest_use_node.lineno ): raise ValueError( f"{component}: `{latest_use_node_id}` found on line {latest_use_node.lineno} despite early" f" return on line {earliest_return_node. lineno}" ) ```

Running this on the sample component provided a much more helpful error (again, at compile time instead of after a conditional toggle!):

ValueError: <function Page at 0x100ebe200>: `use_reactive` found on line 13 despite early return on line 12

I am curious what other cases should be detected and whether you think this function can be adopted into reacton or solara more generally.

In my case, I wrapped solara.component to call this function first and found a few other conditional hook bugs in my existing components.

maartenbreddels commented 2 months ago

I think this is a really good idea. For solara 2.0 we plan to remove a lot of footguns (warn and inform the users when they do something wrong), and this is certainly one of them.

There are rare situations where this is ok (you can imagine having something conditional, but not changing - like depending on a environment variable). For that reason, we probably want an opt-out.

I tend to put it into solara first, because most users are not aware of reacton, and then we can make the error message + hint specific for solara.

Do you think we need to skip this in --production mode for solara?

ntjess commented 2 months ago

My gut reaction would be to specify in the pyproject [tool.solara] section which flags are adjusted in production mode. But since that would require more effort, I say we measure the performance impact of compile-time checks across a large application, and if the slowdown is ~5% or less, we leave it in. It will catch people doing "quick fixes" without disabling the production flag

I agree about the rare cases where conditional hooks are acceptable. I have a DEBUG flag that turns 'use_thread' calls into 'use_effect' because the former chokes the vscode debugger. But even in that case, I would say users must move the conditional check outside the component. So they call a function that resolves which hook should apply. That's what I ended up doing anyway, so I could reuse the same logic across many components.

Or they can pass a list of accepted conditional variables to the decorator, but I want to force as rigid of a hooks layout as possible to prevent footguns

ntjess commented 2 months ago

Closing in favor of widgetti/solara#706