cue/load: evaluate only those packages which are needed

DO NOT REVIEW

This change changes the cue/load logic to expand package
wildcards before invoking the `modpkgload.LoadPackages`
resolution logic. This involves moving the wildcard expansion
code into a new place, independent of the `cue/load.loader`
which is created after doing that. We use the `modimports.AllModuleFiles`
logic to enumerate the packages matched by a wildcard.

All tests pass apart from the ones involving `cue import`,
which expose a significant flaw in the above approach:
`modimports.AllModuleFiles` only looks for CUE files, but
`cue import` relies on the fact that package patterns will currently
match directories that do not contain any CUE files.

Fixes #3155

Signed-off-by: Roger Peppe <rogpeppe@gmail.com>
Change-Id: I9f4b210eb0588e21ae942a97d7cf34c9887363dc

cmd/cue/cmd/testdata/script/cmd_filetypes.txtar

@@ -29,11 +29,9 @@ exec cue export ./somepkg/
 cmp stdout export.golden
 # TODO(mvdan): we should support loading packages from absolute directories.
 ! exec cue export ${WORK}${/}somepkg
-[!windows] stderr 'no location found for package ".*somepkg"'
-[windows] stderr 'no encoding specified for file ".*somepkg.*"'
+stderr 'cannot use absolute directory ".*somepkg" as package path'
 ! exec cue export ${WORK}${/}somepkg${/}
-[!windows] stderr 'no dependency found for package ".*somepkg"'
-[windows] stderr 'no encoding specified for file ".*somepkg.*"'
+stderr 'cannot use absolute directory ".*somepkg.*" as package path'
 
 # Now do some of the same filename tests, but for a file without any extension.
 cp somefile.cue somefile
@@ -54,11 +52,10 @@ stderr 'standard library import path "somefile" cannot be imported as a CUE pack
 
 # TODO(mvdan): "cannot find package" is perhaps confusing to the user given that the file exists.
 ! exec cue export ./somefile
-stderr 'cannot find package "\./somefile"'
+stderr '\./somefile refers to a file not a package directory'
 # TODO(mvdan): we probably should not behave differently on Windows.
 ! exec cue export ${WORK}${/}somefile
-[!windows] stderr 'no location found for package ".*somefile"'
-[windows] stderr 'no encoding specified for file ".*somefile"'
+stderr 'cannot use absolute directory ".*somefile" as package path'
 
 -- cue.mod/module.cue --
 module: "mod.test/test"

cmd/cue/cmd/testdata/script/fmt.txtar

@@ -10,7 +10,7 @@ cmp issue1744/invalid.cue issue1744/invalid.cue.orig
 
 -- expect-stderr --
 expected 'STRING', found '.':
-    ./fmt/error.cue:1:9
+    fmt/error.cue:1:9
 -- fmt/error.cue --
 import a.b "foo"
 
@@ -21,4 +21,4 @@ bb: 3
 ~
 -- issue1744/stderr-golden --
 illegal character U+007E '~':
-    ./issue1744/invalid.cue:1:1
+    ./issue1744/invalid.cue:1:1

cmd/cue/cmd/testdata/script/issue174.txtar

@@ -1,7 +1,8 @@
 ! exec cue export ./issue174
 cmp stderr expect-stderr
 -- expect-stderr --
-cannot enumerate all module imports: invalid import path "'foo'" in issue174/issue174.cue
+build constraints exclude all CUE files in ./issue174:
+    issue174/issue174.cue: no package name
 -- cue.mod/module.cue --
 module: "mod.test"
 language: version: "v0.9.0"

cmd/cue/cmd/testdata/script/pkg_resolution_multiple_packages_no_matching_path_element.txtar

@@ -36,13 +36,7 @@ package z
 
 z: 5
 -- import_stderr.golden --
-import failed: build constraints exclude all CUE files in mod.com/x:
-    root.cue: package is root, want x
-    x/y.cue: package is y, want x
-    x/z.cue: package is z, want x:
+import failed: cannot find package "mod.com/x": no files in package directory with package name "x":
     ./root.cue:3:8
 -- absolute_stderr.golden --
