search

bazel-contrib / rules_go

Go rules for Bazel
Apache License 2.0
1.38k stars 656 forks source link

nogo is very noisy in 0.50.0 #4080

Closed TvdW closed 1 month ago

TvdW commented 1 month ago

What version of rules_go/gazelle/Bazel are you using?

rules_go 0.50.0, gazelle 0.38.0, Bazel 7.3.1, Go 1.23.0

What did you do?

I wrote some Go code that doesn't compile, and then I tried to build it with bazel build //...

What did you expect to see?

An error message

What did you see instead?

28 error messages.

Each nogo analyzer is individually reporting on the compilation failure. For example:

analyzer "asmdecl" failed: analysis skipped due to type-checking error: path/to/file.go:142:71: cannot use b.awsConfig (variable of type *aws.Config) as aws.Config value in argument to s3.NewFromConfig

... repeated for every nogo analyzer.

I can't seem to reproduce this with every compilation failure, which is interesting. The line that manages to trigger this is s3.NewFromConfig(b.awsConfig) from github.com/aws/aws-sdk-go-v2/service/s3, where b.awsConfig is a pointer instead of a struct.

fmeum commented 1 month ago

I didn't intentionally change anything non-trivial about what the actual nogo binary does, so I'm a bit surprised to see this. Are you that it didn't happen with 0.49.0? Could you perhaps run a bisect?

TvdW commented 1 month ago

Right, yes, I should have done that 😄

Took a moment, bisect gave me several different answers until I added --keep_going. Now it's pretty confident that the problem is 29d4e5dbcfa702c479311f0bb4a929a236666a46.

This makes sense: the validation and compilation actions now happen in parallel. This is what caused my bisect to be unstable, but also causes the validation errors to sometimes be reported before the compilation fails.

I guess that adding the compilation as a dependency of the validation defeats the point of your change, so I guess the fix here could be to reduce the output length of nogo if the input file doesn't pass basic validation?

fmeum commented 1 month ago

The nogo action depends on the compilation action if and only if the compile action isn't pure, so that's probably why you are seeing this issue in some cases but not others.

I think that we should just skip all analyzers if type-checking fails.

fmeum commented 1 month ago

@TvdW Could you test https://github.com/bazelbuild/rules_go/pull/4083?

TvdW commented 1 month ago

@fmeum Looks good!

28 analyzers skipped due to type-checking error: [...]