Add looppointer linter

[kamilsk]

https://github.com/kyoh86/looppointer

fix #1404

[ldez]

Hello,

I think that a decision has been already done about the support of looppointer:

#1404 (comment)
#1041 (comment)

ping @sayboras

[kamilsk]

Hi, @ldez! I found the looppointer useful, and it catches some critical bugs when I work on a huge Go project. So, I used it by default in all my projects and templates

• https://github.com/octomation/go-tool/blob/271d0b52b43c560e24c2e66615a2d1c49347a1a3/Makefile#L185-L197
• https://github.com/octomation/go-service/blob/2a6f75f0ed13d4ed4c9fc65e9dc3c3a44598c260/Makefile#L185-L197

I thought it would be helpful not only for me but also for anyone to enable it when needed. And I agree entirely with #1404 (comment)

[ldez]

The test data test/testdata/looppointer.go gives the same output with exportloopref:

looppointer

$ go run ./cmd/golangci-lint/ run --no-config --disable-all --enable=looppointer ./test/testdata/looppointer.go
test/testdata/looppointer.go:11:32: taking a pointer for the loop variable p (looppointer)
                intSlice = append(intSlice, &p) // ERROR "taking a pointer for the loop variable p"
                                             ^
exit status 1

exportloopref

$ go run ./cmd/golangci-lint/ run --no-config --disable-all --enable=exportloopref ./test/testdata/looppointer.go
test/testdata/looppointer.go:12:32: exporting a pointer for the loop variable p (exportloopref)
                intSlice = append(intSlice, &p) // ERROR "taking a pointer for the loop variable p"
                                             ^
exit status 1

---

If I use the test data test/testdata/exportloopref.go:

looppointer

$ go run ./cmd/golangci-lint/ run --no-config --disable-all --enable=looppointer ./test/testdata/exportloopref.go
test/testdata/exportloopref.go:14:11: taking a pointer for the loop variable p (looppointer)
                printp(&p)
                        ^
test/testdata/exportloopref.go:15:26: taking a pointer for the loop variable p (looppointer)
                slice = append(slice, &p) // ERROR "exporting a pointer for the loop variable p"
                                       ^
test/testdata/exportloopref.go:16:15: taking a pointer for the loop variable p (looppointer)
                array[i] = &p             // ERROR "exporting a pointer for the loop variable p"
                            ^

exportloopref

$ go run ./cmd/golangci-lint/ run --no-config --disable-all --enable=exportloopref ./test/testdata/exportloopref.go
test/testdata/exportloopref.go:15:26: exporting a pointer for the loop variable p (exportloopref)
                slice = append(slice, &p) // ERROR "exporting a pointer for the loop variable p"
                                       ^
test/testdata/exportloopref.go:16:15: exporting a pointer for the loop variable p (exportloopref)
                array[i] = &p             // ERROR "exporting a pointer for the loop variable p"
                            ^
test/testdata/exportloopref.go:18:11: exporting a pointer for the loop variable p (exportloopref)
                        ref = &p   // ERROR "exporting a pointer for the loop variable p"
                               ^
exit status 1

With this sample you can see some looppointer problems:

• test/testdata/exportloopref.go:14:11 is a false positive

proof

package main

func main() {
	for _, v := range []int{10, 11, 12, 13} {
		printp(&v)
	}
}

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

go run main.go 
10
11
12
13

• test/testdata/exportloopref.go:18:11 is a real issue but not detected by looppointer

proof

package main

func main() {
	var ref *int

	for _, v := range []int{10, 11, 12, 13} {
		if v == 11 {
			ref = &v
		}
	}
	printp(ref)
}

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

$ go run main.go 
13

---

so for me, the arguments from #1041 (comment) are still valid.

[kamilsk]

Thanks for examples! I will be back with others or close the pr.

[kamilsk]

Does anyone know how to fix that?

test/testdata/looppointer.go:4:8: could not import fmt (Config.Importer.Import(fmt) returned nil but no error) (typecheck)
import "fmt"

@ldez I updated the example. We had it on production and I'm ok with false positive result but really want to exclude similar cases in the future.

compare:
	@(go run ./cmd/golangci-lint/ run --no-config --disable-all --enable=looppointer ./test/testdata/looppointer.go && echo nothing) || true
	@echo ---
	@(go run ./cmd/golangci-lint/ run --no-config --disable-all --enable=exportloopref ./test/testdata/looppointer.go && echo nothig) || true
.PHONY: compare

$ make compare
test/testdata/looppointer.go:22:18: taking a pointer for the loop variable d (looppointer)
                        p.property = &d.prop // ERROR "taking a pointer for the loop variable d"
                                      ^
exit status 1
---
nothig

[kamilsk]

@winmillwill, @scorpionknifes 👋 do you have some critical cases in your practice?

[kamilsk]

Maybe @kyoh86 could add more extra knowledge.

[kamilsk]

