-
Notifications
You must be signed in to change notification settings - Fork 171
Fix: parser rejects formulas with Excel's internal prefixes (#1655) #1771
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
a697a2a
d7f8e4a
fbf7306
a218523
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ import {ParserConfig} from './ParserConfig' | |
| import { | ||
| ALL_WHITESPACE_PATTERN, | ||
| COLUMN_REFERENCE_PATTERN, | ||
| EXCEL_INTERNAL_FUNCTION_PREFIX_PATTERN, | ||
| NON_RESERVED_CHARACTER_PATTERN, | ||
| ODFF_WHITESPACE_PATTERN, | ||
| RANGE_OPERATOR, | ||
|
|
@@ -47,7 +48,58 @@ export const RangeSeparator = createToken({ name: 'RangeSeparator', pattern: new | |
| export const ColumnRange = createToken({ name: 'ColumnRange', pattern: new RegExp(`${COLUMN_REFERENCE_PATTERN}${RANGE_OPERATOR}${COLUMN_REFERENCE_PATTERN}`) }) | ||
| export const RowRange = createToken({ name: 'RowRange', pattern: new RegExp(`${ROW_REFERENCE_PATTERN}${RANGE_OPERATOR}${ROW_REFERENCE_PATTERN}`) }) | ||
|
|
||
| export const ProcedureName = createToken({ name: 'ProcedureName', pattern: new RegExp(`([${UNICODE_LETTER_PATTERN}][${NON_RESERVED_CHARACTER_PATTERN}]*)\\(`) }) | ||
| export const ProcedureName = createToken({ name: 'ProcedureName', pattern: new RegExp(`(?:${EXCEL_INTERNAL_FUNCTION_PREFIX_PATTERN})?([${UNICODE_LETTER_PATTERN}][${NON_RESERVED_CHARACTER_PATTERN}]*)\\(`) }) | ||
|
|
||
| const excelInternalFunctionPrefixRegexp = new RegExp(`^(?:${EXCEL_INTERNAL_FUNCTION_PREFIX_PATTERN})`) | ||
| const UNDERSCORE_CHAR_CODE = '_'.charCodeAt(0) | ||
|
|
||
| /** | ||
| * Strips one of Excel's internal function-name prefixes (see EXCEL_INTERNAL_FUNCTION_PREFIX_PATTERN) | ||
| * off the front of a token image, if present. | ||
| * | ||
| * Every prefix starts with `_`, and this function is called on every ProcedureName and | ||
| * OffsetProcedureName token in every parse, so the common case — a name with no prefix at all — takes | ||
| * a plain character check instead of always paying for the regex match-and-fail. | ||
| * | ||
| * @param {string} nameWithoutTrailingParen - a token image with any trailing `(` already removed | ||
| */ | ||
| function stripExcelInternalFunctionPrefix(nameWithoutTrailingParen: string): string { | ||
| if (nameWithoutTrailingParen.charCodeAt(0) !== UNDERSCORE_CHAR_CODE) { | ||
| return nameWithoutTrailingParen | ||
| } | ||
| return nameWithoutTrailingParen.replace(excelInternalFunctionPrefixRegexp, '') | ||
| } | ||
|
|
||
| /** | ||
| * Reads the canonical function name out of a ProcedureName token. | ||
| * | ||
| * The token image spans the whole match, so it carries the trailing opening parenthesis and, for a | ||
| * formula imported from an .xlsx file, one of the prefixes Excel prepends when it serializes a | ||
| * workbook. Dropping both here is what lets `_xlfn.IFS(A1)` resolve to the same function as `IFS(A1)`. | ||
| * | ||
| * The prefix is removed before the name is upper-cased, because the prefixes are matched in lower | ||
| * case only. | ||
| * | ||
| * @param {string} image - image of the ProcedureName token, for example `_xlfn.IFS(` | ||
| * @param {Record<string, string>} functionMapping - maps a translated function name to its canonical English name | ||
| */ | ||
| export function canonicalProcedureNameFromToken(image: string, functionMapping: Record<string, string>): string { | ||
| const procedureName = stripExcelInternalFunctionPrefix(image.slice(0, -1)).toUpperCase() | ||
| return functionMapping[procedureName] ?? procedureName | ||
| } | ||
|
|
||
| /** | ||
| * Strips an Excel internal function-name prefix off an OffsetProcedureName token's image. | ||
| * | ||
| * OffsetProcedureName has no trailing parenthesis to remove (unlike ProcedureName — OFFSET's grammar | ||
| * rule consumes `(` separately) and no translation lookup (the token pattern already embeds the | ||
| * localized OFFSET name), so it needs only the prefix stripped, not the full canonicalization above. | ||
| * | ||
| * @param {string} image - image of the OffsetProcedureName token, for example `_xlfn.OFFSET` | ||
| */ | ||
| export function canonicalOffsetProcedureNameFromToken(image: string): string { | ||
| return stripExcelInternalFunctionPrefix(image) | ||
| } | ||
|
|
||
| const cellReferenceMatcher = new CellReferenceMatcher() | ||
| export const CellReference = createToken({ | ||
|
|
@@ -93,7 +145,15 @@ export const buildLexerConfig = (config: ParserConfig): LexerConfig => { | |
| const ArrayRowSeparator = createToken({name: 'ArrayRowSep', pattern: config.arrayRowSeparator}) | ||
| const ArrayColSeparator = createToken({name: 'ArrayColSep', pattern: config.arrayColumnSeparator}) | ||
| const NumberLiteral = createToken({ name: 'NumberLiteral', pattern: new RegExp(`(([${config.decimalSeparator}]\\d+)|(\\d+([${config.decimalSeparator}]\\d*)?))(e[+-]?\\d+)?`) }) | ||
| const OffsetProcedureName = createToken({ name: 'OffsetProcedureName', pattern: new RegExp(offsetProcedureNameLiteral, 'i') }) | ||
| // OFFSET has its own token because it has its own grammar rule, so it needs the prefix handling of | ||
| // ProcedureName repeated here. The 'i' flag exists for the translated OFFSET name (pre-dates this | ||
| // prefix support) and applies to the whole pattern, so unlike ProcedureName it also makes the prefix | ||
| // case-insensitive: `_XLFN.OFFSET(...)` is accepted here where `_XLFN.SUM(...)` is a parsing error. | ||
| // Left as-is rather than given a dedicated case-sensitive-prefix matcher: OFFSET predates the OOXML | ||
| // cutoff this whole prefix scheme exists for, so Excel can never actually emit a prefixed OFFSET call | ||
| // in any case — the asymmetry has no reachable real input, only hand-typed formulas (see the | ||
| // "an upper-cased prefix is accepted on OFFSET, unlike everywhere else" test). | ||
| const OffsetProcedureName = createToken({ name: 'OffsetProcedureName', pattern: new RegExp(`(?:${EXCEL_INTERNAL_FUNCTION_PREFIX_PATTERN})?${offsetProcedureNameLiteral}`, 'i') }) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This currently breaks an existing named-expression reference: const engine = HyperFormula.buildFromArray([[null]], {
licenseKey: 'gpl-v3',
})
const address = {sheet: 0, row: 0, col: 0}
engine.addNamedExpression('_xlfn.OFFSET_RATE', 42)
engine.setCellContents(address, [['=_xlfn.OFFSET_RATE']])
engine.getCellValue(address) // Expected: 42The regression test passes on base The new pattern recognizes |
||
|
|
||
| let ArgSeparator: TokenType | ||
| let inject: TokenType[] | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we correct the
IFSexample here and in thegetCellFormula()explanation on line 43?The current formula has three arguments:
IFSexpects condition/result pairs, so this returns#N/Awith “Wrong number of arguments.” Could we use:The corresponding
getCellFormula()output would be: