add hook validation logic

[ntjess]

Supersedes widgetti/reacton#37.

Checks at compile-time whether the following invalid usage of hooks occurs:

class InvalidReactivityCauses(Enum):
    USE_AFTER_RETURN = "early return"
    CONDITIONAL_USE = "conditional"
    LOOP_USE = "loop"
    NESTED_FUNCTION_USE = "nested function"
    VARIABLE_ASSIGNMENT = "assignment"

See some tests:

Test cases source code

import solara as sl

from tapdance.validate_hooks import HookValidationError, HookValidator


def EarlyReturn():
    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)


def Conditional():
    initial = sl.use_reactive(True)

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

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

    if not initial.value:
        another_reactive = sl.use_reactive(0)
    sl.Markdown(str(another_reactive.value))


def NestedFunction():
    initial = sl.use_reactive(1)

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

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

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

    nested()


def Loop(count: int = 0):
    with sl.Column():
        for i in range(count):
            sl.Markdown("Test")
            sl.use_effect(lambda: print(i), [i])


def VariableAssignment():
    my_use = sl.use_reactive
    my_use(0)


for function in [EarlyReturn, Conditional, NestedFunction, Loop, VariableAssignment]:
    try:
        HookValidator(function).run()
    except HookValidationError as e:
        print(e)
    else:
        print(f"{function.__name__}: No errors found")

Outputs:

.../test_hook_validation.py - EarlyReturn: `use_reactive` found on line 17 despite early return on line 16
.../test_hook_validation.py - Conditional: `use_reactive` found on line 29 within a conditional created on line 28
.../test_hook_validation.py - NestedFunction: `use_reactive` found on line 44 within a nested function created on line 41
.../test_hook_validation.py - Loop: `use_effect` found on line 54 within a loop created on line 52
.../test_hook_validation.py - VariableAssignment: Assigning a variable to a reactive function on line 58 is not allowed since it complicates the tracking of valid hook use.

[maartenbreddels]

Awesome to see this, really excited about this one.

I did a quick pass only. Just a few remarks questions

• do you want to start adding tests, or do you want me to help with those?
• should we warn instead of throw, so that we do not raise on false positives>
• if we don't have the above, should we have an opt out way, something like with flake/ruffs noqa?

[maartenbreddels]

I think we can assume all use_ functions are hooks. Also hooks (use_* functions) have to obey these rules.

[ntjess]

• Help with tests would be great; I presume you have a better idea than me of where edge cases can occur.
• I would rather throw than warn -- since actual invalid states completely crash the application, false positives should be explicitly suppressed rather than allowing true positives to compile.
• ast supports parsing with type_comments=True, in which case users can supply # type: ignore loop_use. I'd lean in this direction, since it means users have to be explicit as possible about which error type is being suppressed.
  • (e.g., if they have a loop inside a conditional, and they only suppress the conditional, it should still be an error)

What is the minimum supported python version? I don't see an explicit minimum on pypi. It will tell me what syntax should be expected.

Another case that came up in my code is try-except. In theory, finally blocks should be able to use_state, but I'm inclined to error in any part of a try clause because the use_ statement can just as easily be moved into the main function body. Unless there is cleanup logic that must be executed? I'm curious on your thoughts.

[maartenbreddels]

• ast supports parsing with type_comments=True, in which case users can supply # type: ignore loop_use. I'd lean in this direction, since it means users have to be explicit as possible about which error type is being suppressed.

Yes, although we have to see/think if this is a 'breaking' change or not. With solara 2.0 we can absolutely do this, so we either add this feature in solara 2.0, or warn in version <2, and raise in >= 2.

• ast supports parsing with type_comments=True, in which case users can supply # type: ignore loop_use

Love this, I'll need to dive a bit into what the best syntax is here. I also think that when we raise, we should give this information as well, so we don't force people to search/google this.

What is the minimum supported python version?

Python 3.6 (we see large enterprises being stuck on this version still.. ), but we should have this in our pyproject.toml to be explicit I think.

