fix: use adjusted position

[ldez]

The position must be adjusted to work with cgo.

[coveralls]

coverage: 93.671% (+0.007%) from 93.664%
when pulling 31121bb on ldez:fix/cgo
into c862f08 on bombsimon:master.

[bombsimon]

Thanks!

How will this work with line directives? This was changed after fixing golangci/golangci-lint#3967. I guess this was needed with the old system but shouldn't be used for SuggestedFixes?

[ldez]

I guess this was needed with the old system but shouldn't be used for SuggestedFixes?

No, this is not related because currently, wsl doesn't work with cgo.

When using cgo, the non-adjusted file is never a .go file.

wsl/analyzer.go

Lines 85 to 88 in c862f08

	filename := pass.Fset.PositionFor(file.Pos(), false).Filename
	if !strings.HasSuffix(filename, ".go") {
	continue
	}

Currently, 95% of the linters don't work with yacc.

golangci/golangci-lint#1779 (comment)

And wsl is a part of the 95%.

Now, I think it's better to support cgo first, and the yacc topic will be handled after with all the other linters.

[ldez]

FYI, I don't know if this is expected but some of your test files inside testdata/src/default_config are just ignored (ex: else_if.go).

[bombsimon]

FYI, I don't know if this is expected but some of your test files inside testdata/src/default_config are just ignored (ex: else_if.go).

Nope, that doesn't sound expected at all, will have a look.

When using cgo, the non-adjusted file is never a .go file.

Now, I think it's better to support cgo first

Sounds good to me, but if the !strings.HasSuffix(filename, ".go") is the issue here, shouldn't that also be removed? Or was that a different issue? Sorry if I'm slow to understand here 🙈

Either way, I'm happy to just merge this because this but feels better to understand the full context so hence why I'm asking.

[ldez]

Sounds good to me, but if the !strings.HasSuffix(filename, ".go") is the issue here, shouldn't that also be removed?

I just used this as an example because it's easy to see the problem, but the positions should also be adjusted for cgo to have the right positions because when using cgo the file content is not a real file content but a generated file.

for example:

// Code generated by cmd/cgo; DO NOT EDIT.

//line /home/ldez/sources/golangci/golangci-lint/pkg/golinters/goimports/testdata/goimports_cgo.go:1:1
//golangcitest:args -Egoimports
package testdata

/*
 #include <stdio.h>
 #include <stdlib.h>

 void myprint(char* s) {
    printf("%d\n", s);
 }
*/
import _ "unsafe"

import (
    "unsafe"
)

import (
    "fmt" // want "File is not `goimports`-ed"
    "github.com/golangci/golangci-lint/pkg/config"
)

func _() {
    cs := ( /*line :24:8*/_Cfunc_CString /*line :24:16*/)("Hello from stdio\n")
    ( /*line :25:2*/_Cfunc_myprint /*line :25:10*/)(cs)
    func() { _cgo0 := /*line :26:9*/unsafe.Pointer(cs); _cgoCheckPointer(_cgo0, nil); /*line :26:28*/_Cfunc_free(_cgo0); }()
}

func Bar() {
    fmt.Print("x")
    _ = config.Config{}
}

instead of

//golangcitest:args -Egoimports
package testdata

/*
 #include <stdio.h>
 #include <stdlib.h>

 void myprint(char* s) {
 	printf("%d\n", s);
 }
*/
import "C"

import (
	"unsafe"
)

import (
	"fmt" // want "File is not `goimports`-ed"
	"github.com/golangci/golangci-lint/pkg/config"
)

func _() {
	cs := C.CString("Hello from stdio\n")
	C.myprint(cs)
	C.free(unsafe.Pointer(cs))
}

func Bar() {
	fmt.Print("x")
	_ = config.Config{}
}

Sorry if I'm slow to understand here 🙈

You are not slow, this is not trivial to understand.

[bombsimon]

Cool, thanks!

