fix(extract): a missing final newline is not a partial parse - #1658
Merged
Conversation
A file that does not end with a newline leaves the grammar's mandatory line terminator MISSING. cbm_collect_error_regions counted that node, so the file was reported parse_partial with the last line as its error range. It is not a miss. The node is ZERO-WIDTH and sits at EOF: the parser consumed no source for it, so by construction nothing was dropped - no construct can live in a zero-byte span - and every real instruction above it parsed normally. Proven by dumping the tree: the reporter's two-line Dockerfile yields (source_file (from_instruction ...) (entrypoint_instruction ...) (MISSING "\n")) with both instructions intact and the MISSING node spanning bytes 73-73. It was never Dockerfile-specific. Stripping the trailing newline from the 156 linkable grammar fixtures flips 13 of them to has_error, and SIX produce regions: dockerfile, tcl, fish, gomod, hyprlang - and makefile, which is a genuinely different case (its ERROR has WIDTH; the recipe really is lost). Worse, the ones that stayed silent did so for no principled reason. ini, fsharp, beancount, requirements, gitignore, sshconfig and kconfig omit the same terminator, but theirs is a HIDDEN node and hidden nodes are invisible to ts_node_child(). Whether a user was told their file was partially parsed came down to whether that grammar's author declared the terminator visible. The cost was not cosmetic: a phantom parse_partial writes a "<project>::missed" shadow row, and until #1609 that row made the project fail cross-repo validation as BOTH source and target. A single absent byte could remove an entire repository from cross-repo intelligence with no error shown anywhere. The suppression is deliberately narrow - zero-width AND at EOF. A MISSING or ERROR node with width still counts even at EOF, and anything before EOF is untouched. Both callers pass the raw root, so one source_len is correct for both; verified rather than assumed, since root is bound once and never reassigned. Reported by @vitaliy-shatskiy, who could not share the original file and instead rebuilt the property from scratch with a byte-exact script - an editor would have silently re-added the newline and hidden it. Their isolation matrix ruled out BOM, CRLF vs LF, exec-form vs shell-form and file length before we looked at it once. Reproduce-first, revert-checked: the Dockerfile and cross-grammar tests fail on the previous tree and pass with the fix; forcing the new predicate to return false brings the identical REDs back. Two guards pin the boundary and hold in both directions - a width-bearing failure at EOF (makefile) and a real mid-file ERROR in a file that ALSO lacks its final newline (built from C_IFDEF_SPLIT, the fixture this suite already proves is flagged). parse_coverage 14, extraction 276, language 217, infrascan 3, grammar_regression 1 - 511 passed, 0 failed. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
LeakSanitizer on CI caught all five new tests leaking their CBMFileResult:
Indirect leak of 24 byte(s) ... ts_tree_new
cbm_extract_file_ex cbm.c:1256
do_extract test_parse_coverage.c:39
test_dockerfile_missing_final_newline_not_flagged_issue1610:272
SUMMARY: AddressSanitizer: 706504 byte(s) leaked in 189 allocation(s)
Every pre-existing test in this suite calls cbm_free_result before PASS; the new
ones did not. The local run could not have found it - LeakSanitizer reports
"detect_leaks is not supported on this platform" on macOS arm64, so this class
of defect is CI-only here.
Each test now captures what it asserts, frees, and only then decides, so the
early-FAIL paths do not leak either. The cross-grammar loop prints its
diagnostic before freeing so the failure message keeps naming the grammar.
While correcting the guard, a first attempt left ASSERT_TRUE(flagged ||
has_ranges || true) in real_error_before_eof_still_flagged - always true, and it
would have silently disarmed the guard that stops the EOF suppression from being
over-broad. Removed. The guard is re-proven binding: forcing
cbm_is_eof_terminator_miss to return true makes EIGHT tests fail, including both
guards, and restoring it returns the suite to green.
parse_coverage 14 passed.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
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
1 participant
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 #1610.
A file that does not end with a newline leaves the grammar's mandatory line terminator MISSING, and we counted that node — so the file was reported
parse_partialwith its last line as the error range.It is not a miss. The node is zero-width and sits at EOF: the parser consumed no source for it, so by construction nothing was dropped — no construct can live in a zero-byte span. Dumping the tree for the reporter's two-line Dockerfile:
Both instructions parsed perfectly.
It was never Dockerfile-specific
Stripping the trailing newline from the 156 linkable grammar fixtures flips 13 to
has_error, and six produce regions:dockerfile,tcl,fish,gomod,hyprlang— andmakefile, which is a genuinely different case (its ERROR has width; the recipe really is lost, so that flag is honest and is deliberately preserved).The grammars that stayed silent did so for no principled reason:
ini,fsharp,beancount,requirements,gitignore,sshconfigandkconfigomit the same terminator, but theirs is a hidden node, invisible tots_node_child(). Whether a user was told their file was partially parsed came down to whether that grammar's author declared the terminator token visible.Why it mattered beyond cosmetics
A phantom
parse_partialwrites a<project>::missedshadow row — and until #1609 that row made the project fail cross-repo validation as both source and target. A single absent byte could remove an entire repository from cross-repo intelligence, with no error shown anywhere.Scope
Deliberately narrow: zero-width AND at EOF. A MISSING or ERROR node with width still counts even at EOF; anything before EOF is untouched. Both callers pass the raw root, so one
source_lenserves both — verified (rootis bound once at cbm.c:1231 and never reassigned) rather than assumed.Verification
Reproduce-first and revert-checked. The Dockerfile and cross-grammar tests fail on the previous tree and pass with the fix; forcing the new predicate to
return falsebrings the identical REDs back. Two guards pin the boundary and hold in both directions — a width-bearing failure at EOF (makefile), and a real mid-file ERROR in a file that also lacks its final newline, built fromC_IFDEF_SPLIT, the fixture this suite already proves is flagged.parse_coverage14 ·extraction276 ·language217 ·infrascan3 ·grammar_regression1 — 511 passed, 0 failed.Root-caused by @vitaliy-shatskiy, who could not share the original file and rebuilt the property from scratch with a byte-exact script — an editor would have silently re-added the newline and hidden it.