[dev.typeparams] cmd/compile: refactor import logic

This CL refactors noder's package import logic so it's easier to reuse
with types2 and gcimports. In particular, this allows the types2
integration to now support vendored packages.

Change-Id: I1fd98ad612b4683d2e1ac640839e64de1fa7324b
Reviewed-on: https://go-review.googlesource.com/c/go/+/282919
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
Trust: Robert Griesemer <gri@golang.org>
Trust: Matthew Dempsky <mdempsky@google.com>

src/cmd/compile/internal/noder/import.go

@@ -5,20 +5,25 @@
 package noder
 
 import (
+	"errors"
 	"fmt"
-	"go/constant"
+	"io"
 	"os"
-	"path"
+	pathpkg "path"
 	"runtime"
 	"sort"
+	"strconv"
 	"strings"
 	"unicode"
 	"unicode/utf8"
 
 	"cmd/compile/internal/base"
+	"cmd/compile/internal/importer"
 	"cmd/compile/internal/ir"
+	"cmd/compile/internal/syntax"
 	"cmd/compile/internal/typecheck"
 	"cmd/compile/internal/types"
+	"cmd/compile/internal/types2"
 	"cmd/internal/archive"
 	"cmd/internal/bio"
 	"cmd/internal/goobj"
@@ -26,6 +31,29 @@ import (
 	"cmd/internal/src"
 )
 
+// Temporary import helper to get type2-based type-checking going.
+type gcimports struct {
+	packages map[string]*types2.Package
+}
+
+func (m *gcimports) Import(path string) (*types2.Package, error) {
+	return m.ImportFrom(path, "" /* no vendoring */, 0)
+}
+
+func (m *gcimports) ImportFrom(path, srcDir string, mode types2.ImportMode) (*types2.Package, error) {
+	if mode != 0 {
+		panic("mode must be 0")
+	}
+
+	path, err := resolveImportPath(path)
+	if err != nil {
+		return nil, err
+	}
+
+	lookup := func(path string) (io.ReadCloser, error) { return openPackage(path) }
+	return importer.Import(m.packages, path, srcDir, lookup)
+}
+
 func isDriveLetter(b byte) bool {
 	return 'a' <= b && b <= 'z' || 'A' <= b && b <= 'Z'
 }
@@ -38,160 +66,152 @@ func islocalname(name string) bool {
 		strings.HasPrefix(name, "../") || name == ".."
 }
 
-func findpkg(name string) (file string, ok bool) {
+func openPackage(path string) (*os.File, error) {
-	if islocalname(name) {
+	if islocalname(path) {
 		if base.Flag.NoLocalImports {
-			return "", false
+			return nil, errors.New("local imports disallowed")
 		}
 
 		if base.Flag.Cfg.PackageFile != nil {
-			file, ok = base.Flag.Cfg.PackageFile[name]
+			return os.Open(base.Flag.Cfg.PackageFile[path])
-			return file, ok
 		}
 
-		// try .a before .6.  important for building libraries:
+		// try .a before .o.  important for building libraries:
-		// if there is an array.6 in the array.a library,
+		// if there is an array.o in the array.a library,
-		// want to find all of array.a, not just array.6.
+		// want to find all of array.a, not just array.o.
-		file = fmt.Sprintf("%s.a", name)
+		if file, err := os.Open(fmt.Sprintf("%s.a", path)); err == nil {
-		if _, err := os.Stat(file); err == nil {
+			return file, nil
-			return file, true
 		}
-		file = fmt.Sprintf("%s.o", name)
+		if file, err := os.Open(fmt.Sprintf("%s.o", path)); err == nil {
-		if _, err := os.Stat(file); err == nil {
+			return file, nil
-			return file, true
 		}
-		return "", false
+		return nil, errors.New("file not found")
 	}
 
 	// local imports should be canonicalized already.
 	// don't want to see "encoding/../encoding/base64"
 	// as different from "encoding/base64".
-	if q := path.Clean(name); q != name {
+	if q := pathpkg.Clean(path); q != path {
-		base.Errorf("non-canonical import path %q (should be %q)", name, q)
+		return nil, fmt.Errorf("non-canonical import path %q (should be %q)", path, q)
-		return "", false
 	}
 
 	if base.Flag.Cfg.PackageFile != nil {
-		file, ok = base.Flag.Cfg.PackageFile[name]
+		return os.Open(base.Flag.Cfg.PackageFile[path])
-		return file, ok
 	}
 
 	for _, dir := range base.Flag.Cfg.ImportDirs {
-		file = fmt.Sprintf("%s/%s.a", dir, name)
+		if file, err := os.Open(fmt.Sprintf("%s/%s.a", dir, path)); err == nil {
-		if _, err := os.Stat(file); err == nil {
+			return file, nil
-			return file, true
 		}
-		file = fmt.Sprintf("%s/%s.o", dir, name)
+		if file, err := os.Open(fmt.Sprintf("%s/%s.o", dir, path)); err == nil {
-		if _, err := os.Stat(file); err == nil {
+			return file, nil
-			return file, true
 		}
 	}
 
 	if objabi.GOROOT != "" {
 		suffix := ""
-		suffixsep := ""
 		if base.Flag.InstallSuffix != "" {
-			suffixsep = "_"
+			suffix = "_" + base.Flag.InstallSuffix
-			suffix = base.Flag.InstallSuffix
 		} else if base.Flag.Race {
-			suffixsep = "_"
+			suffix = "_race"
-			suffix = "race"
 		} else if base.Flag.MSan {
-			suffixsep = "_"
+			suffix = "_msan"
-			suffix = "msan"
 		}
 
-		file = fmt.Sprintf("%s/pkg/%s_%s%s%s/%s.a", objabi.GOROOT, objabi.GOOS, objabi.GOARCH, suffixsep, suffix, name)
+		if file, err := os.Open(fmt.Sprintf("%s/pkg/%s_%s%s/%s.a", objabi.GOROOT, objabi.GOOS, objabi.GOARCH, suffix, path)); err == nil {
-		if _, err := os.Stat(file); err == nil {
+			return file, nil
-			return file, true
 		}
-		file = fmt.Sprintf("%s/pkg/%s_%s%s%s/%s.o", objabi.GOROOT, objabi.GOOS, objabi.GOARCH, suffixsep, suffix, name)
+		if file, err := os.Open(fmt.Sprintf("%s/pkg/%s_%s%s/%s.o", objabi.GOROOT, objabi.GOOS, objabi.GOARCH, suffix, path)); err == nil {
-		if _, err := os.Stat(file); err == nil {
+			return file, nil
-			return file, true
 		}
 	}
-
+	return nil, errors.New("file not found")
-	return "", false
 }
 
 // myheight tracks the local package's height based on packages
 // imported so far.
 var myheight int
 
-func importfile(f constant.Value) *types.Pkg {
+// resolveImportPath resolves an import path as it appears in a Go
-	if f.Kind() != constant.String {
+// source file to the package's full path.
-		base.Errorf("import path must be a string")
+func resolveImportPath(path string) (string, error) {
-		return nil
-	}
-
-	path_ := constant.StringVal(f)
-	if len(path_) == 0 {
-		base.Errorf("import path is empty")
-		return nil
-	}
-
-	if isbadimport(path_, false) {
-		return nil
-	}
-
 	// The package name main is no longer reserved,
 	// but we reserve the import path "main" to identify
 	// the main package, just as we reserve the import
 	// path "math" to identify the standard math package.
-	if path_ == "main" {
+	if path == "main" {
-		base.Errorf("cannot import \"main\"")
+		return "", errors.New("cannot import \"main\"")
-		base.ErrorExit()
 	}
 
-	if base.Ctxt.Pkgpath != "" && path_ == base.Ctxt.Pkgpath {
+	if base.Ctxt.Pkgpath != "" && path == base.Ctxt.Pkgpath {
-		base.Errorf("import %q while compiling that package (import cycle)", path_)
+		return "", fmt.Errorf("import %q while compiling that package (import cycle)", path)
-		base.ErrorExit()
 	}
 
-	if mapped, ok := base.Flag.Cfg.ImportMap[path_]; ok {
+	if mapped, ok := base.Flag.Cfg.ImportMap[path]; ok {
-		path_ = mapped
+		path = mapped
 	}
 
-	if path_ == "unsafe" {
+	if islocalname(path) {
-		return ir.Pkgs.Unsafe
+		if path[0] == '/' {
-	}
+			return "", errors.New("import path cannot be absolute path")
-
-	if islocalname(path_) {
-		if path_[0] == '/' {
-			base.Errorf("import path cannot be absolute path")
-			return nil
 		}
 
-		prefix := base.Ctxt.Pathname
+		prefix := base.Flag.D
-		if base.Flag.D != "" {
+		if prefix == "" {
-			prefix = base.Flag.D
+			// Questionable, but when -D isn't specified, historically we
+			// resolve local import paths relative to the directory the
+			// compiler's current directory, not the respective source
+			// file's directory.
+			prefix = base.Ctxt.Pathname
 		}
-		path_ = path.Join(prefix, path_)
+		path = pathpkg.Join(prefix, path)
 
-		if isbadimport(path_, true) {
+		if err := checkImportPath(path, true); err != nil {
-			return nil
+			return "", err
 		}
 	}
 
-	file, found := findpkg(path_)
+	return path, nil
-	if !found {
+}
-		base.Errorf("can't find import: %q", path_)
-		base.ErrorExit()
-	}
 
-	importpkg := types.NewPkg(path_, "")
+// TODO(mdempsky): Return an error instead.
-	if importpkg.Imported {
+func importfile(decl *syntax.ImportDecl) *types.Pkg {
-		return importpkg
+	path, err := strconv.Unquote(decl.Path.Value)
-	}
-
-	importpkg.Imported = true
-
-	imp, err := bio.Open(file)
 	if err != nil {
-		base.Errorf("can't open import: %q: %v", path_, err)
+		base.Errorf("import path must be a string")
+		return nil
+	}
+
+	if err := checkImportPath(path, false); err != nil {
+		base.Errorf("%s", err.Error())
+		return nil
+	}
+
+	path, err = resolveImportPath(path)
+	if err != nil {
+		base.Errorf("%s", err)
+		return nil
+	}
+
+	importpkg := types.NewPkg(path, "")
+	if importpkg.Direct {
+		return importpkg // already fully loaded
+	}
+	importpkg.Direct = true
+	typecheck.Target.Imports = append(typecheck.Target.Imports, importpkg)
+
+	if path == "unsafe" {
+		return importpkg // initialized with universe
+	}
+
+	f, err := openPackage(path)
+	if err != nil {
+		base.Errorf("could not import %q: %v", path, err)
 		base.ErrorExit()
 	}
+	imp := bio.NewReader(f)
 	defer imp.Close()
+	file := f.Name()
 
 	// check object header
 	p, err := imp.ReadString('\n')
@@ -261,12 +281,12 @@ func importfile(f constant.Value) *types.Pkg {
 	var fingerprint goobj.FingerprintType
 	switch c {
 	case '\n':
-		base.Errorf("cannot import %s: old export format no longer supported (recompile library)", path_)
+		base.Errorf("cannot import %s: old export format no longer supported (recompile library)", path)
 		return nil
 
 	case 'B':
 		if base.Debug.Export != 0 {
-			fmt.Printf("importing %s (%s)\n", path_, file)
+			fmt.Printf("importing %s (%s)\n", path, file)
 		}
 		imp.ReadByte() // skip \n after $$B
 
@@ -285,17 +305,17 @@ func importfile(f constant.Value) *types.Pkg {
 		fingerprint = typecheck.ReadImports(importpkg, imp)
 
 	default:
-		base.Errorf("no import in %q", path_)
+		base.Errorf("no import in %q", path)
 		base.ErrorExit()
 	}
 
 	// assume files move (get installed) so don't record the full path
 	if base.Flag.Cfg.PackageFile != nil {
 		// If using a packageFile map, assume path_ can be recorded directly.
-		base.Ctxt.AddImport(path_, fingerprint)
+		base.Ctxt.AddImport(path, fingerprint)
 	} else {
 		// For file "/Users/foo/go/pkg/darwin_amd64/math.a" record "math.a".
-		base.Ctxt.AddImport(file[len(file)-len(path_)-len(".a"):], fingerprint)
+		base.Ctxt.AddImport(file[len(file)-len(path)-len(".a"):], fingerprint)
 	}
 
 	if importpkg.Height >= myheight {
@@ -315,47 +335,37 @@ var reservedimports = []string{
 	"type",
 }
 
-func isbadimport(path string, allowSpace bool) bool {
+func checkImportPath(path string, allowSpace bool) error {
+	if path == "" {
+		return errors.New("import path is empty")
+	}
+
 	if strings.Contains(path, "\x00") {
-		base.Errorf("import path contains NUL")
+		return errors.New("import path contains NUL")
-		return true
 	}
 
 	for _, ri := range reservedimports {
 		if path == ri {
-			base.Errorf("import path %q is reserved and cannot be used", path)
+			return fmt.Errorf("import path %q is reserved and cannot be used", path)
-			return true
 		}
 	}
 
 	for _, r := range path {
-		if r == utf8.RuneError {
+		switch {
-			base.Errorf("import path contains invalid UTF-8 sequence: %q", path)
+		case r == utf8.RuneError:
-			return true
+			return fmt.Errorf("import path contains invalid UTF-8 sequence: %q", path)
-		}
+		case r < 0x20 || r == 0x7f:
-
+			return fmt.Errorf("import path contains control character: %q", path)
-		if r < 0x20 || r == 0x7f {
+		case r == '\\':
-			base.Errorf("import path contains control character: %q", path)
+			return fmt.Errorf("import path contains backslash; use slash: %q", path)
-			return true
+		case !allowSpace && unicode.IsSpace(r):
-		}
+			return fmt.Errorf("import path contains space character: %q", path)
-
+		case strings.ContainsRune("!\"#$%&'()*,:;<=>?[]^`{|}", r):
-		if r == '\\' {
+			return fmt.Errorf("import path contains invalid character '%c': %q", r, path)
-			base.Errorf("import path contains backslash; use slash: %q", path)
-			return true
-		}
-
-		if !allowSpace && unicode.IsSpace(r) {
-			base.Errorf("import path contains space character: %q", path)
-			return true
-		}
-
-		if strings.ContainsRune("!\"#$%&'()*,:;<=>?[]^`{|}", r) {
-			base.Errorf("import path contains invalid character '%c': %q", r, path)
-			return true
 		}
 	}
 
-	return false
+	return nil
 }
 
 func pkgnotused(lineno src.XPos, path string, name string) {

src/cmd/compile/internal/noder/noder.go

@@ -9,7 +9,6 @@ import (
 	"fmt"
 	"go/constant"
 	"go/token"
-	"io"
 	"os"
 	"path/filepath"
 	"runtime"
@@ -20,7 +19,6 @@ import (
 
 	"cmd/compile/internal/base"
 	"cmd/compile/internal/dwarfgen"
-	"cmd/compile/internal/importer"
 	"cmd/compile/internal/ir"
 	"cmd/compile/internal/syntax"
 	"cmd/compile/internal/typecheck"
@@ -126,13 +124,6 @@ func ParseFiles(filenames []string) (lines uint) {
 			},
 			Importer: &gcimports{
 				packages: make(map[string]*types2.Package),
-				lookup: func(path string) (io.ReadCloser, error) {
-					file, ok := findpkg(path)
-					if !ok {
-						return nil, fmt.Errorf("can't find import: %q", path)
-					}
-					return os.Open(file)
-				},
 			},
 			Sizes: &gcSizes{},
 		}
@@ -255,23 +246,6 @@ func Package() {
 
 }
 
-// Temporary import helper to get type2-based type-checking going.
-type gcimports struct {
-	packages map[string]*types2.Package
-	lookup   func(path string) (io.ReadCloser, error)
-}
-
-func (m *gcimports) Import(path string) (*types2.Package, error) {
-	return m.ImportFrom(path, "" /* no vendoring */, 0)
-}
-
-func (m *gcimports) ImportFrom(path, srcDir string, mode types2.ImportMode) (*types2.Package, error) {
-	if mode != 0 {
-		panic("mode must be 0")
-	}
-	return importer.Import(m.packages, path, srcDir, m.lookup)
-}
-
 func (p *noder) errorAt(pos syntax.Pos, format string, args ...interface{}) {
 	base.ErrorfAt(p.makeXPos(pos), format, args...)
 }
@@ -483,7 +457,7 @@ func (p *noder) importDecl(imp *syntax.ImportDecl) {
 		p.checkUnused(pragma)
 	}
 
-	ipkg := importfile(p.basicLit(imp.Path))
+	ipkg := importfile(imp)
 	if ipkg == nil {
 		if base.Errors() == 0 {
 			base.Fatalf("phase error in import")
@@ -498,11 +472,6 @@ func (p *noder) importDecl(imp *syntax.ImportDecl) {
 		p.importedEmbed = true
 	}
 
-	if !ipkg.Direct {
-		typecheck.Target.Imports = append(typecheck.Target.Imports, ipkg)
-	}
-	ipkg.Direct = true
-
 	var my *types.Sym
 	if imp.LocalPkgName != nil {
 		my = p.name(imp.LocalPkgName)

src/cmd/compile/internal/types/pkg.go

@@ -31,8 +31,7 @@ type Pkg struct {
 	// height of their imported packages.
 	Height int
 
-	Imported bool // export data of this package was parsed
+	Direct bool // imported directly
-	Direct   bool // imported directly
 }
 
 // NewPkg returns a new Pkg for the given package path and name.

test/fixedbugs/issue11362.go

@@ -8,8 +8,7 @@
 
 package main
 
-import _ "unicode//utf8" // GC_ERROR "non-canonical import path .unicode//utf8. \(should be .unicode/utf8.\)" "can't find import: .unicode//utf8."
+import _ "unicode//utf8" // GC_ERROR "non-canonical import path .unicode//utf8. \(should be .unicode/utf8.\)"
 
 func main() {
 }
-

test/run.go

@@ -1954,7 +1954,6 @@ var excluded = map[string]bool{
 	"fixedbugs/bug388.go":    true, // types2 not run due to syntax errors
 	"fixedbugs/bug412.go":    true, // types2 produces a follow-on error
 
-	"fixedbugs/issue11362.go":  true, // types2 import path handling
 	"fixedbugs/issue11590.go":  true, // types2 doesn't report a follow-on error (pref: types2)
 	"fixedbugs/issue11610.go":  true, // types2 not run after syntax errors
 	"fixedbugs/issue11614.go":  true, // types2 reports an extra error