fix(types): qualify annotations shadowed by same-class members - #6846
Conversation
A class member whose name matches a builtin also shadows that builtin for annotations elsewhere in the same class body, because type checkers resolve annotations against the class namespace. The worst case is `Var.bool()`, which shadows `bool` for the `value: bool` parameter of the second `Var.create` overload. A checker that resolves the annotation to the method degrades that parameter to an unknown type, so the overload matches every argument and `Var.create(anything)` is inferred as `LiteralBooleanVar`. `Var.to`, `Var.guess_type` and `Var.to_string` are affected the same way, as are `BaseState.dict`, `BaseComponent.set`, `PropsBase.dict` and `ToastProps.dict`. Qualify the 16 affected annotations as `builtins.<name>`. Public method names are unchanged, so this is not a breaking change. `_get_type_hint` needed a matching guard: its fallback qualifies a type by whichever module component of `__module__` it finds in the module globals, so importing `builtins` for the above made it emit `builtins.str` into generated stubs, which do not import `builtins`. Builtins are always in scope and now stay unqualified there.
Greptile SummaryThe PR qualifies class-shadowed builtin annotations and updates stub generation so builtins remain unqualified in generated stubs.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| packages/reflex-base/src/reflex_base/vars/base.py | Qualifies shadowed boolean annotations across Var overloads and methods without changing runtime behavior. |
| packages/reflex-base/src/reflex_base/utils/pyi_generator.py | Keeps builtin types bare when rendering generated stub annotations, avoiding undeclared builtins qualification. |
| reflex/state.py | Qualifies dictionary annotations shadowed by BaseState.dict. |
| tests/type_checking/vars.py | Adds static assertions covering public inference contracts for Var.create, Var.to, and guess_type. |
| .pre-commit-config.yaml | Adds an always-run ty check for inferred-type examples using the repository’s existing uv hook convention. |
| pyproject.toml | Adds ty to development dependencies and raises the reflex-base minimum version. |
Reviews (10): Last reviewed commit: "build: raise the reflex-base floor to 0...." | Re-trigger Greptile
Conflict in reflex/state.py: main added `_dynamic_route_arg_types` directly above `setup_dynamic_args`, whose `args: dict[str, str]` this branch had qualified. Kept both, and qualified the new method's return annotation too, since `dict` is shadowed by `BaseState.dict` there as well.
Merging this PR will not alter performance
Comparing Footnotes
|
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
There was a problem hiding this comment.
4 issues found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="reflex/state.py">
<violation number="1" location="reflex/state.py:1346">
P0: App startup can fail when collecting dynamic route args because `App` still calls `State._dynamic_route_arg_types()` but this method was removed in this change. Keeping a compatibility classmethod (or updating all callers in the same PR) would avoid a runtime `AttributeError` during route registration.</violation>
</file>
<file name="tests/units/test_builtin_shadowing.py">
<violation number="1" location="tests/units/test_builtin_shadowing.py:99">
P2: Use the full relative path (REPO_ROOT-relative) in the parametrize ids. With 83 duplicated basenames across reflex/ and packages/, ids=lambda p: str(p.name) gives ambiguous test names, so when a check fails you can't tell which file from the node id alone. ids=lambda p: str(p.relative_to(REPO_ROOT)) keeps ids unique and readable.</violation>
</file>
<file name="packages/reflex-base/src/reflex_base/components/memo.py">
<violation number="1" location="packages/reflex-base/src/reflex_base/components/memo.py:1066">
P1: Unannotated component-memo params can now fail on first call when the body indexes or accesses attributes, because the placeholder stays `Var[Any]` instead of using the call value’s type. Consider restoring first-call runtime type inference for missing annotations (or making missing annotations a hard error) so deprecated-but-supported memos don’t regress.</violation>
</file>
<file name="packages/reflex-base/src/reflex_base/vars/base.py">
<violation number="1" location="packages/reflex-base/src/reflex_base/vars/base.py:3539">
P2: The `Field.__init__` custom-attribute handling changed from carrying attributes by reference to `copy.deepcopy(value)`, which reverses the rationale documented by the comment this PR removes. The previous code deliberately avoided deep-copying because it crashed on non-copyable custom values (like locks) and broke identity-based consumers of stateful callable markers. Any state var that carries such a custom attribute would now raise `TypeError` or change behavior at class-definition time. This change is also unrelated to the builtin-shadowing fix described in the PR. If the deep copy isn't intentional for this fix, restore the reference-copy behavior; if it is intentional, it should be called out and tested.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
Basenames collide across reflex/ and packages/, so a failing check could not be traced to a file from the node id alone.
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="reflex/state.py">
<violation number="1" location="reflex/state.py:1370">
P2: Dynamic-arg setup now does extra work proportional to all computed vars, which can slow route registration on states with many computed vars. This comes from building a full installed map just to test key existence; keeping per-arg lookups (or a name set) avoids the full dict allocation.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
Inferred types are part of the public contract, but nothing verified them. `tests/type_checking/` holds Reflex code whose `typing.assert_type` calls each checker verifies statically; `tests/units/test_type_checking.py` runs every checker over the directory and requires a clean result. The assertions are checker-agnostic, so the same file gates pyright and ty today and anything added later. A checker that is not installed is skipped. `vars.py` covers the `Var.create`, `Var.to` and `guess_type` overload sets. Reverting the shadowing fix in this branch turns six of them red, naming the wrong inferred type in each case. Adds ty to the dev group so its half actually runs rather than skipping.
The assert_type examples cover the same regression by its observable effect rather than by one syntactic cause.
The pytest wrapper was shelling out to the same checkers CI already runs, and pyright's existing hook covers `reflex` and `tests`, so it was duplicating that half outright. Worse, it skipped when a checker was not installed, so a CI misconfiguration would have read as green. ty gets its own hook scoped to the example directory. A missing checker now fails the hook rather than silently passing.
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".pre-commit-config.yaml">
<violation number="1" location=".pre-commit-config.yaml:49">
P3: The new `ty` hook checks `tests/type_checking/` with an explicit `--python-version 3.14`, which is the top of the project's supported range (`>=3.10,<4.0`) and diverges from the rest of the project, where ruff targets `py310` and the repo `requires-python` floor is 3.10. Per ty's own docs, the recommended target is the `requires-python` lower bound so that the checker doesn't admit stdlib/typing constructs only available in newer versions. Since README.md frames these files as a gate for "what a user's editor shows", type-checking against 3.14 (instead of the 3.10 floor most users actually run) weakens the contract: examples using Python 3.11+‑only typing/stdlib could pass this gate while still failing under the supported minimum. Consider dropping the flag (let ty infer from `requires-python`, i.e. 3.10) or aligning it with the pyright/ruff `py310` target.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
… floor The hook was scoped to `tests/type_checking/`, so it only fired when the examples themselves changed. What they catch is a source edit elsewhere altering an inferred type, which is exactly the commit the hook would have skipped. `typing.assert_type` is 3.11+, so the examples could not be checked at the 3.10 floor reflex supports. `typing_extensions.assert_type` works on every supported version; verified clean under both checkers at 3.10 and 3.14.
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
The examples are written to hold at 3.10 and verified there by hand, but the ty hook pins 3.14 and pyright follows the interpreter, so nothing gates it. Say so rather than implying a guarantee that is not wired up.
`reflex/` uses `AddPageProtocol`, `RegisterRouteContext`, `get_plugin`, `Env.PREVIEW`, `Plugin.register_route`, `EnvironmentVariables.VITE_MINIFY` and `REFLEX_NO_AUTOPREFIXER`, none of which exist in reflex-base 0.9.7, so resolving the declared floor from PyPI fails to type-check. Pre-existing on main rather than introduced here: main's HEAD is the 0.9.8 release commit and min_deps has not run on it, so this branch is the first to exercise the check against it. Confirmed by running the script on a pristine origin/main worktree, which fails identically. All 17 workspace packages pass afterwards.
|
Done — For the record on why it went red here rather than on main: main's HEAD is the 0.9.8 release commit, and min_deps has never run on it (newest run is against 7e365ce). This branch merged that release in, so it was the first thing to exercise the check against it. Confirmed by running The symbols Only the Good luck with the pre-rendering bug. |
Problem
A class member whose name matches a builtin also shadows that builtin for annotations elsewhere in the same class body, because type checkers resolve annotations against the class namespace.
The worst instance is
Var.bool(). It shadowsboolfor thevalue: boolparameter of the secondVar.createoverload:A checker that resolves the annotation to the method degrades that parameter to an unknown type, and an unknown parameter matches every argument, so the overload wins for every call:
pyright happens to resolve these to the builtin, so CI is green today, but the annotations are ambiguous as written and other checkers read them the other way. This surfaced while evaluating ty against a large Reflex app: this one shadow accounted for 124 of its diagnostics, all of them downstream of
Var.createbeing mistyped.Changes
16 annotations across 5 files, qualified as
builtins.<name>:vars/base.pyVar.boolreflex/state.pyBaseState.dictcomponents/component.pyBaseComponent.setcomponents/props.pyPropsBase.dictreflex_components_sonner/toast.pyToastProps.dictNo public method is renamed, so nothing downstream breaks.
_get_type_hintin the pyi generator needed a matching guard. Its fallback qualifies a type by whichever component of__module__it finds in the module globals, so importingbuiltinsfor the above made it emitbuiltins.strinto generated stubs, which do not importbuiltins. Builtins are always in scope and now stay unqualified there. Onlytoast.pyichanges hash, and only because its source annotation changed.Test
Inferred types are part of the public contract, but nothing verified them, so this adds a home for that:
tests/type_checking/holds Reflex code whosetyping.assert_typecalls each checker verifies statically.vars.pycovers theVar.create,Var.toandguess_typeoverload sets.tests/units/test_type_checking.pyruns every checker over that directory and requires a clean result. A checker that is not installed is skipped.The assertions are checker-agnostic, so the same file gates pyright and ty today and anything added later (pyrefly, mypy) for the cost of a small parse function. Reverting the shadowing fix turns six of them red, each naming the wrong inferred type:
tyis added to the dev group so its half runs rather than skipping. Noterequires-python = ">=3.10,<4.0"resolves it to 0.0.64.Verification
uv run pytest tests/unitspasses (7084 tests)uv run ruff check/ruff format --check/uv run pyright reflex testscleanuv run pre-commit run --all-filespasses, includingupdate-pyi-files