Skip to content

HF-131 (5/7): no error reports a cell that did not produce it - #1765

Open
marcin-kordas-hoc wants to merge 11 commits into
feat/hf-131-origin-functionfrom
fix/hf-131-rootless-origin
Open

marcin-kordas-hoc wants to merge 11 commits into
feat/hf-131-origin-functionfrom
fix/hf-131-rootless-origin

Conversation

@marcin-kordas-hoc

@marcin-kordas-hoc marcin-kordas-hoc commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

What and why

Stacked on #1764. Closes the remaining cases where an error could report a cell that did not
produce it — the general case (propagation through a function/operator) is already covered by
the mechanism in #1762; this handles the rest:

  • A parsing error now reports originFunction: 'parser' (ParsingErrorVertex.getCellValue).
  • A value typed directly into a cell (e.g. literal #REF!, no leading =) reports
    originFunction: 'user input'.
  • An error literal written into a formula (e.g. =#REF!) reports originFunction: 'literal'.
  • #SPILL! errors now report the address of the cell that failed to spill, instead of no address
    at all — setNoSpace() now attaches the vertex as root, the same way every other formula result
    does.

Known, documented limit: the six reference-checking branches in the formula parser that build
a #REF!/#NAME? error for a malformed reference (e.g. a range spanning more than one sheet)
still report no originFunction — left unattributed rather than mislabeled as a function that only
read the value. Noted in docs/guide/known-limitations.md.

Verified

  • npx tsc --noEmit — clean
  • npx eslint src/ — 0 errors
  • Full private test suite reproduced against this branch's tip — no regressions beyond the same
    pre-existing branch-pinning mismatch as the rest of this stack
  • Probed directly against the live engine (not just the test suite's own assertions): a formula
    reading another cell's error reports that cell's address, not its own reading cell's, through
    chains up to 3 hops and across sheets; a parsing error's address is preserved through a reader;
    a #SPILL! fixture now reports the failing cell's address where it previously reported none

Stack

5 of 7 — stacked on #1764. Next: fix/hf-131-issue-444-addresses.

🤖 Generated with Claude Code


Note

Medium Risk
Touches core evaluation, parsing, and dependency-graph error paths; behavior of DetailedCellError metadata changes but formula results should stay the same.

Overview
HF-131 (5/7) tightens error origin and address attribution so consumers of DetailedCellError see who produced an error and which cell it belongs to.

Origins at the source: Parsing failures use parser; literal #REF! typed in a cell uses user input; =#REF! in a formula uses literal. Broken or missing references (parser, interpreter, dependency graph) use reference; references invalidated by row/column deletes use removed reference; OFFSET out-of-bounds uses OFFSET. That stops enclosing functions from claiming errors they only read (e.g. SUM on an oversized range).

#SPILL!: setNoSpace() now attaches the formula vertex as root so the spill error reports the cell that could not spill, not a missing address.

Arrays: After evaluation, errors inside ad-hoc array results get stampOriginForArrayElements, so broadcast gaps and per-element failures credit the operator that built the array (e.g. + in INDEX(A1:A2+B1:B3, …)) instead of the function that indexes the array.

CHANGELOG and known-limitations document the origin vocabulary and that #SPILL! / #CYCLE! have no originFunction.

Reviewed by Cursor Bugbot for commit e2ced02. Bugbot is set up for automated code reviews on this repo. Configure here.

Follow-up in this PR: a function no longer claims an error it only read

An error describing a broken reference exists before any function sees it, so the enclosing call is
only a reader. originNameForAstNode returns undefined for reference and error nodes, so such an
error reached the enclosing FUNCTION_CALL unclaimed and non-propagated and the postprocessing tail
handed it over: =SUM(A2:A99999999999) reported SUM, =ABS(A2:A99999999999) reported ABS with
an argument index that belonged to nothing, and =SUM(#REF!) after removing rows reported SUM.

The identity is now attached where the error is built, so the existing first-wins rule blocks the
reader by itself. A reference that cannot be resolved reports reference — whether the parser
rejected it, the sheet is missing, the range spans several sheets, or the name is unknown — and one
destroyed by removing rows or columns reports removed reference. The out-of-sheet error raised
inside the OFFSET heuristic reports OFFSET, because OFFSET really did produce it.

Open question for review: removed reference is a new public vocabulary word, and it is the
naming question raised earlier on this PR. It follows the existing lowercase-prose identities
(parser, user input, literal).

Errors that arise inside an array result are unchanged and still take the reading function's name —
=INDEX(A1:A2+B1:B3, 3, 1) reports INDEX for an #N/A the + operator produced. Covering them
needs either a hook over every element of every ad-hoc array, which changes values in unrelated
specs, or the function name threaded through three methods of the plugin base class. Left out
deliberately and stated in known-limitations.md.

An error read out of a neighbouring cell is not produced by the cell that
read it. This closes the remaining rootless-origin cases the choke-point
redesign's propagated-skip needed but did not itself provide identities for:

- A parsing error is attributed to 'parser' (ParsingErrorVertex.getCellValue).
- An error value typed directly into a cell is attributed to 'user input'
  (CellContentParser's CellContent.Error constructor).
- An error literal written into a formula (e.g. =#VALUE!+1) is attributed to
  'literal' (FormulaParser's ErrorLiteral alternative).
- A #SPILL! error reports the cell that failed to spill, not no address at
  all: setNoSpace() attaches itself as root, since it already has 'this' in
  hand.

getScalarValue's range-to-scalar coercion class was measured, not assumed:
the requesting cell is genuinely the one that made the invalid request, so
'no address' there is honest, not a bug, and is left unchanged.

known-limitations.md gains one bullet: the six buildErrorWithRawInputAst
sites in FormulaParser.ts (malformed references) stay unattributed rather
than mislabeled, pending a future task.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@qunabu

qunabu commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Sep 10, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
hyperformula-docs e2ced02 Commit Preview URL

Branch Preview URL
Sep 15 2026, 08:03 AM

@github-actions

github-actions Bot commented Sep 10, 2026

Copy link
Copy Markdown

Performance comparison of head (e2ced02) vs base (624a8e4)

                                     testName |   base |   head | change
------------------------------------------------------------------------
                                      Sheet A | 265.39 | 268.97 | +1.35%
                                      Sheet B |   87.9 |  90.65 | +3.13%
                                      Sheet T |  76.54 |  77.02 | +0.63%
                                Column ranges | 288.26 |    291 | +0.95%
                                Sorted lookup | 8994.6 |   9200 | +2.28%
Sheet A:  change value, add/remove row/column |   8.36 |   9.18 | +9.81%
 Sheet B: change value, add/remove row/column |   74.4 |  73.26 | -1.53%
                   Column ranges - add column |  85.54 |  87.01 | +1.72%
                Column ranges - without batch |  272.3 | 269.54 | -1.01%
                        Column ranges - batch |  69.29 |  69.76 | +0.68%

@marcin-kordas-hoc

Copy link
Copy Markdown
Collaborator Author

Ran a closed-loop comparison of this stack's origin/address behavior against real Excel via
MS Graph (13 scenarios, full write-up + raw evidence in a private handoff repo — happy to share the
methodology if useful). Everything on the CRUD-address side of this PR checked out — a REF error
from a deleted column reports the same type as Excel and the address stays consistent before/after
the delete.

One thing surfaced that isn't a bug, but is worth a product call: the CRUD-produced #REF! from
Transformer.ts's column/row-removal branches never calls .withOrigin(...), so originFunction
comes back undefined for it — confirmed live, the field is genuinely absent, not empty. That's
consistent with how the mechanism works (an AstNodeType.ERROR node has no entry in
originNameForAstNode, so the stamp is a no-op), but neither this PR's own "deliberate limits"
nor known-limitations.md's existing bullet (which only covers the 6 parser reference-checking
branches) actually says whether this specific case is intentional.

Should a CRUD-removed reference get an originFunction too (e.g. something like 'row/column removed'), or is leaving it unattributed here the right call, same as the parser branches? Either
answer is fine by me — just flagging that it isn't currently written down either way.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

Comment thread CHANGELOG.md Outdated
marcin-kordas-hoc and others added 8 commits September 11, 2026 05:31
An error that describes a broken reference exists before any function sees it, so
the enclosing call is only a reader. originNameForAstNode returns undefined for
reference and error nodes, so such an error reached the enclosing FUNCTION_CALL
unclaimed and non-propagated, and the postprocessing tail handed it to that call:
=SUM(A2:A99999999999) reported SUM, =ABS(A2:A99999999999) reported ABS with an
argument index that belonged to nothing, and =SUM(#REF!) after removing rows
reported SUM.

The identity is now attached where the error is built, which lets the existing
first-wins rule block the reader by itself. A reference that cannot be resolved
reports 'reference', whether the parser rejected it, the sheet is missing, the
range spans several sheets, or the name is unknown; one destroyed by removing rows
or columns reports 'removed reference'; the out-of-sheet error raised inside the
OFFSET heuristic reports 'OFFSET', because OFFSET really did produce it.

Errors that arise inside an array result are unchanged and still take the reading
function's name; that needs either a hook over every ad-hoc array element or the
function name threaded through three methods of the plugin base class, so it is
left out and stated in the known limitations.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…that made it

An error that is one element of a returned array never reached the scalar branch of
evaluateAst's tail, so it arrived at the enclosing call unclaimed and that call took
it: =INDEX(A1:A2+B1:B3, 3, 1) reported INDEX for an #N/A the + operator produced, and
a per-element coercion failure carried an argumentIndex with no originFunction at all,
which the public JSDoc promises never happens.

The tail now stamps the elements of an array the node itself built, which is the same
first-wins rule applied one level down. A range-backed value is skipped: it is a view
over cells whose errors were already marked when they were read, and reading its data
would materialise the whole range on the hot path. That guard is not cosmetic —
without it 'Sheet A' measured +5.7% against a 0.6% base spread; with it, four
alternating rounds against the branch point give -1.09% overall and nothing outside
its own spread.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 2a81d2f. Configure here.

Comment thread src/interpreter/Interpreter.ts
Bugbot caught a site the previous commit missed, in the same family. An array literal
whose rows do not line up builds a #REF! before any call sees it, and originNameForAstNode
had no case for that node, so the enclosing call took it: =SUM({1,2;3}) reported SUM and
=ABS({1,2;3}) reported ABS with an argument index that belonged to nothing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@codecov

codecov Bot commented Sep 15, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.77778% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 97.43%. Comparing base (624a8e4) to head (e2ced02).

Files with missing lines Patch % Lines
src/interpreter/Interpreter.ts 96.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@                       Coverage Diff                       @@
##           feat/hf-131-origin-function    #1765      +/-   ##
===============================================================
+ Coverage                        97.40%   97.43%   +0.02%     
===============================================================
  Files                              195      195              
  Lines                            15846    15862      +16     
  Branches                          3494     3500       +6     
===============================================================
+ Hits                             15435    15455      +20     
+ Misses                             403      399       -4     
  Partials                             8        8              
Files with missing lines Coverage Δ
src/CellContentParser.ts 100.00% <100.00%> (ø)
src/DependencyGraph/DependencyGraph.ts 98.82% <100.00%> (ø)
src/DependencyGraph/FormulaVertex.ts 84.25% <100.00%> (ø)
src/DependencyGraph/ParsingErrorVertex.ts 100.00% <100.00%> (ø)
src/dependencyTransformers/Transformer.ts 98.70% <100.00%> (ø)
src/parser/FormulaParser.ts 98.09% <100.00%> (+0.47%) ⬆️
src/interpreter/Interpreter.ts 96.37% <96.00%> (+0.46%) ⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants