From a42d3fd0aeb6c67c3fd2fb851845a1f88a298972 Mon Sep 17 00:00:00 2001
From: Michael Pratt <mpratt@google.com>
Date: Mon, 26 Jul 2021 10:24:11 -0700
Subject: 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
---
 tools/nogo/config.go | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

(limited to 'tools/nogo/config.go')

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
 }
-- 
cgit v1.2.3