cmd/vet: warn when iterating over an array

[dsnet]

See #18625 for an interesting bug where large stack growth was caused by this. To avoid false positives, maybe vet should only warn if the array size exceeds some number of bytes?

\cc @robpike @dominikh @minux

[minux]

I'm also thinking about this. This is definitely not a correctness issue, but it did have major performance implications (when the array is large enough) The reason I support this check being in cmd/vet is that this kind of issues is far from trivial and we current lack any facilities to catch excess stack usage (even if we have have stack usage tests, the developer still need to remember to use it, and determining the correct threshold is also very difficult and the correct threshold could change from release to release due to compiler improvements.) Of course, determine the correct warning threshold would be the hard part of this issue. I think anything more than a few kB is suspicious.

[JayNakrani]

... maybe vet should only warn if the array size exceeds some number of bytes?

Just out of curiosity, how can vet statically determine the size of an array? Never mind. Just saw that the arrays in question have fixed length.

[robpike]

It is correct to do so, and can even be the right thing to do in many cases. It's therefore not something that vet can complain about. (See the src/cmd/vet/README for details.)

Perhaps golint is a better choice. Feel free to suggest that.

[minux]

@robpike, I've been thinking about a possible use case for this, but couldn't think of any except when you also want to modify the array in the loop body. Is that what you have in mind? Any other use cases? If we do make a golint pass, we might need to exclude this case.

[dsnet]

It's not obvious to me that this is a style issue. Even if it were, for k, v := range array (the "wrong" one) certainly looks nicer and less surprising than for k, v := range array[:] (the "right" one).

However, I can see why this doesn't quite fit vet either, since use of it still produces a correct program. I'm starting to see a significant number of issues that don't quite fit vet (focus on correctness), and doesn't fit lint (focus on style)... there seems to be a number of checks proposed that catch problems that aren't strictly wrong, but heavily suspicious, this being an example of one. Maybe these checks should go into a third-party tool like go-staticcheck?

[dominikh]

I don't think it's a fit for any of my tools, either, for essentially the same reason as for why it shouldn't go into vet.