fix: package-comments false positive on CRLF sources - #1763
Open
Eljees wants to merge 1 commit into
Open
Conversation
Author
|
Ping — this one and #1764 have been open since 7 August with no review. Worth flagging in case it is not obvious from your side: no CI has run on either of them. Every check suite on both PRs sits at The two are independent and can be taken in either order. |
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.
fix:
package-commentsfalse positive on CRLF sourcesCloses #607
Motivation
On a file with CRLF line endings,
package-commentsreportspackage comment is detachedfor a package comment that is directlyattached to the
packageclause. The reporter's file from #607:Cause
rule/package_comments.goderives the end of the doc comment fromlastCG.End().ast.Comment.End()isSlash + len(Text), and the scannerstrips carriage returns from
Text, so on CRLF sourcesEnd()points beforethe real end of a block comment — by one byte per stripped
\r(golang/go#41197, which the Go team closed as unfixable in the parser for
compatibility reasons).
For the file above the comment really ends on line 4, but
End()resolves toline 3, so
endPos.Line+1 < pkgPos.Line(4 < 5) holds and the rule fires.The same drift also mis-anchors genuine findings: with CRLF the failure is
reported one line above the blank line it is meant to point at.
Fix
Compute the end line from the comment's own start position plus the number of
line breaks the comment contains. That is exact by construction and does not
depend on line endings at all:
No new dependency, no line-ending detection, no heuristic.
Tests
testdata/package_comments/holds three CRLF fixtures, pinned to CRLF by alocal
.gitattributes(*.go text eol=crlf) so they stay CRLF on the Linux CIcheckout too:
issue607_not_match.goissue607_match.goissue607_drift_not_match.goBefore the change all three fail (two false positives and one wrong anchor);
after it
go test ./...is green, andrevive --config revive.tomlandgolangci-lint runare clean.Note on the earlier attempt
#1247 tried
endLine + 1whenever the file ends with CRLF and was withdrawn.The drift is not a constant: it equals the number of carriage returns stripped
from the comment, so a
+1correction still missesissue607_drift_not_match.go, and it flips the existingtestdata/golint/package_doc5.goexpectation to a false negative whenever thatfixture is checked out with CRLF (which is what happens on Windows with
core.autocrlf=true). The computation above has neither problem.