nogo: ignore generated source files (#3216)

Under GoCompilePkg actions, there are several ways that source files
could have been modified / generated that is out of end users
control:

- go_test would generate an '_empty.go' file for the missing
  internal/external package to compile it.

- coverage instrumentation would rewrite test source files with a
  wrapper source file that collects coverage data into a determined
  variable.

- CGO usage would transform the original source files into several
  output Go and C source files with `go tool cgo ...`

Implement a mechanism to ignore the generated source files for the first
2 cases and instruct static analyzers to run over the original source
files wherever applicable.  This provides a better default for rules_go
users as static analysis errors over these generated files are outside
of their control and thus, could not be fixed.

Add a note regarding the CGO case for future investigations.

.bazelci/presubmit.yml

@@ -309,6 +309,7 @@ tasks:
     - "-//tests/core/go_test:data_test"
     - "-//tests/core/go_test:pwd_test"
     - "-//tests/core/nogo/coverage:coverage_test"
+    - "-//tests/core/nogo/coverage:gen_code_test"
     - "-//tests/core/race:race_test" # fails on Windows due to upstream bug, see issue #2911
     - "-//tests/core/stdlib:buildid_test"
     - "-//tests/examples/executable_name:executable_name"

go/tools/builders/compilepkg.go

@@ -184,6 +184,15 @@ func compileArchive(
 	}
 	defer cleanup()
 
+	// As part of compilation process, rules_go does generate and/or rewrite code
+	// based on the original source files.  We should only run static analysis
+	// over original source files and not the generated source as end users have
+	// little control over the generated source.
+	//
+	// nogoSrcsOrigin maps generated/rewritten source files back to original source.
+	// If the original source path is an empty string, exclude generated source from nogo run.
+	nogoSrcsOrigin := make(map[string]string)
+
 	if len(srcs.goSrcs) == 0 {
 		// We need to run the compiler to create a valid archive, even if there's
 		// nothing in it. GoPack will complain if we try to add assembly or cgo
@@ -194,13 +203,13 @@ func compileArchive(
 		// otherwise platforms without sandbox support may race to create/remove
 		// the file during parallel compilation.
 		emptyDir := filepath.Join(filepath.Dir(outPath), sanitizePathForIdentifier(importPath))
-		if err := os.Mkdir(emptyDir, 0700); err != nil {
+		if err := os.Mkdir(emptyDir, 0o700); err != nil {
 			return fmt.Errorf("could not create directory for _empty.go: %v", err)
 		}
 		defer os.RemoveAll(emptyDir)
 
 		emptyPath := filepath.Join(emptyDir, "_empty.go")
-		if err := os.WriteFile(emptyPath, []byte("package empty\n"), 0666); err != nil {
+		if err := os.WriteFile(emptyPath, []byte("package empty\n"), 0o666); err != nil {
 			return err
 		}
 
@@ -210,6 +219,8 @@ func compileArchive(
 			matched:  true,
 			pkg:      "empty",
 		})
+
+		nogoSrcsOrigin[emptyPath] = ""
 	}
 	packageName := srcs.goSrcs[0].pkg
 	var goSrcs, cgoSrcs []string
@@ -290,9 +301,11 @@ func compileArchive(
 
 			if i < len(goSrcs) {
 				goSrcs[i] = coverSrc
-			} else {
-				cgoSrcs[i-len(goSrcs)] = coverSrc
+				nogoSrcsOrigin[coverSrc] = origSrc
+				continue
 			}
+
+			cgoSrcs[i-len(goSrcs)] = coverSrc
 		}
 	}
 
@@ -312,7 +325,7 @@ func compileArchive(
 		gcFlags = append(gcFlags, createTrimPath(gcFlags, srcDir))
 	} else {
 		if cgoExportHPath != "" {
-			if err := ioutil.WriteFile(cgoExportHPath, nil, 0666); err != nil {
+			if err := ioutil.WriteFile(cgoExportHPath, nil, 0o666); err != nil {
 				return err
 			}
 		}
@@ -390,11 +403,31 @@ func compileArchive(
 	// Run nogo concurrently.
 	var nogoChan chan error
 	outFactsPath := filepath.Join(workDir, nogoFact)
-	if nogoPath != "" {
+	nogoSrcs := make([]string, 0, len(goSrcs))
+	for _, goSrc := range goSrcs {
+		// If source is found in the origin map, that means it's likely to be a generated source file
+		// so feed the original source file to static analyzers instead of the generated one.
+		//
+		// If origin is empty, that means the generated source file is not based on a user-provided source file
+		// thus ignore that entry entirely.
+		if originSrc, ok := nogoSrcsOrigin[goSrc]; ok {
+			if originSrc != "" {
+				nogoSrcs = append(nogoSrcs, originSrc)
+			}
+			continue
+		}
+
+		// TODO(sluongng): most likely what remains here are CGO-generated source files as the result of calling cgo2()
+		// Need to determine whether we want to feed these CGO-generated files into static analyzers.
+		//
+		// Add unknown origin source files into the mix.
+		nogoSrcs = append(nogoSrcs, goSrc)
+	}
+	if nogoPath != "" && len(nogoSrcs) > 0 {
 		ctx, cancel := context.WithCancel(context.Background())
 		nogoChan = make(chan error)
 		go func() {
-			nogoChan <- runNogo(ctx, workDir, nogoPath, goSrcs, deps, packagePath, importcfgPath, outFactsPath)
+			nogoChan <- runNogo(ctx, workDir, nogoPath, nogoSrcs, deps, packagePath, importcfgPath, outFactsPath)
 		}()
 		defer func() {
 			if nogoChan != nil {

tests/core/nogo/coverage/README.rst

@@ -19,10 +19,5 @@ Verifies `#2146`_.
 
 gen_code_test
 -------------
-Checks how `nogo`_ would interact with source code that was generated as part of
-rules_go's coverage implementation.  Currently `nogo`_ would be run over these
-generated source code, which is not desirable as end-user have very little control
-over how these code were generated.
-
-In a future version, we shall flip this behavior so that `nogo`_ would not run
-over source files that users have no control over.
+Checks how `nogo`_ should not run on source code that was generated as part of
+rules_go's coverage implementation.

tests/core/nogo/coverage/gen_code_test.go

@@ -15,9 +15,6 @@
 package gen_code_test
 
 import (
-	"errors"
-	"os/exec"
-	"strings"
 	"testing"
 
 	"github.com/bazelbuild/rules_go/go/tools/bazel_testing"
@@ -126,17 +123,8 @@ func run(pass *analysis.Pass) (interface{}, error) {
 }
 
 func TestNogoCoverGenCode(t *testing.T) {
-	out, err := bazel_testing.BazelOutput("coverage", "//:simple_test")
-	if err == nil {
-		t.Fatal("test should fail")
-	}
-
-	var eErr *exec.ExitError
-	if errors.As(err, &eErr) && strings.Contains(string(eErr.Stderr), "was generated by rules_go (nocover)") {
-		// Expected failure
-		return
+	if out, err := bazel_testing.BazelOutput("coverage", "//:simple_test"); err != nil {
+		println(string(out))
+		t.Fatal(err)
 	}
-
-	println(string(out))
-	t.Fatal(err)
 }

tests/core/nogo/generate/README.rst

@@ -7,6 +7,6 @@ Tests to ensure `nogo`_ interaction with generated code.
 
 empty_test
 -------------
-Checks that `nogo`_ is running over the `_empty.go` file that was
+Checks that `nogo`_ is not running over the `_empty.go` file that was
 generated as part of GoCompilePkg.
 

tests/core/nogo/generate/empty_test.go

@@ -15,9 +15,6 @@
 package empty_test
 
 import (
-	"errors"
-	"os/exec"
-	"strings"
 	"testing"
 
 	"github.com/bazelbuild/rules_go/go/tools/bazel_testing"
@@ -98,19 +95,8 @@ func run(pass *analysis.Pass) (interface{}, error) {
 }
 
 func TestNogoGenEmptyCode(t *testing.T) {
-	out, err := bazel_testing.BazelOutput("build", "-k", "//:simple_test")
-	if err == nil {
-		t.Fatal("test should fail")
-	}
-
-	var eErr *exec.ExitError
-	if errors.As(err, &eErr) &&
-		strings.Contains(string(eErr.Stderr), "Detected generated source code from rules_go") &&
-		strings.Contains(string(eErr.Stderr), "(noempty)") {
-		// Expected failure
-		return
+	if out, err := bazel_testing.BazelOutput("build", "-k", "//:simple_test"); err != nil {
+		println(string(out))
+		t.Fatal(err)
 	}
-
-	println(string(out))
-	t.Fatal(err)
 }