diff --git a/.bazelci/presubmit.yml b/.bazelci/presubmit.yml index 3282fedd7e..75d5e46b94 100644 --- a/.bazelci/presubmit.yml +++ b/.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 diff --git a/go/tools/builders/compilepkg.go b/go/tools/builders/compilepkg.go index b3a6d283a9..bcbdb64224 100644 --- a/go/tools/builders/compilepkg.go +++ b/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 { diff --git a/tests/core/nogo/coverage/BUILD.bazel b/tests/core/nogo/coverage/BUILD.bazel index b14fcaf48e..4999efeeea 100644 --- a/tests/core/nogo/coverage/BUILD.bazel +++ b/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"], diff --git a/tests/core/nogo/coverage/README.rst b/tests/core/nogo/coverage/README.rst index 2bb6de952c..5dd41eaf78 100644 --- a/tests/core/nogo/coverage/README.rst +++ b/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 diff --git a/tests/core/nogo/coverage/coverage_cgo_test.go b/tests/core/nogo/coverage/coverage_cgo_test.go new file mode 100644 index 0000000000..3458b2ea63 --- /dev/null +++ b/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) + } +}