]> Cypherpunks repositories - gostls13.git/commitdiff
cmd/go: add file names for cyclic import error
authorxzhang39 <xzhang39@tesla.com>
Fri, 11 Oct 2024 03:58:57 +0000 (03:58 +0000)
committerMichael Matloob <matloob@golang.org>
Fri, 11 Oct 2024 19:00:21 +0000 (19:00 +0000)
The PR is to add more details for the error, so that it would be easier to troubleshoot the cyclic imports error.

The change for the error looks like the following:

package cyclic-import-example
        imports cyclic-import-example/packageA from /Users/personal/cyclic-import-example/main.go:4:5
        imports cyclic-import-example/packageB from /Users/personal/cyclic-import-example/packageA/a.go:5:2
        imports cyclic-import-example/packageA from /Users/personal/cyclic-import-example/packageB/bb.go:5:2: import cycle not allowed

Fixes #66078

Change-Id: I162cd348004bf4e4774b195f8355151c1bf0a652
GitHub-Last-Rev: c5a16256d1b5bb4d720c72624a44477be20da743
GitHub-Pull-Request: golang/go#68337
Reviewed-on: https://go-review.googlesource.com/c/go/+/597035
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

src/cmd/go/internal/list/list.go
src/cmd/go/internal/load/pkg.go
src/cmd/go/internal/load/test.go
src/cmd/go/internal/work/action.go
src/cmd/go/testdata/script/list_test_cycle.txt
src/cmd/go/testdata/script/mod_import_cycle.txt

index df3639cba753575368278755e73676af2b1fe2ce..823cfd74dc65d4f1dd2857d7f32cfcff95f87240 100644 (file)
@@ -967,7 +967,7 @@ func collectDepsErrors(p *load.Package) {
                        return false
                }
                pathi, pathj := stki[len(stki)-1], stkj[len(stkj)-1]
-               return pathi < pathj
+               return pathi.Pkg < pathj.Pkg
        })
 }
 
