Repository navigation
HF-131 (2/7): the origin/propagation mechanism, plus the same guarantee outside the interpreter - #1762
Open
marcin-kordas-hoc wants to merge 12 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}
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
hyperformula-docs | c5feea6 | Commit Preview URL Branch Preview URL |
Oct 23 2026, 07:17 AM |
…at/hf-131-error-messages-outside-interpreter # Conflicts: # .eslintrc.js
Performance comparison of head (c5feea6) vs base (7673ddf) |
…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.
marcin-kordas-hoc
changed the title
7 of 12 tasks
A cell that holds a static error - a formula the parser rejected, or an error value entered directly - has no formula vertex to resolve an address through, so DependencyGraph.getCellValue records the address on the error value itself when a dependent reads it. The reading cell caches that value and is not recomputed by a row or column change that merely shifts it, so the recorded address went stale: the dependent kept naming the pre-shift cell, which by then could be empty or hold an unrelated value. The record now carries the LazilyTransformingAstService version it was taken at, and Exporter replays the intervening transformations over it, the same way an error produced by a formula already resolves its address through its root vertex. The replay reuses each transformation's own transformSingleAst with an empty node rather than fixNodeAddress, because MoveCellsTransformer only moves an address that lies inside the range being moved and that decision lives in transformSingleAst. Once a compaction has discarded the transformations the record predates, the address cannot be reconstructed; it is omitted rather than guessed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ages-outside-interpreter # Conflicts: # CHANGELOG.md
8 of 12 tasks
The list's own note says to add every new CellError field here once. This commit adds originAddressVersion, which the address fix introduced, and the two fields that were already missing: propagated and originAddress. All three are recorded by the engine as it reads a value, so an expectation built by hand cannot know them — the same reason root and address are already ignored. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Tobiadefami
reviewed
Sep 21, 2026
Tobiadefami
left a comment
Tobiadefami
left a comment
Collaborator
There was a problem hiding this comment.
Looks good. Verified that propagated errors retain their original source and that static-error addresses follow cell moves and row changes.
…omment without a ticket reference Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ide-interpreter # Conflicts: # CHANGELOG.md # hyperformula/test/_setupFiles/matchers/cellErrorComparison.ts
7 of 12 tasks
…ide-interpreter # Conflicts: # CHANGELOG.md
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/hf-131-error-messages #1762 +/- ##
==============================================================
+ Coverage 97.31% 97.34% +0.02%
==============================================================
Files 207 207
Lines 16229 16322 +93
Branches 3588 3621 +33
==============================================================
+ Hits 15794 15889 +95
+ Misses 427 425 -2
Partials 8 8
🚀 New features to boost your workflow:
|
This branch has not been deployed
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
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.
Context
The PR records where each cell error came from, and gives the remaining cell errors outside the interpreter a message. It is part 2 of 7 of HF-131, stacked on #1761.
src/interpreter/still had no message.hyperformula/src/Cell.ts:CellErrorgainsoriginFunction,argumentIndex,propagated,originAddressandoriginAddressVersion, each set by a first-wins method that returns a new instance.hyperformula/src/interpreter/Interpreter.ts:evaluateAststampsoriginFunctiononce per AST node, and an error read from another cell no longer gets the reading cell as its root.hyperformula/src/DependencyGraph/DependencyGraph.ts:getCellValuemarks every error it reads from another cell as propagated. For a cell that holds a static error, it also records that cell's address and the current transformation version.hyperformula/src/Exporter.ts,hyperformula/src/LazilyTransformingAstService.ts: the recorded address is moved through the row and column changes made since, and omitted once those changes were compacted away.Evaluator.ts(2#CYCLE!),DependencyGraph.ts(1) andFormulaVertex.ts(1): each error gets anErrorMessageconstant. The lint rule from HF-131 (1/7): every interpreter cell error carries a message #1761 now covers all ofsrc/.hyperformula/test/_setupFiles/matchers/cellErrorComparison.ts: the Jest and JasminetoEqualErrormatchers share one comparison.develop)A1: =1+,B1: =A1B1addressSheet1!B1Sheet1!A1A1: #REF!typed,B1: =A1B1addressSheet1!B1Sheet1!A1A1: #REF!typedA1addressSheet1!A1A1: #REF!typed,B1: =A1, then a row added aboveB2addressSheet1!B2Sheet1!A2A1: =B1,B1: =A1A1message''Cell is part of a circular reference.originFunctionandargumentIndexstay internal here; #1764 exposes them.How did you test your changes?
feat/hf-131-error-messages-outside-interpreter, withunit/Cell.spec.tsand new cases inunit/error-address-preservation.spec.tsandunit/parser/parser.spec.ts.src/Evaluator.tsfor a barenew CellError(ErrorType.CYCLE)and for an explicitundefinedmessage.develop),npm run benchmark:write-to-file, three alternating rounds, measured before the merge with the monorepodevelop: total median −0.24%. "Column ranges - add column" was slower in all three rounds, median +3.34% against a 1.91% base spread.Compatibility with Excel and Google Sheets
Not applicable: error types and displayed values do not change. Only
messageandaddressofDetailedCellErrordo.Public API changes
CellError(exported): five new optional constructor parameters, five readonly fields, and four methods:withOrigin,withArgumentIndex,asPropagated,withOriginAddress.DetailedCellError.addressof an error read from a cell that holds a static error is now that cell's address, not the reader's.Out of scope and follow-ups
originFunctionandargumentIndexonDetailedCellError: HF-131 (4/7): expose which function produced an error, and which argument #1764.hyperformula/dev-docs/INTERPRETER.mddoes not describe the first-wins and propagation rules yet.Types of changes
Related issues
Checklist
dev-docs/pages this change affects – or none needed it.