Repository navigation
Treat None as an empty string in optional StringVar operations - #7902
FarhanAliRaza wants to merge 2 commits into
Conversation
Operations on a `str | None` var holding None that compile to a native string method (split with a separator, strip, lower, upper, title, capitalize, contains, startswith, endswith, replace, repeat) threw `Cannot read properties of null` in the browser and took the whole page into the error boundary, while length, indexing and slicing already read None as "" through the code point helpers. StringVar gets the `_or_empty()` view ArrayVar already has: when the var type allows None, the operation reads `(x ?? "")`, typed without None. A string that cannot be None compiles as before, with no guard. Fixes #7873 Claude-Session: https://claude.ai/code/session_01LwJUCTCgM7D4Vj6H4dRWss
[Medium impact] The PR appears safe to merge. SummaryString methods now treat optional strings holding
The previous documentation finding is fixed. No new issues found. Reviews (3) · Last reviewed commit: "Skip the optional check for plain str va..." · Reviewed by Greptile |
There was a problem hiding this comment.
2 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/reflex-base/src/reflex_base/vars/sequence.py">
<violation number="1" location="packages/reflex-base/src/reflex_base/vars/sequence.py:786">
P2: Concatenation is still unguarded, so `None + "x"` renders as `"nullx"` in the browser. The new docs and news entry say an optional string holding `None` acts as `""` in every string operation, so guard `__add__` and `__radd__` with `_or_empty()` as the array `__add__` does. The ordering comparisons (`<`, `>`, `<=`, `>=`) have the same gap, so either guard them or narrow the docs claim.</violation>
</file>
<file name="docs/vars/var-operations.md">
<violation number="1" location="docs/vars/var-operations.md:490">
P3: Qualify these results with non-empty search arguments: `contains("")`, `startswith("")`, and `endswith("")` are true, while replacing `""` in `""` with `"x"` yields `"x"`. Apply the same qualification to the news fragment.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| class StringVar(Var[STRING_TYPE], python_types=str): | ||
| """Base class for immutable string vars.""" | ||
|
|
||
| def _or_empty(self) -> StringVar[Any]: |
There was a problem hiding this comment.
P2: Concatenation is still unguarded, so None + "x" renders as "nullx" in the browser. The new docs and news entry say an optional string holding None acts as "" in every string operation, so guard __add__ and __radd__ with _or_empty() as the array __add__ does. The ordering comparisons (<, >, <=, >=) have the same gap, so either guard them or narrow the docs claim.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/reflex-base/src/reflex_base/vars/sequence.py, line 786:
<comment>Concatenation is still unguarded, so `None + "x"` renders as `"nullx"` in the browser. The new docs and news entry say an optional string holding `None` acts as `""` in every string operation, so guard `__add__` and `__radd__` with `_or_empty()` as the array `__add__` does. The ordering comparisons (`<`, `>`, `<=`, `>=`) have the same gap, so either guard them or narrow the docs claim.</comment>
<file context>
@@ -783,6 +783,16 @@ def create(
class StringVar(Var[STRING_TYPE], python_types=str):
"""Base class for immutable string vars."""
+ def _or_empty(self) -> StringVar[Any]:
+ """View an optional string as an empty string while it holds None.
+
</file context>
There was a problem hiding this comment.
Narrowed the claim instead of guarding + (cbd9751): the docs and the news fragment now say the string methods read None as "", and name + and the comparison operators as not covered.
I left + alone on purpose. An f-string (f"{S.m} x") builds the same concatenation without going through __add__, so guarding __add__ / __radd__ would make S.m + " x" and f"{S.m} x" render differently for the same value. Neither throws, so it is outside what #7873 is about (operations that crash the page); changing what a None renders as in concatenation is a separate decision for both forms together.
Generated by Claude Code
| ) | ||
| ``` | ||
|
|
||
| An optional string var (`str | None`) holding `None` acts as an empty string in every string operation: `lower`, `strip` and `replace` give an empty string, `contains` and `startswith` are false, `length` is 0 and `split(" ")` gives `[""]`, as it does for `""`. |
There was a problem hiding this comment.
P3: Qualify these results with non-empty search arguments: contains(""), startswith(""), and endswith("") are true, while replacing "" in "" with "x" yields "x". Apply the same qualification to the news fragment.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At docs/vars/var-operations.md, line 490:
<comment>Qualify these results with non-empty search arguments: `contains("")`, `startswith("")`, and `endswith("")` are true, while replacing `""` in `""` with `"x"` yields `"x"`. Apply the same qualification to the news fragment.</comment>
<file context>
@@ -487,6 +487,8 @@ def var_string_example():
)
+An optional string var (str | None) holding None acts as an empty string in every string operation: lower, strip and replace give an empty string, contains and startswith are false, length is 0 and split(" ") gives [""], as it does for "".
+
Get Item (Indexing)
</file context>
</details>
Merging this PR will not alter performance1 warning
Comparing Footnotes
|
A var typed plain `str`, the common case, now returns from `_or_empty()`
on an identity check instead of going through `types.is_optional`, which
recovers the cost the guard added to every string operation.
The docs and the news fragment said None acts as "" in every string
operation. Concatenation with `+` and the comparison operators are not
guarded, and a method called with an empty search string gives what it
gives for "" (`contains("")` is true), so both now say that the string
methods give what they give for "" and name what is not covered.
Claude-Session: https://claude.ai/code/session_01LwJUCTCgM7D4Vj6H4dRWss
All Submissions:
Type of change
Changes To Core Features:
Problem
A
str | Nonestate var holdingNonecrashed the page for every string operation that compiles to a native string method:splitwith a separator,strip/lstrip/rstrip,lower,upper,title,capitalize,contains,startswith,endswith,replaceand*all threw, and the whole page fell into the error boundary.length, indexing, slicing andsplit()already readNoneas""through the code point helpers, so the behaviour depended on which operation was used.Fix
This takes option 1 from #7873 (treat
Noneas""everywhere), which is the rule #7800 already set for optional lists and sets.StringVargets the same_or_empty()viewArrayVarhas: when the var type allowsNone, the operation reads(x ?? ""), typed withoutNone. A string that cannot beNonecompiles exactly as before, with no guard. The operations that go through a null-safe helper (pyStrLength,pyStrAt,pyStrSlice,pyStrChars) are left unguarded.Not changed:
+/ f-string concatenation and the comparison operators, which do not throw onnull.Tests
tests/units/reflex_base/vars/test_sequence.py: the exact compiled JS of every guarded operation for an optional and a required string, the result types, and the helper-backed operations that stay unguarded.tests/integration/tests_playwright/test_var_operations.py: the existing app renders the operations on an optional string holdingNoneand on one holding a value. Without the fix the page does not render at all (TypeError: Cannot read properties of nullin the browser).Ran locally:
tests/units(one unrelated browser-launch test inreflex_benchcannot start Chromium in my sandbox), the two string Playwright modules,ruff check,ruff format, andpyrighton the changed files.closes #7873
https://claude.ai/code/session_01LwJUCTCgM7D4Vj6H4dRWss
Generated by Claude Code