-build constraints exclude all CUE files in mod.com/x:
-    root.cue: package is root, want x
-    x/y.cue: package is y, want x
-    x/z.cue: package is z, want x
+cannot find package "mod.com/x": no files in package directory with package name "x"

cmd/cue/cmd/testdata/script/pkg_resolution_multiple_packages_one_matching_path_element.txtar

@@ -10,7 +10,7 @@ cmp stdout stdout.golden
 # TODO the following command fails unexpectedly
 # although it should be consistent with the above.
 ! exec cue eval ./x
-stderr 'found packages "x" \(x.cue\) and "y" \(y.cue\) in ".*script-pkg_resolution_multiple_packages_one_matching_path_element.*"'
+stderr 'found packages "x" \(x.cue\) and "y" \(y.cue\) in ".*script-pkg_resolution_multiple_packages_one_matching_path_element.x"'
 
 -- stdout.golden --
 x: 5

cmd/cue/cmd/testdata/script/pkg_resolution_path_element_invalid_ident.txtar

@@ -47,9 +47,7 @@ x: 5
 -- stdout.golden --
 x: 5
 -- test2-import-stderr.golden --
-import failed: build constraints exclude all CUE files in mod.com/1x:
-    1x/x.cue: package is x, want 1x:
+import failed: cannot find package "mod.com/1x": cannot determine package name from import path "mod.com/1x":
     ./test2/root.cue:3:8
 -- test2-abs-stderr.golden --
-build constraints exclude all CUE files in mod.com/1x:
-    1x/x.cue: package is x, want 1x
+cannot find package "mod.com/1x": cannot determine package name from import path "mod.com/1x"

cmd/cue/cmd/testdata/script/pkg_resolution_single_package_not_matching_path_element.txtar

@@ -31,11 +31,7 @@ package y
 
 y: 5
 -- import_stderr.golden --
-import failed: build constraints exclude all CUE files in mod.com/x:
-    root.cue: package is root, want x
-    x/y.cue: package is y, want x:
+import failed: cannot find package "mod.com/x": no files in package directory with package name "x":
     ./root.cue:3:8
 -- absolute_stderr.golden --
-build constraints exclude all CUE files in mod.com/x:
-    root.cue: package is root, want x
-    x/y.cue: package is y, want x
+cannot find package "mod.com/x": no files in package directory with package name "x"

cue/load/config.go

@@ -38,6 +38,8 @@ const (
 	moduleFile = "module.cue"
 )
 
+type importPath string
+
 // FromArgsUsage is a partial usage message that applications calling
 // FromArgs may wish to include in their -help output.
 //
