nogo: use original source files instead of coverage-instrumented

.bazelci/presubmit.yml

@@ -317,6 +317,7 @@ tasks:
       - "-//tests/core/go_proto_library_importmap:importmap_test"
       - "-//tests/core/go_test:data_test"
       - "-//tests/core/go_test:pwd_test"
+      - "-//tests/core/nogo/coverage:coverage_cgo_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

go/tools/builders/compilepkg.go

@@ -203,15 +203,6 @@ 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.
 		// Otherwise, GoPack will complain if we try to add assembly or cgo objects.
@@ -237,7 +228,6 @@ func compileArchive(
 			pkg:      "empty",
 		})
 
-		nogoSrcsOrigin[emptyGoFile.Name()] = ""
 	}
 	packageName := srcs.goSrcs[0].pkg
 	var goSrcs, cgoSrcs []string
@@ -280,8 +270,18 @@ func compileArchive(
 	// constraints.
 	compilingWithCgo := haveCgo && cgoEnabled
 
+	// When coverage is set, source files will be modified during instrumentation. We should only run static analysis
+	// over original source files and not the modified ones.
+	// goSrcsNogo and cgoSrcsNogo are copies of the original source files for nogo to run static analysis.
+	goSrcsNogo := goSrcs
+	cgoSrcsNogo := cgoSrcs
+
 	// Instrument source files for coverage.
 	if coverMode != "" {
+		// deep copy original source files for nogo static analysis, avoid being modified by coverage.
+		goSrcsNogo = append([]string{}, goSrcs...)
+		cgoSrcsNogo = append([]string{}, cgoSrcs...)
+
 		relCoverPath := make(map[string]string)
 		for _, s := range coverSrcs {
 			relCoverPath[abs(s)] = s
@@ -324,7 +324,6 @@ func compileArchive(
 
 			if i < len(goSrcs) {
 				goSrcs[i] = coverSrc
-				nogoSrcsOrigin[coverSrc] = origSrc
 				continue
 			}
 
@@ -343,6 +342,15 @@ func compileArchive(
 		if err != nil {
 			return err
 		}
+		if coverMode != "" && nogoPath != "" {
+			// Compile original source files, not coverage instrumented, for nogo
+			_, goSrcsNogo, _, err = cgo2(goenv, goSrcsNogo, cgoSrcsNogo, cSrcs, cxxSrcs, objcSrcs, objcxxSrcs, sSrcs, hSrcs, packagePath, packageName, cc, cppFlags, cFlags, cxxFlags, objcFlags, objcxxFlags, ldFlags, cgoExportHPath)
+			if err != nil {
+				return err
+			}
+		} else {
+			goSrcsNogo = goSrcs
+		}
 
 		gcFlags = append(gcFlags, createTrimPath(gcFlags, srcDir))
 	} else {
@@ -429,31 +437,11 @@ func compileArchive(
 	// Run nogo concurrently.
 	var nogoChan chan error
 	outFactsPath := filepath.Join(workDir, nogoFact)
-	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 {
+	if nogoPath != "" && len(goSrcsNogo) > 0 {
 		ctx, cancel := context.WithCancel(context.Background())
 		nogoChan = make(chan error)
 		go func() {
-			nogoChan <- runNogo(ctx, workDir, nogoPath, nogoSrcs, deps, packagePath, importcfgPath, outFactsPath)
+			nogoChan <- runNogo(ctx, workDir, nogoPath, goSrcsNogo, deps, packagePath, importcfgPath, outFactsPath)
 		}()
 		defer func() {
 			if nogoChan != nil {

tests/core/nogo/coverage/BUILD.bazel

@@ -1,5 +1,10 @@
 load("@io_bazel_rules_go//go/tools/bazel_testing:def.bzl", "go_bazel_test")
 
+go_bazel_test(
+    name = "coverage_cgo_test",
+    srcs = ["coverage_cgo_test.go"],
+)
+
 go_bazel_test(
     name = "coverage_test",
     srcs = ["coverage_test.go"],

tests/core/nogo/coverage/README.rst

@@ -2,11 +2,19 @@ nogo test with coverage
 =======================
 
 .. _nogo: /go/nogo.rst
+.. _#3769: https://github.com/bazelbuild/rules_go/issues/3769
 .. _#1940: https://github.com/bazelbuild/rules_go/issues/1940
 .. _#2146: https://github.com/bazelbuild/rules_go/issues/2146
 
 Tests to ensure that `nogo`_ works with coverage.
 
+coverage_cgo_test
+-------------
+Checks that `nogo`_ works when both cgo and coverage are enabled. With coverage
+instrumentation modifying source files, and cgo compilation changing files paths,
+`nogo`_ should be running static analysis against original source files, instead
+of modified ones. Verifies `#3769`_.
+
 coverage_test
 -------------
 Checks that `nogo`_ works when coverage is enabled. All covered libraries gain

tests/core/nogo/coverage/coverage_cgo_test.go

@@ -0,0 +1,124 @@
+// Copyright 2019 The Bazel Authors. All rights reserved.
+//
+// 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 coverage_cgo_test
+
+import (
+	"testing"
+
+	"github.com/bazelbuild/rules_go/go/tools/bazel_testing"
+)
+
+func TestMain(m *testing.M) {
+	bazel_testing.TestMain(m, bazel_testing.Args{
+		Main: `
+-- BUILD.bazel --
+load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test", "nogo")
+
+go_library(
+    name = "foo_cgo",
+    cgo = True,
+    srcs = ["foo_cgo.go"],
+    importpath = "foo_cgo"
+)
+
+go_test(
+    name = "foo_cgo_test",
+    srcs = ["foo_cgo_test.go"],
+    embed = [":foo_cgo"]
+)
+
+nogo(
+    name = "nogo",
+    deps = ["//noinit"],
+    visibility = ["//visibility:public"],
+)
+-- foo_cgo.go --
+package foo_cgo
+
+import "C"
+
+func FooCgo() string {
+	return "foo_cgo"
+}
+-- foo_cgo_test.go --
+package foo_cgo
+
+import "testing"
+
+func TestFooCgo(t *testing.T) {
+	if actual, expected := FooCgo(), "foo_cgo"; actual != expected {
+		t.Errorf("FooCgo() should return foo_cgo")
+	}
+}
+-- noinit/BUILD.bazel --
+load("@io_bazel_rules_go//go:def.bzl", "go_library")
+
+go_library(
+    name = "noinit",
+    srcs = ["analyzer.go"],
+    importpath = "noinit",
+    visibility = ["//visibility:public"],
+    deps = [
+        "@org_golang_x_tools//go/analysis",
+    ],
+)
+-- noinit/analyzer.go --
+package noinit
+import (
+	"fmt"
+	"go/ast"
+
+	"golang.org/x/tools/go/analysis"
+)
+
+const Name = "gochecknoinits"
+
+var Analyzer = &analysis.Analyzer{
+	Name: Name,
+	Doc:  "Checks that no init() functions are present in Go code",
+	Run:  run,
+}
+
+func run(pass *analysis.Pass) (interface{}, error) {
+	for _, f := range pass.Files {
+		for _, decl := range f.Decls {
+			funcDecl, ok := decl.(*ast.FuncDecl)
+			if !ok {
+				continue
+			}
+			name := funcDecl.Name.Name
+			if name == "init" && funcDecl.Recv.NumFields() == 0 {
+				pass.Report(analysis.Diagnostic{
+					Pos:      funcDecl.Pos(),
+					Message:  fmt.Sprintf("don't use init function"),
+					Category: Name,
+				})
+			}
+		}
+	}
+
+	return nil, nil
+}
+`,
+		Nogo: `@//:nogo`,
+	})
+}
+
+func TestNogoWithCoverageAndCgo(t *testing.T) {
+	if out, err := bazel_testing.BazelOutput("coverage", "//:foo_cgo_test"); err != nil {
+		println(string(out))
+		t.Fatal(err)
+	}
+}