Skip to content

Commit 427c522

Browse files
heschiheschi
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 <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Rebecca Stambler <[email protected]>
1 parent d79e56d commit 427c522

File tree

2 files changed

+66
-182
lines changed

2 files changed

+66
-182
lines changed

‎go/packages/golist.go

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

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

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