Skip to content

Fix: parser rejects formulas with Excel's internal prefixes (#1655) - #1771

Open
marcin-kordas-hoc wants to merge 4 commits into
developfrom
fix/hf-217-excel-internal-prefixes
Open

marcin-kordas-hoc wants to merge 4 commits into
developfrom
fix/hf-217-excel-internal-prefixes

Conversation

@marcin-kordas-hoc

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

Copy link
Copy Markdown
Collaborator

Problem

The parser requires a function name to start with a letter, so it rejects formulas
that carry the internal prefixes Excel writes into .xlsx for every function added
after ~2007 (_xlfn., _xlws., and combinations) — e.g. _xlfn.IFS(...). Whether a
user hits this depends on their import library: ExcelJS (the library our own
file-import guide uses in its example) passes the
prefixes straight through, and SheetJS strips _xlfn. but not _xlws., so FILTER
and SORT still fail. Registering the prefixed name as a custom function is not a
workaround — the lexer rejects the leading underscore before name resolution runs.

Fixes #1655

Fix

  • The five known prefixes (_xlfn., _xlfn._xlws., _xlws., _xlpm., _xludf.)
    are now an optional part of the ProcedureName and OffsetProcedureName lexer
    tokens, so _xlfn.IFS(...) tokenizes the same as IFS(...).
  • The prefix is stripped in the lexer, not by rewriting the raw formula string.
    A string-level replace (the fix sketched in the issue) also rewrites the prefix
    inside string literals — ="_xlfn.IFS is a literal" would silently become
    ="IFS is a literal". There's a regression test for this.
  • FormulaParser and ParserWithCaching.computeHashFromTokens both derive the
    canonical function name through shared helpers (canonicalProcedureNameFromToken,
    canonicalOffsetProcedureNameFromToken), so a prefixed formula and its unprefixed
    equivalent — OFFSET included — produce the same cache hash.
  • getCellFormula() / getCellSerialized() return the formula without the prefix.

Scope notes:

  • A bare _xlpm.x identifier (a LAMBDA/LET parameter, not a call) is not
    stripped — HyperFormula has no LAMBDA/LET, so such a formula is #NAME?
    either way.
  • Prefixes are matched in lower case only for ProcedureName (every ordinary
    function), which is all Excel ever writes. OffsetProcedureName is the one
    exception — see "Post-review changes" below.
  • A prefix doesn't add a function: _xlfn.CONCAT(...) is #NAME? (CONCAT isn't
    implemented), not a parsing error — strictly better than before, not silently
    swallowed.

Changed files

File Change
src/parser/parser-consts.ts new EXCEL_INTERNAL_FUNCTION_PREFIX_PATTERN
src/parser/LexerConfig.ts ProcedureName/OffsetProcedureName accept an optional prefix; new canonicalProcedureNameFromToken() / canonicalOffsetProcedureNameFromToken() helpers
src/parser/FormulaParser.ts uses the shared helper
src/parser/ParserWithCaching.ts uses the shared helpers for the cache hash, incl. OFFSET
docs/guide/file-import.md documents the prefixes and which import libraries leave them in
CHANGELOG.md Unreleased entry

Tests

Regression tests in handsontable/hyperformula-tests (branch
fix/hf-217-excel-internal-prefixes, 28 tests, all pass):

Group Tests Coverage
Prefix ignored on a call 5 each of the 5 prefixes evaluates like the unprefixed call
Works for every kind of function 5 OFFSET (own grammar rule), SUM (predates prefixes), XLOOKUP (array fn), a custom function, a localized name
Nested / non-leading calls 3 prefix mid-expression, nested argument, every function in the formula prefixed
Stripped from the stored formula 5 getCellFormula, getCellSerialized, shared cache hash (incl. OFFSET, two prefixes)
Left alone / precedence unchanged 4 string literal, string argument, named expression, a named expression shadowed by a call same as without a prefix
Does not over-match 5 unimplemented function is #NAME? not a parse error, upper-cased prefix rejected (except OFFSET, pinned separately), unrelated underscore name, prefix alone
AST shape 2 function-call node type, canonical name
  • Verified against unpatched src/ at each stage: the relevant new tests go red,
    confirming they detect the bug rather than passing regardless.
  • Full private unit suite: 502 suites / 6257 tests, no regressions.
  • npm run test:performance: exit 0. Targeted lexer microbenchmark (parser is on
    the buildFromSheets hot path) shows no measurable regression across every
    round of changes below.

Post-review changes