@golangci/team, what I have to do next? Does anyone have another point of view/can add alternative feedback/etc?

[ldez]

For now, your PR is blocked.

The arguments from #1041 (comment) are still valid.
Personally, I disable every linter with non-trivial false positives.

My opinion is to close this PR but keep the issue open (I will add my example in the issue)

[kamilsk]

Personally, I disable every linter with non-trivial false positives.

We can make it disabled by default and add a possibility to allow it explicitly. What the problem with this option?

[ldez]

We can make it disabled by default and add a possibility to allow it explicitly. What the problem with this option?

It's not possible because a lot of people use enable-all.

[kamilsk]

I showed the example which didn't catch by other linters in a box. I'm conscious enough to make an informed decision about whether I want to catch non-obvious errors but deal with false positives.

[ldez]

Keep this rule in view:

Rule 1 of open-source: no is temporary, yes is forever.

I prefer to close the PR and wait for the improvements that @kyoh86 can do on false-positives, then later (re)open a PR.

[kyoh86]

I do not have the option of solving all of the false positives that looppointer brings.
looppointer is developed with the policy of finding diagnostics reliably with some false positives.
exportloopref is developed with the policy of finding diagnostics similar to looppointer without false positives.
Instead, exportloopref overlooks some fatal diagnostics.

There is a tradeoff between false positives and oversights, and I haven't found a way to solve both at the same time.
(If I found it, I would fix exportloopref and obsolete looppointer)
So whether to use looppointer or exportloopref is a choice for the user, based on the risk of fatality, the cost of dealing with a few false positives, and which is more important.

I don't expect the golangci-linter to be safe or comprehensive, so I have no opinion on its choice not to incorporate looppointer.

[bombsimon]

I don't really have a strong feeling but I still want to add that I don't think the fact that people use enable-all should stop us from adding more linters, even if they're opinionated or might contain false positives. People opting in to enable-all usually wants to get new linters on an update and if it doesn't fit them I think it's easy enough to disable it.

I'm one of those using enable-all and I've had to disable a bunch of linters several times on new releases.

[winmillwill]

So, looppointer will always have a case of false negatives positives (that are incredibly easy for the user to fix). The only remaining argument for not including this seems to be that users are expected to abuse --enable-all. That seems like a strange argument to make when the tool is highly configurable. If users aren't expected to mindfully configure the tool or use the defaults, why have configuration?

The fix seems to be a better option for users who are abusing --enable-all. Even without that fix, the downside is vanishingly small.

@ldez it sounds like you already have a list of linters that can throw false negatives positives. Can we build that into the tool so that users can have one option for disabling all of them, and even make that the default? Then people can keep mindlessly passing --enable-all.

[ldez]

@ldez it sounds like you already have a list of linters that can throw false negatives positives. Can we build that into the tool so that users can have one option for disabling all of them, and even make that the default? Then people can keep mindlessly passing --enable-all.

I prefer to contribute to linter to remove false positives instead of creating an option.

---

@kyoh86 It can be great if you could merge the 2 linters together and create an option for switching between the 2 behaviors.
I think it would be more understandable for users and it would allow us to have a default behavior without false positives.

Another option is to create a kind of "merge" inside golangci-lint by creating an option inside exportloopref to use looppointer.

example:

linters-settings:
  exportloopref:
    looppointer-mode: true

I will work on this approach and see what I do on that.

[kyoh86]

It's great to let the user choose the linters.
But it is silly to let the user choose the options for each linter.
Make each linter do one thing well.

[kyoh86]

golangci-lint also provides "one thing".
Users can get a choice about linters (and few options) thanks to it. That's enough.
Let me repeat that, I have no opinion on its choice not to incorporate looppointer.

[ldez]

I understand that you don't care about which linter is integrated but you are the author of both.

I'm trying to satisfy everyone then I'm trying to find solutions.

exportloopref and looppointer have the same goal, the same options, they can be seen as one linter.

It's your choice to split this analysis into 2 differents pieces but we can see that as only one, it's just a point of view.

I want the best default behavior, with the few false-positives, by using looppointer as an option of exportloopref it seems to satisfy this goal, but we can discuss.

FYI I created a branch master...ldez:feat/looppointer

[kyoh86]

You think that they do same thing. But I don't think so.
Users can choose one of them. That's enough.

If users cannot do it, that's my fault.
I'm not good at English, so I may fail to describe the difference of them shortly.

[kyoh86]

Users don't always use golangci-lint.
I don't use golangci-lint very often either.
The options are useful when using golangci-lint, but otherwise they are a pain and a hindrance.

[kyoh86]

Make each linter do one thing well.
I make each linter do one thing well. The situation differs depending on the analyzer and the author.

[ldez]

Just to be clear, the decision has been already made to choose exportloopref, I was not involved in this decision because I was not a maintainer at that time.
I'm trying to follow this decision and to find solutions.

