Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

normalize loaded build packages and handle missing packages without panicking #222

Merged
merged 1 commit into from
Nov 29, 2021
Merged

normalize loaded build packages and handle missing packages without panicking #222

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 52 additions & 21 deletions parser/parse.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,9 @@ type Builder struct {
// This might hold the same value for multiple names, e.g. if someone
// referenced ./pkg/name or in the case of vendoring, which canonicalizes
// differently that what humans would type.
buildPackages map[string]*build.Package
//
// This must only be accessed via getLoadedBuildPackage and setLoadedBuildPackage
buildPackages map[importPathString]*build.Package

fset *token.FileSet
// map of package path to list of parsed files
Expand Down Expand Up @@ -102,7 +104,7 @@ func New() *Builder {
c.CgoEnabled = false
return &Builder{
context: &c,
buildPackages: map[string]*build.Package{},
buildPackages: map[importPathString]*build.Package{},
typeCheckedPackages: map[importPathString]*tc.Package{},
fset: token.NewFileSet(),
parsed: map[importPathString][]parsedFile{},
Expand All @@ -118,11 +120,37 @@ func (b *Builder) AddBuildTags(tags ...string) {
b.context.BuildTags = append(b.context.BuildTags, tags...)
}

func (b *Builder) getLoadedBuildPackage(importPath string) (*build.Package, bool) {
canonicalized := canonicalizeImportPath(importPath)
if string(canonicalized) != importPath {
klog.V(5).Infof("getLoadedBuildPackage: %s normalized to %s", importPath, canonicalized)
}
buildPkg, ok := b.buildPackages[canonicalized]
return buildPkg, ok
}
func (b *Builder) setLoadedBuildPackage(importPath string, buildPkg *build.Package) {
canonicalizedImportPath := canonicalizeImportPath(importPath)
if string(canonicalizedImportPath) != importPath {
klog.V(5).Infof("setLoadedBuildPackage: importPath %s normalized to %s", importPath, canonicalizedImportPath)
}

canonicalizedBuildPkgImportPath := canonicalizeImportPath(buildPkg.ImportPath)
if string(canonicalizedBuildPkgImportPath) != buildPkg.ImportPath {
klog.V(5).Infof("setLoadedBuildPackage: buildPkg.ImportPath %s normalized to %s", buildPkg.ImportPath, canonicalizedBuildPkgImportPath)
}

if canonicalizedImportPath != canonicalizedBuildPkgImportPath {
klog.V(5).Infof("setLoadedBuildPackage: normalized importPath (%s) differs from buildPkg.ImportPath (%s)", canonicalizedImportPath, canonicalizedBuildPkgImportPath)
}
b.buildPackages[canonicalizedImportPath] = buildPkg
b.buildPackages[canonicalizedBuildPkgImportPath] = buildPkg
}

// Get package information from the go/build package. Automatically excludes
// e.g. test files and files for other platforms-- there is quite a bit of
// logic of that nature in the build package.
func (b *Builder) importBuildPackage(dir string) (*build.Package, error) {
if buildPkg, ok := b.buildPackages[dir]; ok {
if buildPkg, ok := b.getLoadedBuildPackage(dir); ok {
return buildPkg, nil
}
// This validates the `package foo // github.com/bar/foo` comments.
Expand All @@ -142,17 +170,7 @@ func (b *Builder) importBuildPackage(dir string) (*build.Package, error) {

// Remember it under the user-provided name.
klog.V(5).Infof("saving buildPackage %s", dir)
b.buildPackages[dir] = buildPkg
canonicalPackage := canonicalizeImportPath(buildPkg.ImportPath)
if dir != string(canonicalPackage) {
// Since `dir` is not the canonical name, see if we knew it under another name.
if buildPkg, ok := b.buildPackages[string(canonicalPackage)]; ok {
return buildPkg, nil
}
// Must be new, save it under the canonical name, too.
klog.V(5).Infof("saving buildPackage %s", canonicalPackage)
b.buildPackages[string(canonicalPackage)] = buildPkg
}
b.setLoadedBuildPackage(dir, buildPkg)

return buildPkg, nil
}
Expand Down Expand Up @@ -231,7 +249,11 @@ func (b *Builder) AddDirRecursive(dir string) error {

// filepath.Walk does not follow symlinks. We therefore evaluate symlinks and use that with
// filepath.Walk.
realPath, err := filepath.EvalSymlinks(b.buildPackages[dir].Dir)
buildPkg, ok := b.getLoadedBuildPackage(dir)
if !ok {
return fmt.Errorf("no loaded build package for %s", dir)
}
realPath, err := filepath.EvalSymlinks(buildPkg.Dir)
if err != nil {
return err
}
Expand All @@ -241,7 +263,11 @@ func (b *Builder) AddDirRecursive(dir string) error {
rel := filepath.ToSlash(strings.TrimPrefix(filePath, realPath))
if rel != "" {
// Make a pkg path.
pkg := path.Join(string(canonicalizeImportPath(b.buildPackages[dir].ImportPath)), rel)
buildPkg, ok := b.getLoadedBuildPackage(dir)
if !ok {
return fmt.Errorf("no loaded build package for %s", dir)
}
pkg := path.Join(string(canonicalizeImportPath(buildPkg.ImportPath)), rel)

// Add it.
if _, err := b.importPackage(pkg, true); err != nil {
Expand Down Expand Up @@ -269,7 +295,7 @@ func (b *Builder) AddDirTo(dir string, u *types.Universe) error {
if _, err := b.importPackage(dir, true); err != nil {
return err
}
pkg, ok := b.buildPackages[dir]
pkg, ok := b.getLoadedBuildPackage(dir)
if !ok {
return fmt.Errorf("no such package: %q", dir)
}
Expand All @@ -287,8 +313,8 @@ func (b *Builder) AddDirectoryTo(dir string, u *types.Universe) (*types.Package,
if _, err := b.importPackage(dir, true); err != nil {
return nil, err
}
pkg, ok := b.buildPackages[dir]
if !ok {
pkg, ok := b.getLoadedBuildPackage(dir)
if !ok || pkg == nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two commits for the panic and the normaliziation would help. This is the panic?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there were a bunch of places where we were looking up directly in the map and assuming non-nil... this was just one

return nil, fmt.Errorf("no such package: %q", dir)
}
path := canonicalizeImportPath(pkg.ImportPath)
Expand Down Expand Up @@ -355,10 +381,11 @@ func isErrPackageNotFound(err error) bool {
// needs to import a go package. 'path' is the import path.
func (b *Builder) importPackage(dir string, userRequested bool) (*tc.Package, error) {
klog.V(5).Infof("importPackage %s", dir)

var pkgPath = importPathString(dir)

// Get the canonical path if we can.
if buildPkg := b.buildPackages[dir]; buildPkg != nil {
if buildPkg, _ := b.getLoadedBuildPackage(dir); buildPkg != nil {
canonicalPackage := canonicalizeImportPath(buildPkg.ImportPath)
klog.V(5).Infof("importPackage %s, canonical path is %s", dir, canonicalPackage)
pkgPath = canonicalPackage
Expand All @@ -382,7 +409,7 @@ func (b *Builder) importPackage(dir string, userRequested bool) (*tc.Package, er
}

// Get the canonical path now that it has been added.
if buildPkg := b.buildPackages[dir]; buildPkg != nil {
if buildPkg, _ := b.getLoadedBuildPackage(dir); buildPkg != nil {
canonicalPackage := canonicalizeImportPath(buildPkg.ImportPath)
klog.V(5).Infof("importPackage %s, canonical path is %s", dir, canonicalPackage)
pkgPath = canonicalPackage
Expand Down Expand Up @@ -600,6 +627,10 @@ func (b *Builder) importWithMode(dir string, mode build.ImportMode) (*build.Pack
if err != nil {
return nil, fmt.Errorf("unable to get current directory: %v", err)
}

// normalize to drop /vendor/ if present
dir = string(canonicalizeImportPath(dir))

buildPkg, err := b.context.Import(filepath.ToSlash(dir), cwd, mode)
if err != nil {
return nil, err
Expand Down