Skip to content

Commit 884aa71

Browse files
Bryan C. Millsgopherbot
Bryan C. Mills
authored and
gopherbot
committed
go/internal/gcimporter: propagate errors from FindPkg
Previously, FindPkg returned the empty string as a sentinel value, causing Import to collapse all errors to "can't find import". (See also https://go.dev/wiki/CodeReviewComments#in-band-errors.) For #61064. Change-Id: I21f335d206308b44fe585619e00782abb0b65a94 Reviewed-on: https://go-review.googlesource.com/c/go/+/507360 Run-TryBot: Bryan Mills <bcmills@google.com> Reviewed-by: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Auto-Submit: Bryan Mills <bcmills@google.com>
1 parent 7b625d1 commit 884aa71

File tree

4 files changed

+81
-61
lines changed

4 files changed

+81
-61
lines changed

‎src/cmd/compile/internal/importer/gcimporter.go

Lines changed: 37 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ package importer
88
import (
99
"bufio"
1010
"bytes"
11+
"errors"
1112
"fmt"
1213
"go/build"
1314
"internal/pkgbits"
@@ -21,7 +22,7 @@ import (
2122
"cmd/compile/internal/types2"
2223
)
2324

24-
var exportMap sync.Map // package dir → func() (string, bool)
25+
var exportMap sync.Map // package dir → func() (string, error)
2526

2627
// lookupGorootExport returns the location of the export data
2728
// (normally found in the build cache, but located in GOROOT/pkg
@@ -30,37 +31,42 @@ var exportMap sync.Map // package dir → func() (string, bool)
3031
// (We use the package's directory instead of its import path
3132
// mainly to simplify handling of the packages in src/vendor
3233
// and cmd/vendor.)
33-
func lookupGorootExport(pkgDir string) (string, bool) {
34+
func lookupGorootExport(pkgDir string) (string, error) {
3435
f, ok := exportMap.Load(pkgDir)
3536
if !ok {
3637
var (
3738
listOnce sync.Once
3839
exportPath string
40+
err error
3941
)
40-
f, _ = exportMap.LoadOrStore(pkgDir, func() (string, bool) {
42+
f, _ = exportMap.LoadOrStore(pkgDir, func() (string, error) {
4143
listOnce.Do(func() {
4244
cmd := exec.Command(filepath.Join(build.Default.GOROOT, "bin", "go"), "list", "-export", "-f", "{{.Export}}", pkgDir)
4345
cmd.Dir = build.Default.GOROOT
4446
cmd.Env = append(os.Environ(), "PWD="+cmd.Dir, "GOROOT="+build.Default.GOROOT)
4547
var output []byte
46-
output, err := cmd.Output()
48+
output, err = cmd.Output()
4749
if err != nil {
50+
if ee, ok := err.(*exec.ExitError); ok && len(ee.Stderr) > 0 {
51+
err = errors.New(string(ee.Stderr))
52+
}
4853
return
4954
}
5055

5156
exports := strings.Split(string(bytes.TrimSpace(output)), "\n")
5257
if len(exports) != 1 {
58+
err = fmt.Errorf("go list reported %d exports; expected 1", len(exports))
5359
return
5460
}
5561

5662
exportPath = exports[0]
5763
})
5864

59-
return exportPath, exportPath != ""
65+
return exportPath, err
6066
})
6167
}
6268

63-
return f.(func() (string, bool))()
69+
return f.(func() (string, error))()
6470
}
6571

6672
var pkgExts = [...]string{".a", ".o"} // a file from the build cache will have no extension
@@ -69,10 +75,9 @@ var pkgExts = [...]string{".a", ".o"} // a file from the build cache will have n
6975
// path based on package information provided by build.Import (using
7076
// the build.Default build.Context). A relative srcDir is interpreted
7177
// relative to the current working directory.
72-
// If no file was found, an empty filename is returned.
73-
func FindPkg(path, srcDir string) (filename, id string) {
78+
func FindPkg(path, srcDir string) (filename, id string, err error) {
7479
if path == "" {
75-
return
80+
return "", "", errors.New("path is empty")
7681
}
7782

7883
var noext string
@@ -83,16 +88,19 @@ func FindPkg(path, srcDir string) (filename, id string) {
8388
if abs, err := filepath.Abs(srcDir); err == nil { // see issue 14282
8489
srcDir = abs
8590
}
86-
bp, _ := build.Import(path, srcDir, build.FindOnly|build.AllowBinary)
91+
var bp *build.Package
92+
bp, err = build.Import(path, srcDir, build.FindOnly|build.AllowBinary)
8793
if bp.PkgObj == "" {
88-
var ok bool
8994
if bp.Goroot && bp.Dir != "" {
90-
filename, ok = lookupGorootExport(bp.Dir)
91-
}
92-
if !ok {
93-
id = path // make sure we have an id to print in error message
94-
return
95+
filename, err = lookupGorootExport(bp.Dir)
96+
if err == nil {
97+
_, err = os.Stat(filename)
98+
}
99+
if err == nil {
100+
return filename, bp.ImportPath, nil
101+
}
95102
}
103+
goto notfound
96104
} else {
97105
noext = strings.TrimSuffix(bp.PkgObj, ".a")
98106
}
@@ -117,21 +125,23 @@ func FindPkg(path, srcDir string) (filename, id string) {
117125
}
118126
}
119127

120-
if filename != "" {
121-
if f, err := os.Stat(filename); err == nil && !f.IsDir() {
122-
return
123-
}
124-
}
125128
// try extensions
126129
for _, ext := range pkgExts {
127130
filename = noext + ext
128-
if f, err := os.Stat(filename); err == nil && !f.IsDir() {
129-
return
131+
f, statErr := os.Stat(filename)
132+
if statErr == nil && !f.IsDir() {
133+
return filename, id, nil
134+
}
135+
if err == nil {
136+
err = statErr
130137
}
131138
}
132139

133-
filename = "" // not found
134-
return
140+
notfound:
141+
if err == nil {
142+
return "", path, fmt.Errorf("can't find import: %q", path)
143+
}
144+
return "", path, fmt.Errorf("can't find import: %q: %w", path, err)
135145
}
136146

137147
// Import imports a gc-generated package given its import path and srcDir, adds
@@ -159,12 +169,12 @@ func Import(packages map[string]*types2.Package, path, srcDir string, lookup fun
159169
rc = f
160170
} else {
161171
var filename string
162-
filename, id = FindPkg(path, srcDir)
172+
filename, id, err = FindPkg(path, srcDir)
163173
if filename == "" {
164174
if path == "unsafe" {
165175
return types2.Unsafe, nil
166176
}
167-
return nil, fmt.Errorf("can't find import: %q", id)
177+
return nil, err
168178
}
169179

170180
// no need to re-import if the package was imported completely before

‎src/cmd/compile/internal/importer/gcimporter_test.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -105,9 +105,9 @@ func TestImportTestdata(t *testing.T) {
105105

106106
importMap := map[string]string{}
107107
for _, pkg := range wantImports {
108-
export, _ := FindPkg(pkg, "testdata")
108+
export, _, err := FindPkg(pkg, "testdata")
109109
if export == "" {
110-
t.Fatalf("no export data found for %s", pkg)
110+
t.Fatalf("no export data found for %s: %v", pkg, err)
111111
}
112112
importMap[pkg] = export
113113
}
@@ -268,7 +268,7 @@ var importedObjectTests = []struct {
268268
{"math.Pi", "const Pi untyped float"},
269269
{"math.Sin", "func Sin(x float64) float64"},
270270
{"go/ast.NotNilFilter", "func NotNilFilter(_ string, v reflect.Value) bool"},
271-
{"go/internal/gcimporter.FindPkg", "func FindPkg(path string, srcDir string) (filename string, id string)"},
271+
{"go/internal/gcimporter.FindPkg", "func FindPkg(path string, srcDir string) (filename string, id string, err error)"},
272272

273273
// interfaces
274274
{"context.Context", "type Context interface{Deadline() (deadline time.Time, ok bool); Done() <-chan struct{}; Err() error; Value(key any) any}"},
@@ -437,9 +437,9 @@ func TestIssue13566(t *testing.T) {
437437
t.Fatal(err)
438438
}
439439

440-
jsonExport, _ := FindPkg("encoding/json", "testdata")
440+
jsonExport, _, err := FindPkg("encoding/json", "testdata")
441441
if jsonExport == "" {
442-
t.Fatalf("no export data found for encoding/json")
442+
t.Fatalf("no export data found for encoding/json: %v", err)
443443
}
444444

445445
compile(t, "testdata", "a.go", testoutdir, map[string]string{"encoding/json": jsonExport})

‎src/go/internal/gcimporter/gcimporter.go

Lines changed: 38 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ package gcimporter // import "go/internal/gcimporter"
88
import (
99
"bufio"
1010
"bytes"
11+
"errors"
1112
"fmt"
1213
"go/build"
1314
"go/token"
@@ -25,7 +26,7 @@ import (
2526
// debugging/development support
2627
const debug = false
2728

28-
var exportMap sync.Map // package dir → func() (string, bool)
29+
var exportMap sync.Map // package dir → func() (string, error)
2930

3031
// lookupGorootExport returns the location of the export data
3132
// (normally found in the build cache, but located in GOROOT/pkg
@@ -34,37 +35,42 @@ var exportMap sync.Map // package dir → func() (string, bool)
3435
// (We use the package's directory instead of its import path
3536
// mainly to simplify handling of the packages in src/vendor
3637
// and cmd/vendor.)
37-
func lookupGorootExport(pkgDir string) (string, bool) {
38+
func lookupGorootExport(pkgDir string) (string, error) {
3839
f, ok := exportMap.Load(pkgDir)
3940
if !ok {
4041
var (
4142
listOnce sync.Once
4243
exportPath string
44+
err error
4345
)
44-
f, _ = exportMap.LoadOrStore(pkgDir, func() (string, bool) {
46+
f, _ = exportMap.LoadOrStore(pkgDir, func() (string, error) {
4547
listOnce.Do(func() {
4648
cmd := exec.Command(filepath.Join(build.Default.GOROOT, "bin", "go"), "list", "-export", "-f", "{{.Export}}", pkgDir)
4749
cmd.Dir = build.Default.GOROOT
48-
cmd.Env = append(cmd.Environ(), "GOROOT="+build.Default.GOROOT)
50+
cmd.Env = append(os.Environ(), "PWD="+cmd.Dir, "GOROOT="+build.Default.GOROOT)
4951
var output []byte
50-
output, err := cmd.Output()
52+
output, err = cmd.Output()
5153
if err != nil {
54+
if ee, ok := err.(*exec.ExitError); ok && len(ee.Stderr) > 0 {
55+
err = errors.New(string(ee.Stderr))
56+
}
5257
return
5358
}
5459

5560
exports := strings.Split(string(bytes.TrimSpace(output)), "\n")
5661
if len(exports) != 1 {
62+
err = fmt.Errorf("go list reported %d exports; expected 1", len(exports))
5763
return
5864
}
5965

6066
exportPath = exports[0]
6167
})
6268

63-
return exportPath, exportPath != ""
69+
return exportPath, err
6470
})
6571
}
6672

67-
return f.(func() (string, bool))()
73+
return f.(func() (string, error))()
6874
}
6975

7076
var pkgExts = [...]string{".a", ".o"} // a file from the build cache will have no extension
@@ -73,10 +79,9 @@ var pkgExts = [...]string{".a", ".o"} // a file from the build cache will have n
7379
// path based on package information provided by build.Import (using
7480
// the build.Default build.Context). A relative srcDir is interpreted
7581
// relative to the current working directory.
76-
// If no file was found, an empty filename is returned.
77-
func FindPkg(path, srcDir string) (filename, id string) {
82+
func FindPkg(path, srcDir string) (filename, id string, err error) {
7883
if path == "" {
79-
return
84+
return "", "", errors.New("path is empty")
8085
}
8186

8287
var noext string
@@ -87,16 +92,19 @@ func FindPkg(path, srcDir string) (filename, id string) {
8792
if abs, err := filepath.Abs(srcDir); err == nil { // see issue 14282
8893
srcDir = abs
8994
}
90-
bp, _ := build.Import(path, srcDir, build.FindOnly|build.AllowBinary)
95+
var bp *build.Package
96+
bp, err = build.Import(path, srcDir, build.FindOnly|build.AllowBinary)
9197
if bp.PkgObj == "" {
92-
var ok bool
9398
if bp.Goroot && bp.Dir != "" {
94-
filename, ok = lookupGorootExport(bp.Dir)
95-
}
96-
if !ok {
97-
id = path // make sure we have an id to print in error message
98-
return
99+
filename, err = lookupGorootExport(bp.Dir)
100+
if err == nil {
101+
_, err = os.Stat(filename)
102+
}
103+
if err == nil {
104+
return filename, bp.ImportPath, nil
105+
}
99106
}
107+
goto notfound
100108
} else {
101109
noext = strings.TrimSuffix(bp.PkgObj, ".a")
102110
}
@@ -121,21 +129,23 @@ func FindPkg(path, srcDir string) (filename, id string) {
121129
}
122130
}
123131

124-
if filename != "" {
125-
if f, err := os.Stat(filename); err == nil && !f.IsDir() {
126-
return
127-
}
128-
}
129132
// try extensions
130133
for _, ext := range pkgExts {
131134
filename = noext + ext
132-
if f, err := os.Stat(filename); err == nil && !f.IsDir() {
133-
return
135+
f, statErr := os.Stat(filename)
136+
if statErr == nil && !f.IsDir() {
137+
return filename, id, nil
138+
}
139+
if err == nil {
140+
err = statErr
134141
}
135142
}
136143

137-
filename = "" // not found
138-
return
144+
notfound:
145+
if err == nil {
146+
return "", path, fmt.Errorf("can't find import: %q", path)
147+
}
148+
return "", path, fmt.Errorf("can't find import: %q: %w", path, err)
139149
}
140150

141151
// Import imports a gc-generated package given its import path and srcDir, adds
@@ -163,12 +173,12 @@ func Import(fset *token.FileSet, packages map[string]*types.Package, path, srcDi
163173
rc = f
164174
} else {
165175
var filename string
166-
filename, id = FindPkg(path, srcDir)
176+
filename, id, err = FindPkg(path, srcDir)
167177
if filename == "" {
168178
if path == "unsafe" {
169179
return types.Unsafe, nil
170180
}
171-
return nil, fmt.Errorf("can't find import: %q", id)
181+
return nil, err
172182
}
173183

174184
// no need to re-import if the package was imported completely before

‎src/go/internal/gcimporter/gcimporter_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -391,7 +391,7 @@ var importedObjectTests = []struct {
391391
{"math.Pi", "const Pi untyped float"},
392392
{"math.Sin", "func Sin(x float64) float64"},
393393
{"go/ast.NotNilFilter", "func NotNilFilter(_ string, v reflect.Value) bool"},
394-
{"go/internal/gcimporter.FindPkg", "func FindPkg(path string, srcDir string) (filename string, id string)"},
394+
{"go/internal/gcimporter.FindPkg", "func FindPkg(path string, srcDir string) (filename string, id string, err error)"},
395395

396396
// interfaces
397397
{"context.Context", "type Context interface{Deadline() (deadline time.Time, ok bool); Done() <-chan struct{}; Err() error; Value(key any) any}"},

0 commit comments

Comments
 (0)