nogo nolint comments don't work across dependencies when using nilaway

[illicitonion]

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.

#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.

my-lib/panicboi.go:9:1: error: Potential nil panic detected. Observed nil flow from source to dereference point: 
	-> my-lib/panicboi.go:14:12: literal `nil` returned from `MakeANilPanicStruct()` in position 0
	-> __main__/main.go:12:2: result 0 of `MakeANilPanicStruct()` used as receiver to call `OhnoIMayPanic()`
	-> my-lib/panicboi.go:9:10: read by method receiver `p` accessed field `anField`

In this case, the lint is actually raised when linting main.go, but the Position of the diagnostic is considered to be in my-lib/panicboi.go.

I tried to ignore this diagnostic by adding comments to both main.go and panicboi.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 in main.go that had a comment associated.

I'm not sure how we should be addressing this; it feels like some combination of:

1. We should propagate nolint comments across dependency edges so that nogo would see there were comments in panicboi.go
2. nilaway could propagate the Related field on its Diagnostic showing each file in the flow path, which may allow us to at least filter based on main.go:12 being in the report.

But I don't really have enough context to suggest fixes here.

[illicitonion]

cc @patrickmscott who added nolint support, @sluongng who is maybe the nogo expert

[sluongng]

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.

[sluongng]

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

[sluongng]

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.

[linzhp]

I haven't seen this in Uber, but we do use nilaway. @yuxincs may know more.

[yuxincs]

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):

rules_go/go/tools/builders/nogo_main.go

Lines 205 to 230 in b4b04b8

	// Process nolint directives similar to golangci-lint.
	for _, f := range pkg.syntax {
	// CommentMap will correctly associate comments to the largest node group
	// applicable. This handles inline comments that might trail a large
	// assignment and will apply the comment to the entire assignment.
	commentMap := ast.NewCommentMap(pkg.fset, f, f.Comments)
	for node, groups := range commentMap {
	rng := &Range{
	from: pkg.fset.Position(node.Pos()),
	to: pkg.fset.Position(node.End()).Line,
	}
	for _, group := range groups {
	for _, comm := range group.List {
	linters, ok := parseNolint(comm.Text)
	if !ok {
	continue
	}
	for analyzer, act := range actions {
	if linters == nil || linters[analyzer.Name] {
	act.nolint = append(act.nolint, rng)
	}
	}
	}
	}
	}
	}

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 uber-go/nilaway#91 and uber-go/nilaway#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 😉

[sluongng]

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.

[fmeum]

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.

[linzhp]

.x files have nogo facts:

rules_go/go/tools/builders/compilepkg.go

Lines 538 to 541 in b4b04b8

	// Extract the export data file and pack it in an .x archive together with the
	// nogo facts file (if there is one). This allows compile actions to depend
	// on .x files only, so we don't need to recompile a package when one of its
	// imports changes in a way that doesn't affect export data.

However, we may want to move nogo facts out of .x after #3707