PS: I'll look into the typing issues later

but I'm inclined to error in any part of a try clause because the use_ statement can just as easily be moved into the main function body.

I agree, I'm ok with raising on it, and fix this if it comes up, and a user has a good reason for it.

[ntjess]

Based on your earlier comments, my latest updates check for:

• Any use_.* function
• Any occurrence in any part of a try expression

Are there other fundamental "no-go" situations that lead to unstable reactive states? Or is everything theoretically covered now?

To handle false positives in the meantime, since type: ignore comments can arrive in a future PR, I suggest one of the following:

• Allow a new argument:

@solara.component(ignore_unstable_use: list[InvalidReactivityCause] | InvalidReactivityCause)
def MyComponent(): ...

• Allow opt-in use of HookValidator, e.g:

@solara.component
@validate_hooks(ignore_causes=[...])
def MyComponent(): ...

• Convert to warnings (like your initial proposal) until line-level suppression is supported

This also gives us time to workshop the line-level suppression syntax without delaying the feature entirely. It will also be backwards-compatible when enabled.

[maartenbreddels]

I hope you like 729807d which makes it easier to find the line in your editor back.

Also, I think we should be using # noqa, since it's more of a runtime linting system.

Looking at a similar project, https://github.com/reactive-python/reactpy-flake8 we could choose some prefix names if we do not want to opt out completely of linters.

Example

    # single line
    @solara.component
    def Page():
        for i in range(10):
            solara.use_state(1)  # noqa
        solara.Text("Done")

    # whole function
    @solara.component
    def Page():  # noqa
        for i in range(10):
            solara.use_state(1)
        solara.Text("Done")

For specific opt outs:

    # single line
    @solara.component
    def Page():
        for i in range(10):
            solara.use_state(1)  # noqa: SH100
        solara.Text("Done")

    # whole function
    @solara.component
    def Page():  # noqa 
        for i in range(10):
            solara.use_state(1)  # noqa: SH100
        solara.Text("Done")

I think we can use the settings system with environment variables to control what the checks do SOLARA_HOOKS_CHECK=off/warn/raise. We can put it to warn for now, and raise in solara 2.0.

What do you think?

[maartenbreddels]

Feel free to push back on my changes, that I push code does not mean that this is the way it should be.

I think I want to go over https://react.dev/reference/rules/rules-of-hooks and put those in our docs, see if they are all compatible with the code, and make sure our tests are in line with them.

[ntjess]

I think we should be using # noqa

To my knowledge, ast doesn't natively support noqa comments. So would we depend on an additional flake8 library for parsing them? My preference would be to lean on stdlib as much as possible, even if the suppression isn't as semantically meaningful as noqa.

SOLARA_HOOKS_CHECK=off/warn/raise...

This sounds great to me! But I lean toward not supporting an off setting :) Generally, people with an easy road to silencing warnings/errors will take it instead of resolving their root causes. I would reconsider if there are a large number of false positives; i.e. people with many custom use_ methods in a module's function body. This is easy enough to resolve though -- just import as or use it as a function instead of module attribute.

go over https://react.dev/reference/rules/rules-of-hooks

Also great idea. Nice to know the current code state covers all these cases! Even answering the try->finally question.

[maartenbreddels]

To my knowledge, ast doesn't natively support noqa comments. So would we depend on an additional flake8 library for parsing them?

I now check for # on a line without an external library, it might find false positives (a # noq in a string will also be found) but that should not be a problem I think.

But I lean toward not supporting an off setting :)

Solara 2.0? I really don't want to break existing users, without giving them an escape hatch.

[ntjess]

My instinct is that warn still allows the code to run, and noqa can cover the false positives if desired. So nothing should be broken that is currently working.

The reason I am leaning strongly in this direction is I foresee some users putting export SOLARA_HOOKS_CHECK=off somewhere and forgetting to ever re-enable it. Ideally, the warnings are informative enough that they should catch the user's eye. Additionally, warn-by-default without an off switch may help us receive more issue requests documenting false positives.