I don't like false positives, it's a personal opinion, I want to trust a linter without having to think about the meaning of reports.
In my professional or personal projects, I don't want a colleague or a contributor to spending time trying to fix a false problem or producing unwanted complexity.

In the context of golangci-lint, linters with false positives exist but 99% of the time it's just a bug inside a linter.
The fact to add a linter with expected false positives is not usual.
It's possible to add a linter with this kind of behavior, but behind that, there is a "philosophy", a mindset, and it's the real question.

In conclusion, we don't really care about my personal opinion about false positives, the only decision we have to take is: are we ok to add a linter with expected false positives?
I'm globally not ok with that.
@bombsimon seems ok to add this linter.
Maybe @SVilgelm or @ernado have an opinion on that.

[bombsimon]

the only decision we have to take is: are we ok to add a linter with expected false positives?
I'm globally not ok with that.
@bombsimon seems ok to add this linter.

I might've been a bit unclear. I mostly agree with you and I think it's a bit weird to add a linter that we know has false positives and it's intended. I think that to say the false positives are bad as an argument makes more sense than the fact that users run enable-all, that's just what I wanted to stress.

So to clarify, I'm not a fan of adding linters with checks with known issues. I think that would reduce the overall quality feeling of golangci-lint.

If it's easy to do I think your suggestion about merging them inside golangci-lint could be an alternative even though it feels a lot like a hacky workaround.

[kamilsk]

Rule 1 of open-source: no is temporary, yes is forever.

I completely agree with this point. The only reason I tried to add the linter is that it can catch some issues that appeared in my practice.

Folks, what do you think about excluding some linters with issues from --enable-all? I understand it sounds a little bit weird, but maybe we can "sandboxing" them (aka experimental linters).

[ernado]

We have --exclude-use-default but I think it is a bit different.

AFAIK, using --enable-all is already broken in some cases because some linters can be duplicating or conflicting, but not sure that it is a good reason to complicate the situation.

Personally I have mixed opinion on current topic and can't decide what is better.

[winmillwill]

So to clarify, I'm not a fan of adding linters with checks with known issues. I think that would reduce the overall quality feeling of golangci-lint.

The "known issue" is that it makes you do an unnecessary copy in exactly one vanishingly rare case where you may not need to copy, depending on the exact implementation of the function to which you pass the pointer, which obviously changes independent of the call site. So, in this exact case, it would be false to say "you took the address of a loop variable and then used it unsafely", but it would be true to say "you took the address of a loop variable, which you should almost never do, and it's easy to actually never do it and never have the bugs it can cause, so just don't do it".

If the expectation is that the linter will only mark code that definitely causes odd behavior due to taking the address of the loop variable, then this is a false positive. If the expectation is that your code should be readable and maintainable, then this is NOT a false positive. It's strange to think of this behavior as any kind of obstacle given the choice of linters that are enabled by default: gosimple will flag code that doesn't have any bugs at all BY DESIGN, so everything it does is a false positive if you apply the rationale being used to evaluate looppointer. Conversely, if you just expect linters to help you make your code easier to read and maintain, then looppointer will always do that.

[bombsimon]

I didn't get the impression that this linter warned about style but about safety. The docs for the linter clearly states:

So some false-positves will be reported.

There's a lot of linters that only warns about style such as gochecknoglobals, lll, whitespace etcetera. A more fair comparison would be if gochecknoglobals reported an error for a const prefixed Err because it mistook it for a var based on it's named even though it was a constant. And the author didn't intend to fix it.

If the linters intended purpose is to force you to always make a copy of the pointer value though, for whatever reason and not just safety or avoiding bugs, I would agree with you. Maybe it just boils down to wording, I'm totally open for the fact that I misinterpreted the use case for the linter.

[winmillwill]

If the linters intended purpose is to force you to always make a copy of the pointer value though, for whatever reason and not just safety or avoiding bugs, I would agree with you. Maybe it just boils down to wording, I'm totally open for the fact that I misinterpreted the use case for the linter.

I don't think you misinterpreted the docs, I think it's a case where the documented behavior of software implies many possible uses. In this case, style would be informed by the most straightforward way to avoid a class of bugs. I guess I just don't see the original intent of the author of a linter as relevant to the user, beyond the extent to which it informs ongoing development. I don't see how the stated purpose of looppointer can allow a faithful implementation not to support my use case, so I'm satisfied, and false positives in terms of provable bugs won't be false positives to me, because they signify that we can't easily prove the bugs aren't present, which I consider a "true positive" when it can be solved easily.

The value I see is as you described: just have every contributor add a copy in this case and be done with it, and now it's part of the style of all the code in the repo, in addition to never having this class of bugs. If we have to avoid the copy for some reason, the linter still forced us to have the conversation in the pr because the author had to add the nolint comment, which should naturally start a conversation. Ideally that conversation concludes with an agreement that this probably isn't the right thing to optimize and the bottleneck is elsewhere, or that the design can change to not need a pointer.