cue/load: better treatment of files specified on the command line

Currently in modules mode, imports in files specified on the command
line are not added to the set of root packages.

Also, the entire module is always considered even when a file
specified on the command line doesn't use any of it.

This CL addresses these issues by walking the imports
of files explicitly specified on the command line,
and by avoiding loading all module imports when there
are only files specified on the command line.

This is not a complete solution: it is also desirable that
we walk the dependencies of only the packages and files
that are explicitly mentioned on the command line,
but doing that would require a considerable refactor
and for now, it is likely that mixing CUE files and packages
together on the command line is somewhat rare,
so walking all module imports whenever a package is
specified on the command line should be sufficient.

Fixes #3144
Fixes #3147

Signed-off-by: Roger Peppe <[email protected]>
Change-Id: Ie6236aeadfb14fc891eec2f4e6a905c7b37583bb
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1194765
Reviewed-by: Daniel Martí <[email protected]>
TryBot-Result: CUEcueckoo <[email protected]>
Unity-Result: CUE porcuepine <[email protected]>

cmd/cue/cmd/script_test.go

@@ -73,7 +73,7 @@ func TestLatest(t *testing.T) {
 
 		for _, f := range a.Files {
 			t.Run(path.Join(fullpath, f.Name), func(t *testing.T) {
-				if !strings.HasSuffix(f.Name, ".cue") {
+				if !strings.HasSuffix(f.Name, ".cue") || path.Base(f.Name) == "invalid.cue" {
 					return
 				}
 				v := parser.FromVersion(parser.Latest)

cmd/cue/cmd/testdata/script/issue3144.txtar

@@ -0,0 +1,22 @@
+# Check that unrelated invalid files do not cause an error when only files
+# are specified on the command line.
+
+env CUE_EXPERIMENT=modules
+env CUE_CACHE_DIR=$WORK/tmp/cache
+
+exec cue export valid.cue
+cmp stdout stdout.golden
+
+-- stdout.golden --
+{
+    "out": "bar"
+}
+-- cue.mod/module.cue --
+module: "foo.test/bar"
+language: version: "v0.9.0"
+-- valid.cue --
+#foo: "bar"
+out: #foo
+-- invalid.cue --
+foo :: "bar"
+out: foo

cmd/cue/cmd/testdata/script/issue3147.txtar

@@ -0,0 +1,27 @@
+# Check that imports from files specified on the command line
+# are still considered even when they are not part of the main module.
+
+env CUE_EXPERIMENT=modules
+env CUE_CACHE_DIR=$WORK/tmp/cache
+exec cue export _foo/x.cue
+cmp stdout stdout.golden
+
+-- cue.mod/module.cue --
+module: "mod.com@v0"
+language: {
+    version: "v0.9.0"
+}
+-- cue.mod/pkg/example.com/banana/banana.cue --
+package banana
+
+X: 42
+-- _foo/x.cue --
+package p
+
+import "example.com/banana"
+
+x: banana.X
+-- stdout.golden --
+{
+    "x": 42
+}

cue/load/import.go

@@ -17,6 +17,7 @@ package load
 import (
 	"cmp"
 	"fmt"
+	"io"
 	"os"
 	pathpkg "path"
 	"path/filepath"
@@ -229,12 +230,37 @@ func (l *loader) scanDir(dir string) cachedFileFiles {
 			})
 			continue // skip unrecognized file types
 		}
-
 		sd.buildFiles = append(sd.buildFiles, file)
 	}
 	return sd
 }
 
+func setFileSource(cfg *Config, f *build.File) error {
+	if f.Source != nil {
+		return nil
+	}
+	fullPath := f.Filename
+	if fullPath == "-" {
+		b, err := io.ReadAll(cfg.stdin())
+		if err != nil {
+			return errors.Newf(token.NoPos, "read stdin: %v", err)
+		}
+		f.Source = b
+		return nil
+	}
+	if !filepath.IsAbs(fullPath) {
+		fullPath = filepath.Join(cfg.Dir, fullPath)
+	}
+	if fi := cfg.fileSystem.getOverlay(fullPath); fi != nil {
+		if fi.file != nil {
+			f.Source = fi.file
+		} else {
+			f.Source = fi.contents
+		}
+	}
+	return nil
+}
+
 func (l *loader) loadFunc(pos token.Pos, path string) *build.Instance {
 	impPath := importPath(path)
 	if isLocalImport(path) {

cue/load/instances.go

@@ -17,6 +17,8 @@ package load
 import (
 	"context"
 	"fmt"
+	"sort"
+	"strconv"
 
 	"cuelang.org/go/cue/ast"
 	"cuelang.org/go/cue/build"
@@ -66,17 +68,21 @@ func Instances(args []string, c *Config) []*build.Instance {
 	if err != nil {
 		return []*build.Instance{c.newErrInstance(err)}
 	}
+	for _, f := range otherFiles {
+		if err := setFileSource(c, f); err != nil {
+			return []*build.Instance{c.newErrInstance(err)}
+		}
+	}
 	synCache := newSyntaxCache(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, pkgArgs)
+	pkgs, err := loadPackages(ctx, c, synCache, pkgArgs, otherFiles)
 	if err != nil {
-		return []*build.Instance{c.newErrInstance(err)}
+		return []*build.Instance{c.newErrInstance(fmt.Errorf("xxx: %v", err))}
 	}
 	tg := newTagger(c)
 	l := newLoader(c, tg, synCache, pkgs)
@@ -140,7 +146,9 @@ func Instances(args []string, c *Config) []*build.Instance {
 	return a
 }
 
-func loadPackages(ctx context.Context, cfg *Config, extraPkgs []string) (*modpkgload.Packages, error) {
+// 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) {
 	if cfg.Registry == nil || cfg.modFile == nil || cfg.modFile.Module == "" {
 		return nil, nil
 	}
@@ -154,19 +162,61 @@ func loadPackages(ctx context.Context, cfg *Config, extraPkgs []string) (*modpkg
 		FS:  cfg.fileSystem.ioFS(cfg.ModuleRoot),
 		Dir: ".",
 	}
-	allImports, err := modimports.AllImports(modimports.AllModuleFiles(mainModLoc.FS, mainModLoc.Dir))
-	if err != nil {
-		return nil, fmt.Errorf("cannot enumerate all module imports: %v", err)
+	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
+		}
+	}
+	// Add any imports found in other files.
+	for _, f := range otherFiles {
+		syntaxes, err := synCache.getSyntax(f)
+		if err != nil {
+			return nil, fmt.Errorf("cannot get syntax for %q: %v", f.Filename, err)
+		}
+		for _, syntax := range syntaxes {
+			for _, imp := range syntax.Imports {
+				pkgPath, err := strconv.Unquote(imp.Path.Value)
+				if err != nil {
+					// Should never happen.
+					return nil, fmt.Errorf("invalid import path %q in %s", imp.Path.Value, f.Filename)
+				}
+				// Canonicalize the path.
+				pkgPath = module.ParseImportPath(pkgPath).Canonical().String()
+				pkgPaths[pkgPath] = true
+			}
+		}
+	}
+	// TODO use maps.Keys when we can.
+	pkgPathSlice := make([]string, 0, len(pkgPaths))
+	for p := range pkgPaths {
+		pkgPathSlice = append(pkgPathSlice, p)
 	}
-	// Add any packages specified on the command line so they're always
-	// available.
-	allImports = append(allImports, extraPkgs...)
+	sort.Strings(pkgPathSlice)
 	return modpkgload.LoadPackages(
 		ctx,
 		cfg.Module,
 		mainModLoc,
 		reqs,
 		cfg.Registry,
-		allImports,
+		pkgPathSlice,
 	), nil
 }

cue/load/loader_common.go

@@ -170,8 +170,8 @@ func (fp *fileProcessor) add(root string, file *build.File, mode importMode) (ad
 		if !filepath.IsAbs(fullPath) {
 			fullPath = filepath.Join(root, fullPath)
 		}
+		file.Filename = fullPath
 	}
-	file.Filename = fullPath
 
 	base := filepath.Base(fullPath)
 
@@ -185,6 +185,9 @@ func (fp *fileProcessor) add(root string, file *build.File, mode importMode) (ad
 		p.InvalidFiles = append(p.InvalidFiles, file)
 		return true
 	}
+	if err := setFileSource(fp.c, file); err != nil {
+		return badFile(errors.Promote(err, ""))
+	}
 
 	match, data, err := matchFile(fp.c, file, true, fp.allTags, mode)
 	switch {

cue/load/match.go

@@ -15,7 +15,6 @@
 package load
 
 import (
-	"io"
 	"path/filepath"
 	"strings"
 
@@ -50,26 +49,13 @@ func (e excludeError) Is(err error) bool { return err == errExclude }
 // If allTags is non-nil, matchFile records any encountered build tag
 // by setting allTags[tag] = true.
 func matchFile(cfg *Config, file *build.File, returnImports bool, allTags map[string]bool, mode importMode) (match bool, data []byte, err errors.Error) {
-	if fi := cfg.fileSystem.getOverlay(file.Filename); fi != nil {
-		if fi.file != nil {
-			file.Source = fi.file
-		} else {
-			file.Source = fi.contents
-		}
-	}
-
+	// Note: file.Source should already have been set by setFileSource just
+	// after the build.File value was created.
 	if file.Encoding != build.CUE {
 		return false, nil, nil // not a CUE file, don't record.
 	}
-
 	if file.Filename == "-" {
-		b, err2 := io.ReadAll(cfg.stdin())
-		if err2 != nil {
-			err = errors.Newf(token.NoPos, "read stdin: %v", err)
-			return
-		}
-		file.Source = b
-		return true, b, nil // don't check shouldBuild for stdin
+		return true, file.Source.([]byte), nil // don't check shouldBuild for stdin
 	}
 
 	name := filepath.Base(file.Filename)