Open illicitonion opened 9 months ago
cc @patrickmscott who added nolint support, @sluongng who is maybe the nogo expert
haha, I just tried setting up nilaway
today and then gave up after realizing that it flagged too many false positives.
I think your analysis is on point here. #3562 simply added a fancier report func that consumes a Diagnostic
struct from nogo run. So I think the appropriate fix here is to agree with nilaway
on how their Diagnostic could be programmatically parsed for ignore comments. https://github.com/search?q=repo%3Auber-go%2Fnilaway+%2Fanalysis.Diagnostic%7B%2F&type=code
I don't think propagating things across the build graph should be considered. It's expensive and will impact larger graphs negatively if things were to accumulate. If that is ever needed, we should simply rewrite nogo entirely.
Here is the Diagnostic struct for future curious readers https://cs.opensource.google/go/x/tools/+/master:go/analysis/diagnostic.go;l=17;drc=dbd600188431770bd182cfa975efa023bea79af1
cc: @linzhp
Im curious if Uber has something internally for this, or if this use case is not yet seen with nilaway
usage inside Uber.
I haven't seen this in Uber, but we do use nilaway. @yuxincs may know more.
NilAway is arguably the most sophisticated go (and nogo) analyzer, with support for inference across packages. What this means is that it could well be the case that NilAway finds a problem in an upstream package (and reported the error on the upstream package's Pos
, while analyzing the downstream package, as demonstrated in the issue here.
However, the nolint parsing support in nogo (and I think in other drivers as well), didn't take into account for such scenarios. The nolint comments are parsed only for the current package (and suppressed for current package): https://github.com/bazelbuild/rules_go/blob/b4b04b8dcb221655f64d7eb345ca53b797967d68/go/tools/builders/nogo_main.go#L205-L230 since the ASTs are only available for the current package I think. Therefore nogo doesn't even see the nolint suppressions on the upstream packages.
What we could do is to parse and store the nolint range information to artifact (fact file for example) and load it when analyzing downstream packages, such that we have complete nolint information to support cross-package linters like NilAway.
But I understand that nogo might not really want to do that given almost all other analyzers do not have this need and this may unnecessarily add pressures on caches, so we're thinking of adding such support and do suppressions directly in NilAway (via the Fact
mechanism). See https://github.com/uber-go/nilaway/issues/91 and https://github.com/uber-go/nilaway/issues/143 for more information :)
haha, I just tried setting up nilaway today and then gave up after realizing that it flagged too many false positives.
Please retry in the future, we are constantly trying to reduce false positives as much as possible 😉
What we could do is to parse and store the nolint range information to artifact (fact file for example) and load it when analyzing downstream packages, such that we have complete nolint information to support cross-package linters like NilAway.
I am not against adding an additional artifact to help aid downstream nogo
runs.
In fact, I think @fmeum mentioned something about *.x
files might already contain some of the analysis data to help aid compilation?
As long as the data is fine grain and cached by Bazel, it should be ok to add.
In fact, I think @fmeum mentioned something about
*.x
files might already contain some of the analysis data to help aid compilation?
I still haven't been able to delve into the contents of *.x
files unfortunately.
.x
files have nogo facts: https://github.com/bazelbuild/rules_go/blob/b4b04b8dcb221655f64d7eb345ca53b797967d68/go/tools/builders/compilepkg.go#L538-L541
However, we may want to move nogo facts out of .x
after https://github.com/bazelbuild/rules_go/pull/3707
What version of rules_go are you using?
v0.43.0
What version of gazelle are you using?
v0.34.0
What version of Bazel are you using?
6.4.0
Does this issue reproduce with the latest releases of all the above?
Yes
What operating system and processor architecture are you using?
macOS, arm64
What did you do?
Repro in https://github.com/illicitonion/repro-bazel-rules_go-nogo-nilaway
Note: I'm not confident that rules_go is the place at fault here, there seem to be a few interacting pieces.
nilaway is a static analyser compatible with nogo.
https://github.com/bazelbuild/rules_go/pull/3562 added the ability to ignore diagnostics using comments.
Because nilaway performs call-chain analysis, it may detect diagnostics which "belong" across multiple files, e.g.
In this case, the lint is actually raised when linting
main.go
, but thePosition
of the diagnostic is considered to be inmy-lib/panicboi.go
.I tried to ignore this diagnostic by adding comments to both
main.go
andpanicboi.go
expecting at least one of them to work, but was unable to silence the diagnostic.It appears impossible to ignore these diagnostics using comments. Ignore comments in
panicboi.go
aren't propagated across dependency edges, and there's no way to identify when reporting the diagnostic that it touched some code inmain.go
that had a comment associated.I'm not sure how we should be addressing this; it feels like some combination of:
Related
field on itsDiagnostic
showing each file in the flow path, which may allow us to at least filter based onmain.go:12
being in the report.But I don't really have enough context to suggest fixes here.