cue/load: evaluate only those packages which are needed

This change changes the cue/load logic to expand package wildcards
before invoking the `modpkgload.LoadPackages` resolution logic.
This involves adding new wildcard matching logic in 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.

This then removes the need to enumerate all packages in the entire
module, because we know all the packages up front. This only works
because we do not allow wildcard matching in external packages
(something that is not currently supported anyway) which would require
integrating the matching logic directly into `modload` which is a
considerably larger refactor which we'll punt on for now.

It would be nice to remove the previous wildcard matching logic,
but unfortunately the current behavior relies on wildcards matching
directories with no CUE files, which doesn't fit well with the way
that the module file enumeration happens, so leave the old logic in
place which considerably reduces the number of visible behavior changes.

Fixes #3155.

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

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

@@ -29,10 +29,10 @@ 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 'cannot use absolute directory ".*somepkg" as package path'
 [windows] stderr 'no encoding specified for file ".*somepkg.*"'
 ! exec cue export ${WORK}${/}somepkg${/}
-[!windows] stderr 'no dependency found for package ".*somepkg"'
+[!windows] stderr 'cannot use absolute directory ".*somepkg/" as package path'
 [windows] stderr 'no encoding specified for file ".*somepkg.*"'
 
 # Now do some of the same filename tests, but for a file without any extension.
@@ -52,12 +52,11 @@ cmp stdout export.golden
 ! exec cue export somefile
 stderr 'standard library import path "somefile" cannot be imported as a CUE package'
 
-# 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 is a file and 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 'cannot use absolute directory ".*somefile" as package path'
 [windows] stderr 'no encoding specified for file ".*somefile"'
 
 -- cue.mod/module.cue --

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_patterns.txtar

@@ -35,6 +35,7 @@ stdout -count=1 '^self: "mod.com/alpha1/leaf"$'
 # TODO: Why does this succeed? What does the output mean? This seems broken.
 exec cue eval ./.../leaf
 cmp stderr dot-dots-middle.stderr
+# TODO: Unlike other cases, this still returns a "matched no packages" error.
 ! exec cue eval mod.com/.../leaf
 cmp stderr current-dots-middle.stderr
 
@@ -47,34 +48,25 @@ cmp stderr external-dots-middle.stderr
 cmp stderr external-dots-multimod.stderr
 
 -- external.stderr --
-cue: "example.com/foo/..." matched no packages
-
+pattern not allowed in external package path "example.com/foo/..."
 -- all.stderr --
 cue: "all" matched no packages
-
 -- cmd.stderr --
 cue: "cmd" matched no packages
-
 -- std.stderr --
 cue: "std" matched no packages
-
 -- dots.stderr --
-cue: "..." matched no packages
-
+pattern not allowed in external package path "..."
 -- dots-prefix.stderr --
-cue: ".../leaf" matched no packages
-
+pattern not allowed in external package path ".../leaf"
 -- dot-dots-middle.stderr --
 // mod.com@v0:root
 -- current-dots-middle.stderr --
 cue: "mod.com/.../leaf" matched no packages
-
 -- external-dots-middle.stderr --
-cue: "example.com/foo/.../bar" matched no packages
-
+pattern not allowed in external package path "example.com/foo/.../bar"
 -- external-dots-multimod.stderr --
-cue: "example.com/..." matched no packages
-
+pattern not allowed in external package path "example.com/..."
 -- cue.mod/module.cue --
 module: "mod.com"
 

cmd/cue/cmd/testdata/script/pkg_resolution_focus.txtar

@@ -2,12 +2,11 @@
 # arguments and their dependencies are scanned.
 # See https://cuelang.org/issue/3155 for context.
 
-# TODO: this should not fail.
-! exec cue eval ./a
-
-cmp stderr stderr.golden
--- stderr.golden --
-cannot enumerate all module imports: cannot read "c/invalid.cue": expected 'STRING', found 'INT' 435
+exec cue eval ./a
+cmp stdout stdout.golden
+-- stdout.golden --
+b:   5
+foo: true
 -- cue.mod/module.cue --
 module: "mod.com"
 language: version: "v0.9.0"

cue/load/config.go

@@ -131,6 +131,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
@@ -304,13 +305,13 @@ 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
@@ -353,6 +354,10 @@ func (c Config) complete() (cfg *Config, err error) {
 	// TODO: determine root on a package basis. Maybe we even need a
 	// pkgname.cue.mod
 	// Look to see if there is a cue.mod.
+	//
+	// TODO(mvdan): note that setting Config.ModuleRoot to a directory
+	// without a cue.mod file does not result in any error, which is confusing
+	// or can lead to not using the right CUE module silently.
 	if c.ModuleRoot == "" {
 		// Only consider the current directory by default
 		c.ModuleRoot = c.Dir

cue/load/import.go

@@ -180,7 +180,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)
@@ -299,10 +299,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
@@ -319,44 +330,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"
@@ -77,17 +76,18 @@ func Instances(args []string, c *Config) []*build.Instance {
 		}
 	}
 	synCache := newSyntaxCache(c)
+	tg := newTagger(c)
 	// 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, tg)
+	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)}
 	}
-	tg := newTagger(c)
 	l := newLoader(c, tg, synCache, pkgs)
 
 	if c.Context == nil {
@@ -151,7 +151,7 @@ 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 == "" {
 		return nil, nil
 	}
@@ -167,27 +167,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 {