revive icon indicating copy to clipboard operation
revive copied to clipboard

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

Open mx-psi opened this issue 2 years ago • 5 comments

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
  1. I changed the line endings to be CRLF (e.g. one can run sed -e $'s/$/\r/' main.go > main_crlf.go)
  2. 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).
  3. 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):

  • OS: macOS (doesn't really matter, this is not OS-specific)
  • Version of Go: 1.17.2

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.

mx-psi avatar Oct 25 '21 15:10 mx-psi

revive relies on the GO parser

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

chavacava avatar Oct 26 '21 18:10 chavacava

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.

mx-psi avatar Oct 27 '21 08:10 mx-psi

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

mgechev avatar Aug 07 '22 03:08 mgechev

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.

mx-psi avatar Aug 08 '22 07:08 mx-psi