don't use _outer_type if we don't have to #4528

adhami3310 · 2024-12-12T14:56:52Z

turns out _outer_type strips out None

masenf · 2024-12-12T17:25:11Z

reflex/utils/types.py

+    Returns:
+        The type of the field, if it exists, else None.
+    """
+    type_hints = get_type_hints(cls)


get_type_hints is a really slow function, it doesn't make sense to call it multiple times for the same cls without some kind of caching.

we should profile and see what affect this change may or may not have on component __init__

yea i was thinking the same, we could also be more specific when we use this vs the other one

ElijahAhianyo · 2024-12-12T19:47:28Z

I get these errors when I test with this code:

from typing import Optional

import reflex as rx


class State(rx.State):
    foo: rx.Field[Optional[str]] = rx.field(None) # throws a compile time error (see below)
    simple: rx.Field[str | None] = rx.field(None) # throws a compile time error (see below)
    bar: str | None = "world" # runtime error(see below)
    name: rx.Field[str | None] = rx.field(None) # compile time error(see below)


def index() -> rx.Component:
    return rx.container(
        rx.button("Clear", on_click=lambda: State.set_name(None)),
        rx.text("rx.input"),
        rx.input(value=State.name, on_change=State.set_name),
        rx.text("rx.el.input"),
        rx.el.input(value=State.name),
    )


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

foo: rx.Field[Optional[str]]

  File "/Users/eli/Documents/work/Pynecone-io/pynecone-forks/reflex/demos/oct-04-24/fourth/fourth/fourth.py", line 16, in <module>
    class State(rx.State):
  File "pydantic/main.py", line 282, in pydantic.main.ModelMetaclass.__new__
  File "<frozen abc>", line 106, in __new__
  File "/Users/eli/Documents/work/Pynecone-io/pynecone-forks/reflex/reflex/state.py", line 574, in __init_subclass__
    cls._init_var(prop)
  File "/Users/eli/Documents/work/Pynecone-io/pynecone-forks/reflex/reflex/state.py", line 991, in _init_var
    raise VarTypeError(
reflex.utils.exceptions.VarTypeError: State vars must be primitive Python types, Plotly figures, Pandas dataframes, or subclasses of rx.Base. Found var "reflex___state____state__fourth___fourth____state.foo" with type typing.Optional[reflex.vars.base.Field[typing.Optional[str]]].

simple: rx.Field[str | None] = rx.field(None) [same as above]

    class State(rx.State):
  File "pydantic/main.py", line 282, in pydantic.main.ModelMetaclass.__new__
  File "<frozen abc>", line 106, in __new__
  File "/Users/eli/Documents/work/Pynecone-io/pynecone-forks/reflex/reflex/state.py", line 574, in __init_subclass__
    cls._init_var(prop)
  File "/Users/eli/Documents/work/Pynecone-io/pynecone-forks/reflex/reflex/state.py", line 991, in _init_var
    raise VarTypeError(
reflex.utils.exceptions.VarTypeError: State vars must be primitive Python types, Plotly figures, Pandas dataframes, or subclasses of rx.Base. Found var "reflex___state____state__fourth___fourth____state.simple" with type typing.Optional[reflex.vars.base.Field[str | None]].

bar: str | None = "world"

Task exception was never retrieved
future: <Task finished name='Task-18' coro=<AsyncServer._handle_event_internal() done, defined at /Users/eli/Library/Caches/pypoetry/virtualenvs/reflex-K7o9YeT9-py3.11/lib/python3.11/site-packages/socketio/asyncio_server.py:522> exception=AttributeError("'str' object has no attribute 'items'")>
Traceback (most recent call last):
  File "/Users/eli/Library/Caches/pypoetry/virtualenvs/reflex-K7o9YeT9-py3.11/lib/python3.11/site-packages/socketio/asyncio_server.py", line 524, in _handle_event_internal
    r = await server._trigger_event(data[0], namespace, sid, *data[1:])
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/eli/Library/Caches/pypoetry/virtualenvs/reflex-K7o9YeT9-py3.11/lib/python3.11/site-packages/socketio/asyncio_server.py", line 569, in _trigger_event
    return await self.namespace_handlers[namespace].trigger_event(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/eli/Library/Caches/pypoetry/virtualenvs/reflex-K7o9YeT9-py3.11/lib/python3.11/site-packages/socketio/asyncio_namespace.py", line 37, in trigger_event
    ret = await handler(*args)
          ^^^^^^^^^^^^^^^^^^^^
  File "/Users/eli/Documents/work/Pynecone-io/pynecone-forks/reflex/reflex/app.py", line 1567, in on_event
    **{k: v for k, v in fields.items() if k not in ("handler", "event_actions")}
                        ^^^^^^^^^^^^
AttributeError: 'str' object has no attribute 'items'

name: rx.Field[str | None] = rx.field(None) [same as simple]

masenf · 2024-12-13T17:36:20Z

reflex/utils/types.py

-        field = cls.__fields__[name]
-        type_ = field.outer_type_
-        if isinstance(type_, ModelField):
-            type_ = type_.type_
-        if (
-            not field.required
-            and field.default is None
-            and field.default_factory is None
-        ):
-            # Ensure frontend uses null coalescing when accessing.
-            type_ = Optional[type_]
-        return type_


i'm really hesitant about removing these lines, unless we're sure this is not going to be hit.

@benedikt-bartscher do you know under what circumstances this bit of code gets used?

import reflex as rx class M1(rx.Base): foo: str = "foo" class M2(rx.Base): m1: M1 = None class State(rx.State): m2: M2 = M2() def index() -> rx.Component: return rx.container( rx.text(State.m2.m1.foo), ) app = rx.App() app.add_page(index)

This is at least one of the cases; default is None, but the field is not annotated as Optional (which is incorrect and i'm surprised pydantic doesn't yell), but Reflex still try to be helpful and use "null coalescing" to avoid frontend exceptions.

Again, why pydantic allows the default to not match the annotation and pass validation i'm unsure.

This one is interesting, default is None when it's unset and when it's set to None :(

@masenf sorry, I did not see your question for some reason. But it seems like you found a case.

This one is interesting, default is None when it's unset and when it's set to None :(

@adhami3310 you can use this to determine unset defaults

from pydantic.v1 import BaseModel class User(BaseModel): no_default: str | None none_default: str | None = None no_default_field = User.__fields__["no_default"] none_default_field = User.__fields__["none_default"] print(no_default_field.field_info.default) print(none_default_field.field_info.default)

TIL PydanticUndefined

last time i checked, FieldInfo doesn't contain the type, but I wouldn't be surprised if i missed something

I mean for default and default factory values.

i made the check for default use None, there's little I can do with default_factory returning None (i hope no one thinks that's a good idea)

Are you sure this is a bad idea?

diff --git a/reflex/state.py b/reflex/state.py index e7e6bcf3..46ba3133 100644 --- a/reflex/state.py +++ b/reflex/state.py @@ -1099,7 +1099,10 @@ class BaseState(Base, ABC, extra=pydantic.Extra.allow): if ( not field.required and field.default is None - and field.default_factory is None + and ( + field.default_factory is None + or types.default_factory_can_be_none(field.default_factory) + ) and not types.is_optional(prop._var_type) ): # Ensure frontend uses null coalescing when accessing. diff --git a/reflex/utils/types.py b/reflex/utils/types.py index b8bcbf2d..60eb456d 100644 --- a/reflex/utils/types.py +++ b/reflex/utils/types.py @@ -333,7 +333,10 @@ def get_attribute_access_type(cls: GenericType, name: str) -> GenericType | None if ( not field.required and field.default is None - and field.default_factory is None + and ( + field.default_factory is None + or default_factory_can_be_none(field.default_factory) + ) ): # Ensure frontend uses null coalescing when accessing. type_ = Optional[type_] @@ -893,3 +896,18 @@ def typehint_issubclass(possible_subclass: Any, possible_superclass: Any) -> boo for provided_arg, accepted_arg in zip(provided_args, accepted_args) if accepted_arg is not Any ) + + +def default_factory_can_be_none(default_factory: Callable) -> bool: + """Check if the default factory can return None. + + Args: + default_factory: The default factory to check. + + Returns: + Whether the default factory can return None. + """ + type_hints = get_type_hints(default_factory) + if hint := type_hints.get("return"): + return is_optional(hint) + return default_factory() is None

most default factories are lambdas so i would imagine you're not getting a type hint in most cases, calling it could give you should be a reliable indicator but i'm afraid of side effects or performance costs

masenf

Something else is going on with this PR too,

DeprecationWarning: mismatched-type-assignment has been deprecated in version 0.6.5 Tried to assign value None of 
type <class 'NoneType'> to field State.parent_state of type <class 'reflex.state.BaseState'>. This might lead to 
unexpected behavior. It will be completely removed in 0.7.0

This seems to come from setting parent_state to None in BaseState.__init__, but it's annotated as an Optional so passing None should be legitimate

    # The parent state.
    parent_state: Optional[BaseState] = None

adhami3310 · 2024-12-13T20:18:04Z

This seems to come from setting parent_state to None in BaseState.init, but it's annotated as an Optional so passing None should be legitimate

So this issue is annoying, it's almost impossible to know if something is truly optional or not, one thing we could check is if annotation starts with Optional ?

…d tests without rx.Field

adhami3310 · 2024-12-18T18:04:05Z

got unblocked on pyright, will investigate later to see if it's working

don't use _outer_type if we don't have to

c9fe2e1

masenf reviewed Dec 12, 2024

View reviewed changes

ElijahAhianyo mentioned this pull request Dec 12, 2024

[ENG-4153]Use empty string for None values in rx.input and rx.el.input #4521

Merged

adhami3310 added 2 commits December 13, 2024 07:12

apparently we should use .annotation, and .allow_none is useless

c2a39b4

have a shorter path for get_field_type if it's nice

d3fbf12

masenf reviewed Dec 13, 2024

View reviewed changes

check against optional in annotation str

3fb8450

benedikt-bartscher added a commit to benedikt-bartscher/reflex that referenced this pull request Dec 14, 2024

currently only rx.Field is working, waiting for reflex-dev#4528 to ad…

872c0e5

…d tests without rx.Field

benedikt-bartscher mentioned this pull request Dec 14, 2024

test #4445 #4448

Draft

add check for default value being null

9e96333

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

don't use _outer_type if we don't have to #4528

don't use _outer_type if we don't have to #4528

adhami3310 commented Dec 12, 2024

masenf Dec 12, 2024

adhami3310 Dec 13, 2024

ElijahAhianyo commented Dec 12, 2024

masenf Dec 13, 2024

masenf Dec 13, 2024

adhami3310 Dec 14, 2024

benedikt-bartscher Dec 14, 2024

adhami3310 Dec 14, 2024

adhami3310 Dec 17, 2024

benedikt-bartscher Dec 17, 2024

adhami3310 Dec 17, 2024

benedikt-bartscher Dec 17, 2024

adhami3310 Dec 17, 2024

masenf left a comment

adhami3310 commented Dec 13, 2024

adhami3310 commented Dec 18, 2024

don't use _outer_type if we don't have to #4528

Are you sure you want to change the base?

don't use _outer_type if we don't have to #4528

Conversation

adhami3310 commented Dec 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElijahAhianyo commented Dec 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masenf left a comment

Choose a reason for hiding this comment

adhami3310 commented Dec 13, 2024

adhami3310 commented Dec 18, 2024