Repository navigation
fix(vars): compare array vars element-wise with Python semantics - #7656
anishgurjar wants to merge 1 commit into
Conversation
Array comparison operations (<, <=, >, >=) compiled to direct JS array comparison, which coerces arrays to comma-joined strings, so [1, 10, 0] < [1, 9, 0] evaluated true while Python compares element-wise and returns false. Compile the four ordering operations through a new compareArrays frontend helper that mirrors Python list semantics: pairwise element comparison, shorter-prefix ordering, recursive nested arrays, booleans as numbers (bool subclasses int), and NaN for NaN operands or incomparable element types so every ordering result is false. Refs reflex-dev#7654
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue 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/.templates/web/utils/helpers/array.js">
<violation number="1" location="packages/reflex-base/src/reflex_base/.templates/web/utils/helpers/array.js:35">
P2: This numeric path does not preserve Python’s arbitrary-precision integer ordering: values above `Number.MAX_SAFE_INTEGER` have already collapsed to the same JavaScript `Number` before `compareArrays` compares them. Preserve integer precision in the frontend representation or explicitly reject/document this unsupported range so the helper does not claim Python list semantics for it.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
| const rightIsNumeric = | ||
| typeof right === "number" || typeof right === "boolean"; | ||
| if (leftIsNumeric && rightIsNumeric) { | ||
| const leftNumber = Number(left); |
There was a problem hiding this comment.
P2: This numeric path does not preserve Python’s arbitrary-precision integer ordering: values above Number.MAX_SAFE_INTEGER have already collapsed to the same JavaScript Number before compareArrays compares them. Preserve integer precision in the frontend representation or explicitly reject/document this unsupported range so the helper does not claim Python list semantics for it.
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/.templates/web/utils/helpers/array.js, line 35:
<comment>This numeric path does not preserve Python’s arbitrary-precision integer ordering: values above `Number.MAX_SAFE_INTEGER` have already collapsed to the same JavaScript `Number` before `compareArrays` compares them. Preserve integer precision in the frontend representation or explicitly reject/document this unsupported range so the helper does not claim Python list semantics for it.</comment>
<file context>
@@ -0,0 +1,51 @@
+ const rightIsNumeric =
+ typeof right === "number" || typeof right === "boolean";
+ if (leftIsNumeric && rightIsNumeric) {
+ const leftNumber = Number(left);
+ const rightNumber = Number(right);
+ if (Number.isNaN(leftNumber) || Number.isNaN(rightNumber)) {
</file context>
 [Medium impact] The change affects browser comparisons for Reflex array-ordering expressions. The comparison change appears sound within its stated scope, but the repository’s release-note and test requirements must be satisfied before merging. FindingsSummaryArray ordering now calls
Intentional limits supplied by Reviews (1) · Last reviewed commit: "fix(vars): compare array vars element-wi..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return var_operation_return( | ||
| js_expression=f"(compareArrays({lhs!s}, {rhs!s}) < 0)", | ||
| var_type=bool, | ||
| var_data=VarData(imports=_ARRAY_COMPARE_IMPORT), |
There was a problem hiding this comment.
This user-facing array ordering change has no bugfix fragment under packages/reflex-base/news/. CLAUDE.md requires a fragment for each affected package. Add a short entry explaining that arrays now compare element by element instead of as comma-joined strings. This repository requirement must be satisfied before merging.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| (op.ge, ">="), | ||
| ], | ||
| ) | ||
| def test_array_comparison_uses_compare_arrays(operation, operator): |
There was a problem hiding this comment.
The new array comparison tests cover reflex_base.vars.sequence but sit in the top-level test_var.py. CLAUDE.md requires subpackage unit tests to include the corresponding package path. Move these tests under tests/units/reflex_base/vars/ before merging to satisfy that requirement.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| (op.ge, ">="), | ||
| ], | ||
| ) | ||
| def test_array_comparison_uses_compare_arrays(operation, operator): |
There was a problem hiding this comment.
Neither test_array_comparison_uses_compare_arrays nor test_array_comparison_imports_compare_arrays has a docstring. CLAUDE.md requires Google-style docstrings on all functions. Add a one-line summary to each test before merging to satisfy that requirement.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Fixes #7654
Problem
Array ordering comparisons (
<,<=,>,>=) on array vars compile to a direct JS comparison between the two arrays. JS does not compare arrays element-wise; it coerces both sides to comma-joined strings, so[1, 10, 0] < [1, 9, 0]evaluates"1,10,0" < "1,9,0"and returnstrue, where Python compares element-wise and returnsFalse.Fix
The four array ordering operations now compile through a new
compareArraysfrontend helper ($/utils/helpers/array.js, same pattern ascompareDatetime) that mirrors Python list comparison:boolsubclassesint([True] == [1],[True] < [2])NaNoperand pair yieldsNaN, matching Python where every ordering comparison againstfloat("nan")is falseTypeError) also yieldsNaN, so every ordering result is false instead of a silent string comparisonEquality (
==/!=) is unchanged and keeps its existing comparison path.Scope and remaining divergences
This covers scalar (number/string/bool) and nested-array elements, which is what serialized array vars contain in practice. Remaining known divergences from Python, unchanged by this PR:
Tests
tests/units/test_var.py: JS emission for all four operators plus thecompareArraysimport wiringtests/units/vars/array_helpers.test.mjs: runtime suites against the real template module (numeric ordering, prefix rule, strings/booleans, nested arrays,NaNoperands, bool/number mixing, incomparable types)tests/units/vars/test_array_helpers_template.py: pytest wrapper that executes the runtime suites (skips when Node.js is unavailable)Test limits:
ruff check,ruff format, codespell, biome, pyright on the touched files, the touched pytest modules, andnode --testwere run directly and pass; the uv-managed pre-commit env hooks and full CI have not run on this branch.@masenf - following the
compareDatetimeprecedent here; would appreciate a look when you have a moment.