Skip to content

Commit 952e2c0

Browse files
heschistamblerre
heschi
authored and
stamblerre
committed
go/packages: revert "handle invalid files in overlays"
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>
1 parent 940a726 commit 952e2c0

File tree

2 files changed

+66
-181
lines changed

2 files changed

+66
-181
lines changed

‎go/packages/golist.go‎

Lines changed: 34 additions & 102 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@ import (
2626
"golang.org/x/tools/go/internal/packagesdriver"
2727
"golang.org/x/tools/internal/gopathwalk"
2828
"golang.org/x/tools/internal/semver"
29-
"golang.org/x/tools/internal/span"
3029
)
3130

3231
// debug controls verbose logging.
@@ -287,42 +286,43 @@ func runContainsQueries(cfg *Config, driver driver, response *responseDeduper, q
287286
return fmt.Errorf("could not determine absolute path of file= query path %q: %v", query, err)
288287
}
289288
dirResponse, err := driver(cfg, pattern)
290-
if err != nil {
289+
if err != nil || (len(dirResponse.Packages) == 1 && len(dirResponse.Packages[0].Errors) == 1) {
290+
// There was an error loading the package. Try to load the file as an ad-hoc package.
291+
// Usually the error will appear in a returned package, but may not if we're in modules mode
292+
// and the ad-hoc is located outside a module.
291293
var queryErr error
292-
if dirResponse, queryErr = adHocPackage(cfg, driver, pattern, query); queryErr != nil {
293-
return err // return the original error
294+
dirResponse, queryErr = driver(cfg, query)
295+
if queryErr != nil {
296+
// Return the original error if the attempt to fall back failed.
297+
return err
294298
}
295-
}
296-
// `go list` can report errors for files that are not listed as part of a package's GoFiles.
297-
// In the case of an invalid Go file, we should assume that it is part of package if only
298-
// one package is in the response. The file may have valid contents in an overlay.
299-
if len(dirResponse.Packages) == 1 {
300-
pkg := dirResponse.Packages[0]
301-
for i, err := range pkg.Errors {
302-
s := errorSpan(err)
303-
if !s.IsValid() {
304-
break
305-
}
306-
if len(pkg.CompiledGoFiles) == 0 {
307-
break
308-
}
309-
dir := filepath.Dir(pkg.CompiledGoFiles[0])
310-
filename := filepath.Join(dir, filepath.Base(s.URI().Filename()))
311-
if info, err := os.Stat(filename); err != nil || info.IsDir() {
312-
break
313-
}
314-
if !contains(pkg.CompiledGoFiles, filename) {
315-
pkg.CompiledGoFiles = append(pkg.CompiledGoFiles, filename)
316-
pkg.GoFiles = append(pkg.GoFiles, filename)
317-
pkg.Errors = append(pkg.Errors[:i], pkg.Errors[i+1:]...)
318-
}
299+
// If we get nothing back from `go list`, try to make this file into its own ad-hoc package.
300+
if len(dirResponse.Packages) == 0 && queryErr == nil {
301+
dirResponse.Packages = append(dirResponse.Packages, &Package{
302+
ID: "command-line-arguments",
303+
PkgPath: query,
304+
GoFiles: []string{query},
305+
CompiledGoFiles: []string{query},
306+
Imports: make(map[string]*Package),
307+
})
308+
dirResponse.Roots = append(dirResponse.Roots, "command-line-arguments")
319309
}
320-
}
321-
// A final attempt to construct an ad-hoc package.
322-
if len(dirResponse.Packages) == 1 && len(dirResponse.Packages[0].Errors) == 1 {
323-
var queryErr error
324-
if dirResponse, queryErr = adHocPackage(cfg, driver, pattern, query); queryErr != nil {
325-
return err // return the original error
310+
// Special case to handle issue #33482:
311+
// If this is a file= query for ad-hoc packages where the file only exists on an overlay,
312+
// and exists outside of a module, add the file in for the package.
313+
if len(dirResponse.Packages) == 1 && (dirResponse.Packages[0].ID == "command-line-arguments" ||
314+
filepath.ToSlash(dirResponse.Packages[0].PkgPath) == filepath.ToSlash(query)) {
315+
if len(dirResponse.Packages[0].GoFiles) == 0 {
316+
filename := filepath.Join(pattern, filepath.Base(query)) // avoid recomputing abspath
317+
// TODO(matloob): check if the file is outside of a root dir?
318+
for path := range cfg.Overlay {
319+
if path == filename {
320+
dirResponse.Packages[0].Errors = nil
321+
dirResponse.Packages[0].GoFiles = []string{path}
322+
dirResponse.Packages[0].CompiledGoFiles = []string{path}
323+
}
324+
}
325+
}
326326
}
327327
}
328328
isRoot := make(map[string]bool, len(dirResponse.Roots))
@@ -350,74 +350,6 @@ func runContainsQueries(cfg *Config, driver driver, response *responseDeduper, q
350350
return nil
351351
}
352352

353-
// adHocPackage attempts to construct an ad-hoc package given a query that failed.
354-
func adHocPackage(cfg *Config, driver driver, pattern, query string) (*driverResponse, error) {
355-
// There was an error loading the package. Try to load the file as an ad-hoc package.
356-
// Usually the error will appear in a returned package, but may not if we're in modules mode
357-
// and the ad-hoc is located outside a module.
358-
dirResponse, err := driver(cfg, query)
359-
if err != nil {
360-
return nil, err
361-
}
362-
// If we get nothing back from `go list`, try to make this file into its own ad-hoc package.
363-
if len(dirResponse.Packages) == 0 && err == nil {
364-
dirResponse.Packages = append(dirResponse.Packages, &Package{
365-
ID: "command-line-arguments",
366-
PkgPath: query,
367-
GoFiles: []string{query},
368-
CompiledGoFiles: []string{query},
369-
Imports: make(map[string]*Package),
370-
})
371-
dirResponse.Roots = append(dirResponse.Roots, "command-line-arguments")
372-
}
373-
// Special case to handle issue #33482:
374-
// If this is a file= query for ad-hoc packages where the file only exists on an overlay,
375-
// and exists outside of a module, add the file in for the package.
376-
if len(dirResponse.Packages) == 1 && (dirResponse.Packages[0].ID == "command-line-arguments" || dirResponse.Packages[0].PkgPath == filepath.ToSlash(query)) {
377-
if len(dirResponse.Packages[0].GoFiles) == 0 {
378-
filename := filepath.Join(pattern, filepath.Base(query)) // avoid recomputing abspath
379-
// TODO(matloob): check if the file is outside of a root dir?
380-
for path := range cfg.Overlay {
381-
if path == filename {
382-
dirResponse.Packages[0].Errors = nil
383-
dirResponse.Packages[0].GoFiles = []string{path}
384-
dirResponse.Packages[0].CompiledGoFiles = []string{path}
385-
}
386-
}
387-
}
388-
}
389-
return dirResponse, nil
390-
}
391-
392-
func contains(files []string, filename string) bool {
393-
for _, f := range files {
394-
if f == filename {
395-
return true
396-
}
397-
}
398-
return false
399-
}
400-
401-
// errorSpan attempts to parse a standard `go list` error message
402-
// by stripping off the trailing error message.
403-
//
404-
// It works only on errors whose message is prefixed by colon,
405-
// followed by a space (": "). For example:
406-
//
407-
// attributes.go:13:1: expected 'package', found 'type'
408-
//
409-
func errorSpan(err Error) span.Span {
410-
if err.Pos == "" {
411-
input := strings.TrimSpace(err.Msg)
412-
msgIndex := strings.Index(input, ": ")
413-
if msgIndex < 0 {
414-
return span.Parse(input)
415-
}
416-
return span.Parse(input[:msgIndex])
417-
}
418-
return span.Parse(err.Pos)
419-
}
420-
421353
// modCacheRegexp splits a path in a module cache into module, module version, and package.
422354
var modCacheRegexp = regexp.MustCompile(`(.*)@([^/\\]*)(.*)`)
423355

‎go/packages/packages_test.go‎

Lines changed: 32 additions & 79 deletions
Original file line numberDiff line numberDiff line change
@@ -1099,51 +1099,6 @@ func testNewPackagesInOverlay(t *testing.T, exporter packagestest.Exporter) {
10991099
}
11001100
}
11011101

1102-
func TestContainsInOverlays(t *testing.T) { packagestest.TestAll(t, testcontainsInOverlays) }
1103-
func testcontainsInOverlays(t *testing.T, exporter packagestest.Exporter) {
1104-
// This test checks a specific case where a file is empty on disk.
1105-
// In this case, `go list` will return the package golang.org/fake/c
1106-
// with only c.go as a GoFile, with an error message for c2.go.
1107-
// Since there is only one possible package for c2.go to be a part of,
1108-
// go/packages will parse the filename out of error and add it to GoFiles and CompiledGoFiles.
1109-
exported := packagestest.Export(t, exporter, []packagestest.Module{{
1110-
Name: "golang.org/fake",
1111-
Files: map[string]interface{}{
1112-
"c/c.go": `package c; const C = "c"`,
1113-
"c/c2.go": ``,
1114-
}}})
1115-
defer exported.Cleanup()
1116-
1117-
dir := filepath.Dir(exported.File("golang.org/fake", "c/c.go"))
1118-
1119-
for _, test := range []struct {
1120-
overlay map[string][]byte
1121-
want string
1122-
}{
1123-
{
1124-
overlay: map[string][]byte{filepath.Join(dir, "c2.go"): []byte(`nonsense`)},
1125-
want: "golang.org/fake/c",
1126-
},
1127-
} {
1128-
exported.Config.Overlay = test.overlay
1129-
exported.Config.Mode = packages.LoadImports
1130-
exported.Config.Logf = t.Logf
1131-
for filename := range exported.Config.Overlay {
1132-
initial, err := packages.Load(exported.Config, "file="+filename)
1133-
if err != nil {
1134-
t.Fatal(err)
1135-
}
1136-
if len(initial) == 0 {
1137-
t.Fatalf("no packages for %s", filename)
1138-
}
1139-
pkg := initial[0]
1140-
if pkg.PkgPath != test.want {
1141-
t.Errorf("got %s, want %s", pkg.PkgPath, test.want)
1142-
}
1143-
}
1144-
}
1145-
}
1146-
11471102
func TestAdHocPackagesBadImport(t *testing.T) {
11481103
// This test doesn't use packagestest because we are testing ad-hoc packages,
11491104
// which are outside of $GOPATH and outside of a module.
@@ -1210,36 +1165,38 @@ const A = 1
12101165

12111166
// Make sure that the user's value of GO111MODULE does not affect test results.
12121167
for _, go111module := range []string{"off", "auto", "on"} {
1213-
config := &packages.Config{
1214-
Dir: tmp,
1215-
Env: append(os.Environ(), "GOPACKAGESDRIVER=off", fmt.Sprintf("GO111MODULE=%s", go111module)),
1216-
Mode: packages.LoadAllSyntax,
1217-
Overlay: map[string][]byte{
1218-
filename: content,
1219-
},
1220-
Logf: t.Logf,
1221-
}
1222-
initial, err := packages.Load(config, fmt.Sprintf("file=%s", filename))
1223-
if err != nil {
1224-
t.Fatal(err)
1225-
}
1226-
if len(initial) == 0 {
1227-
t.Fatalf("no packages for %s with GO111MODULE=%s", filename, go111module)
1228-
}
1229-
// Check value of a.A.
1230-
a := initial[0]
1231-
if a.Errors != nil {
1232-
t.Fatalf("a: got errors %+v, want no error", err)
1233-
}
1234-
aA := constant(a, "A")
1235-
if aA == nil {
1236-
t.Errorf("a.A: got nil")
1237-
return
1238-
}
1239-
got := aA.Val().String()
1240-
if want := "1"; got != want {
1241-
t.Errorf("a.A: got %s, want %s", got, want)
1242-
}
1168+
t.Run("GO111MODULE="+go111module, func(t *testing.T) {
1169+
config := &packages.Config{
1170+
Dir: tmp,
1171+
Env: append(os.Environ(), "GOPACKAGESDRIVER=off", fmt.Sprintf("GO111MODULE=%s", go111module)),
1172+
Mode: packages.LoadAllSyntax,
1173+
Overlay: map[string][]byte{
1174+
filename: content,
1175+
},
1176+
Logf: t.Logf,
1177+
}
1178+
initial, err := packages.Load(config, fmt.Sprintf("file=%s", filename))
1179+
if err != nil {
1180+
t.Fatal(err)
1181+
}
1182+
if len(initial) == 0 {
1183+
t.Fatalf("no packages for %s", filename)
1184+
}
1185+
// Check value of a.A.
1186+
a := initial[0]
1187+
if a.Errors != nil {
1188+
t.Fatalf("a: got errors %+v, want no error", err)
1189+
}
1190+
aA := constant(a, "A")
1191+
if aA == nil {
1192+
t.Errorf("a.A: got nil")
1193+
return
1194+
}
1195+
got := aA.Val().String()
1196+
if want := "1"; got != want {
1197+
t.Errorf("a.A: got %s, want %s", got, want)
1198+
}
1199+
})
12431200
}
12441201
}
12451202

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

22752232
exported.Config.Mode = packages.NeedImports | packages.NeedFiles
2276-
exported.Config.Logf = t.Logf
22772233
pkgs, err := packages.Load(exported.Config, "file="+filename)
22782234
if err != nil {
22792235
t.Fatal(err)
22802236
}
2281-
if len(pkgs) == 0 {
2282-
t.Fatalf("no packages for %s", filename)
2283-
}
22842237
if len(pkgs) != 1 && pkgs[0].PkgPath != "command-line-arguments" {
22852238
t.Fatalf("packages.Load: want [command-line-arguments], got %v", pkgs)
22862239
}

0 commit comments

Comments
 (0)