@@ -131,6 +133,7 @@ type Config struct {
 	// A Module is a collection of packages and instances that are within the
 	// directory hierarchy rooted at the module root. The module root can be
 	// marked with a cue.mod file.
+	// If this is a relative path, it will be interpreted relative to [Config.Dir].
 	ModuleRoot string
 
 	// Module specifies the module prefix. If not empty, this value must match
@@ -300,17 +303,15 @@ func (c *Config) stdin() io.Reader {
 	return c.Stdin
 }
 
-type importPath string
-
 type fsPath string
 
-func addImportQualifier(pkg importPath, name string) (importPath, errors.Error) {
+func addImportQualifier(pkg importPath, name string) (importPath, error) {
 	if name == "" {
 		return pkg, nil
 	}
 	ip := module.ParseImportPath(string(pkg))
 	if ip.ExplicitQualifier && ip.Qualifier != name {
-		return "", errors.Newf(token.NoPos, "non-matching package names (%s != %s)", ip.Qualifier, name)
+		return "", fmt.Errorf("non-matching package names (%s != %s)", ip.Qualifier, name)
 	}
 	ip.Qualifier = name
 	return importPath(ip.String()), nil
@@ -349,7 +350,7 @@ func (c Config) complete() (cfg *Config, err error) {
 	if err := c.fileSystem.init(c.Dir, c.Overlay); err != nil {
 		return nil, err
 	}
-
+	//	log.Printf("complete with module root %q", c.ModuleRoot)
 	// TODO: determine root on a package basis. Maybe we even need a
 	// pkgname.cue.mod
 	// Look to see if there is a cue.mod.
@@ -389,6 +390,7 @@ func (c Config) complete() (cfg *Config, err error) {
 func (c *Config) loadModule() error {
 	// TODO: also make this work if run from outside the module?
 	mod := filepath.Join(c.ModuleRoot, modDir)
+	//	log.Printf("loading module from %q", mod)
 	info, cerr := c.fileSystem.stat(mod)
 	if cerr != nil {
 		return nil

cue/load/import.go

@@ -55,6 +55,7 @@ import (
 //	_       anonymous files (which may be marked with _)
 //	*       all packages
 func (l *loader) importPkg(pos token.Pos, p *build.Instance) []*build.Instance {
+	//	log.Printf("importPkg %q", p.ImportPath)
 	retErr := func(errs errors.Error) []*build.Instance {
 		// XXX: move this loop to ReportError
 		for _, err := range errors.Errors(errs) {
@@ -181,7 +182,7 @@ func (l *loader) importPkg(pos token.Pos, p *build.Instance) []*build.Instance {
 		impPath, err := addImportQualifier(importPath(p.ImportPath), p.PkgName)
 		p.ImportPath = string(impPath)
 		if err != nil {
-			p.ReportError(err)
+			p.ReportError(errors.Promote(err, ""))
 		}
 
 		all = append(all, p)
@@ -300,10 +301,21 @@ func (l *loader) newRelInstance(pos token.Pos, path, pkgName string) *build.Inst
 	}
 
 	dir := filepath.Join(l.cfg.Dir, filepath.FromSlash(path))
-	if importPath, e := l.importPathFromAbsDir(fsPath(dir), path); e != nil {
+	if pkgPath, e := importPathFromAbsDir(l.cfg, dir, path); e != nil {
 		// Detect later to keep error messages consistent.
 	} else {
-		p.ImportPath = string(importPath)
+		// Add package qualifier if the configuration requires it.
+		name := l.cfg.Package
+		switch name {
+		case "_", "*":
+			name = ""
+		}
+		pkgPath, e := addImportQualifier(pkgPath, name)
+		if e != nil {
+			// Detect later to keep error messages consistent.
+		} else {
+			p.ImportPath = string(pkgPath)
+		}
 	}
 
 	p.Dir = dir
@@ -320,44 +332,33 @@ func (l *loader) newRelInstance(pos token.Pos, path, pkgName string) *build.Inst
 	return p
 }
 
-func (l *loader) importPathFromAbsDir(absDir fsPath, key string) (importPath, errors.Error) {
-	if l.cfg.ModuleRoot == "" {
-		return "", errors.Newf(token.NoPos,
-			"cannot determine import path for %q (root undefined)", key)
+func importPathFromAbsDir(c *Config, absDir string, origPath string) (importPath, error) {
+	if c.ModuleRoot == "" {
+		return "", fmt.Errorf("cannot determine import path for %q (root undefined)", origPath)
 	}
 
-	dir := filepath.Clean(string(absDir))
-	if !strings.HasPrefix(dir, l.cfg.ModuleRoot) {
-		return "", errors.Newf(token.NoPos,
-			"cannot determine import path for %q (dir outside of root)", key)
+	dir := filepath.Clean(absDir)
+	if !strings.HasPrefix(dir, c.ModuleRoot) {
+		return "", fmt.Errorf("cannot determine import path for %q (dir outside of root)", origPath)
 	}
 
-	pkg := filepath.ToSlash(dir[len(l.cfg.ModuleRoot):])
+	pkg := filepath.ToSlash(dir[len(c.ModuleRoot):])
 	switch {
 	case strings.HasPrefix(pkg, "/cue.mod/"):
 		pkg = pkg[len("/cue.mod/"):]
 		if pkg == "" {
-			return "", errors.Newf(token.NoPos,
-				"invalid package %q (root of %s)", key, modDir)
+			return "", fmt.Errorf("invalid package %q (root of %s)", origPath, modDir)
 		}
 
-	case l.cfg.Module == "":
-		return "", errors.Newf(token.NoPos,
-			"cannot determine import path for %q (no module)", key)
+	case c.Module == "":
+		return "", fmt.Errorf("cannot determine import path for %q (no module)", origPath)
 	default:
-		impPath := module.ParseImportPath(l.cfg.Module)
+		impPath := module.ParseImportPath(c.Module)
 		impPath.Path += pkg
 		impPath.Qualifier = ""
 		pkg = impPath.String()
 	}
-
-	name := l.cfg.Package
-	switch name {
-	case "_", "*":
-		name = ""
-	}
-
-	return addImportQualifier(importPath(pkg), name)
+	return importPath(pkg), nil
 }
 
 func (l *loader) newInstance(pos token.Pos, p importPath) *build.Instance {

cue/load/import_test.go

@@ -16,6 +16,7 @@ package load
 
 import (
 	"fmt"
+	"os"
 	"path/filepath"
 	"reflect"
 	"testing"
@@ -27,12 +28,21 @@ func testdata(elems ...string) string {
 	return filepath.Join(append([]string{"testdata"}, elems...)...)
 }
 
+var pkgRootDir, _ = os.Getwd()
+
 func getInst(pkg, cwd string) (*build.Instance, error) {
-	// Set ModuleRoot as well; otherwise we walk the parent directories
-	// all the way to the root of the git repository, causing Go's test caching
-	// to never kick in, as the .git directory almost always changes.
-	// Moreover, it's extra work that isn't useful to the tests.
-	insts := Instances([]string{pkg}, &Config{ModuleRoot: ".", Dir: cwd})
+	insts := Instances([]string{pkg}, &Config{
+		// Set ModuleRoot as well; otherwise we walk the parent directories
+		// all the way to the root of the git repository, causing Go's test caching
+		// to never kick in, as the .git directory almost always changes.
+		// Moreover, it's extra work that isn't useful to the tests.
+		//
+		// Note that we can't set ModuleRoot to cwd because if ModuleRoot is
+		// set, the logic will only look for a module file in that exact directory.
+		// So we set it to the module root actually used by all the callers of getInst: ./testdata/testmod.
+		ModuleRoot: filepath.Join(pkgRootDir, testdata("testmod")),
+		Dir:        cwd,
+	})
 	if len(insts) != 1 {
 		return nil, fmt.Errorf("expected one instance, got %d", len(insts))
 	}
@@ -58,7 +68,7 @@ func TestEmptyFolderImport(t *testing.T) {
 	path := testdata("testmod", "empty")
 	_, err := getInst(".", path)
 	if _, ok := err.(*NoFilesError); !ok {
-		t.Fatalf(`Import(%q) did not return NoCUEError.`, path)
+		t.Fatalf(`Import(%q) did not return NoCUEError, but instead %#v (%v)`, path, err, err)
 	}
 }
 
@@ -67,7 +77,7 @@ func TestMultiplePackageImport(t *testing.T) {
 	_, err := getInst(".", path)
 	mpe, ok := err.(*MultiplePackageError)
 	if !ok {
-		t.Fatalf(`Import(%q) did not return MultiplePackageError.`, path)
+		t.Fatalf(`Import(%q) did not return MultiplePackageError, but instead %#v (%v)`, path, err, err)
 	}
 	mpe.Dir = ""
 	want := &MultiplePackageError{

cue/load/instances.go

@@ -24,7 +24,6 @@ import (
 	"cuelang.org/go/cue/build"
 	"cuelang.org/go/internal/cueexperiment"
 	"cuelang.org/go/internal/filetypes"
-	"cuelang.org/go/internal/mod/modimports"
 	"cuelang.org/go/internal/mod/modpkgload"
 	"cuelang.org/go/internal/mod/modrequirements"
 	"cuelang.org/go/mod/module"
@@ -46,6 +45,7 @@ func Instances(args []string, c *Config) []*build.Instance {
 	if c == nil {
 		c = &Config{}
 	}
+	//	log.Printf("Instances with module root %q", c.ModuleRoot)
 	// We want to consult the CUE_EXPERIMENT flag to see whether
 	// consult external registries by default.
 	if err := cueexperiment.Init(); err != nil {
@@ -80,13 +80,22 @@ func Instances(args []string, c *Config) []*build.Instance {
 	// Pass all arguments that look like packages to loadPackages
 	// so that they'll be available when looking up the packages
 	// that are specified on the command line.
-	// Relative import paths create a package with an associated
-	// error but it turns out that's actually OK because the cue/load
-	// logic resolves such paths without consulting pkgs.
-	pkgs, err := loadPackages(ctx, c, synCache, pkgArgs, otherFiles)
+	expandedPaths, err := expandPackageArgs(c, pkgArgs, c.Package)
 	if err != nil {
 		return []*build.Instance{c.newErrInstance(err)}
 	}
+	pkgs, err := loadPackages(ctx, c, synCache, expandedPaths, otherFiles)
+	if err != nil {
+		return []*build.Instance{c.newErrInstance(err)}
+	}
+	//	if pkgs != nil {
+	//		log.Printf("got modpkgload.Packages")
+	//		for _, p := range pkgs.All() {
+	//			log.Printf("loaded pkg %v: %v", p.ImportPath(), p.Error())
+	//		}
+	//	} else {
+	//		log.Printf("no modpkgload.Packages")
+	//	}
 	tg := newTagger(c)
 	l := newLoader(c, tg, synCache, pkgs)
 
@@ -151,8 +160,9 @@ func Instances(args []string, c *Config) []*build.Instance {
 
 // loadPackages returns packages loaded from the given package list and also
 // including imports from the given build files.
-func loadPackages(ctx context.Context, cfg *Config, synCache *syntaxCache, extraPkgs []string, otherFiles []*build.File) (*modpkgload.Packages, error) {
+func loadPackages(ctx context.Context, cfg *Config, synCache *syntaxCache, pkgs []resolvedPackageArg, otherFiles []*build.File) (*modpkgload.Packages, error) {
 	if cfg.Registry == nil || cfg.modFile == nil || cfg.modFile.Module == "" {
+		//		log.Printf("no packages (registry %v; modFile %#v)", cfg.Registry != nil, cfg.modFile)
 		return nil, nil
 	}
 	reqs := modrequirements.NewRequirements(
@@ -167,27 +177,8 @@ func loadPackages(ctx context.Context, cfg *Config, synCache *syntaxCache, extra
 	}
 	pkgPaths := make(map[string]bool)
 	// Add any packages specified directly on the command line.
-	for _, pkg := range extraPkgs {
-		pkgPaths[pkg] = true
-	}
-	if len(otherFiles) == 0 || len(extraPkgs) > 0 {
-		// Resolve all the imports in the current module. We specifically
-		// avoid doing this when files are specified on the command line
-		// but no packages because we don't want to fail because of
-		// invalid files in the module when we're just evaluating files.
-		// TODO this means that if files _and_ packages are specified,
-		// we can still error when there are problems with packages that
-		// aren't used by anything explicitly specified on the command line;
-		// a proper solution would involve pushing the pattern evaluation
-		// down into the module loading code, but that implies a significant
-		// refactoring of the modules code, so this will do for now.
-		modImports, err := modimports.AllImports(modimports.AllModuleFiles(mainModLoc.FS, mainModLoc.Dir))
-		if err != nil {
-			return nil, fmt.Errorf("cannot enumerate all module imports: %v", err)
-		}
-		for _, pkg := range modImports {
-			pkgPaths[pkg] = true
-		}
+	for _, pkg := range pkgs {
+		pkgPaths[pkg.resolved] = true
 	}
 	// Add any imports found in other files.
 	for _, f := range otherFiles {