index 05f24415573a55eedc16cf467e7f60501a3146b8..1f222a543461ba7824803e198fa9da5e0fce965e 100644 (file)
@@ -327,7 +327,7 @@ func (p *Package) setLoadPackageDataError(err error, path string, stk *ImportSta
        // move the modload errors into this package to avoid a package import cycle,
        // and from having to export an error type for the errors produced in build.
        if !isMatchErr && (nogoErr != nil || isScanErr) {
-               stk.Push(path)
+               stk.Push(ImportInfo{Pkg: path, Pos: extractFirstImport(importPos)})
                defer stk.Pop()
        }
 
@@ -338,7 +338,8 @@ func (p *Package) setLoadPackageDataError(err error, path string, stk *ImportSta
        }
        p.Incomplete = true
 
-       if path != stk.Top() {
+       top, ok := stk.Top()
+       if ok && path != top.Pkg {
                p.Error.setPos(importPos)
        }
 }
@@ -455,11 +456,11 @@ func (p *Package) copyBuild(opts PackageOpts, pp *build.Package) {
 
 // A PackageError describes an error loading information about a package.
 type PackageError struct {
-       ImportStack      []string // shortest path from package named on command line to this one
-       Pos              string   // position of error
-       Err              error    // the error itself
-       IsImportCycle    bool     // the error is an import cycle
-       alwaysPrintStack bool     // whether to always print the ImportStack
+       ImportStack      ImportStack // shortest path from package named on command line to this one with position
+       Pos              string      // position of error
+       Err              error       // the error itself
+       IsImportCycle    bool        // the error is an import cycle
+       alwaysPrintStack bool        // whether to always print the ImportStack
 }
 
 func (p *PackageError) Error() string {
@@ -485,7 +486,11 @@ func (p *PackageError) Error() string {
        if p.Pos != "" {
                optpos = "\n\t" + p.Pos
        }
-       return "package " + strings.Join(p.ImportStack, "\n\timports ") + optpos + ": " + p.Err.Error()
+       imports := p.ImportStack.Pkgs()
+       if p.IsImportCycle {
+               imports = p.ImportStack.PkgsWithPos()
+       }
+       return "package " + strings.Join(imports, "\n\timports ") + optpos + ": " + p.Err.Error()
 }
 
 func (p *PackageError) Unwrap() error { return p.Err }
@@ -494,10 +499,10 @@ func (p *PackageError) Unwrap() error { return p.Err }
 // and non-essential fields are omitted.
 func (p *PackageError) MarshalJSON() ([]byte, error) {
        perr := struct {
-               ImportStack []string
+               ImportStack []string // use []string for package names
                Pos         string
                Err         string
-       }{p.ImportStack, p.Pos, p.Err.Error()}
+       }{p.ImportStack.Pkgs(), p.Pos, p.Err.Error()}
        return json.Marshal(perr)
 }
 
@@ -558,12 +563,21 @@ func (e *importError) ImportPath() string {
        return e.importPath
 }
 
+type ImportInfo struct {
+       Pkg string
+       Pos *token.Position
+}
+
 // An ImportStack is a stack of import paths, possibly with the suffix " (test)" appended.
 // The import path of a test package is the import path of the corresponding
 // non-test package with the suffix "_test" added.
-type ImportStack []string
+type ImportStack []ImportInfo
+
+func NewImportInfo(pkg string, pos *token.Position) ImportInfo {
+       return ImportInfo{Pkg: pkg, Pos: pos}
+}
 
-func (s *ImportStack) Push(p string) {
+func (s *ImportStack) Push(p ImportInfo) {
        *s = append(*s, p)
 }
 
@@ -571,15 +585,35 @@ func (s *ImportStack) Pop() {
        *s = (*s)[0 : len(*s)-1]
 }
 
-func (s *ImportStack) Copy() []string {
-       return append([]string{}, *s...)
+func (s *ImportStack) Copy() ImportStack {
+       return slices.Clone(*s)
+}
+
+func (s *ImportStack) Pkgs() []string {
+       ss := make([]string, 0, len(*s))
+       for _, v := range *s {
+               ss = append(ss, v.Pkg)
+       }
+       return ss
+}
+
+func (s *ImportStack) PkgsWithPos() []string {
+       ss := make([]string, 0, len(*s))
+       for _, v := range *s {
+               if v.Pos != nil {
+                       ss = append(ss, v.Pkg+" from "+filepath.Base(v.Pos.Filename))
+               } else {
+                       ss = append(ss, v.Pkg)
+               }
+       }
+       return ss
 }
 
-func (s *ImportStack) Top() string {
+func (s *ImportStack) Top() (ImportInfo, bool) {
        if len(*s) == 0 {
-               return ""
+               return ImportInfo{}, false
        }
-       return (*s)[len(*s)-1]
+       return (*s)[len(*s)-1], true
 }
 
 // shorterThan reports whether sp is shorter than t.
@@ -592,8 +626,9 @@ func (sp *ImportStack) shorterThan(t []string) bool {
        }
        // If they are the same length, settle ties using string ordering.
        for i := range s {
-               if s[i] != t[i] {
-                       return s[i] < t[i]
+               siPkg := s[i].Pkg
+               if siPkg != t[i] {
+                       return siPkg < t[i]
                }
        }
        return false // they are equal
@@ -655,7 +690,7 @@ const (
        cmdlinePkgLiteral
 )
 
-// LoadPackage does Load import, but without a parent package load contezt
+// LoadPackage does Load import, but without a parent package load context
 func LoadPackage(ctx context.Context, opts PackageOpts, path, srcDir string, stk *ImportStack, importPos []token.Position, mode int) *Package {
        p, err := loadImport(ctx, opts, nil, path, srcDir, nil, stk, importPos, mode)
        if err != nil {
@@ -707,7 +742,7 @@ func loadImport(ctx context.Context, opts PackageOpts, pre *preload, path, srcDi
                        // sequence that empirically doesn't trigger for these errors, guarded by
                        // a somewhat complex condition. Figure out how to generalize that
                        // condition and eliminate the explicit calls here.
-                       stk.Push(path)
+                       stk.Push(ImportInfo{Pkg: path, Pos: extractFirstImport(importPos)})
                        defer stk.Pop()
                }
                p.setLoadPackageDataError(err, path, stk, nil)
@@ -726,7 +761,7 @@ func loadImport(ctx context.Context, opts PackageOpts, pre *preload, path, srcDi
        importPath := bp.ImportPath
        p := packageCache[importPath]
        if p != nil {
-               stk.Push(path)
+               stk.Push(ImportInfo{Pkg: path, Pos: extractFirstImport(importPos)})
                p = reusePackage(p, stk)
                stk.Pop()
                setCmdline(p)
@@ -792,6 +827,13 @@ func loadImport(ctx context.Context, opts PackageOpts, pre *preload, path, srcDi
        return p, nil
 }
 
+func extractFirstImport(importPos []token.Position) *token.Position {
+       if len(importPos) == 0 {
+               return nil
+       }
+       return &importPos[0]
+}
+
 // loadPackageData loads information needed to construct a *Package. The result
 // is cached, and later calls to loadPackageData for the same package will return
 // the same data.
@@ -1412,7 +1454,8 @@ func reusePackage(p *Package, stk *ImportStack) *Package {
        }
        // Don't rewrite the import stack in the error if we have an import cycle.
        // If we do, we'll lose the path that describes the cycle.
-       if p.Error != nil && !p.Error.IsImportCycle && stk.shorterThan(p.Error.ImportStack) {
+       if p.Error != nil && p.Error.ImportStack != nil &&
+               !p.Error.IsImportCycle && stk.shorterThan(p.Error.ImportStack.Pkgs()) {
                p.Error.ImportStack = stk.Copy()
        }
        return p
@@ -1739,7 +1782,8 @@ func (p *Package) load(ctx context.Context, opts PackageOpts, path string, stk *
                        // then the cause of the error is not within p itself: the error
                        // must be either in an explicit command-line argument,
                        // or on the importer side (indicated by a non-empty importPos).
-                       if path != stk.Top() && len(importPos) > 0 {
+                       top, ok := stk.Top()
+                       if ok && path != top.Pkg && len(importPos) > 0 {
                                p.Error.setPos(importPos)
                        }
                }
@@ -1905,7 +1949,7 @@ func (p *Package) load(ctx context.Context, opts PackageOpts, path string, stk *
        // Errors after this point are caused by this package, not the importing
        // package. Pushing the path here prevents us from reporting the error
        // with the position of the import declaration.
-       stk.Push(path)
+       stk.Push(ImportInfo{Pkg: path, Pos: extractFirstImport(importPos)})
        defer stk.Pop()
 
        pkgPath := p.ImportPath
index 31fe23a61c135c9a9864b5b30fcc98168d1fcbce..5f0be712556459fa512e063f76e1dd9f2d534d5a 100644 (file)
@@ -114,7 +114,7 @@ func TestPackagesAndErrors(ctx context.Context, done func(), opts PackageOpts, p
        var stk ImportStack
        var testEmbed, xtestEmbed map[string][]string
        var incomplete bool
-       stk.Push(p.ImportPath + " (test)")
+       stk.Push(ImportInfo{Pkg: p.ImportPath + " (test)"})
        rawTestImports := str.StringList(p.TestImports)
        for i, path := range p.TestImports {
                p1, err := loadImport(ctx, opts, pre, path, p.Dir, p, &stk, p.Internal.Build.TestImportPos[path], ResolveImport)
@@ -141,7 +141,7 @@ func TestPackagesAndErrors(ctx context.Context, done func(), opts PackageOpts, p
        }
        stk.Pop()
 
-       stk.Push(p.ImportPath + "_test")
+       stk.Push(ImportInfo{Pkg: p.ImportPath + "_test"})
        pxtestNeedsPtest := false
        var pxtestIncomplete bool
        rawXTestImports := str.StringList(p.XTestImports)
@@ -304,7 +304,7 @@ func TestPackagesAndErrors(ctx context.Context, done func(), opts PackageOpts, p
 
        // The generated main also imports testing, regexp, and os.
        // Also the linker introduces implicit dependencies reported by LinkerDeps.
-       stk.Push("testmain")
+       stk.Push(ImportInfo{Pkg: "testmain"})
        deps := TestMainDeps // cap==len, so safe for append
        if cover != nil && cfg.Experiment.CoverageRedesign {
                deps = append(deps, "internal/coverage/cfile")
@@ -544,9 +544,16 @@ func recompileForTest(pmain, preal, ptest, pxtest *Package) *PackageError {
                        // The stack is supposed to be in the order x imports y imports z.
                        // We collect in the reverse order: z is imported by y is imported
                        // by x, and then we reverse it.
-                       var stk []string
+                       var stk ImportStack
                        for p != nil {
-                               stk = append(stk, p.ImportPath)
+                               importer, ok := importerOf[p]
+                               if importer == nil && ok { // we set importerOf[p] == nil for the initial set of packages p that are imports of ptest
+                                       importer = ptest
+                               }
+                               stk = append(stk, ImportInfo{
+                                       Pkg: p.ImportPath,
+                                       Pos: extractFirstImport(importer.Internal.Build.ImportPos[p.ImportPath]),
+                               })
                                p = importerOf[p]
                        }
                        // complete the cycle: we set importer[p] = nil to break the cycle
@@ -554,9 +561,10 @@ func recompileForTest(pmain, preal, ptest, pxtest *Package) *PackageError {
                        // back here since we reached nil in the loop above to demonstrate
                        // the cycle as (for example) package p imports package q imports package r
                        // imports package p.
-                       stk = append(stk, ptest.ImportPath)
+                       stk = append(stk, ImportInfo{
+                               Pkg: ptest.ImportPath,
+                       })
                        slices.Reverse(stk)
-
                        return &PackageError{
                                ImportStack:   stk,
                                Err:           errors.New("import cycle not allowed in test"),
index 48ed3b764142308c03581e3083b9973003a4dea6..9d481412c210f833df4525270c973d59b64eb707 100644 (file)
@@ -631,7 +631,7 @@ func (b *Builder) vetAction(mode, depMode BuildMode, p *load.Package) *Action {
 
                // vet expects to be able to import "fmt".
                var stk load.ImportStack
-               stk.Push("vet")
+               stk.Push(load.NewImportInfo("vet", nil))
                p1, err := load.LoadImportWithFlags("fmt", p.Dir, p, &stk, nil, 0)
                if err != nil {
                        base.Fatalf("unexpected error loading fmt package from package %s: %v", p.ImportPath, err)
index 67edf183374b0b6cf096f0e4a50c648822b2248f..7480e6111dd8ce7551fc53b6cac223c3b826a078 100644 (file)
@@ -15,9 +15,9 @@ cmp stderr wanterr.txt
 
 -- wanterr.txt --
 go: can't load test package: package example/p
-       imports example/q
-       imports example/r
-       imports example/p: import cycle not allowed in test
+       imports example/q from p_test.go
+       imports example/r from q.go
+       imports example/p from r.go: import cycle not allowed in test
 -- go.mod --
 module example
 go 1.20
index 7be074973a933b602863f1a7444ffba2033a4d9a..218efc6e840890adbe8c5b37bb47d68958fb21e0 100644 (file)
@@ -2,7 +2,7 @@ env GO111MODULE=on
 
 # 'go list all' should fail with a reasonable error message
 ! go list all
-stderr '^package m\n\timports m/a\n\timports m/b\n\timports m/a: import cycle not allowed'
+stderr '^package m\n\timports m/a from m.go\n\timports m/b from a.go\n\timports m/a from b.go: import cycle not allowed'
 
 # 'go list -e' should not print to stderr, but should mark all three
 # packages (m, m/a, and m/b) as Incomplete.