False positive for table tests

[cloudlena]

The linter returns false positives when being run on the following test code:

func TestSomething(t *testing.T) {
	cases := []struct {
		description string
		expected    string
	}{
		{
			description: "foo",
			expected:    "bar",
		},
		{
			description: "fizz",
			expected:    "buzz",
		},
	}

	for _, tc := range cases {
		t.Run(tc.description, func(t *testing.T) {
			result := thingUnderTest()
			if result != tc.expected {
				t.Fatal("failed")
			}
		})
	}
}

In this case since the testing closure is executed immediately in the same iteration, the mistake discussed here: https://github.com/golang/go/wiki/CommonMistakes is not relevant.

More information can be found on golangci/golangci-lint#281.

[bombsimon]

This is exactly how I write tests as well and I'm seeing the same false positive effect.

EDIT: I was looking for a fix for this but I'm not sure what a feasible solution might be. In fact even running the linter on itself (main.go) I get errors:

go run main.go -- main.go
main.go:314:44: Using the variable on range scope "src" in function literal
main.go:324:38: Using the variable on range scope "src" in function literal
Found 2 lint problems; failing.
exit status 1

Both lines are related to variables used in a function (filepath.Walk) which takes another function as input just like t.Run(). Since I don't know a way to tell if the passed function will be called directly, in a go routine, added to a slice for later usage or any other way I'm not sure how this would be calculated from the code. A fixed whitelist with known functions could be an alternative but that sounds like a bad workaround to me.

I'm not too familiar with the ast package so I'll have a deeper look at it when I get some time. If someone already got an idea of a good solution I gladly help to implement it.

---

Shorter example to test:

package main

func main() {
	for i := range make([]int, 1) {
		f := func() int { return i }
		f()
	}
}

This will work since ast.FuncLit are handled in ast.CallExpr:

package main

func main() {
	for i := range make([]int, 1) {
		func() int { return i }()
	}
}

[jirfag]

We also should take into account call of t.Parallell inside a function: if we have such call we should report an issue

[kyoh86]

Sorry, I don't have any solutions for this.
Maybe scopelint should not be used regularly, we should check them in investigation of problems.

[sean-]

I have a slightly different opinion on this:

	for _, tc := range cases {
		tc := tc
		t.Run(tc.description, func(t *testing.T) {

is preferred atm because it acts as a constant reminder of https://github.com/golang/go/wiki/CommonMistakes . Consistency in code and treatment of the language is a feature, not a bug, and special-casing this quirk - regardless of what your feelings on this irritation may be - is actually harmful to developers. Where it's now idiomatic to explicitly test for nil, we also pin variables in loops coming out of range. I understand that there's a potential solution that could be had here, but that's trading a reduction in code complexity for human mental complexity. Which is to say, humans writ-large are awful at understanding the behavior of compilers or nuance in programming languages.

TL;DR: there is a huge developer value in embracing consistency because the mental overhead for consistent application of rules should be preferred to exotic understanding of language quirks. (I can't wait for this implementation detail to actually be handled by the language, but until then...)

[vearutop]

We also should take into account call of t.Parallell inside a function: if we have such call we should report an issue

@jirfag this is actually not correct as t.Parallel does not run sub tests of a single test concurrently.

Parallel signals that this test is to be run in parallel with (and only with) other parallel tests.

https://golang.org/pkg/testing/#T.Parallel

[fredbi]

It is okay that linters produce false positives from time to time, but we should be able to explicitly disable the warning with some comment, e.g. // noscopelint. How about that?

[jaimem88]

if you're using golangci-lint add this line to your .golangci.yml config
under issues -> exclude

issues:
  # List of regexps of issue texts to exclude, empty list by default.
  # But independently from this option we use default exclude patterns,
  # it can be disabled by `exclude-use-default: false`. To list all
  # excluded by default patterns execute `golangci-lint run --help`
  exclude:
     - Using the variable on range scope `tc` in function literal

[vuon9]

if you're using golangci-lint add this line to your .golangci.yml config ...

Thanks for your suggestion. After tried, I have done with below one.

issues:
  exclude:
    - Using the variable on range scope .* in function literal