fix(#6040): report Solidity entity lines from the declaration, not the comment above it - #6041
Conversation
Every Solidity entity carried a StartLine that was too small, and import entities carried no line at all. Measured across seven production Solidity repositories: 8078 of 8635 entities reported the wrong line, every error in the same direction, so an entity reported at :200 lands mid-body of some function that precedes it. Shift min +1, median +6, p95 +52, max +316. Four independent causes, and they compound. stripCommentsAndStrings wrote ' ' over every byte of a block comment or string literal including '\n', and findContracts computes every line by counting newlines in that text, so a multi-line comment silently shortened every line below it. NatSpec is ubiquitous in Solidity. Blanking is byte-for-byte, so one post-pass restoring newlines is enough, and it covers any blanking construct added later. The five declaration regexes began (?m)^\s*. \s matches \n, so the match start was dragged back over every blank line and every comment line above the declaration, the first cause having already blanked those comments to whitespace. ^[ \t]* is exactly the set that can precede a declaration on its own line. No CRLF regression: (?m)^ anchors after the \n. Members were anchored on the contract's match start rather than its opening brace, which shifts every member up by the height of the header whenever the inheritance list wraps. They now anchor on lineOf(src, bodyStart-1). buildImportEntities used FindAllStringSubmatch, which yields no offsets, so every import entity reported line 0: 2783 of the 8635. The entity id set is identical before and after, so this is purely positional. line 0 falls from 2783 to 557, and the residual 557 are file-level entities whose name is the path, where 0 is correct. TestSolidity_LineAttribution pins all six entity kinds against a fixture combining a wrapped contract header, multi-line NatSpec, doc comments and a block comment. The package had no StartLine assertion anywhere in its 29 tests, which is how four separate defects survived. Also collapses the four open-coded strings.Count(src[:off], "\n") + 1 expressions in this file onto a lineOf helper, matching the razor, lisp, vue, bicep and deprecation extractors.
Adds buildImportEntities and lineOf to the mapped symbols and the new line-attribution suite to the mapped tests, and states in the registry note that StartLine is the declaration line. Also drops a parenthetical citing a corpus outside this repository. The claim it qualified is unchanged.
|
This is a really good one. Thank you. The part I want to call out first is the newline restoration. Rather than patching each blanking site — the block-comment loop, both string-literal loops, both escape branches — you did one byte-for-byte post-pass over the output. That covers all five paths and anything added later, which is exactly the property you want in a scrubber that other code derives positions from. And then you wrote the invariant into the doc comment ("same byte length AND the same newline positions as src, so an offset into it names the same byte, and the same line"), so the next person knows what they're allowed to rely on instead of having to infer it. That's the difference between a fix and a fix that stays fixed. The diagnosis is precise in a way that made the whole thing easy to follow: three causes, each located exactly, and the observation that they compound — a block comment collapsing to one line, then
Two smaller things I liked: collapsing the four open-coded And thank you for the scope note about Coverage registry and capability map updated again, correctly, without being asked. That keeps being appreciated. Merging as-is. Nothing needed from you. |
Fixes the three line-attribution causes in #6040 plus the missing import line, one change each.
stripCommentsAndStringsrestores newlines after blanking. The blanking pass is byte-for-byte, so a single post-pass over the output covers the block-comment loop, both string-literal loops and their escape branches, and any blanking construct added later. Its doc comment now states the invariant callers rely on: same byte length and same newline positions as the input.The five declaration regexes anchor
^[ \t]*instead of^\s*. That is the set of characters that can precede a declaration on its own line.(?m)^anchors after the\n, so a CRLF\ris always trailing and this is not a CRLF regression.importREis in that set because the next change gives import entities a line, which makes its anchoring load-bearing for the first time.Members anchor on
lineOf(src, bodyStart-1), the opening brace, instead of the contract's match start. That also covers the case where a member declared on the first body line reported the contract's own line, because the member regex could match at offset 0 of the body.buildImportEntitiesusesFindAllStringSubmatchIndexand setsStartLineandEndLine.strings.Count(src[:off], "\n") + 1expressions in this file onto alineOfhelper, matching the razor, lisp, vue, bicep and deprecation extractors.buildImportEntitiesandcollectImportPathsstill run on raw source rather than the scrubbed text, so animportwritten at a line start inside a block comment produces an entity. It previously carried line 0 and now carries a confident line. Fixing that changes entity counts rather than positions, so it is not in this PR.core_extractionregistry note drops a parenthetical that cited a corpus outside this repository. The claim it qualified is unchanged.TestSolidity_LineAttributionpins both imports, the contract, the event, the modifier and the function against a fixture combining a wrapped contract header, multi-line NatSpec, a doc comment and a block comment. Every subtest fails at 18d466f and passes after.go test ./...green (229 packages with tests of 242),go vet ./...andgofmt -l internal/extractors/solidityclean, andtools/coverage validate(0 errors),backfill --checkandfmt --checkexit 0 withgenreproducing the committeddocs/coverageexactly. Coverage matrix updated per AGENTS.md.Closes #6040