From 3511c822f766fdf16817c1a1949971806c4eeb7e Mon Sep 17 00:00:00 2001 From: Michael Matloob Date: Wed, 2 Nov 2022 14:36:51 -0400 Subject: [PATCH] go/build: don't add unparsable non-Go files to InvalidGoFiles go/build attempts to parse comments at the beginning of non-Go files looking for //go:build or //+go build comments. Before this change, if the beginning of the non-Go file failed to parse (perhaps because it is in a format that isn't even meant to be built with Go code) the file would be added to InvalidGoFiles. The comment for InvalidGoFiles states that it contains Go files, so this is clearly incorrect behavior. Further, if there was a directory that only contained these unparsable non-Go files, it would have a non-zero number of InvalidGoFiles, and the matching code in cmd/go/internal/search/search.go in (*Match).MatchDirs would treat it as a directory containing (invalid) Go files and would match the directory as a Go package. This incorrect behavior is also fixed by this CL. Fixes #56509 Change-Id: Id0d905827c71f7927f7c2fa42b236181950af7e9 Reviewed-on: https://go-review.googlesource.com/c/go/+/447357 Run-TryBot: Michael Matloob TryBot-Result: Gopher Robot Reviewed-by: Bryan Mills Reviewed-by: Michael Matloob --- src/cmd/go/internal/modindex/read.go | 30 +++++++++------- .../go/testdata/script/list_issue_56509.txt | 34 +++++++++++++++++++ src/go/build/build.go | 29 ++++++++-------- src/go/build/build_test.go | 16 +++++++++ src/go/build/testdata/bads/bad.s | 1 + 5 files changed, 83 insertions(+), 27 deletions(-) create mode 100644 src/cmd/go/testdata/script/list_issue_56509.txt create mode 100644 src/go/build/testdata/bads/bad.s diff --git a/src/cmd/go/internal/modindex/read.go b/src/cmd/go/internal/modindex/read.go index e9cfbca8aed..f01ca6ec172 100644 --- a/src/cmd/go/internal/modindex/read.go +++ b/src/cmd/go/internal/modindex/read.go @@ -458,14 +458,14 @@ func (rp *IndexPackage) Import(bctxt build.Context, mode build.ImportMode) (p *b // We need to do a second round of bad file processing. var badGoError error - badFiles := make(map[string]bool) - badFile := func(name string, err error) { + badGoFiles := make(map[string]bool) + badGoFile := func(name string, err error) { if badGoError == nil { badGoError = err } - if !badFiles[name] { + if !badGoFiles[name] { p.InvalidGoFiles = append(p.InvalidGoFiles, name) - badFiles[name] = true + badGoFiles[name] = true } } @@ -480,12 +480,16 @@ func (rp *IndexPackage) Import(bctxt build.Context, mode build.ImportMode) (p *b allTags := make(map[string]bool) for _, tf := range rp.sourceFiles { name := tf.name() - if error := tf.error(); error != "" { - badFile(name, errors.New(tf.error())) - continue - } else if parseError := tf.parseError(); parseError != "" { - badFile(name, parseErrorFromString(tf.parseError())) - // Fall through: we still want to list files with parse errors. + // Check errors for go files and call badGoFiles to put them in + // InvalidGoFiles if they do have an error. + if strings.HasSuffix(name, ".go") { + if error := tf.error(); error != "" { + badGoFile(name, errors.New(tf.error())) + continue + } else if parseError := tf.parseError(); parseError != "" { + badGoFile(name, parseErrorFromString(tf.parseError())) + // Fall through: we still want to list files with parse errors. + } } var shouldBuild = true @@ -555,7 +559,7 @@ func (rp *IndexPackage) Import(bctxt build.Context, mode build.ImportMode) (p *b // TODO(#45999): The choice of p.Name is arbitrary based on file iteration // order. Instead of resolving p.Name arbitrarily, we should clear out the // existing Name and mark the existing files as also invalid. - badFile(name, &MultiplePackageError{ + badGoFile(name, &MultiplePackageError{ Dir: p.Dir, Packages: []string{p.Name, pkg}, Files: []string{firstFile, name}, @@ -574,7 +578,7 @@ func (rp *IndexPackage) Import(bctxt build.Context, mode build.ImportMode) (p *b for _, imp := range imports { if imp.path == "C" { if isTest { - badFile(name, fmt.Errorf("use of cgo in test %s not supported", name)) + badGoFile(name, fmt.Errorf("use of cgo in test %s not supported", name)) continue } isCgo = true @@ -582,7 +586,7 @@ func (rp *IndexPackage) Import(bctxt build.Context, mode build.ImportMode) (p *b } if directives := tf.cgoDirectives(); directives != "" { if err := ctxt.saveCgo(name, (*Package)(p), directives); err != nil { - badFile(name, err) + badGoFile(name, err) } } diff --git a/src/cmd/go/testdata/script/list_issue_56509.txt b/src/cmd/go/testdata/script/list_issue_56509.txt new file mode 100644 index 00000000000..d0ed9e45175 --- /dev/null +++ b/src/cmd/go/testdata/script/list_issue_56509.txt @@ -0,0 +1,34 @@ +# Test that a directory with an .s file that has a comment that can't +# be parsed isn't matched as a go directory. (This was happening because +# non-go files with unparsable comments were being added to InvalidGoFiles +# leading the package matching code to think there were Go files in the +# directory.) + +go list ./... +! stdout . + + +[short] skip + +# Test that an unparsable .s file is completely ignored when its name +# has build tags that cause it to be filtered out, but produces an error +# when it is included + +env GOARCH=arm64 +env GOOS=linux +go build ./baz + +env GOARCH=amd64 +env GOOS=linux +! go build ./baz + +-- go.mod -- +module example.com/foo + +go 1.20 +-- bar/bar.s -- +;/ +-- baz/baz.go -- +package bar +-- baz/baz_amd64.s -- +;/ diff --git a/src/go/build/build.go b/src/go/build/build.go index 1cb10f50bf9..ccdc657e367 100644 --- a/src/go/build/build.go +++ b/src/go/build/build.go @@ -824,14 +824,14 @@ Found: } var badGoError error - badFiles := make(map[string]bool) - badFile := func(name string, err error) { + badGoFiles := make(map[string]bool) + badGoFile := func(name string, err error) { if badGoError == nil { badGoError = err } - if !badFiles[name] { + if !badGoFiles[name] { p.InvalidGoFiles = append(p.InvalidGoFiles, name) - badFiles[name] = true + badGoFiles[name] = true } } @@ -860,8 +860,8 @@ Found: ext := nameExt(name) info, err := ctxt.matchFile(p.Dir, name, allTags, &p.BinaryOnly, fset) - if err != nil { - badFile(name, err) + if err != nil && strings.HasSuffix(name, ".go") { + badGoFile(name, err) continue } if info == nil { @@ -874,7 +874,6 @@ Found: } continue } - data, filename := info.header, info.name // Going to save the file. For non-Go files, can stop here. switch ext { @@ -891,8 +890,10 @@ Found: continue } + data, filename := info.header, info.name + if info.parseErr != nil { - badFile(name, info.parseErr) + badGoFile(name, info.parseErr) // Fall through: we might still have a partial AST in info.parsed, // and we want to list files with parse errors anyway. } @@ -920,7 +921,7 @@ Found: // TODO(#45999): The choice of p.Name is arbitrary based on file iteration // order. Instead of resolving p.Name arbitrarily, we should clear out the // existing name and mark the existing files as also invalid. - badFile(name, &MultiplePackageError{ + badGoFile(name, &MultiplePackageError{ Dir: p.Dir, Packages: []string{p.Name, pkg}, Files: []string{firstFile, name}, @@ -936,12 +937,12 @@ Found: if line != 0 { com, err := strconv.Unquote(qcom) if err != nil { - badFile(name, fmt.Errorf("%s:%d: cannot parse import comment", filename, line)) + badGoFile(name, fmt.Errorf("%s:%d: cannot parse import comment", filename, line)) } else if p.ImportComment == "" { p.ImportComment = com firstCommentFile = name } else if p.ImportComment != com { - badFile(name, fmt.Errorf("found import comments %q (%s) and %q (%s) in %s", p.ImportComment, firstCommentFile, com, name, p.Dir)) + badGoFile(name, fmt.Errorf("found import comments %q (%s) and %q (%s) in %s", p.ImportComment, firstCommentFile, com, name, p.Dir)) } } } @@ -951,13 +952,13 @@ Found: for _, imp := range info.imports { if imp.path == "C" { if isTest { - badFile(name, fmt.Errorf("use of cgo in test %s not supported", filename)) + badGoFile(name, fmt.Errorf("use of cgo in test %s not supported", filename)) continue } isCgo = true if imp.doc != nil { if err := ctxt.saveCgo(filename, p, imp.doc); err != nil { - badFile(name, err) + badGoFile(name, err) } } } @@ -1454,7 +1455,7 @@ func (ctxt *Context) matchFile(dir, name string, allTags map[string]bool, binary } f.Close() if err != nil { - return nil, fmt.Errorf("read %s: %v", info.name, err) + return info, fmt.Errorf("read %s: %v", info.name, err) } // Look for go:build comments to accept or reject the file. diff --git a/src/go/build/build_test.go b/src/go/build/build_test.go index 33223777151..db50d45d84e 100644 --- a/src/go/build/build_test.go +++ b/src/go/build/build_test.go @@ -702,6 +702,22 @@ func TestIssue23594(t *testing.T) { } } +// TestIssue56509 tests that go/build does not add non-go files to InvalidGoFiles +// when they have unparsable comments. +func TestIssue56509(t *testing.T) { + // The directory testdata/bads contains a .s file that has an unparsable + // comment. (go/build parses initial comments in non-go files looking for + // //go:build or //+go build comments). + p, err := ImportDir("testdata/bads", 0) + if err == nil { + t.Fatalf("could not import testdata/bads: %v", err) + } + + if len(p.InvalidGoFiles) != 0 { + t.Fatalf("incorrectly added non-go file to InvalidGoFiles") + } +} + // TestMissingImportErrorRepetition checks that when an unknown package is // imported, the package path is only shown once in the error. // Verifies golang.org/issue/34752. diff --git a/src/go/build/testdata/bads/bad.s b/src/go/build/testdata/bads/bad.s new file mode 100644 index 00000000000..b670f8213ea --- /dev/null +++ b/src/go/build/testdata/bads/bad.s @@ -0,0 +1 @@ +;/