Repository navigation
fix(event): declare LambdaEventCallback.__code__ read-only so bound methods match the protocol - #7397
benedikt-bartscher wants to merge 8 commits into
Conversation
…ethods match the protocol
[Medium impact] The PR appears safe to merge; no actionable issues remain. SummaryChanges
Reviews (7) · Last reviewed commit: "Merge remote-tracking branch 'upstream/m..." · Reviewed by Greptile |
Merging this PR will degrade performance by 4.09%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | test_isinstance_container[list_typeddict] |
108.8 ms | 113.4 ms | -4.09% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing benedikt-bartscher:lambda-callback-readonly-code (6949923) with main (4be8b4d)
|
This change has no observable effect today. I ran the same probe file against The read-only But import types
from typing import Any, Protocol, TypeVar, TypeVarTuple
from reflex.vars.base import Var
P = TypeVarTuple("P")
V = TypeVar("V")
class Settable(Protocol[*P]):
__code__: types.CodeType
def __call__(self: "Settable[V]", value: Var[V], /) -> Any: ...
class ReadOnly(Protocol[*P]):
@property
def __code__(self) -> types.CodeType: ...
def __call__(self: "ReadOnly[V]", value: Var[V], /) -> Any: ...
class H:
def one(self, value: Var[str]) -> None: ...
def fn(value: Var[str]) -> None: ...
a: Settable[str] = fn # ok
b: ReadOnly[str] = fn # ok
c: Settable[str] = H().one # ty: `__call__` is incompatible
d: ReadOnly[str] = H().one # ty: `__call__` is incompatibleSo I could not reproduce "This change covers what is left for bound methods once that is fixed." Please show a ty build where a bound method passes with this change and fails without it, or narrow that claim. The news fragment says bound methods now match the event handler type. No released checker shows that, so please reword it. |
This change has no observable effect with current type checkers. See #7397 (comment)
No released type checker shows bound methods newly matching the event handler type yet: ty still rejects them through astral-sh/ty#4098 and astral-sh/ty#4746. File the fragment as misc and state only that __code__ is now declared read-only.
|
Thanks for the thorough probe. You're right that this has no observable effect today. I reproduced your results, also on ty 0.0.89. In your astral-sh/ty#4098 simulation, bound methods still fail on class HasMeth(Protocol):
def meth(self: "HasMeth", value: int, /) -> Any: ...
class Impl:
def meth(self, value: int) -> None: ...
x: HasMeth = Impl() # ty >= 0.0.65: invalid-assignmentFor a callback protocol, every callable except a plain The builds you asked for:
So this change shows once both astral-sh/ty#4098 and astral-sh/ty#4746 are fixed. I've narrowed the description to say that and reworded the fragment. |
…donly-code # Conflicts: # tests/units/test_event.py
LambdaEventCallbackdeclares__code__: types.CodeType. A plain annotation on aprotocol means a settable attribute, but a bound method's
__code__is a read-onlyproperty on
types.MethodTypethat reads its function's. So under the typing spec abound method never satisfies the protocol, and passing one to an event trigger, for
example reflex's own
ClientStateVar.set_valueason_change, is rejected bycheckers that enforce this (ty does, pyright does not):
Declaring
__code__as a read-only property keeps functions and lambdas matching andlets bound methods match as well.
ty rejects bound methods passed as event handlers for two other reasons as well:
astral-sh/ty#4098 (
__call__overloads whoseselfdoes not fit the specializationare not skipped) and astral-sh/ty#4746 (protocol methods with a protocol-typed
selfreject valid implementations, fix in astral-sh/ruff#29450). With both fixed, this
change is what lets bound methods through. I checked that on ty 0.0.64 and on current
ty with astral-sh/ruff#29450, each with the astral-sh/ty#4098 fix simulated by keeping
only the
[V]overload.pyright still accepts lambdas, functions and bound methods as event handlers, and
tests/units/test_event.pypasses.