x/tools/go/analysis: add check for redundant maps.Keys/Values calls in range statements

[kwjw]

Range statements like

for k := range maps.Keys(m) {}
for v := range maps.Values(m) {}

should be replaced with

for k := range m {}
for _, v := range m {}

respectively.

Possibly in scope is making similar corrections for the slice-based Keys/Values functions in x/exp/maps.

[adonovan]

The x/exp package is (one hopes) not too widely used, though we should measure how often this pattern occurs in practice.
This quick search turned up no occurrences of misuse of either Keys or Values: https://cs.opensource.google/search?q=for.*range.*maps.Keys&sq=

BTW, this is a perfect candidate for a "third-party analyzer", a (hypothetical) mechanism that would allow the x/exp module to declare to tools that client code of x/exp should run a set of analyzers provided by x/exp.

[gopherbot]

Change https://go.dev/cl/658695 mentions this issue: go/analysis/passes/maprange: check for redundant Keys/Values calls

[findleyr]

Given @adonovan's comment above, I'm not sure we want to add (and therefore support) an analyzer for such an infrequent error.

Nevermind: I see now that it also applies to the standard library maps package.

[adonovan]

In its favor, it is very cheap when not in use, very precise (essentially zero false positives), and the code is largely already written.

[findleyr]

In its favor, it is very cheap when not in use, very precise (essentially zero false positives), and the code is largely already written.

I'm a little confused about what we're discussing. The analyzer applies to the standard library "maps" pkg as well. That certainly seems worthwhile considering as a standard analyzer. (provided the errors are frequent enough)

https://pkg.go.dev/maps#Keys

[adonovan]

I'm a little confused about what we're discussing. The analyzer applies to the standard library "maps" pkg as well. That certainly seems worthwhile considering as a standard analyzer. (provided the errors are frequent enough)

We're discussing the same thing. My point is that the analyzer (which indeed handles std maps and x/exp/maps) is both fast (because it's a no-op when you don't import at least one of those packages) and precise (because zero false positives), so the cost of including it is low. Even if it only handled x/exp, which is relatively rarely used, the cost would be low enough to justify the benefit.

BTW, when https://go.dev/cl/657958 lands and is used by all our analyzers, they'll become even faster because they will short-circuit based not only on which imports are present, but which symbols from those imports are actually used---and it provides a quick way to enumerate the uses directly.