New linter to forbid unusual map keys (e.g. pointers & interfaces)

[nightlyone]

It happens quite a few times that people add interfaces or pointer types as map keys.
Sometimes it can also be a regression introduced by an seemingly harmless change of a datatype can trigger an unintended change in a map key.

Of course there will be valid use cases for such keys, that's why the language allows them, but by default it usually is a correctness issue leading to duplicate data elsewhere.

What do you think?

[ldez]

First, just for the context:

The comparison operators == and != must be fully defined for operands of the key type; thus the key type must not be a function, map, or slice. If the key type is an interface type, these comparison operators must be defined for the dynamic key values; failure will cause a run-time panic.

https://go.dev/ref/spec#Map_types
https://go.dev/ref/spec#Comparison_operators

---

I can see 2 ways:

• checking map definition, will lead to detecting all the maps with any as a key.
  • not good because, for example, parsers could need this kind of key type.
  • also an interface can be used with non-pointer structures.
  • So this is not a good way.
• checking the value type of each key put into the map
  • but the problem is not on the type but on the value: for example, if you are using a pointer, it's ok if it's the same pointer but not ok if the content is the same but not the same pointer.
  • So it can lead to false positives.

We can also think of map wrappers like sync.Map.

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

I have the feeling this will lead to a lot of false positives 🤔

[nightlyone]

Thank you for the feedback. I am aware that there are valid use cases for such map keys.

I see some situations

• structs as keys where accidentally an element is added that is a pointer
• a type in that structure accidentally changes from value type to interface due to refactoring.
• a map key is intentionally a pointer and comes from a static set of pointers. In the Go compiler that is done heavily. So I wouldn't enable that by default. But still allow to configure this and allow nolint markings.
• a map key is reflect.Type. that is one of most popular interfaces used as map key, so allowing this by default is sane. Here I would suggest to maintain a list of interfaces that are always comparable where this is safe and also allow nolint markings for the odd cases where the author knows better.
• For codebases where this is rarely an issue the linter can always be disabled.

Intention of the suggested linter isn't expert level code, but code written by young talents learning Go on the job an being surprised by those issues every now and then.

Also catching effects of large scale refactorings when changing type usages from value to pointer and e.g. having a local type alias on it may even surprise expert Gophers.