Compile-time detection of @component functions with inconsistent state

[ntjess]

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:

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.

• use_* inside an if or for block
• return before use_*
• use_* outside a render context (this would be a bit harder, but still possible by tracing ast.Call usage)

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]

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]

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]

Closing in favor of widgetti/solara#706