An exhaustive multi-agent review (5 independent dimensions, each finding
adversarially verified by 3 more agents before being trusted) ran against this PR
after the first green CI, on top of my own manual review. It surfaced 17 raw
findings; 6 survived verification, 3 of which converged independently on the same
root cause. What changed as a result:

  1. CHANGELOG entry linked the issue instead of the PR (Bugbot). Fixed — now
    links Fix: parser rejects formulas with Excel's internal prefixes (#1655) #1771, matching the majority of neighboring Unreleased entries.
  2. _xlfn.OFFSET(...) and OFFSET(...) did not share a cache hash, unlike
    every other function — computeHashFromTokens only canonicalized
    ProcedureName tokens; OffsetProcedureName is a separate, uncategorized token
    type (OFFSET has its own grammar rule) and fell through to the generic
    raw-image branch. Verified empirically before/after with the parser's own
    tokenizeFormula(). Fixed with a dedicated branch + canonicalOffsetProcedureNameFromToken().
  3. canonicalProcedureNameFromToken was measurably slower per call than the
    inline code it replaced
    for the common (no-prefix) case — real but bounded
    (the full-pipeline lexer benchmark never showed it). Added a cheap
    leading-underscore guard before invoking the prefix-stripping regex at all,
    since every prefix starts with _. A reorder alternative suggested by the
    review was measured and found to not help (and to slightly hurt the prefixed
    case), so it was not adopted — verifying a benchmark's own claim mattered here.
  4. OffsetProcedureName's prefix matching is case-insensitive, unlike
    ProcedureName_XLFN.OFFSET(...) is silently accepted where _XLFN.SUM(...)
    is a parse error. Real and now documented and pinned by a test, not fixed in
    code: OFFSET predates the OOXML cutoff this whole prefix scheme exists for, so
    Excel can never actually emit a prefixed OFFSET call in any case — a dedicated
    case-sensitive-prefix matcher would add real lexer complexity for an input
    nothing can produce.
  5. A named expression named e.g. _xlfn.Sum is shadowed by a call to SUM
    the moment it's followed by (. Verified this is pre-existing, general engine
    behavior (an unprefixed named expression named Sum is shadowed identically,
    unrelated to this PR) — this PR only changes what used to be a parse error into
    the same precedence rule every other name already follows. Pinned with a test
    rather than changed.
  6. docs/guide/file-import.md overclaimed _xlpm. handling — it said
    HyperFormula "ignores" the prefix, but the code only strips a prefix in front of
    a function call, and _xlpm. never appears there in a real .xlsx (it always
    prefixes a bare LAMBDA/LET parameter name, which this fix deliberately does
    not touch). Corrected.

Also found and fixed in the same pass: the existing "shares one cache entry" test
tokenized through a separately-built lexer instead of the parser's own
tokenizeFormula() — harmless for ProcedureName (a module-level singleton token
type) but would have silently never caught a bug in OffsetProcedureName (rebuilt
fresh per buildLexerConfig() call, so a second lexer's token fails the identity
check tokenMatcher relies on). Both the existing test and the new ones now
tokenize via the parser's own method.

🤖 Generated with Claude Code


Note

Medium Risk
Changes core formula lexing/parsing on a hot path for every function call, but behavior is additive (ignore known prefixes) with shared canonicalization for caching; main risk is incorrect prefix matching edge cases.

Overview
Fixes formula parsing when workbooks are imported with Excel’s internal function-name prefixes (_xlfn., _xlfn._xlws., _xlws., _xlpm., _xludf.), which commonly appear in .xlsx strings from parsers such as ExcelJS. Formulas like =_xlfn.IFS(...) previously failed at lex time; they now tokenize and evaluate like =IFS(...).

The lexer treats those prefixes as optional on ProcedureName and OffsetProcedureName, strips them via shared canonicalProcedureNameFromToken / canonicalOffsetProcedureNameFromToken helpers (fast path when the name does not start with _), and uses the same canonicalization in ParserWithCaching so prefixed and unprefixed formulas share a cache hash. Prefixes are not rewritten in the raw formula string, so text inside string literals stays unchanged. getCellFormula() is documented to return the unprefixed form; unsupported functions still yield #NAME?, not a parse error.

Docs in file-import.md and CHANGELOG.md describe the prefixes and import-library behavior.

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

Excel marks every function added after the original OOXML specification with an
internal prefix when it serializes a workbook, so an imported file can contain
`_xlfn.IFS(...)` where the user typed `IFS(...)`. The ProcedureName token required
the name to start with a letter, so none of the prefixes matched and the formula
failed with "Parsing error. Redundant input, expecting EOF but found: (".

The prefixes are now an optional part of the ProcedureName and OffsetProcedureName
patterns, and are dropped when the token is turned into a function name. Doing this
in the lexer rather than by rewriting the formula string keeps the prefixes intact
wherever they are not a function name, such as inside a string literal.

