diff --git a/go/packages/golist.go b/go/packages/golist.go index 88e0cbc708b..112c81e0e02 100644 --- a/go/packages/golist.go +++ b/go/packages/golist.go @@ -832,8 +832,7 @@ func golistargs(cfg *Config, words []string) []string { fmt.Sprintf("-compiled=%t", cfg.Mode&(NeedCompiledGoFiles|NeedSyntax|NeedTypesInfo|NeedTypesSizes) != 0), fmt.Sprintf("-test=%t", cfg.Tests), fmt.Sprintf("-export=%t", usesExportData(cfg)), - fmt.Sprintf("-deps=%t", cfg.Mode&NeedDeps != 0 || - cfg.Mode&NeedTypesInfo != 0), // Dependencies are required to do typechecking on sources, which is required for the TypesInfo. + fmt.Sprintf("-deps=%t", cfg.Mode&NeedImports != 0), // go list doesn't let you pass -test and -find together, // probably because you'd just get the TestMain. fmt.Sprintf("-find=%t", !cfg.Tests && cfg.Mode&findFlags == 0), diff --git a/go/packages/packages.go b/go/packages/packages.go index 4ca2b117320..7450d21bc98 100644 --- a/go/packages/packages.go +++ b/go/packages/packages.go @@ -48,8 +48,7 @@ const ( // "placeholder" Packages with only the ID set. NeedImports - // NeedDeps adds the fields requested by the LoadMode in the packages in Imports. If NeedImports - // is not set NeedDeps has no effect. + // NeedDeps adds the fields requested by the LoadMode in the packages in Imports. NeedDeps // NeedExportsFile adds ExportsFile. @@ -75,7 +74,7 @@ const ( // Deprecated: LoadImports exists for historical compatibility // and should not be used. Please directly specify the needed fields using the Need values. - LoadImports = LoadFiles | NeedImports | NeedDeps + LoadImports = LoadFiles | NeedImports // Deprecated: LoadTypes exists for historical compatibility // and should not be used. Please directly specify the needed fields using the Need values. @@ -87,7 +86,7 @@ const ( // Deprecated: LoadAllSyntax exists for historical compatibility // and should not be used. Please directly specify the needed fields using the Need values. - LoadAllSyntax = LoadSyntax + LoadAllSyntax = LoadSyntax | NeedDeps ) // A Config specifies details about how packages should be loaded. @@ -416,11 +415,13 @@ type loader struct { parseCacheMu sync.Mutex exportMu sync.Mutex // enforces mutual exclusion of exportdata operations - // TODO(matloob): Add an implied mode here and use that instead of mode. - // Implied mode would contain all the fields we need the data for so we can - // get the actually requested fields. We'll zero them out before returning - // packages to the user. This will make it easier for us to get the conditions - // where we need certain modes right. + // Config.Mode contains the implied mode (see impliedLoadMode). + // Implied mode contains all the fields we need the data for. + // In requestedMode there are the actually requested fields. + // We'll zero them out before returning packages to the user. + // This makes it easier for us to get the conditions where + // we need certain modes right. + requestedMode LoadMode } type parseValue struct { @@ -462,6 +463,10 @@ func newLoader(cfg *Config) *loader { } } + // Save the actually requested fields. We'll zero them out before returning packages to the user. + ld.requestedMode = ld.Mode + ld.Mode = impliedLoadMode(ld.Mode) + if ld.Mode&NeedTypes != 0 { if ld.Fset == nil { ld.Fset = token.NewFileSet() @@ -476,6 +481,7 @@ func newLoader(cfg *Config) *loader { } } } + return ld } @@ -496,7 +502,7 @@ func (ld *loader) refine(roots []string, list ...*Package) ([]*Package, error) { } lpkg := &loaderPackage{ Package: pkg, - needtypes: (ld.Mode&(NeedTypes|NeedTypesInfo) != 0 && rootIndex < 0) || rootIndex >= 0, + needtypes: (ld.Mode&(NeedTypes|NeedTypesInfo) != 0 && ld.Mode&NeedDeps != 0 && rootIndex < 0) || rootIndex >= 0, needsrc: (ld.Mode&(NeedSyntax|NeedTypesInfo) != 0 && ld.Mode&NeedDeps != 0 && rootIndex < 0) || rootIndex >= 0 || len(ld.Overlay) > 0 || // Overlays can invalidate export data. TODO(matloob): make this check fine-grained based on dependencies on overlaid files pkg.ExportFile == "" && pkg.PkgPath != "unsafe", @@ -544,10 +550,8 @@ func (ld *loader) refine(roots []string, list ...*Package) ([]*Package, error) { lpkg.color = grey stack = append(stack, lpkg) // push stubs := lpkg.Imports // the structure form has only stubs with the ID in the Imports - // If NeedTypesInfo we need dependencies (at least for the roots) to typecheck the package. // If NeedImports isn't set, the imports fields will all be zeroed out. - // If NeedDeps isn't also set we want to keep the stubs. - if ld.Mode&NeedTypesInfo != 0 || (ld.Mode&NeedImports != 0 && ld.Mode&NeedDeps != 0) { + if ld.Mode&NeedImports != 0 { lpkg.Imports = make(map[string]*Package, len(stubs)) for importPath, ipkg := range stubs { var importErr error @@ -566,11 +570,8 @@ func (ld *loader) refine(roots []string, list ...*Package) ([]*Package, error) { continue } - // If !NeedDeps, just fill Imports for the root. No need to recurse further. - if ld.Mode&NeedDeps != 0 { - if visit(imp) { - lpkg.needsrc = true - } + if visit(imp) { + lpkg.needsrc = true } lpkg.Imports[importPath] = imp.Package } @@ -587,7 +588,7 @@ func (ld *loader) refine(roots []string, list ...*Package) ([]*Package, error) { return lpkg.needsrc } - if ld.Mode&(NeedImports|NeedDeps|NeedTypesInfo) == 0 { + if ld.Mode&NeedImports == 0 { // We do this to drop the stub import packages that we are not even going to try to resolve. for _, lpkg := range initial { lpkg.Imports = nil @@ -598,7 +599,7 @@ func (ld *loader) refine(roots []string, list ...*Package) ([]*Package, error) { visit(lpkg) } } - if ld.Mode&NeedDeps != 0 { // TODO(matloob): This is only the case if NeedTypes is also set, right? + if ld.Mode&NeedImports != 0 && ld.Mode&NeedTypes != 0 { for _, lpkg := range srcPkgs { // Complete type information is required for the // immediate dependencies of each source package. @@ -623,54 +624,44 @@ func (ld *loader) refine(roots []string, list ...*Package) ([]*Package, error) { } result := make([]*Package, len(initial)) - importPlaceholders := make(map[string]*Package) for i, lpkg := range initial { result[i] = lpkg.Package } for i := range ld.pkgs { // Clear all unrequested fields, for extra de-Hyrum-ization. - if ld.Mode&NeedName == 0 { + if ld.requestedMode&NeedName == 0 { ld.pkgs[i].Name = "" ld.pkgs[i].PkgPath = "" } - if ld.Mode&NeedFiles == 0 { + if ld.requestedMode&NeedFiles == 0 { ld.pkgs[i].GoFiles = nil ld.pkgs[i].OtherFiles = nil } - if ld.Mode&NeedCompiledGoFiles == 0 { + if ld.requestedMode&NeedCompiledGoFiles == 0 { ld.pkgs[i].CompiledGoFiles = nil } - if ld.Mode&NeedImports == 0 { + if ld.requestedMode&NeedImports == 0 { ld.pkgs[i].Imports = nil } - if ld.Mode&NeedExportsFile == 0 { + if ld.requestedMode&NeedExportsFile == 0 { ld.pkgs[i].ExportFile = "" } - if ld.Mode&NeedTypes == 0 { + if ld.requestedMode&NeedTypes == 0 { ld.pkgs[i].Types = nil ld.pkgs[i].Fset = nil ld.pkgs[i].IllTyped = false } - if ld.Mode&NeedSyntax == 0 { + if ld.requestedMode&NeedSyntax == 0 { ld.pkgs[i].Syntax = nil } - if ld.Mode&NeedTypesInfo == 0 { + if ld.requestedMode&NeedTypesInfo == 0 { ld.pkgs[i].TypesInfo = nil } - if ld.Mode&NeedTypesSizes == 0 { + if ld.requestedMode&NeedTypesSizes == 0 { ld.pkgs[i].TypesSizes = nil } - if ld.Mode&NeedDeps == 0 { - for j, pkg := range ld.pkgs[i].Imports { - ph, ok := importPlaceholders[pkg.ID] - if !ok { - ph = &Package{ID: pkg.ID} - importPlaceholders[pkg.ID] = ph - } - ld.pkgs[i].Imports[j] = ph - } - } } + return result, nil } @@ -691,7 +682,6 @@ func (ld *loader) loadRecursive(lpkg *loaderPackage) { }(imp) } wg.Wait() - ld.loadPackage(lpkg) }) } @@ -818,7 +808,7 @@ func (ld *loader) loadPackage(lpkg *loaderPackage) { if ipkg.Types != nil && ipkg.Types.Complete() { return ipkg.Types, nil } - log.Fatalf("internal error: nil Pkg importing %q from %q", path, lpkg) + log.Fatalf("internal error: package %q without types was imported from %q", path, lpkg) panic("unreachable") }) @@ -829,7 +819,7 @@ func (ld *loader) loadPackage(lpkg *loaderPackage) { // Type-check bodies of functions only in non-initial packages. // Example: for import graph A->B->C and initial packages {A,C}, // we can ignore function bodies in B. - IgnoreFuncBodies: (ld.Mode&(NeedDeps|NeedTypesInfo) == 0) && !lpkg.initial, + IgnoreFuncBodies: ld.Mode&NeedDeps == 0 && !lpkg.initial, Error: appendError, Sizes: ld.sizes, @@ -1091,10 +1081,25 @@ func (ld *loader) loadFromExportData(lpkg *loaderPackage) (*types.Package, error return tpkg, nil } +// impliedLoadMode returns loadMode with it's dependencies +func impliedLoadMode(loadMode LoadMode) LoadMode { + if loadMode&NeedTypesInfo != 0 && loadMode&NeedImports == 0 { + // If NeedTypesInfo, go/packages needs to do typechecking itself so it can + // associate type info with the AST. To do so, we need the export data + // for dependencies, which means we need to ask for the direct dependencies. + // NeedImports is used to ask for the direct dependencies. + loadMode |= NeedImports + } + + if loadMode&NeedDeps != 0 && loadMode&NeedImports == 0 { + // With NeedDeps we need to load at least direct dependencies. + // NeedImports is used to ask for the direct dependencies. + loadMode |= NeedImports + } + + return loadMode +} + func usesExportData(cfg *Config) bool { - return cfg.Mode&NeedExportsFile != 0 || - // If NeedTypes but not NeedTypesInfo we won't typecheck using sources, so we need export data. - (cfg.Mode&NeedTypes != 0 && cfg.Mode&NeedTypesInfo == 0) || - // If NeedTypesInfo but not NeedDeps, we're typechecking a package using its sources plus its dependencies' export data - (cfg.Mode&NeedTypesInfo != 0 && cfg.Mode&NeedDeps == 0) + return cfg.Mode&NeedExportsFile != 0 || cfg.Mode&NeedTypes != 0 && cfg.Mode&NeedDeps == 0 } diff --git a/go/packages/packages_test.go b/go/packages/packages_test.go index 1f73688c48c..7e8487e6113 100644 --- a/go/packages/packages_test.go +++ b/go/packages/packages_test.go @@ -577,7 +577,7 @@ func testLoadTypesBits(t *testing.T, exporter packagestest.Exporter) { }}}) defer exported.Cleanup() - exported.Config.Mode = packages.NeedTypes | packages.NeedDeps | packages.NeedImports + exported.Config.Mode = packages.NeedTypes | packages.NeedImports initial, err := packages.Load(exported.Config, "golang.org/fake/a", "golang.org/fake/c") if err != nil { t.Fatal(err) @@ -678,14 +678,16 @@ func testLoadSyntaxOK(t *testing.T, exporter packagestest.Exporter) { } for _, test := range []struct { - id string + id string + wantSyntax bool + wantComplete bool }{ - {"golang.org/fake/a"}, // source package - {"golang.org/fake/b"}, // source package - {"golang.org/fake/c"}, // source package - {"golang.org/fake/d"}, // export data package - {"golang.org/fake/e"}, // export data package - {"golang.org/fake/f"}, // export data package + {"golang.org/fake/a", true, true}, // source package + {"golang.org/fake/b", true, true}, // source package because depends on initial package + {"golang.org/fake/c", true, true}, // source package + {"golang.org/fake/d", false, true}, // export data package + {"golang.org/fake/e", false, false}, // export data package + {"golang.org/fake/f", false, false}, // export data package } { // TODO(matloob): LoadSyntax and LoadAllSyntax are now equivalent, wantSyntax and wantComplete // are true for all packages in the transitive dependency set. Add test cases on the individual @@ -698,9 +700,19 @@ func testLoadSyntaxOK(t *testing.T, exporter packagestest.Exporter) { if p.Types == nil { t.Errorf("missing types.Package for %s", p) continue + } else if p.Types.Complete() != test.wantComplete { + if test.wantComplete { + t.Errorf("incomplete types.Package for %s", p) + } else { + t.Errorf("unexpected complete types.Package for %s", p) + } } - if p.Syntax == nil { - t.Errorf("missing ast.Files for %s", p) + if (p.Syntax != nil) != test.wantSyntax { + if test.wantSyntax { + t.Errorf("missing ast.Files for %s", p) + } else { + t.Errorf("unexpected ast.Files for for %s", p) + } } if p.Errors != nil { t.Errorf("errors in package: %s: %s", p, p.Errors) @@ -796,7 +808,7 @@ func testLoadSyntaxError(t *testing.T, exporter packagestest.Exporter) { {"golang.org/fake/c", true, true}, {"golang.org/fake/d", true, true}, {"golang.org/fake/e", true, true}, - {"golang.org/fake/f", true, false}, + {"golang.org/fake/f", false, false}, } { p := all[test.id] if p == nil { @@ -1407,7 +1419,7 @@ func testContainsOverlayXTest(t *testing.T, exporter packagestest.Exporter) { }}}) defer exported.Cleanup() bOverlayXTestFile := filepath.Join(filepath.Dir(exported.File("golang.org/fake", "b/b.go")), "b_overlay_x_test.go") - exported.Config.Mode = packages.NeedName | packages.NeedFiles | packages.NeedImports | packages.NeedDeps + exported.Config.Mode = packages.NeedName | packages.NeedFiles | packages.NeedImports exported.Config.Overlay = map[string][]byte{bOverlayXTestFile: []byte(`package b_test; import "golang.org/fake/b"`)} initial, err := packages.Load(exported.Config, "file="+bOverlayXTestFile) if err != nil { @@ -2258,6 +2270,115 @@ func testIssue32814(t *testing.T, exporter packagestest.Exporter) { } } +func TestLoadTypesInfoWithoutNeedDeps(t *testing.T) { + packagestest.TestAll(t, testLoadTypesInfoWithoutNeedDeps) +} +func testLoadTypesInfoWithoutNeedDeps(t *testing.T, exporter packagestest.Exporter) { + exported := packagestest.Export(t, exporter, []packagestest.Module{{ + Name: "golang.org/fake", + Files: map[string]interface{}{ + "a/a.go": `package a; import _ "golang.org/fake/b"`, + "b/b.go": `package b`, + }}}) + defer exported.Cleanup() + + exported.Config.Mode = packages.NeedTypes | packages.NeedTypesInfo | packages.NeedImports + pkgs, err := packages.Load(exported.Config, "golang.org/fake/a") + if err != nil { + t.Fatal(err) + } + pkg := pkgs[0] + if pkg.IllTyped { + t.Fatal("Loaded package is ill typed") + } + const expectedImport = "golang.org/fake/b" + if _, ok := pkg.Imports[expectedImport]; !ok || len(pkg.Imports) != 1 { + t.Fatalf("Imports of loaded package: want [%s], got %v", expectedImport, pkg.Imports) + } +} + +func TestLoadWithNeedDeps(t *testing.T) { + packagestest.TestAll(t, testLoadWithNeedDeps) +} +func testLoadWithNeedDeps(t *testing.T, exporter packagestest.Exporter) { + exported := packagestest.Export(t, exporter, []packagestest.Module{{ + Name: "golang.org/fake", + Files: map[string]interface{}{ + "a/a.go": `package a; import _ "golang.org/fake/b"`, + "b/b.go": `package b; import _ "golang.org/fake/c"`, + "c/c.go": `package c`, + }}}) + defer exported.Cleanup() + + exported.Config.Mode = packages.NeedTypes | packages.NeedTypesInfo | packages.NeedImports | packages.NeedDeps + pkgs, err := packages.Load(exported.Config, "golang.org/fake/a") + if err != nil { + t.Fatal(err) + } + if len(pkgs) != 1 { + t.Fatalf("Expected 1 package, got %d", len(pkgs)) + } + + pkgA := pkgs[0] + if pkgA.IllTyped { + t.Fatal("Loaded package is ill typed") + } + + pkgB := pkgA.Imports["golang.org/fake/b"] + if pkgB == nil || len(pkgA.Imports) != 1 { + t.Fatalf("Imports of loaded package 'a' are invalid: %v", pkgA.Imports) + } + if pkgB.Types == nil || !pkgB.Types.Complete() || pkgB.TypesInfo == nil { + t.Fatalf("Types of package 'b' are nil or incomplete: %v, %v", pkgB.Types, pkgB.TypesInfo) + } + + pkgC := pkgB.Imports["golang.org/fake/c"] + if pkgC == nil || len(pkgB.Imports) != 1 { + t.Fatalf("Imports of loaded package 'c' are invalid: %v", pkgB.Imports) + } + if pkgC.Types == nil || !pkgC.Types.Complete() || pkgC.TypesInfo == nil { + t.Fatalf("Types of package 'b' are nil or incomplete: %v, %v", pkgC.Types, pkgC.TypesInfo) + } +} + +func TestImpliedLoadMode(t *testing.T) { + packagestest.TestAll(t, testImpliedLoadMode) +} +func testImpliedLoadMode(t *testing.T, exporter packagestest.Exporter) { + exported := packagestest.Export(t, exporter, []packagestest.Module{{ + Name: "golang.org/fake", + Files: map[string]interface{}{ + "a/a.go": `package a; import _ "golang.org/fake/b"`, + "b/b.go": `package b`, + }}}) + defer exported.Cleanup() + + exported.Config.Mode = packages.NeedTypes | packages.NeedTypesInfo + pkgs, err := packages.Load(exported.Config, "golang.org/fake/a") + if err != nil { + t.Fatal(err) + } + if len(pkgs) != 1 { + t.Fatalf("Expected 1 package, got %d", len(pkgs)) + } + + pkg := pkgs[0] + if pkg.IllTyped { + t.Fatalf("Loaded package is ill typed: %v", pkg.Errors) + } + + // Check that packages.NeedTypesInfo worked well. + if !pkg.Types.Complete() { + t.Fatalf("Loaded package types are incomplete") + } + + // Check that implied packages.NeedImports by packages.NeedTypesInfo + // didn't add Imports. + if len(pkg.Imports) != 0 { + t.Fatalf("Package imports weren't requested but were returned: %v", pkg.Imports) + } +} + func errorMessages(errors []packages.Error) []string { var msgs []string for _, err := range errors {