Fix per-analyzer overrides of default-disabled groups

Currently behavior of config groups with `default: false` is buggy. The
intention is that adding an empty suppression section for that group to a
specific analyzer config should enable reporting for that analyzer. i.e.,

```
groups:
  - name: foo
    regex: "^foo/"
    default: false
global:
  ...
analyzers:
  asmdecl:
    foo: # Enabled.
```

This should enable the foo group only for asmdecl. Unfortunately, today the
actual behavior depends on the contents of the `global:` section. If `global:`
contains an entry for foo, then it will work as described. If `global:` does
_not_ contain an entry for foo, then the group default (disabled) always
applies and the individual analyzer options have no effect.

The cause of this is confusion in `AnalyzerConfig.shouldReport`, which doesn't
distinguish between explicit suppression via a global suppression/exclude and
simply having no configuration at all. Make this more explicit, so that the no
configuration case can continue to per-analyzer configuration before falling
back to the group default.

The last test case in the added test fails without this change.

This re-enables several opted-in analyzers for external dependencies, which
have gained a few more false positives to suppress.

PiperOrigin-RevId: 386904725

5 files changed, 324 insertions, 7 deletions

diff --git a/nogo.yaml b/nogo.yaml
index 9b7fc5c8f..d7a3ad956 100644
--- a/nogo.yaml
+++ b/nogo.yaml
@@ -130,6 +130,8 @@ analyzers:
     external: # Enabled.
   unreachable:
     external: # Enabled.
+      exclude:
+        - ".*protobuf/.*.go"
   unsafeptr:
     internal:
       exclude:
diff --git a/tools/nogo/BUILD b/tools/nogo/BUILD
index a7e280b32..27fe48680 100644
--- a/tools/nogo/BUILD
+++ b/tools/nogo/BUILD
@@ -1,4 +1,4 @@
-load("//tools:defs.bzl", "bzl_library", "go_library", "select_goarch", "select_goos")
+load("//tools:defs.bzl", "bzl_library", "go_library", "go_test", "select_goarch", "select_goos")
 load("//tools/nogo:defs.bzl", "nogo_objdump_tool", "nogo_stdlib", "nogo_target")
 
 package(licenses = ["notice"])
@@ -73,6 +73,12 @@ go_library(
     ],
 )
 
+go_test(
+    name = "nogo_test",
+    srcs = ["config_test.go"],
+    library = ":nogo",
+)
+
 bzl_library(
     name = "defs_bzl",
     srcs = ["defs.bzl"],
diff --git a/tools/nogo/config.go b/tools/nogo/config.go
index 6436f9d34..ee2533610 100644
--- a/tools/nogo/config.go
+++ b/tools/nogo/config.go
@@ -186,16 +186,19 @@ func (a AnalyzerConfig) merge(other AnalyzerConfig) {
 	}
 }
 
