/cc @dominikh for staticcheck consideration

I would generalize slightly to checking something else:

// Check whether it always return true or false
func (o opaqueList) Less(i, j int) bool {
	return o.l[i] < o.l[i] // <-- bug here
}

// Check whether these are useless assignments if it is: o.l[i], o.l[i] = o.l[j], o.l[i].
func (o opaqueList) Swap(i, j int) {
	o.l[i], o.l[j] = o.l[j], o.l[i]
}

for staticcheck consideration

@mdlayher already exists: https://staticcheck.io/docs/checks#SA4000

Does https://staticcheck.io/docs/checks#SA4018 also cover @guodongli-google's “useless assignment” case? (It's not obvious to me whether it would recognize index expressions as “variables”.)

@nik0sc Your expectation could be easily handled by a static check tool, like staticcheck.

https://github.com/dominikh/go-tools

@zihengCat

Your expectation could be easily handled by a static check tool,

cmd/vet is a static checking tool. That is literally what this issue is asking for. 😅

@bcmills and @guodongli-google return o.l[i] < o.l[i] is caught by staticcheck as SA4000. o.l[i], o.l[i] = o.l[j], o.l[i] is caught as SA4018. Tested using staticcheck 2020.2.3 (v0.1.3).

One thing to consider with the proposal is lexicographic order for a struct:

type pair struct {
  x int
  y int
}
...
func (o pairList) Less(i, j int) bool {
         if o.l[i].x < o.l[j].x {
           return true
        }
	return o.l[i].x == o.l[i].x && o.l[i].y < o.l[i].y // <-- bug here
}

The proposed checker would not report this as both i and j get used. Just not in each comparison. staticcheck's SA4000 would likely still catch this. My hunch is that each comparison is roughly equally likely to be buggy. But I could be wrong. @nik0sc If you have a non-minimized example, you might be able to invalidate my hunch.

An alternative check to this proposal is to have vet warn when a parameter is not used (and not named "_"). This generalization would also not handle the example I just gave. I assume this proposal has come up before if someone has a reference.

@timothy-king I now realize that a rule like I proposed would not help in complex implementations of Less like yours. The more general rule of warning for unused parameters is better, but would probably break some CI pipelines (I know I've written code that ignores parameters just so it fulfills an interface...)

But what is the relationship between go vet and staticcheck? Is staticcheck simply the more inclusive version of go vet?

@nik0sc vet and Staticcheck are two independent static analysis tools for Go. vet is part of the Go project and focuses on a smaller number of checks. Staticcheck is a third party tool with more checks than vet, but it is not a superset of vet – that is, vet has some checks that aren't in Staticcheck.