update scopelint with looppointer or exportloopref

[agnivade]

Hi there,

I am totally aware that the project is going through some rough times. So there's no pressure to work on this right now. Nevertheless, I wanted to file this to just to keep a record.

The author of scopelint has deprecated it for other better linters: https://github.com/kyoh86/scopelint#obsoleted.

Let's use either of them to remain up to date.

Thanks for all the work.

[ernado]

Should we just add both looppointer and exportloopref?

[agnivade]

I think so. Let the user choose what they want. And document it adequately mentioning which is to be used when.

[tamalsaha]

If you want to find lints as nervous as possible (with some false-positives), you should use looppointer.

If you want to find lints as accurately as possible (with some lints ignored), you should use exportloopref.

If you are going to include only one, please include exportloopref. No body wants false positives from their linter.

[sayboras]

Agreed with comment from @tamalsaha above, maybe exportloopref is good enough :)

[kyoh86]

looppointer can be an option.
Maybe this is not so important, but I'd like to mention it.

Since exportloopref cannot find some diagnostics like below.

Case 1: pass the pointer to the function to export.

Case 2: pass the pointer to the local variable, and export it.

package main

type List []*int

func (l *List) AppendP(p *int) {
	*l = append(*l, p)
}

func main() {
	var slice []*int
	list := List{}

	println("loop expect exporting 10, 11, 12, 13")
	for _, v := range []int{10, 11, 12, 13} {
		list.AppendP(&v) // Case 1: wanted "exporting a pointer for the loop variable v", but cannot be found

		p := &v                  // p is the local variable
		slice = append(slice, p) // Case 2: wanted "exporting a pointer for the loop variable v", but cannot be found
	}

	println(`slice expecting "10, 11, 12, 13" but "13, 13, 13, 13"`)
	for _, p := range slice {
		printp(p)
	}
	println(`array expecting "10, 11, 12, 13" but "13, 13, 13, 13"`)
	for _, p := range ([]*int)(list) {
		printp(p)
	}
}

func printp(p *int) {
	println(*p)
}

So we may use looppointer instead of exportloopref if we want to find them all (with some false-positives).

https://github.com/kyoh86/exportloopref#known-false-negatives

[may98ank]

Hi, I just changed the version of golangci-lint that I was using from v1.33.0 to v1.44.2. I am using go1.17.
I got the warning that scopelint has been deprecated. So, I change the configuration to use exportloopref.
After making this change, I noticed some difference between the reported linting issue between scopelint and exportloopref.
For the following test:

import "testing"

func Test_isPrime(t *testing.T) {
	tests := []struct {
		name string
		args int
		want bool
	}{
		// TODO: Add test cases.
		{"TC1", 2, true},
		{"TC2", 3, true},
		{"TC3", 1, false},
		{"TC4", 16, false},
		{"TC5", 11, true},
	}
	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			if got := isPrime(tt.args); got != tt.want {
				t.Errorf("isPrime() = %v, want %v", got, tt.want)
			}
		})
	}
}

With scopelint, I got:

golangci-lint -c golangci.yml run
main_test.go:20:22: Using the variable on range scope tt in function literal (scopelint)
if got := isPrime(tt.args); got != tt.want {
^
main_test.go:21:46: Using the variable on range scope tt in function literal (scopelint)
t.Errorf("isPrime() = %v, want %v", got, tt.want)

With exportloopref, I got no issue.

Not sure, which is the correct behavior for a linter? Is it not a linter issue now?

[kyoh86]

@may98ank You can read comments above, README of exportloopref and README of looppointer to understand the difference.

They have different policies.

[titpetric]

Does this have a use since recent loop changes in go 1.22? https://tip.golang.org/doc/go1.22#language

Previously, the variables declared by a “for” loop were created once and updated by each iteration. In Go 1.22, each iteration of the loop creates new variables, to avoid accidental sharing bugs. The transition support tooling described in the proposal continues to work in the same way it did in Go 1.21.

Unsure what needs to change if anything, as this would seem to avoid the root reason for loop ref issues.