go/build: avoid duplicates in InvalidGoFiles

For #45827 For #39986 Updates #45999 Change-Id: I0c919b6a2e56e7003b90425487eafe0f0eadc609 Reviewed-on: https://go-review.googlesource.com/c/go/+/317299 Trust: Bryan C. Mills <[email protected]> Run-TryBot: Bryan C. Mills <[email protected]> TryBot-Result: Go Bot <[email protected]> Reviewed-by: Jay Conrod <[email protected]>
golang · May 10, 2021 · e18a8b4 · e18a8b4
1 parent 2870259
commit e18a8b4
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 17 deletions.
diff --git a/src/go/build/build.go b/src/go/build/build.go
@@ -810,6 +810,17 @@ Found:
 	}
 
 	var badGoError error
+	badFiles := make(map[string]bool)
+	badFile := func(name string, err error) {
+		if badGoError == nil {
+			badGoError = err
+		}
+		if !badFiles[name] {
+			p.InvalidGoFiles = append(p.InvalidGoFiles, name)
+			badFiles[name] = true
+		}
+	}
+
 	var Sfiles []string // files with ".S"(capital S)/.sx(capital s equivalent for case insensitive filesystems)
 	var firstFile, firstCommentFile string
 	embedPos := make(map[string][]token.Position)
@@ -834,16 +845,9 @@ Found:
 		name := d.Name()
 		ext := nameExt(name)
 
-		badFile := func(err error) {
-			if badGoError == nil {
-				badGoError = err
-			}
-			p.InvalidGoFiles = append(p.InvalidGoFiles, name)
-		}
-
 		info, err := ctxt.matchFile(p.Dir, name, allTags, &p.BinaryOnly, fset)
 		if err != nil {
-			badFile(err)
+			badFile(name, err)
 			continue
 		}
 		if info == nil {
@@ -874,7 +878,7 @@ Found:
 		}
 
 		if info.parseErr != nil {
-			badFile(info.parseErr)
+			badFile(name, info.parseErr)
 			continue
 		}
 		pf := info.parsed
@@ -896,12 +900,14 @@ Found:
 			p.Name = pkg
 			firstFile = name
 		} else if pkg != p.Name {
-			badFile(&MultiplePackageError{
+			// 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{
 				Dir:      p.Dir,
 				Packages: []string{p.Name, pkg},
 				Files:    []string{firstFile, name},
 			})
-			p.InvalidGoFiles = append(p.InvalidGoFiles, name)
 		}
 		// Grab the first package comment as docs, provided it is not from a test file.
 		if pf.Doc != nil && p.Doc == "" && !isTest && !isXTest {
@@ -913,12 +919,12 @@ Found:
 			if line != 0 {
 				com, err := strconv.Unquote(qcom)
 				if err != nil {
-					badFile(fmt.Errorf("%s:%d: cannot parse import comment", filename, line))
+					badFile(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(fmt.Errorf("found import comments %q (%s) and %q (%s) in %s", p.ImportComment, firstCommentFile, com, name, p.Dir))
+					badFile(name, fmt.Errorf("found import comments %q (%s) and %q (%s) in %s", p.ImportComment, firstCommentFile, com, name, p.Dir))
 				}
 			}
 		}
@@ -928,13 +934,13 @@ Found:
 		for _, imp := range info.imports {
 			if imp.path == "C" {
 				if isTest {
-					badFile(fmt.Errorf("use of cgo in test %s not supported", filename))
+					badFile(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(err)
+						badFile(name, err)
 					}
 				}
 			}

diff --git a/src/go/build/build_test.go b/src/go/build/build_test.go
@@ -104,7 +104,8 @@ func TestEmptyFolderImport(t *testing.T) {
 }
 
 func TestMultiplePackageImport(t *testing.T) {
-	_, err := Import(".", "testdata/multi", 0)
+	pkg, err := Import(".", "testdata/multi", 0)
+
 	mpe, ok := err.(*MultiplePackageError)
 	if !ok {
 		t.Fatal(`Import("testdata/multi") did not return MultiplePackageError.`)
@@ -115,7 +116,20 @@ func TestMultiplePackageImport(t *testing.T) {
 		Files:    []string{"file.go", "file_appengine.go"},
 	}
 	if !reflect.DeepEqual(mpe, want) {
-		t.Errorf("got %#v; want %#v", mpe, want)
+		t.Errorf("err = %#v; want %#v", mpe, want)
+	}
+
+	// TODO(#45999): Since the name is ambiguous, pkg.Name should be left empty.
+	if wantName := "main"; pkg.Name != wantName {
+		t.Errorf("pkg.Name = %q; want %q", pkg.Name, wantName)
+	}
+
+	if wantGoFiles := []string{"file.go", "file_appengine.go"}; !reflect.DeepEqual(pkg.GoFiles, wantGoFiles) {
+		t.Errorf("pkg.GoFiles = %q; want %q", pkg.GoFiles, wantGoFiles)
+	}
+
+	if wantInvalidFiles := []string{"file_appengine.go"}; !reflect.DeepEqual(pkg.InvalidGoFiles, wantInvalidFiles) {
+		t.Errorf("pkg.InvalidGoFiles = %q; want %q", pkg.InvalidGoFiles, wantInvalidFiles)
 	}
 }