Do not allow check warnings (#1670)

Remove the option for checks to emit only warning messages. Checks that
are included in the go_checker target now fail builds. The only way to
change this behavior is to remove the check from go_checker so that it
does not run at compile time.

go/checks.rst

@@ -142,8 +142,8 @@ repository are imported into other workspaces and built there.
 Configuring checks
 ~~~~~~~~~~~~~~~~~~
 
-By default, checks print their findings but do not interrupt compilation. This
-default behavior can be overridden with a JSON configuration file.
+By default, checks apply to all files. This behavior can be changed with a JSON
+configuration file.
 
 The top-level JSON object in the file must be keyed by the name of the check
 being configured. These names must match the Analysis.Name of the registered
@@ -157,14 +157,6 @@ contain the following key-value pairs:
 +----------------------------+---------------------------------------------------------------------+
 | Description of this check configuration.                                                         |
 +----------------------------+---------------------------------------------------------------------+
-| ``"severity"``             | :type:`string`                                                      |
-+----------------------------+---------------------------------------------------------------------+
-| Determines the actions taken when a check violation is found. It must take on one of the         |
-| following values:                                                                                |
-|                                                                                                  |
-| - ``"WARNING"`` : warning message emitted at compile-time                                        |
-| - ``"ERROR"``   : fails compilation in addition to emitting an error message                     |
-+----------------------------+---------------------------------------------------------------------+
 | ``"apply_to"``             | :type:`dictionary, string to string`                                |
 +----------------------------+---------------------------------------------------------------------+
 | Specifies files that this check will exclusively apply to.                                       |
@@ -182,31 +174,26 @@ contain the following key-value pairs:
 Example
 ^^^^^^^
 
-The following configuration file configures the checks named ``importunsafe``,
-``unsafedom``, and ``loopclosure``.
+The following configuration file configures the checks named ``importunsafe``
+and ``unsafedom``. Since the ``loopclosure`` check is not explicitly configured,
+it will apply to all Go files built by Bazel.
 
 .. code:: json
 
     {
       "importunsafe": {
-        "severity": "ERROR",
         "whitelist": {
           "src/foo.go": "manually verified that behavior is working-as-intended",
           "src/bar.go": "see issue #1337"
         }
       },
       "unsafedom": {
-        "severity": "WARNING",
         "apply_to": {
           "src/js/*": ""
         },
         "whitelist": {
           "src/(third_party|vendor)/*": "enforce DOM safety requirements only on first-party code"
         }
-      },
-      "loopclosure": {
-        "description": "fail builds without exception since we know this check is 100% accurate",
-        "severity": "ERROR"
       }
     }
 

go/tools/builders/checker_main.go

@@ -98,7 +98,6 @@ func run(args []string) error {
 	}
 	// Collate analysis results.
 	var allFindings []*analysis.Finding
-	failBuild := false
 	for i := 0; i < len(analysis.Analyses()); i++ {
 		result := <-c
 		if result.err != nil {
@@ -111,13 +110,10 @@ func run(args []string) error {
 		}
 		config, ok := configs[result.name]
 		if !ok {
-			// The default behavior is not to fail builds but print analysis findings.
+			// If the check is not explicitly configured, it applies to all files.
 			allFindings = append(allFindings, result.findings...)
 			continue
 		}
-		if config.severity == severityError {
-			failBuild = true
-		}
 		// Discard findings based on the check configuration.
 		for _, finding := range result.findings {
 			filename := fset.File(finding.Pos).Name()
@@ -141,35 +137,24 @@ func run(args []string) error {
 			}
 		}
 	}
-	// Print analysis results, returning an error to fail the build if necessary.
-	if len(allFindings) != 0 {
-		sort.Slice(allFindings, func(i, j int) bool {
-			return allFindings[i].Pos < allFindings[j].Pos
-		})
-		errMsg := "errors found during build-time code analysis:\n"
-		for _, f := range allFindings {
-			errMsg += fmt.Sprintf("%s: %s\n", fset.Position(f.Pos), f.Message)
-		}
-		if failBuild {
-			return errors.New(errMsg)
-		}
-		log.Println(errMsg)
+	// Return an error that fails the build if we have any findings.
+	if len(allFindings) == 0 {
+		return nil
 	}
-	return nil
+	sort.Slice(allFindings, func(i, j int) bool {
+		return allFindings[i].Pos < allFindings[j].Pos
+	})
+	errMsg := "errors found during build-time code analysis:\n"
+	for _, f := range allFindings {
+		errMsg += fmt.Sprintf("%s: %s\n", fset.Position(f.Pos), f.Message)
+	}
+	return errors.New(errMsg)
 }
 
 type config struct {
-	severity           severity
 	applyTo, whitelist map[string]bool
 }
 
-type severity uint8
-
-const (
-	severityWarning severity = iota
-	severityError
-)
-
 func main() {
 	log.SetFlags(0) // no timestamp
 	log.SetPrefix("GoChecker: ")

go/tools/builders/generate_checker_main.go

@@ -41,7 +41,6 @@ import (
 var configs = map[string]config{
 {{- range $name, $config := .Configs}}
 	{{printf "%q" $name}}: config{
-		severity:  {{$config.Severity}},
 		{{- if $config.ApplyTo -}}
 		applyTo:  map[string]bool{
 			{{range $path, $comment := $config.ApplyTo -}}
@@ -125,13 +124,8 @@ func buildConfig(path string) (Configs, error) {
 		return Configs{}, fmt.Errorf("failed to unmarshal config file: %v", err)
 	}
 	for name, config := range configs {
-		s, ok := severityStringtoEnumName[config.Severity]
-		if !ok {
-			return Configs{}, fmt.Errorf("invalid severity %q", config.Severity)
-		}
 		configs[name] = Config{
 			// Description is currently unused.
-			Severity:  s,
 			ApplyTo:   config.ApplyTo,
 			Whitelist: config.Whitelist,
 		}
@@ -142,12 +136,7 @@ func buildConfig(path string) (Configs, error) {
 type Configs map[string]Config
 
 type Config struct {
-	Description, Severity string
-	ApplyTo               map[string]string `json:"apply_to"`
-	Whitelist             map[string]string
-}
-
-var severityStringtoEnumName = map[string]string{
-	"WARNING": "severityWarning",
-	"ERROR":   "severityError",
+	Description string
+	ApplyTo     map[string]string `json:"apply_to"`
+	Whitelist   map[string]string
 }

tests/core/checks/custom/BUILD.bazel

@@ -84,18 +84,14 @@ bazel_test(
     name = "custom_checks_default_config",
     build = BUILD_TMPL.format(config = ""),
     go_checker = "@//:my_checker",
-    check = BUILD_PASSED_TMPL.format(
+    check = BUILD_FAILED_TMPL.format(
         check_err =
-            CONTAINS_ERR_TMPL.format(err = "custom/foo_func.go:4:1: function must not be named Foo") +
-            CONTAINS_ERR_TMPL.format(err = "custom/return_bool/return_bool.go:4:1: function must not return bool") +
-            CONTAINS_ERR_TMPL.format(err = "custom/import_fmt.go:5:2: package fmt must not be imported")
+            CONTAINS_ERR_TMPL.format(err = "custom/has_errors.go:4:2: package fmt must not be imported") +
+            CONTAINS_ERR_TMPL.format(err = "custom/has_errors.go:7:1: function must not return bool") +
+            CONTAINS_ERR_TMPL.format(err = "custom/has_errors.go:7:1: function must not be named Foo")
     ),
     command = "build",
-    targets = [
-        ":return_bool",
-        ":import_fmt",
-        ":foo_func",
-    ],
+    targets = [":has_errors"],
     extra_files = EXTRA_FILES,
 )
 
@@ -105,18 +101,12 @@ bazel_test(
     go_checker = "@//:my_checker",
     check = BUILD_FAILED_TMPL.format(
         check_err =
-            CONTAINS_ERR_TMPL.format(err = "custom/foo_func.go:4:1: function must not be named Foo") +
-            DOES_NOT_CONTAIN_ERR_TMPL.format(err = "custom/return_bool/return_bool.go:") +
-            DOES_NOT_CONTAIN_ERR_TMPL.format(err = "custom/import_fmt.go:")
+            CONTAINS_ERR_TMPL.format(err = "custom/has_errors.go:4:2: package fmt must not be imported") +
+            DOES_NOT_CONTAIN_ERR_TMPL.format(err = "custom/has_errors.go:7:1: function must not return bool") +
+            CONTAINS_ERR_TMPL.format(err = "custom/has_errors.go:7:1: function must not be named Foo")
     ),
     command = "build",
-    targets = [
-        ":return_bool",
-        ":import_fmt",
-        # Note: we have configured foo_func_check to fail builds, so we need :foo_func
-        # to be built last so as not to short-circuit the test.
-        ":foo_func",
-    ],
+    targets = [":has_errors"],
     extra_files = EXTRA_FILES,
 )
 
@@ -137,21 +127,9 @@ bazel_test(
 # fail builds or interfere with other analyses.
 
 go_library(
-    name = "import_fmt",
-    srcs = ["import_fmt.go"],
-    importpath = "importfmt",
-)
-
-go_library(
-    name = "return_bool",
-    srcs = ["return_bool/return_bool.go"],
-    importpath = "returnbool",
-)
-
-go_library(
-    name = "foo_func",
-    srcs = ["foo_func.go"],
-    importpath = "foofunc",
+    name = "has_errors",
+    srcs = ["has_errors.go"],
+    importpath = "haserrors",
 )
 
 go_library(

tests/core/checks/custom/README.rst

@@ -10,13 +10,13 @@ errors.
 
 custom_checks_default_config
 ----------------------------
-Verifies that custom checks print warnings without failing a go_library build
-when the checks are not explicitly configured.
+Verifies that custom checks print errors and fail a go_library build when a
+configuration file is not provided.
 
 custom_checks_custom_config
 ---------------------------
-Verifies that custom checks can be configured to fail builds and ignore errors
-in certain files using a custom configuration file.
+Verifies that custom checks can be configured to apply only to certain file
+paths using a custom configuration file.
 
 custom_checks_no_errors
 ------------------------

tests/core/checks/custom/config.json

@@ -1,18 +1,15 @@
 {
-  "return_bool_check": {
-    "severity": "WARNING",
-    "whitelist": {
-      "return_bool/*": "security-reviewed functions that safely return bool"
-    }
-  },
   "import_fmt_check": {
-    "severity": "WARNING",
     "apply_to": {
-      "return_bool/*": ""
+      "has_errors\\.go": ""
+    }
+  },
+  "return_bool_check": {
+    "whitelist": {
+      "has_.*\\.go": "security-reviewed functions that safely return bool"
     }
   },
   "foo_func_check": {
-    "description": "no exemptions since we know this check is 100% accurate",
-    "severity": "ERROR"
+    "description": "no exemptions since we know this check is 100% accurate"
   }
 }

tests/core/checks/custom/has_errors.go

@@ -0,0 +1,9 @@
+package haserrors
+
+import (
+	_ "fmt" // This should fail import_fmt_check
+)
+
+func Foo() bool { // This should fail foo_func_check
+	return true // This should fail return_bool_check
+}