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 <matloob@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>

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)
 			}
 		}
 

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 --
+;/

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.

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.

src/go/build/testdata/bads/bad.s

@@ -0,0 +1 @@
+;/