That's my take, but your reason for providing off is still warranted.

All to say, I respectfully disagree that users need an off escape hatch, since (1) warnings still allow operating code, (2) I don't think there are too many false positives with this implementation, and (3) per-function noqa can disable checks on problematic functions if needed. But, if you wanted to add the option anyway, I would support your decision to do so because I can understand the need to avoid disturbing existing workflows.

[maartenbreddels]

Removing off is I think ok with a 2.0 release, but too much of a breaking change for < 2.

We could open an issue for 2.0 milestone so we do not forget about it. What do you think?

Also, I'm not sure what the test_hook_use_invalid_assign should do / what VARIABLE_ASSIGNMENT means?

The only thing left to do is to write the docs for this, so https://solara.dev/documentation/advanced/understanding/rules-of-hooks explains the why, and that the off/warn/raise options are possible.

I hope you like the error msg, e.g.:

E           solara.validate_hooks.HookValidationError: /Users/maartenbreddels/github/widgetti/solara/tests/unit/hook_use_invalid_test.py:131: test_hook_use_in_try.<locals>.Page: `use_state` found within a exception created on line 130
E           To suppress this check, replace the line with:
E                           solara.use_state(1)  # noqa: SH106
E           
E           Make sure you understand the consequences of this, by reading about the rules of hooks at:
E               https://solara.dev/documentation/advanced/understanding/rules-of-hooks

[maartenbreddels]

PS: I was worried about performance, but starting up the solara docs made it only a few percentage slower, I don't think it is needed to disable it in production mode.

[ntjess]

We could open an issue for 2.0 milestone so we do not forget about it

Works for me!

not sure what VARIABLE_ASSIGNMENT means

Consider this case:

my_use = solara.use_thread

if True:
    return

my_use(...)

It would be difficult to detect these cases unless we do full syntactic tracing of variable assignments. Alternatively, if VARIABLE_ASSIGNMENt of a use_* value is forbidden, this is a non-issue. It purely exists to simplify the invalid state detection logic. Does this make sense? Do you think it will trigger too many false positives?

Similar to the statements earlier about if CONSTANT_BOOL: ..., I would say users who need this pattern should outsource the conditional / variable assignment to an outer function, and call that instead.

I hope you like the error msg:

Looks great to me!

[ntjess]

See comments. The only potential additional "work item" would be if use_* inside generators and comprehensions should also be identified

Otherwise, it looks good to me!

[ntjess]

Is there any way for us not to error for the user here, but see whether this case occurs? Perhaps there is a valid ast.Call case I am missing, and if we raise a value error here, the user may not get an informative message of how to move forward

Would it violate user privacy too badly if we add a telemetry metric to log just the ast node type that occurred here, rather than raise a value error?

[ntjess]

According to https://docs.python.org/3/library/ast.html#ast.Call:

func is the function, which will often be a Name or Attribute object

"Often" doesn't say what the exhaustive possibilities include...

[maartenbreddels]

I think it's better we do a warnings.warn call, so we don't break code because of things we don't know that happen in real life code. Hopefully users who see the warning will inform us, and we can handle it. But I agree we should not break code.

[ntjess]

It's unlikely for users to do this, but visit_ListComp, SetComp, etc. should also be defined if we want to be rigorous

[maartenbreddels]

Good call, I opened #725 so we can get this PR in

[ntjess]

Perhaps add checks for

def foo(x,  # noqa: SH102
    y
):

[maartenbreddels]

done

[ntjess]

Suggested change

	# use_lala = 1
	# my_use = solara.use_state
	# if 1 < 3:
	# my_use(1)

[maartenbreddels]

I'd really like to get this in, so once green, I'll merge this. We can always improve the docs, and extend with #725

[maartenbreddels]

Good call, I opened #725 so we can get this PR in

[maartenbreddels]

done

[maartenbreddels]

thanks a lot @ntjess !
This removes another footgun in a way that I didn't think of.
Next #595 :)