Adding import "C" makes warnings for some linters to go away

[kolyshkin]

Welcome

• Yes, I'm using a binary release within 2 latest major releases. Only such installations are supported.
• Yes, I've searched similar issues on GitHub and didn't find any.
• Yes, I've included all information below (version, config, etc).
• Yes, I've tried with the standalone linter if available. (https://golangci-lint.run/usage/linters/)

Description of the problem

This is a five star bug.

Essentially, it looks like having import "C" in the source file makes all the format linters (gofmt, gofumpt, goimports) to stop seeing formatting issues.

Here's a quick repro using the golangci-lint repo.

[kir@kir-rhat golangci-lint]$ pwd; git status; git show | head -2
/home/kir/go/src/github.com/golangci/golangci-lint
On branch master
Your branch is up to date with 'origin/master'.

nothing to commit, working tree clean
commit c531fc2ad559624436ff66ca77aa306f47c47442
Author: Marat Reymers <[email protected]>

[kir@kir-rhat golangci-lint]$ golangci-lint run --no-config --disable-all --enable gofmt test/testdata/cgo/main.go 
[kir@kir-rhat golangci-lint]$ echo $?
0

[kir@kir-rhat golangci-lint]$ echo >> test/testdata/cgo/main.go
[kir@kir-rhat golangci-lint]$ git diff
diff --git a/test/testdata/cgo/main.go b/test/testdata/cgo/main.go
index cbb692fe..4df7d95d 100644
--- a/test/testdata/cgo/main.go
+++ b/test/testdata/cgo/main.go
@@ -19,3 +19,4 @@ func Example() {
        C.myprint(cs)
        C.free(unsafe.Pointer(cs))
 }
+

[kir@kir-rhat golangci-lint]$ gofmt -d test/testdata/cgo/main.go
diff -u test/testdata/cgo/main.go.orig test/testdata/cgo/main.go
--- test/testdata/cgo/main.go.orig	2022-06-06 19:18:55.848930364 -0700
+++ test/testdata/cgo/main.go	2022-06-06 19:18:55.848930364 -0700
@@ -19,4 +19,3 @@
 	C.myprint(cs)
 	C.free(unsafe.Pointer(cs))
 }
-

[kir@kir-rhat golangci-lint]$ golangci-lint run --no-config --disable-all --enable gofmt test/testdata/cgo/main.go 
[kir@kir-rhat golangci-lint]$ echo $?
0

If we try doing the same thing with the file without import "C", it all works as expected:

[kir@kir-rhat golangci-lint]$ golangci-lint run --no-config --disable-all --enable gofmt test/testdata/godot.go 
[kir@kir-rhat golangci-lint]$ echo $?
0
[kir@kir-rhat golangci-lint]$ echo >> test/testdata/godot.go
[kir@kir-rhat golangci-lint]$ golangci-lint run --no-config --disable-all --enable gofmt test/testdata/godot.go 
test/testdata/godot.go:8: File is not `gofmt`-ed with `-s` (gofmt)

Now, let's add import "C":

[kir@kir-rhat golangci-lint]$ git reset --hard HEAD
HEAD is now at c531fc2a gosec: allow `global` config (#2880)
kir@kir-rhat golangci-lint]$ vim test/testdata/godot.go
[kir@kir-rhat golangci-lint]$ git diff
diff --git a/test/testdata/godot.go b/test/testdata/godot.go
index 819ce941..73e888e1 100644
--- a/test/testdata/godot.go
+++ b/test/testdata/godot.go
@@ -1,6 +1,8 @@
 //args: -Egodot
 package testdata
 
