Add linter to error on unchecked map reads

[mattysweeps]

Your feature request related to a problem? Please describe.

The following code sample highlights an issue where a map reference was unchecked (did not check ok == true) resulting in a nil pointer panic.

https://go.dev/play/p/XwjkvaEshlu

Describe the solution you'd like.

A linter to catch map references which only return 1 result, instead of both the value and the check.

Describe alternatives you've considered.

None

Additional context.

I looked at golangci-lint's linters but couldn't tell if this is already solved by existing linters.

I tested with golangci-lint 1.50.0 with flags --max-issues-per-linter 0 --enable-all --verbose but did not see any lint results related to the unsafe map usage.

[mattysweeps]

I checked staticcheck's docs and found related lint rules:

• SA5000 Assignment to nil map
• S1036 Unnecessary guard around map access

But no rule to enforce the guard when it's necessary (inverse of S1036)

I created dominikh/go-tools#1328 to track adding this feature to staticcheck

[ldez]

Hello,

From a certain perspective, your proposal is close to forcetypeassert

[mattysweeps]

Update: dominikh/go-tools#1328 has been closed as out of scope for staticcheck:

From dominikh/go-tools#1328 (comment)

I'm afraid this is out of scope for Staticcheck. We cannot tell if another code path or convention guarantees that the key exists in the map, rendering the guard unnecessary. Eagerly enforcing the use of guards would thus lead to false positives. Staticcheck only aims to flag code that is guaranteed to be problematic, not that could be problematic.

[mattysweeps]

Hello,

From a certain perspective, your proposal is close to forcetypeassert

@ldez Agreed, forcetypeassert looks very similar.

Ideally the linter should look for IndexExpr where X is a map literal, and then ensure the left hand side of the wrapping assignment has both variables.

It might be complicated for expressions which are not directly part of an assignment - an additional restriction might be necessary to ensure IndexExpr on map values are always an assignment