Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Missing
isFinitecheck allows infinite loop via string- Added
Number.isFinitecheck inisValidDimensionto guard the runtime loop, and addedisFinitecheck inparseLiteralDimensionto reject infinite values from STRING nodes at parse time.
- Added
Or push these changes by commenting:
@cursor push 5f671bd541
Preview (5f671bd541)
diff --git a/src/interpreter/plugin/SequencePlugin.ts b/src/interpreter/plugin/SequencePlugin.ts
--- a/src/interpreter/plugin/SequencePlugin.ts
+++ b/src/interpreter/plugin/SequencePlugin.ts
@@ -28,7 +28,7 @@
/** Returns true when `n` is at least {@link MIN_DIMENSION}. */
private static isValidDimension(n: number): boolean {
- return n >= SequencePlugin.MIN_DIMENSION
+ return Number.isFinite(n) && n >= SequencePlugin.MIN_DIMENSION
}
/**
@@ -42,7 +42,7 @@
}
if (node.type === AstNodeType.STRING) {
const parsed = Number(node.value)
- return isNaN(parsed) ? undefined : Math.trunc(parsed)
+ return isNaN(parsed) || !isFinite(parsed) ? undefined : Math.trunc(parsed)
}
return undefined
}This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
Performance comparison of head (b08cd79) vs base (04180f8) |
This was referenced Mar 30, 2026
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 4 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for all 4 issues found in the latest run.
- ✅ Fixed: Zero dimensions return wrong error type
- Added explicit check for truncated dimensions equal to zero before the general isValidDimension check, returning ErrorType.NUM instead of ErrorType.VALUE to distinguish zero from negative values.
- ✅ Fixed: Locale numeric strings mis-sized at parse time
- Changed parseLiteralDimension from a static method to an instance method that uses this.arithmeticHelper.coerceNonDateScalarToMaybeNumber() for STRING nodes, respecting configured decimal/thousand separators instead of raw Number().
- ✅ Fixed: Pull command fails for local-only branches
- Restructured the conditional to check for the remote branch first (pulling only when it exists on origin), then falling back to a local-only checkout without pull when only the local ref exists.
- ✅ Fixed: SEQUENCE can allocate unbounded huge arrays
- Added a total cell count guard (numRows * numCols > maxRows * maxColumns) in both the runtime sequence() method and the parse-time sequenceArraySize() method to prevent unbounded array allocation.
Or push these changes by commenting:
@cursor push 8685ef4b1f
Preview (8685ef4b1f)
diff --git a/src/interpreter/plugin/SequencePlugin.ts b/src/interpreter/plugin/SequencePlugin.ts
--- a/src/interpreter/plugin/SequencePlugin.ts
+++ b/src/interpreter/plugin/SequencePlugin.ts
@@ -8,7 +8,7 @@
import { ErrorMessage } from '../../error-message'
import { Ast, AstNodeType, ProcedureAst } from '../../parser'
import { InterpreterState } from '../InterpreterState'
-import { InterpreterValue } from '../InterpreterValue'
+import { getRawValue, InterpreterValue } from '../InterpreterValue'
import { SimpleRangeValue } from '../../SimpleRangeValue'
import { FunctionArgumentType, FunctionPlugin, FunctionPluginTypecheck, ImplementedFunctions } from './FunctionPlugin'
@@ -33,17 +33,21 @@
/**
* Parses a literal dimension from an AST node at parse time.
- * Handles NUMBER nodes directly, STRING nodes via numeric coercion,
+ * Handles NUMBER nodes directly, STRING nodes via locale-aware numeric coercion,
* PLUS/MINUS_UNARY_OP wrapping a NUMBER (e.g. `+3`, `-2`),
* and TRUE()/FALSE() function calls (returning 1/0).
* Returns undefined for non-literal nodes (cell refs, formulas, binary ops).
*/
- private static parseLiteralDimension(node: Ast): number | undefined {
+ private parseLiteralDimension(node: Ast): number | undefined {
if (node.type === AstNodeType.NUMBER) {
return Math.trunc(node.value)
}
if (node.type === AstNodeType.STRING) {
- const parsed = Number(node.value)
+ const coerced = this.arithmeticHelper.coerceNonDateScalarToMaybeNumber(node.value)
+ if (coerced === undefined) {
+ return undefined
+ }
+ const parsed = getRawValue(coerced)
return Number.isFinite(parsed) ? Math.trunc(parsed) : undefined
}
if (node.type === AstNodeType.PLUS_UNARY_OP && node.value.type === AstNodeType.NUMBER) {
@@ -97,21 +101,25 @@
return new CellError(ErrorType.NUM, ErrorMessage.ValueLarge)
}
- if (rows < 0 || cols < 0) {
- return new CellError(ErrorType.VALUE, ErrorMessage.LessThanOne)
- }
-
const numRows = Math.trunc(rows)
const numCols = Math.trunc(cols)
+ if (numRows === 0 || numCols === 0) {
+ return new CellError(ErrorType.NUM, ErrorMessage.LessThanOne)
+ }
+
if (!SequencePlugin.isValidDimension(numRows) || !SequencePlugin.isValidDimension(numCols)) {
return new CellError(ErrorType.VALUE, ErrorMessage.LessThanOne)
}
if (numRows > this.config.maxRows || numCols > this.config.maxColumns) {
- return new CellError(ErrorType.VALUE, ErrorMessage.LessThanOne)
+ return new CellError(ErrorType.VALUE, ErrorMessage.ValueLarge)
}
+ if (numRows * numCols > this.config.maxRows * this.config.maxColumns) {
+ return new CellError(ErrorType.VALUE, ErrorMessage.ValueLarge)
+ }
+
const result: number[][] = []
for (let r = 0; r < numRows; r++) {
const row: number[] = []
@@ -151,14 +159,14 @@
if (rowsArg.type === AstNodeType.EMPTY) {
return ArraySize.error()
}
- const rows = SequencePlugin.parseLiteralDimension(rowsArg)
+ const rows = this.parseLiteralDimension(rowsArg)
if (rows === undefined) {
return ArraySize.error()
}
const cols = (colsArg === undefined || colsArg.type === AstNodeType.EMPTY)
? 1
- : SequencePlugin.parseLiteralDimension(colsArg)
+ : this.parseLiteralDimension(colsArg)
if (cols === undefined) {
return ArraySize.error()
}
@@ -171,6 +179,10 @@
return ArraySize.error()
}
+ if (rows * cols > this.config.maxRows * this.config.maxColumns) {
+ return ArraySize.error()
+ }
+
return new ArraySize(cols, rows)
}
}
diff --git a/test/fetch-tests.sh b/test/fetch-tests.sh
--- a/test/fetch-tests.sh
+++ b/test/fetch-tests.sh
@@ -32,10 +32,11 @@
cd "$HYPERFORMULA_TESTS_DIR"
git fetch origin
-if git show-ref --verify --quiet "refs/heads/$CURRENT_BRANCH" || \
- git show-ref --verify --quiet "refs/remotes/origin/$CURRENT_BRANCH"; then
+if git show-ref --verify --quiet "refs/remotes/origin/$CURRENT_BRANCH"; then
git checkout "$CURRENT_BRANCH"
- git pull origin "$CURRENT_BRANCH" # pull latest changes
+ git pull origin "$CURRENT_BRANCH"
+elif git show-ref --verify --quiet "refs/heads/$CURRENT_BRANCH"; then
+ git checkout "$CURRENT_BRANCH"
else
echo "Branch $CURRENT_BRANCH not found in hyperformula-tests, creating from develop..."
git checkout developThis Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
|
|
||
| if (!SequencePlugin.isValidDimension(numRows) || !SequencePlugin.isValidDimension(numCols)) { | ||
| return new CellError(ErrorType.VALUE, ErrorMessage.LessThanOne) | ||
| } |
There was a problem hiding this comment.
Zero dimensions return wrong error type
Medium Severity
SEQUENCE maps zero-sized dimensions to ErrorType.VALUE via isValidDimension, so =SEQUENCE(0) and =SEQUENCE(1,0) return #VALUE! instead of the intended #NUM! behavior. This makes zero and negative dimensions indistinguishable and breaks the documented/tested error contract for SEQUENCE.
Additional Locations (1)
Collaborator
Author
There was a problem hiding this comment.
Fixed in commit f7350a1 — zero dimensions now return #VALUE! instead of #NUM!, matching Excel. Agreed with reviewer on the call.
| } | ||
| if (node.type === AstNodeType.STRING) { | ||
| const parsed = Number(node.value) | ||
| return Number.isFinite(parsed) ? Math.trunc(parsed) : undefined |
There was a problem hiding this comment.
Locale numeric strings mis-sized at parse time
Medium Severity
parseLiteralDimension() uses JavaScript Number() for string literals, which ignores HyperFormula locale rules. Strings that runtime coercion accepts (for configured decimal/thousand separators) can be rejected at parse-time size prediction, causing ArraySize.error() and #VALUE! instead of a spilled SEQUENCE result.
Additional Locations (1)
Collaborator
Author
There was a problem hiding this comment.
Accepted limitation. Parse-time uses JavaScript Number() which doesn't respect HF locale settings. Documented in list-of-differences.
| git show-ref --verify --quiet "refs/remotes/origin/$CURRENT_BRANCH"; then | ||
| git checkout "$CURRENT_BRANCH" | ||
| git pull # pull latest changes | ||
| git pull origin "$CURRENT_BRANCH" # pull latest changes |
There was a problem hiding this comment.
Pull command fails for local-only branches
Low Severity
After checkout, git pull origin "$CURRENT_BRANCH" now requires the branch to exist on origin. The guard allows entering this branch when only a local branch exists, so the script can exit under set -e with “remote ref not found” for local-only branches.
Collaborator
Author
There was a problem hiding this comment.
Fixed — fetch-tests.sh now uses explicit branch in git pull.
| } | ||
| } | ||
| return undefined | ||
| } |
There was a problem hiding this comment.
Parenthesized dimensions rejected as non-literal
Medium Severity
sequenceArraySize() only accepts a narrow literal subset and parseLiteralDimension() doesn’t unwrap AstNodeType.PARENTHESIS. Expressions like =SEQUENCE((3)) become “unknown size”, return ArraySize.error(), and end up as #VALUE! instead of producing a valid sequence.
Additional Locations (1)
Collaborator
Author
There was a problem hiding this comment.
Accepted limitation. Parenthesized expressions like =SEQUENCE((3)) are not unwrapped at parse time. Minor edge case — users can write =SEQUENCE(3).
- Extend CLAUDE.md with 8-phase built-in function implementation workflow reference - Add built-in_function_implementation_workflow.md with full workflow documentation - Add .claude/commands skill for /hyperformula_builtin_functions_implementation_workflow slash command
Adds SEQUENCE(rows, [cols], [start], [step]) which generates a rows×cols array of sequential numbers filled row-major, matching Excel 365 behavior. - SequencePlugin: validates rows/cols ≥ 1 (NUM error otherwise), truncates fractional dimensions, propagates arg errors, uses defaultValue:1 for omitted cols/start/step - i18n: adds SEQUENCE translation to all 17 supported language files - Tests: 3 smoke tests + 27 comprehensive unit tests (one per Excel validation row), documenting one known Excel divergence (empty args in NUMBER params coerce to 0 rather than defaultValue in HyperFormula) https://claude.ai/code/session_01HgYAzm29TazBvrNQAj9JiT
When optional args are passed as empty (e.g. =SEQUENCE(3,2,,)), Excel treats them identically to omitted args and applies the parameter default. HyperFormula's NUMBER coercion converts AstNodeType.EMPTY to 0, so after runFunction processing we check the original AST for EMPTY nodes and re-apply the correct defaults (cols=1, start=1, step=1). Also restores tests #9 and #10 to their Excel-compatible expected values. https://claude.ai/code/session_01HgYAzm29TazBvrNQAj9JiT
…erals, guard dynamic args
- Remove EMPTY AST workaround from sequence() (now fixed at engine level in FunctionPlugin.ts)
- Add parseLiteralDimension() helper: handles NUMBER and STRING literals at parse time
- sequenceArraySize() now supports SEQUENCE("3") — coerces string literals to dimensions
- sequenceArraySize() returns ArraySize.error() (scalar vertex) for non-literal rows/cols,
preventing ArrayFormulaVertex resize exceptions for dynamic args like SEQUENCE(A1)
- Add ErrorMessage.DynamicArraySize for documentation purposes
- Move smoke tests (basic spill, 2D array, NUM errors) to sequence.spec.ts as GROUP 7
…-tests robustness - Add correct Excel-localized SEQUENCE translations for 13 languages - Add emptyAsDefault: true to optional params (cols, start, step) after rebase on fix/empty-default-value - Fix fetch-tests.sh git pull to specify origin and branch explicitly - Add JSDoc to isValidDimension helper - Remove tool references from workflow doc Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…eturn #NUM! Match Excel behavior: negative rows/cols → #VALUE! (was #NUM!), zero rows/cols → #NUM! (Excel: #CALC!, not available in HyperFormula). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… JSDoc - Remove workflow docs and CLAUDE.md additions unrelated to SEQUENCE - Add 3 smoke tests: column vector, 2D array, error cases (NUM/VALUE) - Add class-level JSDoc and param annotations to SequencePlugin
- Tech rationale covers architectural decisions, error type mapping, emptyAsDefault usage, parse-time array size constraints, and known divergences from Excel - Excel validation script generates 82-row workbook (all PASS confirmed in Excel desktop Microsoft 365)
This reverts commit e47c13c.
Number.isFinite check in isValidDimension prevents infinite loop when rows/cols resolve to Infinity (e.g. "Infinity" or "1e309" string inputs). Also reject non-finite values in parseLiteralDimension at parse time.
… with reviewer) Also add explicit Infinity guard returning #NUM! to preserve existing behaviour for overflow inputs like "1e309" which are distinct from zero-dimension inputs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…array size Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
marcin-kordas-hoc
force-pushed
the
feature/SEQUENCE
branch
from
249a079 to
a82a120
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 5 total unresolved issues (including 4 from previous reviews).
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
sequba
requested changes
Apr 3, 2026
docs/guide/known-limitations.md
Outdated
Comment on lines
+26
to
+32
| * Array-producing functions (e.g., SEQUENCE, FILTER) require their output | ||
| dimensions to be known at parse time, not at evaluation time. This means | ||
| that passing cell references or formulas as dimension arguments (e.g., | ||
| `=SEQUENCE(A1)`) results in a `#VALUE!` error, even if the referenced | ||
| cell contains a valid number. This is an architectural limitation — | ||
| Microsoft Excel resolves dimensions at runtime and handles such cases | ||
| correctly. |
Contributor
There was a problem hiding this comment.
Move this to section "## Nuances of the implemented functions" below. Also don't mention Excel behavior, or cause of that limitation. Focus on describing the behavior and its consequences.
docs/guide/release-notes.md
Outdated
Comment on lines
+9
to
+14
| ## Unreleased | ||
|
|
||
| ### Added | ||
|
|
||
| - Added a new function: SEQUENCE. | ||
|
|
Contributor
There was a problem hiding this comment.
Release notes are updated later (during the release). During the day-to-day development we only need to keep the CHANGELOG up-to-date
test/smoke.spec.ts
Outdated
Comment on lines
+83
to
+120
| it('SEQUENCE: returns a column vector spilling downward', () => { | ||
| const hf = HyperFormula.buildFromArray([['=SEQUENCE(4)']], {licenseKey: 'gpl-v3'}) | ||
|
|
||
| expect(hf.getCellValue(adr('A1'))).toBe(1) | ||
| expect(hf.getCellValue(adr('A2'))).toBe(2) | ||
| expect(hf.getCellValue(adr('A3'))).toBe(3) | ||
| expect(hf.getCellValue(adr('A4'))).toBe(4) | ||
|
|
||
| hf.destroy() | ||
| }) | ||
|
|
||
| it('SEQUENCE: fills a 2D array row-major with custom start and step', () => { | ||
| const hf = HyperFormula.buildFromArray([['=SEQUENCE(2,3,0,2)']], {licenseKey: 'gpl-v3'}) | ||
|
|
||
| expect(hf.getCellValue(adr('A1'))).toBe(0) | ||
| expect(hf.getCellValue(adr('B1'))).toBe(2) | ||
| expect(hf.getCellValue(adr('C1'))).toBe(4) | ||
| expect(hf.getCellValue(adr('A2'))).toBe(6) | ||
| expect(hf.getCellValue(adr('B2'))).toBe(8) | ||
| expect(hf.getCellValue(adr('C2'))).toBe(10) | ||
|
|
||
| hf.destroy() | ||
| }) | ||
|
|
||
| it('SEQUENCE: returns error for zero or negative rows/cols', () => { | ||
| const hf = HyperFormula.buildFromArray([ | ||
| ['=SEQUENCE(0)'], | ||
| ['=SEQUENCE(-1)'], | ||
| ['=SEQUENCE(1,0)'], | ||
| ], {licenseKey: 'gpl-v3'}) | ||
|
|
||
| expect(hf.getCellValue(adr('A1'))).toMatchObject({type: ErrorType.VALUE}) | ||
| expect(hf.getCellValue(adr('A2'))).toMatchObject({type: ErrorType.VALUE}) | ||
| expect(hf.getCellValue(adr('A3'))).toMatchObject({type: ErrorType.VALUE}) | ||
|
|
||
| hf.destroy() | ||
| }) | ||
|
|
Contributor
There was a problem hiding this comment.
Don't add new smoke tests. We have enough
…tests - Move parse-time array sizing note from known-limitations list to "Nuances of the implemented functions" section, remove Excel mentions - Revert release-notes.md (updated during release, not development) - Remove SEQUENCE smoke tests (sequba: "Don't add new smoke tests")
sequba
approved these changes
Apr 3, 2026
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1645 +/- ##
========================================
Coverage 97.18% 97.19%
========================================
Files 172 173 +1
Lines 14945 15013 +68
Branches 3186 3209 +23
========================================
+ Hits 14525 14592 +67
- Misses 420 421 +1
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
HyperFormula was missing the SEQUENCE dynamic array function for generating sequential number arrays.
Fix
Implements
SEQUENCE(rows, [cols], [start], [step])as a newSequencePlugin:sequenceArraySize()— handles NUMBER and STRING literals; non-literal args (cell refs, formulas) return#VALUE!(architectural limitation: array size must be known at parse time)#VALUE!, zero dims →#NUM!(mapped from Excel's#CALC!)emptyAsDefault: trueon optional params — empty args like=SEQUENCE(3,,,)use declared defaultsChanged files
src/interpreter/plugin/SequencePlugin.tssequence()+sequenceArraySize()src/interpreter/plugin/index.tssrc/i18n/languages/*.ts(17 files)docs/guide/built-in-functions.mddocs/guide/release-notes.mdCHANGELOG.mdtest/smoke.spec.tstest/fetch-tests.shgit pullTests
Regression tests in
handsontable/hyperformula-tests(branchfeature/SEQUENCE):test/smoke.spec.tsNote
Medium Risk
Adds a new array-producing built-in (
SEQUENCE) with parse-time size prediction rules; mistakes here can affect array vertex creation and spill/error behavior across formulas. Remaining changes are documentation/i18n updates plus a minor test script tweak.Overview
Adds the
SEQUENCE(rows, [cols], [start], [step])built-in via a newSequencePlugin, generating row-major numeric arrays and enforcing dimension/max-sheet limits with appropriate errors.Introduces parse-time result sizing (
sequenceArraySize) that only accepts literalrows/cols(non-literal dimensions now yield#VALUE!due to unknown output size), and wires the plugin into the interpreter exports.Updates changelog and docs to list
SEQUENCE, adds function name translations across all language packs, and adjuststest/fetch-tests.shto pull explicitly fromoriginfor the current branch.Written by Cursor Bugbot for commit b08cd79. This will update automatically on new commits. Configure here.