mgechev / revive

🔥 ~6x faster, stricter, configurable, extensible, and beautiful drop-in replacement for golint
https://revive.run
MIT License
4.8k stars 279 forks source link

False positive on `package comment is detached` lint on CRLF ended files #607

Open mx-psi opened 2 years ago

mx-psi commented 2 years ago

Describe the bug

revive will incorrectly report package comment is detached on CRLF ended files. This is likely related to golang/go#41197.

To Reproduce Steps to reproduce the behavior:

  1. I updated revive go get -u github.com/mgechev/revive
  2. I created a main.go file with the following content:
    /*
    Package main has a multi-line comment.
    Its line endings have a carriage return.
    */
    package main
  3. I changed the line endings to be CRLF (e.g. one can run sed -e $'s/$/\r/' main.go > main_crlf.go)
  4. I verified that the line endings have been changed (e.g. file main_crlf.go should say ASCII text, with CRLF line terminators and cat -v main_crlf.go should show ^M control codes at the end of lines).
  5. I ran revive main_crlf.go.

Expected behavior

revive should not report anything for that file (i.e. it should have the same behavior as if I skipped steps 3-4).

Logs

revive output is

❯ revive main_crlf.go
main_crlf.go:4:1: package comment is detached; there should be no blank lines between it and the package statement

Desktop (please complete the following information):

Additional context

The Go team deemed the issue above unfixable in the parser side because of backwards-compatibility concerns (see golang/go@a14e7bf6d42d9a8b0d698c0a47422c12e38b3f6c). This does not mean the bug is unfixable on revive, but the solution may be a bit more cumbersome.

chavacava commented 2 years ago

revive relies on the GO parser

https://github.com/mgechev/revive/blob/2c895fb33f8f3bb0008ab96dcd8619b2ec79927d/lint/file.go#L33

mx-psi commented 2 years ago

gopls also relies on the Go parser, they had a similar-ish issue (golang/go#41057) and fixed it (golang/tools@cf7880770c1e80a90b5973b87c1583154b05af5f) while still relying on the Go parser. I was hoping a similar thing could be achieved here.

mgechev commented 2 years ago

Is this still relevant? The issue seems fixed and we're using the latest parser.

mx-psi commented 2 years ago

This can still be reproduced with the latest version (main.go being the file on my original comment):

❯ go install github.com/mgechev/revive@latest
go: downloading github.com/mgechev/revive v1.2.3
go: downloading github.com/chavacava/garif v0.0.0-20220630083739-93517212f375
❯ revive main.go
<no output>
❯ sed -e $'s/$/\r/' main.go > main_crlf.go
❯ file main_crlf.go
main_crlf.go: ASCII text, with CRLF line terminators
❯ revive main_crlf.go
main_crlf.go:4:1: package comment is detached; there should be no blank lines between it and the package statement

The issue upstream was not fixed by fixing the parser, but by applying a workaround on gopls. The solution would be to apply the same workaround here.

WNiels commented 4 months ago

grafik Also getting this bug with revive 1.3.7 and go1.22.3 linux/amd64.