Skip to content

Commit

Permalink
go/packages: revert "handle invalid files in overlays"
Browse files Browse the repository at this point in the history 
This reverts commit 6f5e273, golang.org/cl/201220.

Reason for revert: Produces unusable Package results. See golang/go#35949 and golang/go#35973.

Fixes golang/go#35949.

Change-Id: Ic4632fe7f00366b1edf59840c83160d66499ba12
Reviewed-on: https://go-review.googlesource.com/c/tools/+/209978
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
(cherry picked from commit 427c522)
Reviewed-on: https://go-review.googlesource.com/c/tools/+/210206
Reviewed-by: Heschi Kreinick <heschi@google.com>
  • Loading branch information
@heschi @stamblerre
heschi authored and stamblerre committed Dec 6, 2019
1 parent 940a726 commit 952e2c0
Show file tree
Hide file tree
Showing 2 changed files with 66 additions and 181 deletions.
136 changes: 34 additions & 102 deletions go/packages/golist.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import (
"golang.org/x/tools/go/internal/packagesdriver"
"golang.org/x/tools/internal/gopathwalk"
"golang.org/x/tools/internal/semver"
"golang.org/x/tools/internal/span"
)

// debug controls verbose logging.
Expand Down Expand Up @@ -287,42 +286,43 @@ func runContainsQueries(cfg *Config, driver driver, response *responseDeduper, q
return fmt.Errorf("could not determine absolute path of file= query path %q: %v", query, err)
}
dirResponse, err := driver(cfg, pattern)
if err != nil {
if err != nil || (len(dirResponse.Packages) == 1 && len(dirResponse.Packages[0].Errors) == 1) {
// There was an error loading the package. Try to load the file as an ad-hoc package.
// Usually the error will appear in a returned package, but may not if we're in modules mode
// and the ad-hoc is located outside a module.
var queryErr error
if dirResponse, queryErr = adHocPackage(cfg, driver, pattern, query); queryErr != nil {
return err // return the original error
dirResponse, queryErr = driver(cfg, query)
if queryErr != nil {
// Return the original error if the attempt to fall back failed.
return err
}
}
// `go list` can report errors for files that are not listed as part of a package's GoFiles.
// In the case of an invalid Go file, we should assume that it is part of package if only
// one package is in the response. The file may have valid contents in an overlay.
if len(dirResponse.Packages) == 1 {
pkg := dirResponse.Packages[0]
for i, err := range pkg.Errors {
s := errorSpan(err)
if !s.IsValid() {
break
}
if len(pkg.CompiledGoFiles) == 0 {
break
}
dir := filepath.Dir(pkg.CompiledGoFiles[0])
filename := filepath.Join(dir, filepath.Base(s.URI().Filename()))
if info, err := os.Stat(filename); err != nil || info.IsDir() {
break
}
if !contains(pkg.CompiledGoFiles, filename) {
pkg.CompiledGoFiles = append(pkg.CompiledGoFiles, filename)
pkg.GoFiles = append(pkg.GoFiles, filename)
pkg.Errors = append(pkg.Errors[:i], pkg.Errors[i+1:]...)
}
// If we get nothing back from `go list`, try to make this file into its own ad-hoc package.
if len(dirResponse.Packages) == 0 && queryErr == nil {
dirResponse.Packages = append(dirResponse.Packages, &Package{
ID: "command-line-arguments",
PkgPath: query,
GoFiles: []string{query},
CompiledGoFiles: []string{query},
Imports: make(map[string]*Package),
})
dirResponse.Roots = append(dirResponse.Roots, "command-line-arguments")
}
}
// A final attempt to construct an ad-hoc package.
if len(dirResponse.Packages) == 1 && len(dirResponse.Packages[0].Errors) == 1 {
var queryErr error
if dirResponse, queryErr = adHocPackage(cfg, driver, pattern, query); queryErr != nil {
return err // return the original error
// Special case to handle issue #33482:
// If this is a file= query for ad-hoc packages where the file only exists on an overlay,
// and exists outside of a module, add the file in for the package.
if len(dirResponse.Packages) == 1 && (dirResponse.Packages[0].ID == "command-line-arguments" ||
filepath.ToSlash(dirResponse.Packages[0].PkgPath) == filepath.ToSlash(query)) {
if len(dirResponse.Packages[0].GoFiles) == 0 {
filename := filepath.Join(pattern, filepath.Base(query)) // avoid recomputing abspath
// TODO(matloob): check if the file is outside of a root dir?
for path := range cfg.Overlay {
if path == filename {
dirResponse.Packages[0].Errors = nil
dirResponse.Packages[0].GoFiles = []string{path}
dirResponse.Packages[0].CompiledGoFiles = []string{path}
}
}
}
}
}
isRoot := make(map[string]bool, len(dirResponse.Roots))
Expand Down Expand Up @@ -350,74 +350,6 @@ func runContainsQueries(cfg *Config, driver driver, response *responseDeduper, q
return nil
}

// adHocPackage attempts to construct an ad-hoc package given a query that failed.
func adHocPackage(cfg *Config, driver driver, pattern, query string) (*driverResponse, error) {
// There was an error loading the package. Try to load the file as an ad-hoc package.
// Usually the error will appear in a returned package, but may not if we're in modules mode
// and the ad-hoc is located outside a module.
dirResponse, err := driver(cfg, query)
if err != nil {
return nil, err
}
// If we get nothing back from `go list`, try to make this file into its own ad-hoc package.
if len(dirResponse.Packages) == 0 && err == nil {
dirResponse.Packages = append(dirResponse.Packages, &Package{
ID: "command-line-arguments",
PkgPath: query,
GoFiles: []string{query},
CompiledGoFiles: []string{query},
Imports: make(map[string]*Package),
})
dirResponse.Roots = append(dirResponse.Roots, "command-line-arguments")
}
// Special case to handle issue #33482:
// If this is a file= query for ad-hoc packages where the file only exists on an overlay,
// and exists outside of a module, add the file in for the package.
if len(dirResponse.Packages) == 1 && (dirResponse.Packages[0].ID == "command-line-arguments" || dirResponse.Packages[0].PkgPath == filepath.ToSlash(query)) {
if len(dirResponse.Packages[0].GoFiles) == 0 {
filename := filepath.Join(pattern, filepath.Base(query)) // avoid recomputing abspath
// TODO(matloob): check if the file is outside of a root dir?
for path := range cfg.Overlay {
if path == filename {
dirResponse.Packages[0].Errors = nil
dirResponse.Packages[0].GoFiles = []string{path}
dirResponse.Packages[0].CompiledGoFiles = []string{path}
}
}
}
}
return dirResponse, nil
}

func contains(files []string, filename string) bool {
for _, f := range files {
if f == filename {
return true
}
}
return false
}

// errorSpan attempts to parse a standard `go list` error message
// by stripping off the trailing error message.
//
// It works only on errors whose message is prefixed by colon,
// followed by a space (": "). For example:
//
// attributes.go:13:1: expected 'package', found 'type'
//
func errorSpan(err Error) span.Span {
if err.Pos == "" {
input := strings.TrimSpace(err.Msg)
msgIndex := strings.Index(input, ": ")
if msgIndex < 0 {
return span.Parse(input)
}
return span.Parse(input[:msgIndex])
}
return span.Parse(err.Pos)
}

// modCacheRegexp splits a path in a module cache into module, module version, and package.
var modCacheRegexp = regexp.MustCompile(`(.*)@([^/\\]*)(.*)`)

Expand Down
111 changes: 32 additions & 79 deletions go/packages/packages_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1099,51 +1099,6 @@ func testNewPackagesInOverlay(t *testing.T, exporter packagestest.Exporter) {
}
}

func TestContainsInOverlays(t *testing.T) { packagestest.TestAll(t, testcontainsInOverlays) }
func testcontainsInOverlays(t *testing.T, exporter packagestest.Exporter) {
// This test checks a specific case where a file is empty on disk.
// In this case, `go list` will return the package golang.org/fake/c
// with only c.go as a GoFile, with an error message for c2.go.
// Since there is only one possible package for c2.go to be a part of,
// go/packages will parse the filename out of error and add it to GoFiles and CompiledGoFiles.
exported := packagestest.Export(t, exporter, []packagestest.Module{{
Name: "golang.org/fake",
Files: map[string]interface{}{
"c/c.go": `package c; const C = "c"`,
"c/c2.go": ``,
}}})
defer exported.Cleanup()

dir := filepath.Dir(exported.File("golang.org/fake", "c/c.go"))

for _, test := range []struct {
overlay map[string][]byte
want string
}{
{
overlay: map[string][]byte{filepath.Join(dir, "c2.go"): []byte(`nonsense`)},
want: "golang.org/fake/c",
},
} {
exported.Config.Overlay = test.overlay
exported.Config.Mode = packages.LoadImports
exported.Config.Logf = t.Logf
for filename := range exported.Config.Overlay {
initial, err := packages.Load(exported.Config, "file="+filename)
if err != nil {
t.Fatal(err)
}
if len(initial) == 0 {
t.Fatalf("no packages for %s", filename)
}
pkg := initial[0]
if pkg.PkgPath != test.want {
t.Errorf("got %s, want %s", pkg.PkgPath, test.want)
}
}
}
}

func TestAdHocPackagesBadImport(t *testing.T) {
// This test doesn't use packagestest because we are testing ad-hoc packages,
// which are outside of $GOPATH and outside of a module.
Expand Down Expand Up @@ -1210,36 +1165,38 @@ const A = 1

// Make sure that the user's value of GO111MODULE does not affect test results.
for _, go111module := range []string{"off", "auto", "on"} {
config := &packages.Config{
Dir: tmp,
Env: append(os.Environ(), "GOPACKAGESDRIVER=off", fmt.Sprintf("GO111MODULE=%s", go111module)),
Mode: packages.LoadAllSyntax,
Overlay: map[string][]byte{
filename: content,
},
Logf: t.Logf,
}
initial, err := packages.Load(config, fmt.Sprintf("file=%s", filename))
if err != nil {
t.Fatal(err)
}
if len(initial) == 0 {
t.Fatalf("no packages for %s with GO111MODULE=%s", filename, go111module)
}
// Check value of a.A.
a := initial[0]
if a.Errors != nil {
t.Fatalf("a: got errors %+v, want no error", err)
}
aA := constant(a, "A")
if aA == nil {
t.Errorf("a.A: got nil")
return
}
got := aA.Val().String()
if want := "1"; got != want {
t.Errorf("a.A: got %s, want %s", got, want)
}
t.Run("GO111MODULE="+go111module, func(t *testing.T) {
config := &packages.Config{
Dir: tmp,
Env: append(os.Environ(), "GOPACKAGESDRIVER=off", fmt.Sprintf("GO111MODULE=%s", go111module)),
Mode: packages.LoadAllSyntax,
Overlay: map[string][]byte{
filename: content,
},
Logf: t.Logf,
}
initial, err := packages.Load(config, fmt.Sprintf("file=%s", filename))
if err != nil {
t.Fatal(err)
}
if len(initial) == 0 {
t.Fatalf("no packages for %s", filename)
}
// Check value of a.A.
a := initial[0]
if a.Errors != nil {
t.Fatalf("a: got errors %+v, want no error", err)
}
aA := constant(a, "A")
if aA == nil {
t.Errorf("a.A: got nil")
return
}
got := aA.Val().String()
if want := "1"; got != want {
t.Errorf("a.A: got %s, want %s", got, want)
}
})
}
}

Expand Down Expand Up @@ -2273,14 +2230,10 @@ func testAdHocContains(t *testing.T, exporter packagestest.Exporter) {
}()

exported.Config.Mode = packages.NeedImports | packages.NeedFiles
exported.Config.Logf = t.Logf
pkgs, err := packages.Load(exported.Config, "file="+filename)
if err != nil {
t.Fatal(err)
}
if len(pkgs) == 0 {
t.Fatalf("no packages for %s", filename)
}
if len(pkgs) != 1 && pkgs[0].PkgPath != "command-line-arguments" {
t.Fatalf("packages.Load: want [command-line-arguments], got %v", pkgs)
}
Expand Down

0 comments on commit 952e2c0

Please sign in to comment.