Detect looping over a `map` to compare its key exactly

[gannett-ggreer]

Is your feature request related to a problem? Please describe.
In a recent code change, we modified a variable from a slice to a map:

-       TagIDs             []string
+       TagIDs             map[string]float64

but a loop across the now-map was changed in an obvious but inefficient way:

-       for _, id := range a.TagIDs {
+       for id := range a.TagIDs {
                if id == someStaticValue {
                        return nil
                }
        }

given that is now a map there's a really direct way to do that test.

Describe the solution you'd like
The linter should flag the loop or suggest a fix to make it:

if _, exists := a.TagIDs[someStaticValue]; exists {
    return nil
}

Describe alternatives you've considered
This isn't a correctness issue, just inefficiency, so when in doubt it shouldn't be too picky.

Additional context
This happens remarkably often in my experience for people new to Go or who are making scattered changes and don't always think too deeply about fixing the resulting compilation errors as they focus on the end goal. None of the golangci-lint linters or current version of revive tripped on that loop as anything of note, but it is also more of a learning opportunity for the programmer rather than anything wrong necessarily with the program.

[alexandear]

Thanks for the suggestion!

While the idea makes sense from an efficiency and readability perspective, this type of pattern—looping over a map to check for the presence of a single key—is more of a performance/style concern than a correctness issue. Introducing a rule for this case may lead to false positives or overly opinionated suggestions.

That said, this could be a great candidate for a more targeted static analysis tool or as a custom rule in projects where this pattern is a common pain point.

@ccoVeille @chavacava what do you think?

[gannett-ggreer]

It is similar in a sense to linters that say "you're not using any %-formatting codes in fmt.Errorf, you should use errors.New instead".

For example:

authoring/embed.go:40:11: use-errors-new: replace fmt.Errorf by errors.New (revive)
			Err:   fmt.Errorf("no body found for embed"),

[ccoVeille]

I'm fine with having the rule described in the issue in revive

[chavacava]

revive already has (too many?) "style-related" (enforce-, max-, ...) rules, one more or one less will not change affairs so why not.

The rule will need type information (to recognise loops over maps)