cue/load: keep the module root of dependency instances

A build instance for a package in a dependency module gets its Root set
to that module's own root directory when the instance is created.
importPkg then clobbered it via rewriteFiles, which unconditionally
reset Root to the main module root, and setFSLoc re-derived RootLoc from
that. The embed injector rejects files outside an instance's RootLoc, so
@embed inside a dependency module only worked when the dependency's
files happened to sit below the main module root: it broke for modules
replaced with a local directory via replaceWith, reporting a spurious
"cannot embed files from a file outside of the current module" error, or
panicking in v0.17.1.

Move the Root assignment from rewriteFiles to its call sites, keeping an
already-set Root, and make setFSLoc only derive RootLoc once, mirroring
how Dir/DirLoc are already handled. rewriteFiles also loses its unused
isLocal parameter.

Fixes #4439.

Signed-off-by: Daniel Martí <mvdan@mvdan.cc>
Change-Id: Ia80a77b5d4f017ffe55d9bef08290a9603a2b8ed
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1242404
TryBot-Result: CUEcueckoo <cueckoo@cuelang.org>
Reviewed-by: Roger Peppe <rogpeppe@gmail.com>
Unity-Result: CUE porcuepine <cue.porcuepine@gmail.com>

Author: mvdan

cmd/cue/cmd/testdata/script/embed_mod_replace_dir.txtar

@@ -6,10 +6,8 @@
 # Issue: https://cuelang.org/issue/4439
 
 cd app
-
-# TODO: this should succeed and print the contents of $WORK/out/export.
-! exec cue export
-cmp stderr $WORK/out/export-stderr
+exec cue export
+cmp stdout $WORK/out/export
 
 -- dep/cue.mod/module.cue --
 module: "example.com/dep@v0"
@@ -23,7 +21,6 @@ scripts: _ @embed(glob=*.sh, type=text)
 
 -- dep/assets/hello.sh --
 echo hello
-
 -- app/cue.mod/module.cue --
 module: "main.org@v0"
 language: version: "v0.17.0"
@@ -38,9 +35,6 @@ package app
 import "example.com/dep/assets"
 
 out: assets.scripts
--- out/export-stderr --
-@embed: cannot embed files from a file outside of the current module:
-    ../dep/assets/assets.cue:4:12
 -- out/export --
 {
     "out": {

cue/load/import.go

@@ -210,7 +210,12 @@ func (l *loader) importPkg(pos token.Pos, p *build.Instance) []*build.Instance {
 			continue
 		}
 		all = append(all, p)
-		rewriteFiles(p, cfg.ModuleRoot, false, cfg.pathOS)
+		// Instances for packages in dependency modules already have their
+		// own module root; the rest belong to the main module.
+		if p.Root == "" {
+			p.Root = cfg.ModuleRoot
+		}
+		rewriteFiles(p, cfg.pathOS)
 		setFSLoc(cfg, p)
 		if errs := fp.finalize(p); errs != nil {
 			p.ReportError(errs)

cue/load/loader.go

@@ -20,6 +20,8 @@ package load
 //    - go/build
 
 import (
+	"cmp"
+
 	"cuelang.org/go/cue/build"
 	"cuelang.org/go/cue/errors"
 	"cuelang.org/go/cue/parser"
@@ -102,11 +104,8 @@ func (l *loader) cueFilesPackage(files []*build.File) *build.Instance {
 		pkg.ModuleVersion, _ = module.NewVersion(l.cfg.Module, "")
 	}
 	pkg.ModuleFile = l.cfg.modFile
-	root := l.cfg.Dir
-	if l.cfg.ModuleRoot != "" {
-		root = l.cfg.ModuleRoot
-	}
-	rewriteFiles(pkg, root, true, l.cfg.pathOS)
+	pkg.Root = cmp.Or(l.cfg.ModuleRoot, l.cfg.Dir)
+	rewriteFiles(pkg, l.cfg.pathOS)
 	setFSLoc(l.cfg, pkg)
 	for _, err := range errors.Errors(fp.finalize(pkg)) { // ImportDir(&ctxt, dir, 0)
 		var x *NoFilesError

cue/load/loader_common.go

@@ -55,9 +55,7 @@ type excludeError struct {
 
 func (e excludeError) Is(err error) bool { return err == errExclude }
 
-func rewriteFiles(p *build.Instance, root string, isLocal bool, os pkgpath.OS) {
-	p.Root = root
-
+func rewriteFiles(p *build.Instance, os pkgpath.OS) {
 	normalizeFiles(p.BuildFiles, os)
 	normalizeFiles(p.IgnoredFiles, os)
 	normalizeFiles(p.OrphanedFiles, os)
@@ -70,9 +68,7 @@ func rewriteFiles(p *build.Instance, root string, isLocal bool, os pkgpath.OS) {
 // maps Dir/Root to display paths.
 //
 // Dir/Root must hold loader-internal paths when this is called.
-// DirLoc is only set on the first call (Dir is set once and not
-// overwritten). RootLoc is always updated because rewriteFiles
-// may reset Root between calls.
+// setFSLoc is idempotent: locations already set are left untouched.
 func setFSLoc(c *Config, p *build.Instance) {
 	if c.FS != nil {
 		if p.DirLoc.IsZero() {
@@ -81,16 +77,18 @@ func setFSLoc(c *Config, p *build.Instance) {
 				p.Dir = c.FromFSPath(p.Dir)
 			}
 		}
-		p.RootLoc = makeFSLoc(c.FS, p.Root, c.FromFSPath)
-		if c.FromFSPath != nil {
-			p.Root = c.FromFSPath(p.Root)
+		if p.RootLoc.IsZero() {
+			p.RootLoc = makeFSLoc(c.FS, p.Root, c.FromFSPath)
+			if c.FromFSPath != nil {
+				p.Root = c.FromFSPath(p.Root)
+			}
 		}
 	} else {
 		ov, _ := c.fileSystem.(*overlayFileSystem)
 		if p.DirLoc.IsZero() && p.Dir != "" {
 			p.DirLoc = makeOSFSLoc(p.Dir, c.pathOS, ov)
 		}
-		if p.Root != "" {
+		if p.RootLoc.IsZero() && p.Root != "" {
 			p.RootLoc = makeOSFSLoc(p.Root, c.pathOS, ov)
 		}
 	}