FormulaParser and ParserWithCaching derive the name through one shared helper, so a
prefixed formula and its unprefixed equivalent also produce the same cache hash.

Closes #1655

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

cloudflare-workers-and-pages Bot commented Sep 12, 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 a218523 Commit Preview URL

Branch Preview URL
Sep 12 2026, 07:08 PM

@github-actions

github-actions Bot commented Sep 12, 2026

Copy link
Copy Markdown

Performance comparison of head (a218523) vs base (c920375)

                                     testName |    base |    head | change
--------------------------------------------------------------------------
                                      Sheet A |   441.1 |  443.58 | +0.56%
                                      Sheet B |  141.24 |  137.62 | -2.56%
                                      Sheet T |   124.3 |  121.55 | -2.21%
                                Column ranges |  580.67 |  574.86 | -1.00%
                                Sorted lookup | 16814.2 | 17405.5 | +3.52%
Sheet A:  change value, add/remove row/column |   12.56 |      12 | -4.46%
 Sheet B: change value, add/remove row/column |  121.22 |  118.14 | -2.54%
                   Column ranges - add column |  159.17 |  149.82 | -5.87%
                Column ranges - without batch |  493.04 |  480.04 | -2.64%
                        Column ranges - batch |  126.15 |  119.75 | -5.07%

@marcin-kordas-hoc
marcin-kordas-hoc marked this pull request as ready for review September 12, 2026 06:42

@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 a697a2a. Configure here.

Comment thread CHANGELOG.md Outdated
marcin-kordas-hoc and others added 3 commits September 12, 2026 14:23
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
computeHashFromTokens only special-cased ProcedureName tokens; OffsetProcedureName
is a separate, uncategorized token type (OFFSET has its own grammar rule), so it
fell through to the generic branch that hashes the raw token image — meaning
_xlfn.OFFSET(...) and OFFSET(...) did NOT share a cache entry, contradicting this
fix's own stated design goal. Verified empirically before and after (hashes differ
pre-fix, match post-fix) using the parser's own tokenizeFormula(), not a separately
built lexer — a second buildLexerConfig() call creates a distinct OffsetProcedureName
token-type object, so testing this via an external lexer silently never exercises
the real bug (fixed the same latent issue in the existing 'shares one cache entry'
spec test).

Also adds a cheap leading-underscore guard to canonicalProcedureNameFromToken/
canonicalOffsetProcedureNameFromToken before invoking the prefix-stripping regex,
since every prefix starts with '_' and the overwhelming majority of tokens don't
have one. Verified as a real, correctness-preserving improvement for the common
case with a standalone microbenchmark; a reorder alternative suggested by review
was tried and measured to NOT help (and slightly hurt the prefixed case), so it
was not adopted.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- parser-consts.ts / LexerConfig.ts: the 'prefixes are matched in lower case only'
  claim is true for ProcedureName but not for OffsetProcedureName, which reuses the
  prefix pattern inside an already case-insensitive regex (the 'i' flag exists for
  the translated OFFSET name, pre-dating this fix). Replaced 'harmless' with the
  actual asymmetry and why it's not worth a dedicated case-sensitive-prefix matcher:
  OFFSET predates the OOXML cutoff this prefix scheme exists for, so Excel can never
  emit a prefixed OFFSET call in any case — there is no reachable real input, only
  hand-typed formulas.
- docs/guide/file-import.md: corrected the claim that HyperFormula 'ignores' the
  _xlpm. prefix. It only strips a prefix in front of a function call; _xlpm. never
  appears there in a real .xlsx — it always prefixes a bare LAMBDA/LET parameter
  name, which this fix deliberately does not touch (HF has no LAMBDA/LET, so such a
  formula is #NAME? either way).
- LexerConfig.ts: added {type} JSDoc annotations to match the majority convention
  already used elsewhere in src/parser/.

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

codecov Bot commented Sep 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.32%. Comparing base (c920375) to head (a218523).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1771   +/-   ##
========================================
  Coverage    97.32%   97.32%           
========================================
  Files          195      195           
  Lines        15739    15751   +12     
  Branches      3390     3462   +72     
========================================
+ Hits         15318    15330   +12     
+ Misses         421      413    -8     
- Partials         0        8    +8     
Files with missing lines Coverage Δ
src/parser/FormulaParser.ts 97.37% <100.00%> (-0.01%) ⬇️
src/parser/LexerConfig.ts 100.00% <100.00%> (ø)
src/parser/ParserWithCaching.ts 95.08% <100.00%> (+0.02%) ⬆️
src/parser/parser-consts.ts 100.00% <100.00%> (ø)

... and 5 files 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.

@GreenFlux GreenFlux left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, thanks for the quick response!

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