What version of rules_go are you using? v0.43.0 <h3 dir="auto"

cc <a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url="

Here is the Diagnostic struct for future curious readers <a href="https://cs.opensourc

cc: <a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url=

I haven't seen this in Uber, but we do use nilaway. <a class="user-mention notranslate

In fact, I think <a class="user-mention notranslate" data-hovercard-type=

.x files have nogo facts: <div class="Box Box--co

nogo nolint comments don't work across dependencies when using nilaway about rules_go HOT 9 OPEN

illicitonion commented on May 29, 2024

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

from rules_go.

Comments (9)

sluongng commented on May 29, 2024 1

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.

from rules_go.

illicitonion commented on May 29, 2024

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

from rules_go.

sluongng commented on May 29, 2024

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

from rules_go.

sluongng commented on May 29, 2024

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.

from rules_go.

linzhp commented on May 29, 2024

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

from rules_go.

yuxincs commented on May 29, 2024

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 😉

from rules_go.

sluongng commented on May 29, 2024

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.

from rules_go.

fmeum commented on May 29, 2024

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.

from rules_go.

linzhp commented on May 29, 2024

.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

from rules_go.

nogo nolint comments don't work across dependencies when using nilaway about rules_go HOT 9 OPEN

Comments (9)

Related Issues (20)

Recommend Projects

React

Vue.js

Typescript

TensorFlow

Django

Laravel

D3

Recommend Topics

javascript

web

server

Machine learning

Visualization

Game

Recommend Org

Facebook

Microsoft

Google

Alibaba

D3

Tencent

	// 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)
	}
	}
	}
	}
	}
	}

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