From e18a8b4fb27df3ee04f7eb00c4f1ed77e5486ce8 Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Wed, 5 May 2021 17:05:51 -0400 Subject: [PATCH] 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 Run-TryBot: Bryan C. Mills TryBot-Result: Go Bot Reviewed-by: Jay Conrod --- src/go/build/build.go | 36 +++++++++++++++++++++--------------- src/go/build/build_test.go | 18 ++++++++++++++++-- 2 files changed, 37 insertions(+), 17 deletions(-) diff --git a/src/go/build/build.go b/src/go/build/build.go index 8d1a107c6e..b85fa96de1 100644 --- 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 index 7e78568083..80f930f3c2 100644 --- 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) } } -- 2.50.0