-func (a AnalyzerConfig) shouldReport(groupConfig *Group, fullPos, msg string) bool {
+// shouldReport returns whether the finding should be reported or suppressed.
+// It returns !ok if there is no configuration sufficient to decide one way or
+// another.
+func (a AnalyzerConfig) shouldReport(groupConfig *Group, fullPos, msg string) (report, ok bool) {
 	gc, ok := a[groupConfig.Name]
 	if !ok {
-		return groupConfig.Default
+		return false, false
 	}
 
 	// Note that if a section appears for a particular group
 	// for a particular analyzer, then it will now be enabled,
 	// and the group default no longer applies.
-	return gc.shouldReport(fullPos, msg)
+	return gc.shouldReport(fullPos, msg), true
 }
 
 // Config is a nogo configuration.
@@ -298,7 +301,8 @@ func (c *Config) ShouldReport(finding Finding) bool {
 	}
 
 	// Suppress via global rule?
-	if !c.Global.shouldReport(groupConfig, fullPos, finding.Message) {
+	report, ok := c.Global.shouldReport(groupConfig, fullPos, finding.Message)
+	if ok && !report {
 		return false
 	}
 
@@ -307,5 +311,9 @@ func (c *Config) ShouldReport(finding Finding) bool {
 	if !ok {
 		return groupConfig.Default
 	}
-	return ac.shouldReport(groupConfig, fullPos, finding.Message)
+	report, ok = ac.shouldReport(groupConfig, fullPos, finding.Message)
+	if !ok {
+		return groupConfig.Default
+	}
+	return report
 }
diff --git a/tools/nogo/config_test.go b/tools/nogo/config_test.go
new file mode 100644
index 000000000..685cffbec
--- /dev/null
+++ b/tools/nogo/config_test.go
@@ -0,0 +1,301 @@
+// Copyright 2021 The gVisor Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.package nogo
+package nogo
+
+import (
+	"go/token"
+	"testing"
+)
+
+// TestShouldReport validates the suppression behavior of Config.ShouldReport.
+func TestShouldReport(t *testing.T) {
+	config := &Config{
+		Groups: []Group{
+			{
+				Name:    "default-enabled",
+				Regex:   "^default-enabled/",
+				Default: true,
+			},
+			{
+				Name:    "default-disabled",
+				Regex:   "^default-disabled/",
+				Default: false,
+			},
+			{
+				Name:    "default-disabled-omitted-from-global",
+				Regex:   "^default-disabled-omitted-from-global/",
+				Default: false,
+			},
+		},
+		Global: AnalyzerConfig{
+			"default-enabled": &ItemConfig{
+				Exclude:  []string{"excluded.go"},
+				Suppress: []string{"suppressed"},
+			},
+			"default-disabled": &ItemConfig{
+				Exclude:  []string{"excluded.go"},
+				Suppress: []string{"suppressed"},
+			},
+			// Omitting default-disabled-omitted-from-global here
+			// has no effect on configuration below.
+		},
+		Analyzers: map[AnalyzerName]AnalyzerConfig{
+			"analyzer-suppressions": AnalyzerConfig{
+				// Suppress some.
+				"default-enabled": &ItemConfig{
+					Exclude:  []string{"limited-exclude.go"},
+					Suppress: []string{"limited suppress"},
+				},
+				// Enable all.
+				"default-disabled": nil,
+			},
+			"enabled-for-default-disabled": AnalyzerConfig{
+				"default-disabled":                     nil,
+				"default-disabled-omitted-from-global": nil,
+			},
+		},
+	}
+
+	if err := config.Compile(); err != nil {
+		t.Fatalf("Compile(%+v) = %v, want nil", config, err)
+	}
+
+	cases := []struct {
+		name    string
+		finding Finding
+		want    bool
+	}{
+		{
+			name: "enabled",
+			finding: Finding{
+				Category: "foo",
+				Position: token.Position{
+					Filename: "default-enabled/file.go",
+					Offset:   0,
+					Line:     1,
+					Column:   1,
+				},
+				Message: "message",
+			},
+			want: true,
+		},
+		{
+			name: "ungrouped",
+			finding: Finding{
+				Category: "foo",
+				Position: token.Position{
+					Filename: "ungrouped/file.go",
+					Offset:   0,
+					Line:     1,
+					Column:   1,
+				},
+				Message: "message",
+			},
+			want: true,
+		},
+		{
+			name: "suppressed",
+			finding: Finding{
+				Category: "foo",
+				Position: token.Position{
+					Filename: "default-enabled/file.go",
+					Offset:   0,
+					Line:     1,
+					Column:   1,
+				},
+				Message: "message suppressed",
+			},
+			want: false,
+		},
+		{
+			name: "excluded",
+			finding: Finding{
+				Category: "foo",
+				Position: token.Position{
+					Filename: "default-enabled/excluded.go",
+					Offset:   0,
+					Line:     1,
+					Column:   1,
+				},
+				Message: "message",
+			},
+			want: false,
+		},
+		{
+			name: "disabled",
+			finding: Finding{
+				Category: "foo",
+				Position: token.Position{
+					Filename: "default-disabled/file.go",
+					Offset:   0,
+					Line:     1,
+					Column:   1,
+				},
+				Message: "message",
+			},
+			want: false,
+		},
+		{
+			name: "analyzer suppressed",
+			finding: Finding{
+				Category: "analyzer-suppressions",
+				Position: token.Position{
+					Filename: "default-enabled/file.go",
+					Offset:   0,
+					Line:     1,
+					Column:   1,
+				},
+				Message: "message limited suppress",
+			},
+			want: false,
+		},
+		{
+			name: "analyzer suppressed not global",
+			finding: Finding{
+				// Doesn't apply outside of analyzer-suppressions.
+				Category: "foo",
+				Position: token.Position{
+					Filename: "default-enabled/file.go",
+					Offset:   0,
+					Line:     1,
+					Column:   1,
+				},
+				Message: "message limited suppress",
+			},
+			want: true,
+		},
+		{
+			name: "analyzer suppressed grouped",
+			finding: Finding{
+				Category: "analyzer-suppressions",
+				Position: token.Position{
+					// Doesn't apply outside of default-enabled.
+					Filename: "default-disabled/file.go",
+					Offset:   0,
+					Line:     1,
+					Column:   1,
+				},
+				Message: "message limited suppress",
+			},
+			want: true,
+		},
+		{
+			name: "analyzer excluded",
+			finding: Finding{
+				Category: "analyzer-suppressions",
+				Position: token.Position{
+					Filename: "default-enabled/limited-exclude.go",
+					Offset:   0,
+					Line:     1,
+					Column:   1,
+				},
+				Message: "message",
+			},
+			want: false,
+		},
+		{
+			name: "analyzer excluded not global",
+			finding: Finding{
+				// Doesn't apply outside of analyzer-suppressions.
+				Category: "foo",
+				Position: token.Position{
+					Filename: "default-enabled/limited-exclude.go",
+					Offset:   0,
+					Line:     1,
+					Column:   1,
+				},
+				Message: "message",
+			},
+			want: true,
+		},
+		{
+			name: "analyzer excluded grouped",
+			finding: Finding{
+				Category: "analyzer-suppressions",
+				Position: token.Position{
+					// Doesn't apply outside of default-enabled.
+					Filename: "default-disabled/limited-exclude.go",
+					Offset:   0,
+					Line:     1,
+					Column:   1,
+				},
+				Message: "message",
+			},
+			want: true,
+		},
+		{
+			name: "disabled-omitted",
+			finding: Finding{
+				Category: "foo",
+				Position: token.Position{
+					Filename: "default-disabled-omitted-from-global/file.go",
+					Offset:   0,
+					Line:     1,
+					Column:   1,
+				},
+				Message: "message",
+			},
+			want: false,
+		},
+		{
+			name: "default enabled applies to customized analyzer",
+			finding: Finding{
+				Category: "enabled-for-default-disabled",
+				Position: token.Position{
+					Filename: "default-enabled/file.go",
+					Offset:   0,
+					Line:     1,
+					Column:   1,
+				},
+				Message: "message",
+			},
+			want: true,
+		},
+		{
+			name: "default overridden in customized analyzer",
+			finding: Finding{
+				Category: "enabled-for-default-disabled",
+				Position: token.Position{
+					Filename: "default-disabled/file.go",
+					Offset:   0,
+					Line:     1,
+					Column:   1,
+				},
+				Message: "message",
+			},
+			want: true,
+		},
+		{
+			name: "default overridden in customized analyzer even when omitted from global",
+			finding: Finding{
+				Category: "enabled-for-default-disabled",
+				Position: token.Position{
+					Filename: "default-disabled-omitted-from-global/file.go",
+					Offset:   0,
+					Line:     1,
+					Column:   1,
+				},
+				Message: "message",
+			},
+			want: true,
+		},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			if got := config.ShouldReport(tc.finding); got != tc.want {
+				t.Errorf("ShouldReport(%+v) = %v, want %v", tc.finding, got, tc.want)
+			}
+		})
+	}
+}
diff --git a/tools/nogo/findings.go b/tools/nogo/findings.go
index 329a7062e..a73bf1a09 100644
--- a/tools/nogo/findings.go
+++ b/tools/nogo/findings.go
@@ -109,7 +109,7 @@ func ExtractFindingsFromFile(filename string, asJSON bool) (FindingSet, error) {
 	return ExtractFindingsFrom(r, asJSON)
 }
 
-// ExtractFindingsFromBytes loads findings from bytes.
+// ExtractFindingsFrom loads findings from an io.Reader.
 func ExtractFindingsFrom(r io.Reader, asJSON bool) (findings FindingSet, err error) {
 	if asJSON {
 		dec := json.NewDecoder(r)