Repository navigation
fix(HF-135): enforce array dimensions and spill boundaries - #1774
Tobiadefami wants to merge 2 commits into
Conversation
|
@Tobiadefami thanks for the pull request. No CLA step needed here — our records show you signed the Contributor License Agreement on 2026-07-31. That signature came from our previous signing form and has been carried over, so there is nothing for you to re-sign. |
![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 using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7820d3c. Configure here.
Guard predicted array dimensions before mapping cells and check actual dimensions before VSTACK, HSTACK, TRANSPOSE and inline-array concatenation allocate oversized results. Return #VALUE! when maxRows or maxColumns is exceeded, including for intermediate arrays. Reuse the existing SEQUENCE dimension guards and preserve whole-row/column reference behavior. Also return #SPILL! when valid result dimensions extend beyond the sheet edge from the formula's position. With maxRows set to 4, SEQUENCE(4) in A2 needs A2:A5 and now returns #SPILL!; the same formula still fits at A1. Document both boundary checks.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
hyperformula-docs | e450ecb | Commit Preview URL Branch Preview URL |
Oct 02 2026, 05:56 PM |
7820d3c to
ea75425
Compare
Performance comparison of head (e450ecb) vs base (c920375) |
marcin-kordas-hoc
left a comment
marcin-kordas-hoc
left a comment
There was a problem hiding this comment.
Recommendation: Approve.
I reproduced the remaining scope of GH #1646 on the current base (the SEQUENCE guard itself
already exists on develop; what was missing is the check on intermediate and literal arrays): with
maxRows/maxColumns set to 50/40, =SUM(VSTACK(SEQUENCE(30,1),SEQUENCE(30,1))) silently
computed 930 instead of erroring, and an oversized inline array literal silently spilled past
maxRows instead of erroring — the intermediate array size was never checked when hidden behind a
scalar wrapper or a literal. After this PR, all four cases (direct VSTACK, VSTACK behind SUM,
TRANSPOSE, inline array) correctly return #VALUE!.
I also checked the #SPILL! change independently. The precise behaviour: a SEQUENCE whose spill
range would extend past maxRows/maxColumns now returns #SPILL!, where develop silently
spilled past the limit (maxRows: 4, A2=SEQUENCE(4) → height 5 on develop, #SPILL! here),
while SUM(SEQUENCE(…)) at the same position is unaffected. A spill blocked by a value already
returned #SPILL! on develop and is unchanged.
Measured via MS Graph against a live Excel Online session: Excel returns #SPILL! both for a
spill blocked by a value and for a spill that would run past the sheet edge (SEQUENCE(3) in row
1048575, TRANSPOSE(SEQUENCE(3)) in column XFC), while the scalar SUM(SEQUENCE(3)) is
unaffected — the same three behaviours this PR implements for maxRows/maxColumns. Full engine
- paired test suite passes (503 suites, 6275 tests, 3 skipped — identical to this PR's CI run).
I checked this against the open thread on #1734 (finite-height staleness breaking column spills
on CHOOSECOLS) since the areas are adjacent. The two do overlap in one place — this PR adds a
range.exceedsSheetSizeLimits(...) check inside DependencyGraph.isThereSpaceForArray(), which
takes an ArrayFormulaVertex — but that is a sheet-size cap, not the finite-height snapshotting
the #1734 bug lives in; choosecolsArraySize, effectiveHeight and ArrayValue.resize are all
untouched here. I merged the two branches locally to confirm the practical answer: the #1734 crash
reproduces identically before and after, same-sheet and cross-sheet. So this PR neither resolves
nor worsens it — that thread stays open and needs its own fix.
marcin-kordas-hoc
left a comment
marcin-kordas-hoc
left a comment
There was a problem hiding this comment.
My approval stands. Three non-blocking follow-ups from a second pass, all measured on ea75425. I also filed a pre-existing crash I hit while probing as #1780: it is not caused by this PR, and this PR turns part of it into #VALUE!. In the tests PR (hyperformula-tests#57), 24 of 42 tests fail without this fix and all pass with it.
| @@ -468,7 +470,7 @@ export class DependencyGraph { | |||
|
|
|||
| public isThereSpaceForArray(arrayVertex: ArrayFormulaVertex): boolean { | |||
| const range = arrayVertex.getRangeOrUndef() | |||
There was a problem hiding this comment.
Not a blocker, but worth a follow-up ticket before this ships. Once isThereSpaceForArray returns false here, setNoSpace() replaces the vertex's size with ArraySize.error() (1×1, isRef), and Evaluator.recomputeFormulaVertexValue then short-circuits to setNoSpace() on every later recompute. So a #SPILL! produced by this new branch is permanent until the formula is re-entered or the engine is rebuilt.
Example: with maxRows: 4, =SEQUENCE(4) in A2 is #SPILL!. removeRows(0, [0, 1]) moves it to A1, where it fits, and it stays #SPILL!. The same happens after undo/redo, removeColumns, moveRows, or a dependency change. Excel Online re-spills in that case: deleting row 1 above a #SPILL! SEQUENCE(3) at row 1048575 gives 1,2,3.
The mechanism is pre-existing: a value-blocked array on develop does not re-spill when the blocker is cleared either. But before this change the only way into that state was a collision with a value; now any array near the bottom or right edge gets there. The paired tests only cover recovery by re-entering the formula, updateConfig, or undo of the formula edit, so nothing in the suite observes this. I'd suggest tracking "re-evaluate space for arrays in #SPILL! state after structural changes" as its own task and linking it from the docs paragraph added here.
There was a problem hiding this comment.
Filed as #1785, and the Arrays guide links to it in e450ecb20. Before filing I reproduced every case you listed on this branch, with one refinement: redo of the removeRows stays stuck, but undoing the write that blocked the spill recovers. I also confirmed the value-blocked case on develop, and that Excel Online re-spills in all three setups (rows deleted above, a column deleted to the left, a blocking value cleared).
One correction to the scope: the tests#57 undo and dependency-change tests check that the error stays when the array still doesn't fit. Recovery is only covered via editing the formula and updateConfig. The issue says so.
|
|
||
| ## Array rules | ||
|
|
||
| ### Array size limits |
There was a problem hiding this comment.
DEV_DOCS.md asks that deviations from Excel be recorded in docs/guide/list-of-differences.md, and this paragraph introduces one. Measured in Excel Online: only rows are capped for in-memory arrays (=SUM(VSTACK(SEQUENCE(1048576),SEQUENCE(1))) → #NUM!); columns are not (=SUM(HSTACK(SEQUENCE(1,16384),SEQUENCE(1,1))) → 134225921, and an over-wide array only fails when it spills, with #SPILL!). HyperFormula caps both dimensions and returns #VALUE!. Capping both is a reasonable safety choice. Could you add a row for it? For example: array dimensions vs. maxRows/maxColumns — HyperFormula: #VALUE! for either dimension, including intermediates; Excel: #NUM! for rows only.
There was a problem hiding this comment.
Added an "Array size limits" row to list-of-differences.md in e450ecb20, using your example:
=SUM(VSTACK(SEQUENCE(1048576), SEQUENCE(1))) |
|
|---|---|
| HyperFormula (default limits) | #VALUE! |
| Excel Online | #NUM! |
| Google Sheets | 549756338177 |
The Excel column says rows are capped for in-memory arrays but columns aren't, so an over-wide array fails only when it spills, with #SPILL!. I checked Google Sheets directly: it caps neither dimension (=SUM(HSTACK(SEQUENCE(1,18278),SEQUENCE(1,1))) → 167051782, and even a 20-million-cell =SUM(SEQUENCE(20000000)) computes).
|
|
||
| ### Fixed | ||
|
|
||
| - Fixed array results exceeding the configured row or column limits, including intermediate arrays created by `VSTACK`, `HSTACK`, `TRANSPOSE`, and array literals. Arrays with valid dimensions now return `#SPILL!` when their spill area extends beyond the sheet edge. [#1646](https://github.com/handsontable/hyperformula/issues/1646) [#1774](https://github.com/handsontable/hyperformula/pull/1774) |
There was a problem hiding this comment.
The limits are per dimension by design (the docs say so), so a formula that respects both limits can still exhaust memory. With {maxRows: 20000, maxColumns: 5000}, a single =SUM(SEQUENCE(20000,5000)) (100 M cells) kills a 1 GB Node heap in about 5 s, and at the defaults =SEQUENCE(40000,18278) (731 M cells) passes every guard.
#1646 is titled "OOM crash", and its last maintainer comment sets the bar at "all the other affected functions". If this PR is meant to close it, the description should say that the remaining exposure is the element count and that it is accepted. Otherwise #1646 should stay open for a total-size cap. Either is fine; it just needs to be written down.
There was a problem hiding this comment.
I went with keeping #1646 open. In e450ecb20 the CHANGELOG no longer links #1646, the description says "Part of #1646" and explains why it stays open (=SEQUENCE(40000, 18278) passes every check at the default limits), and the Arrays guide now says an array within both limits can still exhaust memory, using that example.
- list-of-differences: add an "Array size limits" row. HyperFormula caps both dimensions with #VALUE!, Excel caps only rows for in-memory arrays (#NUM!), Google Sheets caps neither (verified in Excel Online and Google Sheets). - Arrays guide: note that a #SPILL! array does not re-spill when space becomes available (#1785), and that per-dimension limits do not cap the total number of cells. - CHANGELOG: drop the #1646 link; this PR is part of #1646, which stays open for a total-size limit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1774 +/- ##
===========================================
+ Coverage 97.32% 97.34% +0.01%
===========================================
Files 195 195
Lines 15739 15758 +19
Branches 3390 3501 +111
===========================================
+ Hits 15318 15339 +21
+ Misses 421 411 -10
- Partials 0 8 +8
🚀 New features to boost your workflow:
|
This branch has not been deployed
3 participants
Context
Task: HF-135. Part of #1646.
SEQUENCEalready returned#VALUE!when its dimensions exceedmaxRows/maxColumns, but other array-producing paths did not check their size. Oversized intermediate arrays (e.g.=SUM(VSTACK(SEQUENCE(30,1),SEQUENCE(30,1)))withmaxRows: 50) were computed silently, and oversized array literals spilled pastmaxRows.This PR:
Checks predicted array dimensions before mapping cells, and actual dimensions before
VSTACK,HSTACK,TRANSPOSEand array-literal concatenation allocate their results. An array that exceedsmaxRowsormaxColumnsreturns#VALUE!(Value too large.), including intermediate arrays. Whole-row/column references keep their existing behavior.Returns
#SPILL!when an array with valid dimensions would extend past the sheet edge from the formula's position. WithmaxRows: 4,=SEQUENCE(4)fits at A1 but returns#SPILL!at A2. Excel does the same at its sheet edge (=SEQUENCE(3)in row 1048575 →#SPILL!).Documents both checks in the Arrays guide, and records the difference from Excel and Google Sheets in
list-of-differences.md:=SUM(VSTACK(SEQUENCE(1048576), SEQUENCE(1)))#VALUE!#NUM!(rows are capped for in-memory arrays, columns are not)549756338177(in-memory arrays are not capped)Why this does not close #1646. The limits apply to each dimension separately, so an array within both limits can still exhaust memory: at the default limits,
=SEQUENCE(40000, 18278)(over 731 million cells) passes every check. #1646 stays open for a limit on the total number of cells. The Arrays guide now says so.Known limitation, tracked separately: an array formula that returns
#SPILL!does not spill again when space becomes available (rows removed above it, a blocking value cleared). This is pre-existing for value-blocked arrays; this PR adds a second way into that state (the sheet edge). See #1785, linked from the Arrays guide.How did you test your changes?
Types of changes
Related issues:
Checklist:
Note
Medium Risk
Changes array evaluation and spill error behavior across the interpreter and dependency graph; formulas that previously produced or consumed oversized intermediate arrays will now error instead of computing silently.
Overview
Enforces
maxRowsandmaxColumnson every array result, including intermediates fromVSTACK,HSTACK,TRANSPOSE, inline array literals, and size prediction before the dependency graph maps spill cells. Oversized dimensions now return#VALUE!(Value too large.) instead of being built or spilled past limits; wrapping inSUMorARRAY_CONSTRAINno longer bypasses this.Arrays whose size is within config limits but whose spill rectangle would extend past the sheet edge (given the formula position) return
#SPILL!, withDependencyGraph.isThereSpaceForArrayalso treating over-limit predicted ranges as having no spill space. Shared sizing logic lives onArraySize.exceedsSheetSizeLimits.Documentation adds an Array size limits section in the arrays guide, a compatibility row in list-of-differences, and a CHANGELOG entry; per-dimension limits still allow very large total cell counts (noted as out of scope for this fix).
Reviewed by Cursor Bugbot for commit e450ecb. Bugbot is set up for automated code reviews on this repo. Configure here.