I'm still not sure about the !strings.HasSuffix(filename, ".go") part, it's been bugging me for a while. Do you suggest removing that part completely? Is it required to make it work with Cgo?

I think the same code path exist in other linters such as gochecknoglobals (written by me 🙈).

The thing is, in a lot of cases I don't want to analyze non go files. I actually created golang/go#41771 back in 2020 but closed it since it wasn't really right and then again golang/go#64436 a year ago but closed it as a duplicate. I closed it because the comments in golang/go#43481 suggested that filtering in or out specific files was the facto the recommended way.

[ldez]

Do you suggest removing that part completely? Is it required to make it work with Cgo?

Yes I recommend removing it because this is not required.

[ldez]

I used your snippet, added logs, added a package with cgo content and line directive.

code

.
├── cgo
│   └── cgo.go
├── go.mod
├── go.sum
├── line
│   ├── hello.tmpl
│   └── line.go
├── main.go
└── main_test.go

package main

import (
	"go/ast"
	"log"
	"strings"

	"golang.org/x/tools/go/analysis"
	"golang.org/x/tools/go/analysis/singlechecker"
)

var Analyzer = &analysis.Analyzer{
	Name: "example",
	Doc:  "example",
	Run:  run,
}

func run(pass *analysis.Pass) (interface{}, error) {
	for _, file := range pass.Files {
		filename := pass.Fset.PositionFor(file.Pos(), false).Filename
		log.Println("Is .go?", strings.HasSuffix(filename, ".go"), ":", "IsGenerated", ast.IsGenerated(file), ":", filename)

	}

	return nil, nil
}

func main() {
	singlechecker.Main(Analyzer)
}

package cgo

/*
#include "math.h"
double add(double a, double b) {
    return a + b;
}
*/
import "C"
import "fmt"

func Foo() {
	fmt.Println(C.floor(C.add(1, 2.1)))
}

//line hello.tmpl:1
package line

import (
	"fmt"

	"golang.org/x/tools/go/analysis"
)

func _() {
	var x int
	_ = x
	x = 0 // x
}

func main() {
	a()
	b()
	wsl()
}

func a() {
	fmt.Println("foo")
}

func b() {
	fmt.Println("foo")
}

func c() {
	_ = analysis.Analyzer{}
}

func wsl() bool {

	return true
}

func notFormatted() {
}

# This is a template file for some code generator, and is not a valid go source file.

output

# adjusted: false
$ go run . ./...
example: Is .go? false : IsGenerated true : /home/ldez/.cache/go-build/42/42102fe476e4a606613a5b565e71218d109f6b06a5dfb121e376f66de471497b-d
example: Is .go? true : IsGenerated false : /home/ldez/sources/golangci/sandbox/line/line.go
example: Is .go? true : IsGenerated false : /home/ldez/sources/golangci/sandbox/main.go
example: Is .go? true : IsGenerated false : /home/ldez/sources/golangci/sandbox/main_test.go
example: Is .go? false : IsGenerated true : /home/ldez/.cache/go-build/62/6248342cd6ede467fbde8a115d730d771a2fcb65488a2bf6ac6dd981cfcf0e6b-d
example: Is .go? false : IsGenerated true : /home/ldez/.cache/go-build/d2/d202ddb4ff2eb75186fb85b2d45b3abbede18e343180e149a0ae28f0849fa877-d
example: Is .go? false : IsGenerated false : /home/ldez/.cache/go-build/52/52709e04346f162eeabb16dbe9df656d978effaafca09c371368789c95dd32d3-d
example: Is .go? true : IsGenerated false : /home/ldez/sources/golangci/sandbox/main.go

