search

bazelbuild / rules_go

Go rules for Bazel
Apache License 2.0
1.36k stars 635 forks source link

Update go vet with latest analyzers #3902

Open sluongng opened 3 months ago

sluongng commented 3 months ago

Upgrade golang.org/x/tools to v0.18.0. Note that the latest v0.19.0 has a bug that we should avoid using.

Update the analyzers (passes) in TOOLS_NOGO and the ones automatically included when vet = True is configured. The vet analyzers are taken from go tool vet help documentation.

fmeum commented 3 months ago

The nogo failure looks like it's easier to fix than ignore.

sluongng commented 3 months ago

@fmeum is it though?

The fieldalignment analyzer is quite noisy and is triggered on proto-generated code? 🤔

Im in favor of disabling it with a TODO.

fmeum commented 3 months ago

@fmeum is it though?

The fieldalignment analyzer is quite noisy and is triggered on proto-generated code? 🤔

Im in favor of disabling it with a TODO.

True, I missed that this was the generated code, not the test. In that case I agree that disabling it by default is the right move.

sluongng commented 3 months ago

Yeah. The other option is to add a relevant nogo_config.json file to exclude proto-generated files for that analyzer.

I don't think it's worth the noise it gonna generate for downstream users though.

sluongng commented 3 months ago

Hmm, these dependencies upgrades turn out to be a huge PITA.

Especially, for the popular_repo test, I think we should consider dropping x/tools testing entirely. The repo is where gopls is being actively developed and the recent code quality is not that high. v0.19.0 contains a nil pointer bug and a bunch of build/test targets in that repo are not gazelle/rules_go compatible.

I also had to patch Gazelle temporarily to avoid analyzer duplication. It should be a temp workaround until (a) we remove that in Gazelle, then (b) release a new Gazelle version, and then (c) upgrade the Gazelle version in rules_go.