HF-131 (2/7): the origin/propagation mechanism, plus the same guarantee outside the interpreter - #1762
Open
marcin-kordas-hoc wants to merge 6 commits into
Conversation
Reading another cell's value funnels through DependencyGraph.getCellValue, so that is where an error now gets marked as propagated -- one choke point instead of a marking at each interpreter case, which is what let three read paths (named expressions, range reads via SimpleRangeValue, and getScalarValue's aggregation callers) slip through an earlier attempt. getScalarValue delegates to getCellValue rather than reading the address mapping itself, so its callers inherit the mark instead of bypassing it. wrapperForRootVertex then skips a propagated error, so the reading cell is no longer attached as its root. A vertex holding a STATIC error has no FormulaVertex to serve as a lazily-resolved root, so getCellValue stamps the address it already has; CellError is immutable, so this copies rather than mutating what is stored, which is what keeps the address correct across row and column changes. evaluateAst's overflow guard now assigns instead of returning early, so a NaN/Infinity result reaches the same postprocessing as every other value and gets a root like anything else -- previously =SQRT(-1) had neither. Measured on this branch: A1='=1+' reports Sheet1!A1 where it reported no address; B1='=A1' reports Sheet1!A1 where it reported its own address; the same holds two hops out, through a range, and through a named expression; it follows the cell across addRows; and =1/0 with a propagating SUM still reports A1. Full private suite: 5 failed / 6157 passed, byte-identical to this worktree's baseline (those 5 are a pre-existing branch-pinning mismatch in the test repo). tsc and eslint clean. Also consolidates the toEqualError matcher. It existed twice, once per runner, with hand-copied logic and two strip lists that could drift -- and had drifted in a way that made a change appear inert under Jest. The decision now lives in one shared module both wrappers call with their own equality function, so there is a single strip list to extend. Verified by short-circuiting the shared module: 30 tests went red and came back green, proving Jest really reads it. The Karma half could not be executed here (no browser in this environment). NOT yet observable through the public API: originFunction and argumentIndex are carried on CellError and stamped in evaluateAst, but DetailedCellError does not expose them yet. Exposing them is one line plus a test migration -- 10 assertions in error-address-preservation.spec.ts, arrays.spec.ts and matrix-plugin.spec.ts use plain toEqual, which no matcher edit reaches, and they must NOT be moved to toEqualError because it also strips address, which is exactly what those tests assert. That split is the natural PR boundary here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Task 2 of the HF-131 explainability plan: the 19 CellError construction sites outside src/interpreter/ (17) plus src/Evaluator.ts's two explicit-undefined CYCLE sites that passed no message at all now do, mirroring what Task 1 already did for src/interpreter/. Seven new catalogue constants cover semantically distinct groups rather than one generic REF/NAME sentence: a removed-reference in an existing formula (Transformer.ts, 4 sites), an array's source cell being overwritten (DependencyGraph.ts), a position outside an array's own computed bounds (FormulaVertex.ts), an unresolved parser reference (FormulaParser.ts, 6 REF sites), a reference exceeding sheet size limits (FormulaParser.ts, 4 NAME sites), an error value typed directly into a formula (FormulaParser.ts, 1 site), and a circular reference (Evaluator.ts, 2 CYCLE sites). No error type changed, no function signature changed, i18n/Cell.ts/ CellValue.ts/Exporter.ts/the DependencyGraph choke point untouched. Originally attempted via prep-ship's autonomous pipeline; the run crashed twice with an empty error at its own 'authoring AC specs' step (unrelated to this diff — see the private-test patch README for the diagnosis). The production src/ implementation it left behind was verified correct against every constraint in the task file and kept; the pre-existing private-test breakage it hadn't yet reached was fixed by hand. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Enforces the guarantee this PR's message-filling work establishes: a message-less new CellError(...) construction under src/interpreter/ is now a lint error. Two selectors — a bare one-argument call, and an explicit literal undefined as the second argument (the exact shape the two CYCLE sites in src/Evaluator.ts still use; Evaluator.ts is outside src/interpreter/ so this rule doesn't reach them yet, and is unaffected by this commit). Verified: 0 lint errors on the full src/ tree (same pre-existing warning count as before this commit — no new warnings). The rule was proven to actually fire, not just parse, by temporarily reverting one src/interpreter/ plugin site to a bare 'new CellError(ErrorType.NUM)', observing the expected lint error, then restoring it (git diff empty afterward). Widened to all of src/ in the next PR in this stack (feat/hf-131-error-messages-outside-interpreter). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Changes the override's files glob from src/interpreter/ to all of src/, so the guard this PR's own message-filling work (and the earlier commit's interpreter-only rule) establishes covers the whole engine, not just the interpreter plugins. Documents the one call site the rule structurally cannot catch: CellContentParser.ts's CellContent.Error constructor keeps message optional on purpose (its one no-message caller — a user typing e.g. #REF! literally into a cell — has no engine-side cause to state), and the call 'new CellError(errorType, message)' is syntactically a two-argument construction that passes the rule cleanly regardless of what the caller actually passes. Verified: 0 lint errors on the full src/ tree (same pre-existing warning count as before this commit). The widened rule was proven to actually reach files outside src/interpreter/ by temporarily reverting one CYCLE site in src/Evaluator.ts to a bare 'new CellError(ErrorType.CYCLE)', observing the expected lint error, then restoring it (git diff empty afterward). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
|
Task linked: HF-131 Verbose and actionable formula error messages |
This was referenced Sep 10, 2026
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
|
||
| ### Fixed | ||
|
|
||
| - A cell error read from another cell no longer reports the reading cell as its origin. Errors that come from a cell holding a static error value — a formula with a syntax error, or an error value entered directly — now report that cell's address instead of the address of whichever cell happened to read them first. [#131](https://github.com/handsontable/hyperformula/issues/131) |
There was a problem hiding this comment.
Changelog bullets lack PR links
Low Severity
The new Unreleased bullets end with an issue link to #131 rather than a pull-request URL. Engine changelog entries need a [#NNNN](https://github.com/handsontable/hyperformula/pull/NNNN) link; an issue-only reference is not a substitute.
Triggered by learned rule: CHANGELOG bullets need a PR link
Reviewed by Cursor Bugbot for commit 9c4015e. Configure here.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
hyperformula-docs | 749044d | Commit Preview URL Branch Preview URL |
Sep 11 2026, 05:36 AM |
…at/hf-131-error-messages-outside-interpreter # Conflicts: # .eslintrc.js
| if (this.originAddress !== undefined) { | ||
| return this | ||
| } | ||
| return new CellError(this.type, this.message, this.root, this.originFunction, this.argumentIndex, this.propagated, address) |
There was a problem hiding this comment.
Static error origin can go stale
Medium Severity
withOriginAddress snapshots a raw SimpleCellAddress onto the copy stored in the reading formula. After rows or columns move that cell, a cached formula value can keep the old coordinates. root.getAddress() stays current; this snapshot does not, so the exported origin can point at the wrong cell.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit f667a23. Configure here.
| address: undefined, | ||
| originFunction: undefined, | ||
| argumentIndex: undefined, | ||
| } |
There was a problem hiding this comment.
Error matcher omits new fields
Low Severity
IGNORED_IN_STRUCTURAL_COMPARE lists originFunction and argumentIndex but not propagated or originAddress. The helper’s own comment says every new CellError field belongs here once; leaving these two in the structural compare will make toEqualError fail on otherwise matching errors.
Reviewed by Cursor Bugbot for commit f667a23. Configure here.
Performance comparison of head (749044d) vs base (0ea5c64) |
…es-outside-interpreter
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 4 total unresolved issues (including 3 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 749044d. Configure here.
| const holdsAStaticError = vertex instanceof ParsingErrorVertex || vertex instanceof ValueCellVertex | ||
| return holdsAStaticError | ||
| ? value.withOriginAddress(address).asPropagated() | ||
| : value.asPropagated() |
There was a problem hiding this comment.
Stale origin address after structural edits
Medium Severity
withOriginAddress snapshots a concrete address, and the dependent formula then caches that stamped CellError. Row and column inserts only lazily rewrite ASTs and do not recompute those values, so DetailedCellError.address for a static error (typed value or parse error) can keep pointing at the pre-shift cell.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 749044d. Configure here.
marcin-kordas-hoc
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/hf-131-error-messages #1762 +/- ##
==============================================================
+ Coverage 97.36% 97.38% +0.01%
==============================================================
Files 195 195
Lines 15750 15825 +75
Branches 3461 3491 +30
==============================================================
+ Hits 15335 15411 +76
+ Misses 407 406 -1
Partials 8 8
🚀 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
2 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.
What and why
Stacked on #1761.
Revision note: an earlier version of this description covered only the second half below.
The first half — the origin/propagation mechanism — was always in this PR's actual diff; the
description just didn't say so. #1764's description has been corrected to match (it does not
build the mechanism, only exposes two of its fields publicly).
1. The origin/propagation mechanism.
CellErrorneeds to know, for any error value anywherein a sheet, which function or operator originally produced it, whether it's since been merely
propagated by a cell that read it, and (for static errors with no formula root) which address it
originated at. The hard problem isn't computing that — it's stamping it exactly once, at the
moment of truth, regardless of which of the ~200 call sites across the interpreter and dependency
graph produced or merely relayed the error.
Two choke points, first-wins immutability (the same shape
attachRootVertexalready used forroot):Cell.tsgainswithOrigin/withArgumentIndex/asPropagated/withOriginAddress— each ano-op once the relevant field (or
propagated) is already set, otherwise a fresh immutable copy.Interpreter.ts'sevaluateAststampsoriginFunctionin a single postprocessing tail(
stampOriginForAstNode) that runs after every AST node evaluates — including the NaN-overflowguard's freshly-minted error, and including the fix that a propagated error no longer gets
attachRootVertex'd to whichever cell happened to read it.DependencyGraph.ts'sgetCellValue— the one place every cross-cell read funnels through —marks every error read from another cell as
propagated, and additionally stamps the addressfor a vertex holding a static error (no
FormulaVertexto serve as a root).Exporter.tsfalls back to the stampedoriginAddresswhen there is noroot.Measured:
A1='=1+'reportsSheet1!A1where it reported no address;B1='=A1'reportsSheet1!A1where it reported its own address; the same holds two hops out, through a range, andthrough a named expression; it follows the cell across
addRows;=1/0with a propagatingSUMstill reports
A1.Also consolidates the
toEqualErrormatcher: it existed twice, once per test runner, withhand-copied logic and two strip lists that could drift — and had drifted in a way that made a
change appear inert under Jest. The decision now lives in one shared module both wrappers call
with their own equality function.
Not yet public:
originFunction/argumentIndexare carried onCellErrorand stamped here,but
DetailedCellErrordoes not expose them yet — that's #1764.2. The same guarantee outside the interpreter. Extends #1761's guarantee to the 17
message-less sites (+2 with an explicit
undefined) outsidesrc/interpreter/: the parser's#REF!/#NAME?branches, dependency transformers' 4 reference-removal sites,DependencyGraph'sarray-source-removed site,
FormulaVertex's outside-array-result site, andEvaluator's two#CYCLE!sites.Kuba's scope ruling was all places throwing a cell error, not only
src/interpreter/— these 19sites are outside the spec's original 78-site counter but within that ruling.
Widens the ESLint rule from #1761 to all of
src/, and documents the one call site it structurallycannot catch:
CellContentParser.ts'sCellContent.Errorconstructor keepsmessageoptional onpurpose (its one no-message caller — a user typing e.g.
#REF!literally into a cell — has noengine-side cause to state), and
new CellError(errorType, message)is syntactically a two-argumentcall that passes the rule regardless of what the caller actually passes.
Two new
ErrorMessageconstants for user-supplied/literal error values are deliberately honestrather than invented: an error literal typed into a formula or a value typed directly into a cell
round-trips exactly what the user wrote, so the message says that rather than fabricating an
engine-side cause.
Verified
npx tsc --noEmit— cleannpx eslint src/— 0 errors (same pre-existing warning count)src/interpreter/: a#CYCLE!site insrc/Evaluator.tswas temporarily reverted to a barenew CellError(ErrorType.CYCLE), confirmedto produce the expected lint error, then restored (
git diffclean afterward)pre-existing branch-pinning mismatch as HF-131 (1/7): every interpreter cell error carries a message #1761
=SUM(SQRT(-1))reports
SQRTnotSUM;=1/0then=SUM(A1)reportsdivide,propagated: true; no codepath exists to overwrite a propagated error's address; zero surviving message-less
CellErrorconstruction sites in
src/(338 call sites individually audited)Stack
2 of 7 — stacked on #1761. Next:
feat/hf-131-has-message.🤖 Generated with Claude Code