# adjusted: true
$ go run . ./...
example: Is .go? false : IsGenerated true : /home/ldez/.cache/go-build/42/42102fe476e4a606613a5b565e71218d109f6b06a5dfb121e376f66de471497b-d
example: Is .go? true : IsGenerated false : /home/ldez/sources/golangci/sandbox/main.go
example: Is .go? true : IsGenerated false : /home/ldez/sources/golangci/sandbox/main.go
example: Is .go? true : IsGenerated false : /home/ldez/sources/golangci/sandbox/main_test.go
example: Is .go? false : IsGenerated true : /home/ldez/.cache/go-build/62/6248342cd6ede467fbde8a115d730d771a2fcb65488a2bf6ac6dd981cfcf0e6b-d
example: Is .go? true : IsGenerated true : /home/ldez/sources/golangci/sandbox/cgo/cgo.go
example: Is .go? false : IsGenerated false : /home/ldez/.cache/go-build/52/52709e04346f162eeabb16dbe9df656d978effaafca09c371368789c95dd32d3-d
example: Is .go? false : IsGenerated false : /home/ldez/sources/golangci/sandbox/line/hello.tmpl

About cgo:

First, the go-build are files filtered by the name but also by ast.Generated inside wsl.
Second, when using non-adjusted positions, the cgo file is replaced by a generated file (like inside my previous comment)

So when using non-adjusted positions cgo files are skipped by ast.Generated and the name.

About line directives:

First, the go-build are files filtered by the name but also by ast.Generated inside wsl.

Second, when using adjusted positions, the template comes in the files, and the line.go disappears.

---

So to handle both elements:

code

package main

import (
	"go/ast"
	"go/token"
	"log"
	"strings"

	"golang.org/x/tools/go/analysis"
	"golang.org/x/tools/go/analysis/singlechecker"
)

var Analyzer = &analysis.Analyzer{
	Name: "example",
	Doc:  "example",
	Run:  run,
}

func run(pass *analysis.Pass) (interface{}, error) {
	for _, file := range pass.Files {
		GetFilename(pass.Fset, file)

		filename := GetFilename(pass.Fset, file)
		log.Println("Is .go?", strings.HasSuffix(filename, ".go"), ":", "IsGenerated", ast.IsGenerated(file), ":", filename)
	}

	return nil, nil
}

func GetFilename(fset *token.FileSet, file *ast.File) string {
	filename := fset.PositionFor(file.Pos(), true).Filename // same as fset.Position()
	if !strings.HasSuffix(filename, ".go") {
		return fset.PositionFor(file.Pos(), false).Filename
	}

	return filename
}

func main() {
	singlechecker.Main(Analyzer)
}

output

$ go run . ./...
example: Is .go? false : IsGenerated true : /home/ldez/.cache/go-build/42/42102fe476e4a606613a5b565e71218d109f6b06a5dfb121e376f66de471497b-d
example: Is .go? true : IsGenerated false : /home/ldez/sources/golangci/sandbox/main.go
example: Is .go? false : IsGenerated true : /home/ldez/.cache/go-build/62/6248342cd6ede467fbde8a115d730d771a2fcb65488a2bf6ac6dd981cfcf0e6b-d
example: Is .go? true : IsGenerated false : /home/ldez/sources/golangci/sandbox/main_test.go
example: Is .go? true : IsGenerated true : /home/ldez/sources/golangci/sandbox/cgo/cgo.go
example: Is .go? false : IsGenerated false : /home/ldez/.cache/go-build/52/52709e04346f162eeabb16dbe9df656d978effaafca09c371368789c95dd32d3-d
example: Is .go? true : IsGenerated false : /home/ldez/sources/golangci/sandbox/main.go
example: Is .go? true : IsGenerated false : /home/ldez/sources/golangci/sandbox/line/line.go

Now it's possible to see all the files (go, cgo, line).
But the positions are different: one is related to the adjusted position, and the other from the non-adjusted position.

---

So now you can understand why I said the line directive/yacc is a dedicated topic.

Also, I think that using pass.Files is not the right way for a linter.

I have learned a lot with the suggested fixes PR, but I'm still not an expert, I'm still learning.