fix: parse a keyword list that is a whole inner reference - #32
Open
jafin wants to merge 1 commit into
Open
Conversation
INNER_REFERENCE has two alternatives:
fragment INNER_REFERENCE
: KEYWORD_LIST
| (KEYWORD_LIST SPACED_COMMA)? COLUMN_RANGE
;
ParseIntraTableReference only implemented the second. Each keyword block ended
with an unconditional SkipComma, which assumes a column range follows the
keyword list, so the first alternative walked off the end of the span:
SkipComma stepped past the closing bracket - its Debug.Assert on the comma is
exactly the violated invariant, and compiles out in Release - and
GetStructuredName then indexed input[input.Length].
Every form the grammar allows with a bare keyword list therefore threw
IndexOutOfRangeException rather than parsing: [[#All]], [[#Headers]] and the
two-item lists [[#Headers],[#Data]] and [[#Data],[#Totals]]. The lexer accepts
them all, so this was reachable from any workbook holding one, and it escaped
as a runtime exception rather than as ParsingException.
Each keyword block now checks whether the inner reference is over - only the
SPACED_RBRACKET left - and returns the keyword list with no columns when it is.
A comma is skipped only when one is actually there, so every input that parsed
before takes the same path.
The two InlineData cases marked Skip = "Parser fail" in
FormulaConverterToR1C1Tests are this bug; they are unskipped and pass.
There was a problem hiding this comment.
Pull request overview
Fixes a tokenizer crash when parsing structured references whose [[...]] inner-reference consists solely of a keyword list (e.g. [[#All]], [[#Headers],[#Data]]) by correctly treating the keyword list as a complete inner reference with no column range.
Changes:
- Update
TokenParser.ParseIntraTableReferenceto detect end-of-inner-reference after parsing keyword list items and return withfirstColumn/lastColumn = nullwhen appropriate. - Add/extend lexer and rule-level tests to cover single-keyword and two-keyword-list inner references (including whitespace-padded forms).
- Unskip and extend existing R1C1 conversion round-trip tests for previously failing structured references.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/ClosedXML.Parser/TokenParser.cs | Prevents walking past the end of the intra-table-reference span when the inner reference ends after a keyword list. |
| src/ClosedXML.Parser.Tests/Rules/StructureReferenceRuleTests.cs | Adds AST-shape coverage for keyword-list-only inner references (with/without table name). |
| src/ClosedXML.Parser.Tests/Lexers/IntraTableReferenceTokenTests.cs | Adds tokenization coverage for keyword-list-only inner references and whitespace-padded variants. |
| src/ClosedXML.Parser.Tests/FormulaConverterToR1C1Tests.cs | Unskips previously failing cases and adds round-trip coverage for [[#All]] / [[#Headers]] normalization. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Fixes #31
The grammar has two alternatives; only one was implemented
FormulaLexer.g4:TokenParser.ParseIntraTableReferenceonly handled the second. Each keyword block ended with an unconditionalSkipComma, which assumes a column range follows the keyword list — so the first alternative walked off the end of the span.Tracing
[[#All]](length 8):GetAreareads#All, theni += GetLength(All) + 1→i = 7, andinput[7] == ']'. The reference is over.SkipComma(input, 7)is called anyway. ItsDebug.Assert(input[i] == ',')is precisely the violated invariant, but it compiles out in Release, so it doesi++and returns8 == input.Length.GetStructuredName(input, 8, …)readsinput[startIdx]and throwsIndexOutOfRangeException.[[#Headers],[#Data]]reaches the same place via the second keyword block.Every bare-keyword-list form the grammar allows was affected:
[[#All]]IndexOutOfRangeExceptionAll, no columns[[#Headers]]IndexOutOfRangeExceptionHeaders, no columns[[#Data]]/[[#Totals]]/[[#This Row]]IndexOutOfRangeException[[#Headers],[#Data]]IndexOutOfRangeExceptionHeaders | Data, no columns[[#Data],[#Totals]]IndexOutOfRangeExceptionData | Totals, no columns[ [#All] ]IndexOutOfRangeExceptionAll, no columnsThe lexer accepts all of them —
AssertTokenType(…, INTRA_TABLE_REFERENCE)passes for each — so this was reachable from any workbook holding one, and surfaced as a runtime exception rather than asParsingException.The shorthand
[#All]was never affected: it returns early on theKEYWORDbranch, before this code.The fix
Each keyword block now asks whether the inner reference is over — only the
SPACED_RBRACKETleft — and returns the keyword list with no columns when it is:A comma is skipped only when one is actually there, so every input that parsed before takes exactly the same path as before. The two
ParsingExceptionrejections for the illegal combinations —[[#All],[#Data]]and[[#Headers],[#Totals]], neither of which is a legalKEYWORD_LIST— are untouched and still rejected.Tests
Two existing tests were already parked on this bug.
FormulaConverterToR1C1Tests.StructureReferencecarriedBoth are unskipped here and pass, round-tripping unchanged. Added
[[#All]]and[[#Headers]]alongside them, which normalise to the[#All]/[#Headers]shorthand on the way out.Also added, following each file's existing grammar-comment style:
IntraTableReferenceTokenTests— all five single keywords, both two-item lists, and the whitespace-padded forms. This test asserts the token type first, so it also pins that the lexer accepts them.StructureReferenceRuleTests— the node-level shape, with and without a table name.