From: xzhang39 Date: Fri, 11 Oct 2024 03:58:57 +0000 (+0000) Subject: cmd/go: add file names for cyclic import error X-Git-Tag: go1.24rc1~706 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=1c6288f7e1005a1217859c20e4892a9f2cfd8091;p=gostls13.git cmd/go: add file names for cyclic import error 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 Reviewed-by: Michael Matloob LUCI-TryBot-Result: Go LUCI --- diff --git a/src/cmd/go/internal/list/list.go b/src/cmd/go/internal/list/list.go index df3639cba7..823cfd74dc 100644 --- a/src/cmd/go/internal/list/list.go +++ b/src/cmd/go/internal/list/list.go @@ -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 }) } diff --git a/src/cmd/go/internal/load/pkg.go b/src/cmd/go/internal/load/pkg.go index 05f2441557..1f222a5434 100644 --- a/src/cmd/go/internal/load/pkg.go +++ b/src/cmd/go/internal/load/pkg.go @@ -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 diff --git a/src/cmd/go/internal/load/test.go b/src/cmd/go/internal/load/test.go index 31fe23a61c..5f0be71255 100644 --- a/src/cmd/go/internal/load/test.go +++ b/src/cmd/go/internal/load/test.go @@ -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"), diff --git a/src/cmd/go/internal/work/action.go b/src/cmd/go/internal/work/action.go index 48ed3b7641..9d481412c2 100644 --- a/src/cmd/go/internal/work/action.go +++ b/src/cmd/go/internal/work/action.go @@ -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) diff --git a/src/cmd/go/testdata/script/list_test_cycle.txt b/src/cmd/go/testdata/script/list_test_cycle.txt index 67edf18337..7480e6111d 100644 --- a/src/cmd/go/testdata/script/list_test_cycle.txt +++ b/src/cmd/go/testdata/script/list_test_cycle.txt @@ -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 diff --git a/src/cmd/go/testdata/script/mod_import_cycle.txt b/src/cmd/go/testdata/script/mod_import_cycle.txt index 7be074973a..218efc6e84 100644 --- a/src/cmd/go/testdata/script/mod_import_cycle.txt +++ b/src/cmd/go/testdata/script/mod_import_cycle.txt @@ -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.