Skip to content

Commit

Permalink
go/packages: allow types loading without NeedDeps
Browse files Browse the repository at this point in the history 
Before separating Load* into Need* we could use LoadSyntax
to get types information by loading inital packages
from source code and loading it's direct dependencies from export data.
It was broken when separation was introduced and before this commit
everything was loading from source code what resulted into slow
loading times. This commit fixes it.

Also, do backwards-incompatible fix of definition of deprecated
LoadImports and LoadAllSyntax.

Improve an internal error message
"internal error: nil Pkg importing x from y": replace it with
"internal error: package x without types was imported from y".

Remove packages.NeedDeps request for loading in tests
TestLoadTypesBits and TestContainsOverlayXTest.

Fixes golang/go#31752, fixes golang/go#33077, fixes golang/go#32814,
          fixes golang/go#31699, fixes golang/go#31930

Change-Id: I416e3c1035d555d67039e45a4fdd1deb9b2184ef
GitHub-Last-Rev: 2e3a46e
GitHub-Pull-Request: golang#139
Reviewed-on: https://go-review.googlesource.com/c/tools/+/186337
Reviewed-by: Michael Matloob <[email protected]>
  • Loading branch information
@jirfag @clintjedwards
jirfag authored and clintjedwards committed Sep 19, 2019
1 parent 3420a31 commit bf4e910
Show file tree
Hide file tree
Showing 3 changed files with 187 additions and 62 deletions.
3 changes: 1 addition & 2 deletions go/packages/golist.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
101 changes: 53 additions & 48 deletions go/packages/packages.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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.
Expand All @@ -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.
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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()
Expand All @@ -476,6 +481,7 @@ func newLoader(cfg *Config) *loader {
}
}
}

return ld
}

Expand All @@ -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",
Expand Down Expand Up @@ -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
Expand All @@ -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
}
Expand All @@ -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
Expand All @@ -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.
Expand All @@ -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
}

Expand All @@ -691,7 +682,6 @@ func (ld *loader) loadRecursive(lpkg *loaderPackage) {
}(imp)
}
wg.Wait()

ld.loadPackage(lpkg)
})
}
Expand Down Expand Up @@ -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")
})

Expand All @@ -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,
Expand Down Expand Up @@ -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
}
Loading

0 comments on commit bf4e910

Please sign in to comment.