+import "C"
+
 // Godot checks top-level comments // ERROR "Comment should end in a period"
 func Godot() {
        // nothing to do here
[kir@kir-rhat golangci-lint]$ golangci-lint run --no-config --disable-all --enable gofmt test/testdata/godot.go 
[kir@kir-rhat golangci-lint]$ echo >> test/testdata/godot.go
[kir@kir-rhat golangci-lint]$ git diff
diff --git a/test/testdata/godot.go b/test/testdata/godot.go
index 819ce941..e258956b 100644
--- a/test/testdata/godot.go
+++ b/test/testdata/godot.go
@@ -1,7 +1,10 @@
 //args: -Egodot
 package testdata
 
+import "C"
+
 // Godot checks top-level comments // ERROR "Comment should end in a period"
 func Godot() {
        // nothing to do here
 }
+
[kir@kir-rhat golangci-lint]$ golangci-lint run --no-config --disable-all --enable gofmt test/testdata/godot.go 
[kir@kir-rhat golangci-lint]$ echo $?
0

Version of golangci-lint

$ golangci-lint --version

golangci-lint has version 1.46.2 built from a333689 on 2022-05-17T11:31:29Z

Configuration file

Standard .golangci.yml from this repo. Unrelated, since I have used `--no-config` in the above repro.

Go environment

$ go version && go env
go version go1.18.1 linux/amd64
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/kir/.cache/go-build"
GOENV="/home/kir/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/kir/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/kir/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/kir/sdk/go1.18.1"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/kir/sdk/go1.18.1/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.18.1"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/kir/go/src/github.com/golangci/golangci-lint/go.mod"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build1837761077=/tmp/go-build -gno-record-gcc-switches"

Verbose output of running

$ golangci-lint cache clean
$ golangci-lint run -v
# paste output here

Cleaning the cache does not change anything.

Code example or link to a public repository

See the repro above.

[kolyshkin]

• adding build-tags cgo doesn't help
• adding CGO_ENABLED=1 doesn't help
• apparently, other linters might be affected, too, not just godot

Here's an example of godot that stops working:

[kir@kir-rhat golangci-lint]$ git diff
diff --git a/test/testdata/godot.go b/test/testdata/godot.go
index 819ce941..e258956b 100644
--- a/test/testdata/godot.go
+++ b/test/testdata/godot.go
@@ -1,7 +1,10 @@
 //args: -Egodot
 package testdata
 
+import "C"
+
 // Godot checks top-level comments // ERROR "Comment should end in a period"
 func Godot() {
        // nothing to do here
 }
+
[kir@kir-rhat golangci-lint]$ golangci-lint run --no-config --disable-all --enable godot test/testdata/godot.go 
[kir@kir-rhat golangci-lint]$ echo $?
0

[kolyshkin]

OK, looks like some linters are affected, and some are not.

Say, inefassign is affected:

[kir@kir-rhat golangci-lint]$ golangci-lint run --no-config --disable-all --enable ineffassign test/testdata/ineffassign.go 
test/testdata/ineffassign.go:10:3: ineffectual assignment to x (ineffassign)
		x = 0 // ERROR "ineffectual assignment to x"
		^
[kir@kir-rhat golangci-lint]$ vim test/testdata/ineffassign.go 
[kir@kir-rhat golangci-lint]$ git diff test/testdata/ineffassign.go
diff --git a/test/testdata/ineffassign.go b/test/testdata/ineffassign.go
index 69d3cc81..6f4d93ff 100644
--- a/test/testdata/ineffassign.go
+++ b/test/testdata/ineffassign.go
@@ -1,6 +1,8 @@
 //args: -Eineffassign
 package testdata
 
+import "C"
+
 import "math"
 
 func _() {
[kir@kir-rhat golangci-lint]$ CGO_ENABLED=1 golangci-lint run --build-tags cgo --no-config --disable-all --enable ineffassign test/testdata/ineffassign.go 
[kir@kir-rhat golangci-lint]$ echo $?
0

but deadcode is not:

[kir@kir-rhat golangci-lint]$ git diff test/testdata/deadcode.go
[kir@kir-rhat golangci-lint]$ golangci-lint run  --no-config --disable-all --enable deadcode test/testdata/deadcode.go
test/testdata/deadcode.go:6:5: `unused` is unused (deadcode)
var unused int // ERROR "`unused` is unused"
    ^
test/testdata/deadcode.go:11:6: `g` is unused (deadcode)
func g(x int) { // ERROR "`g` is unused"
     ^
[kir@kir-rhat golangci-lint]$ vim test/testdata/deadcode.go
[kir@kir-rhat golangci-lint]$ git diff test/testdata/deadcode.go
diff --git a/test/testdata/deadcode.go b/test/testdata/deadcode.go
index 82379efd..520a89db 100644
--- a/test/testdata/deadcode.go
+++ b/test/testdata/deadcode.go
@@ -1,6 +1,8 @@
 //args: -Edeadcode
 package testdata
 
+import "C"
+
 var y int
 
 var unused int // ERROR "`unused` is unused"
[kir@kir-rhat golangci-lint]$ golangci-lint run  --no-config --disable-all --enable deadcode test/testdata/deadcode.go
test/testdata/deadcode.go:8:5: `unused` is unused (deadcode)
var unused int // ERROR "`unused` is unused"
    ^
test/testdata/deadcode.go:13:6: `g` is unused (deadcode)
func g(x int) { // ERROR "`g` is unused"
     ^
[kir@kir-rhat golangci-lint]$

My guess, those linters that check comments and whitespace are affected, and those that check the actual code are not.

[kolyshkin]

Linters that are affected:

• gofmt
• goimports
• gofumpt
• godot
• ineffassign

Linters that are not affected:

• deadcode

[kolyshkin]

To check which linters are affected:

$ pwd
/home/kir/go/src/github.com/golangci/golangci-lint
$ cd test
$ # Add import "C" to every test file in testdata.
$ for f in testdata/*.go; do grep -q '^import "C"$' $f || sed -i -s 's/^package testdata$/\0\n\nimport "C"/' $f; done
$ go test -run TestSourcesFromTestdataWithIssuesDir

(there are some false positives)

[kolyshkin]

Similar to #2612, #2449.

[kolyshkin]

I checked various versions and it seems that v1.26.0 is working as it should, and v1.27.0 does not.

Next, I did a git-bisect using gofmt linter and found out that commit 7f48cc8 (PR #1065) is the one that breaks things.

Indeed, reverting it (partially) helps:

[kir@kir-rhat golangci-lint]$ git diff test/testdata/gofmt.go
diff --git a/test/testdata/gofmt.go b/test/testdata/gofmt.go
index 61c93d28..f4bb8b5c 100644
--- a/test/testdata/gofmt.go
+++ b/test/testdata/gofmt.go
@@ -1,6 +1,8 @@
 //args: -Egofmt
 package testdata
 
+import "C"
+
 import "fmt"
 
 func GofmtNotSimplified() {
[kir@kir-rhat golangci-lint]$ ./golangci-lint run --no-config --disable-all -Egofmt  ./test/testdata/gofmt.go 
[kir@kir-rhat golangci-lint]$ # ^^^ ignoring the issue
[kir@kir-rhat golangci-lint]$ patch -p1 < 1.diff
patching file pkg/golinters/gofmt.go
[kir@kir-rhat golangci-lint]$ git diff pkg test/testdata/gofmt.go
diff --git a/pkg/golinters/gofmt.go b/pkg/golinters/gofmt.go
index 1d50bfc5..03640f35 100644
--- a/pkg/golinters/gofmt.go
+++ b/pkg/golinters/gofmt.go
@@ -55,7 +55,7 @@ func NewGofmt(settings *config.GoFmtSettings) *goanalysis.Linter {
 func runGofmt(lintCtx *linter.Context, pass *analysis.Pass, settings *config.GoFmtSettings) ([]goanalysis.Issue, error) {
        var fileNames []string
        for _, f := range pass.Files {
-               pos := pass.Fset.PositionFor(f.Pos(), false)
+               pos := pass.Fset.Position(f.Pos())
                fileNames = append(fileNames, pos.Filename)
        }
 
diff --git a/test/testdata/gofmt.go b/test/testdata/gofmt.go
index 61c93d28..f4bb8b5c 100644
--- a/test/testdata/gofmt.go
+++ b/test/testdata/gofmt.go
@@ -1,6 +1,8 @@
 //args: -Egofmt
 package testdata
 
+import "C"
+
 import "fmt"
 
 func GofmtNotSimplified() {
[kir@kir-rhat golangci-lint]$ make build
go build -o golangci-lint ./cmd/golangci-lint
[kir@kir-rhat golangci-lint]$ ./golangci-lint run --no-config --disable-all -Egofmt  ./test/testdata/gofmt.go 
test/testdata/gofmt.go:10: File is not `gofmt`-ed with `-s` (gofmt)
	fmt.Print(x[1:len(x)]) // ERROR "File is not `gofmt`-ed with `-s`"
[kir@kir-rhat golangci-lint]$ # ^^^ No longer ignoring the issue
[kir@kir-rhat golangci-lint]$

[kolyshkin]

Since I don't have any idea of how all this works, I need help fixing this.

@ksoichiro @ldez @idenx perhaps you can take a look 🙏🏻

[kolyshkin]

@jirfag PTAL 🙏🏻

[ansiwen]

Great find @kolyshkin! This is in fact what is keeping me from using golangci-lint in the CI of our highly cgo-entangled binding-library. We use the standalone revive so far, so if I want to migrate, the builtin revive of golangci-lint must be able to produce the same errors, which it cannot because of this issue. (It works fine in the files without import "C".)