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

Add validation to function vars #4379

Open
wants to merge 96 commits into
base: main
Choose a base branch
from

Conversation

adhami3310
Copy link
Member

No description provided.

@adhami3310
Copy link
Member Author

blocked on microsoft/pyright#9470

@benedikt-bartscher
Copy link
Contributor

blocked on microsoft/pyright#9470

Are we able to mitigate this with smth like this?

from typing import TYPE_CHECKING
from typing_extensions import Self, reveal_type


class Function:
    def __call__(self, *args, **kwargs) -> Self:
        return self


class ArrayLike:
    __getitem__ = Function()

    if TYPE_CHECKING:
        def __getitem__(self, index) -> Function: ...


x = ArrayLike()[0]
reveal_type(x)  # Type of "x" is "Function"
print(type(x))

@adhami3310
Copy link
Member Author

Are we able to mitigate this with smth like this?

No :( the reason is that __getitem__ will not be a def func, but a more complicated class with overloaded __call__s. So we can't just force it to be one. At least not without sacrificing on some other things i would like to maintain.

We could give up on function vars being like that for dunder methods though, and keep them for normal ones. But I do like the symmetry and I don't want to break it.

Copy link
Collaborator

@masenf masenf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i have to admit this one scares me from a maintainability perspective.

i read through all the code, but i think i'd prefer if we can walk through it together and help me put the pieces together in my head.

not sure if dev documentation is planned for this change, but i think it would be really helpful for future maintainers to understand some of these constructs, like the type_computer.

frozen=True,
**{"slots": True} if sys.version_info >= (3, 10) else {},
)
class MatchOperation(CachedVarOperation, Var[VAR_TYPE]):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this in the number module?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's there because the bool is there (same goes for ternary), although i admit it's pretty confusing

tests/units/test_var.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
Copy link
Collaborator

@masenf masenf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generated code is looking much more concise, thanks.

Still not working with form-designer though:

reflex.utils.exceptions.MatchTypeError: Match cases should have the same return types. Expected return types to be of type Component or Var[Component]. Return type of case 0 is typing.Union[typing.List[reflex.components.radix.themes.layout.box.Box], reflex.components.radix.themes.typography.text.Text]

@adhami3310
Copy link
Member Author

Still not working with form-designer though

form-designer works now!

@masenf
Copy link
Collaborator

masenf commented Jan 25, 2025

I'm wanting to wait on this one until we can properly turn the components in cond/foreach into StatefulComponents that only bring in the state that they depend on.

We tried to convert to StatefulComponent in LiteralComponentVar.create, but the problem was these vars also depended on the loop vars and other locals from the outer scope of the foreach.

A plausible fix to make this work is to identify any non-state vars in the StatefulComponent and replace those with props that get passed into the resulting component. For example, if something in the stateful component references xyzlocal then the resulting extracted component would expect to take xyzlocal as a prop. We should just use object destructuring in the component function signature to avoid having to rewrite the actual component body as much as possible. Where the StatefulComponent is used (the scope containing the local var), any local vars will just be passed through as normal jsx props on the component.

@masenf
Copy link
Collaborator

masenf commented Jan 25, 2025

Here is an example of how this goes off the rails:

Using this patch

diff --git a/reflex/components/component.py b/reflex/components/component.py
index 8c3bc518..5fc3d843 100644
--- a/reflex/components/component.py
+++ b/reflex/components/component.py
@@ -2274,6 +2274,10 @@ class StatefulComponent(BaseComponent):
 
         return _compile_component(self)
 
+    def __getattr__(self, name) -> Any:
+        # if we don't provide the attribute, get it from the wrapped component
+        return getattr(self.component, name)
+
     @classmethod
     def compile_from(cls, component: BaseComponent) -> BaseComponent:
         """Walk through the component tree and memoize all stateful components.
@@ -2474,6 +2478,8 @@ class LiteralComponentVar(CachedVarOperation, LiteralVar, ComponentVar):
         Returns:
             The var.
         """
+        if not isinstance(value, StatefulComponent):
+            value = StatefulComponent.compile_from(value) or value
         return LiteralComponentVar(
             _js_expr="",
             _var_type=type(value),

With the following app:

import reflex as rx


def index() -> rx.Component:
    return rx.vstack(
        rx.foreach(
            ["one", "two", "three"],
            lambda item: rx.text(item, on_click=rx.console_log(item)),
        )
    )


app = rx.App()
app.add_page(index)

We currently end up generating code like this

export function Text_719e7690cbbecf6b33f0b7a94350b86a() {
  const [addEvents, connectErrors] = useContext(EventLoopContext);

  const on_click_63778f856f6473ddf372e334cf3dc24c = useCallback(
    (...args) =>
      addEvents(
        [
          Event(
            "_call_function",
            {
              ["function"]: () => console["log"](sefdkzeh),
              ["callback"]: null,
            },
            {}
          ),
        ],
        args,
        {}
      ),
    [addEvents, Event]
  );

  return (
    <RadixThemesText
      as={"p"}
      onClick={on_click_63778f856f6473ddf372e334cf3dc24c}
    >
      {sefdkzeh}
    </RadixThemesText>
  );
}
export function Flex_614876435cb70b6f7eb23c7998c05171() {
  return (
    <RadixThemesFlex
      align={"start"}
      className={"rx-Stack"}
      direction={"column"}
      gap={"3"}
    >
      {Array.prototype.map.apply(
        ["one", "two", "three"],
        [(sefdkzeh) => jsx(Text_719e7690cbbecf6b33f0b7a94350b86a, {})]
      )}
    </RadixThemesFlex>
  );
}

But really we need to be passing the loop variable into the stateful component function as a prop

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants