cue/load: more correct treatment of module paths with major versions

Currently if the module path contains a major version,
the cue/load logic naively appends the relative package
path, but that's not correct, as the major version comes after
the import path and before the qualifier.

We can use the `module.ImportPath` logic to correctly form such
an import path.

Also return a `*PackageError` in modules mode too.

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

cmd/cue/cmd/testdata/script/registry_lazy_config.txtar

@@ -12,7 +12,7 @@ cmp stdout expect-stdout
 cp OTHER/main.cue main.cue
 cp OTHER/cue.mod/module.cue cue.mod/module.cue
 ! exec cue export .
-stderr '^import failed: cannot find package "example.com/e": cannot fetch example.com/e@v0.0.1: module example.com/e@v0.0.1: cannot do HTTP request: Get ".*": cannot load OCI auth configuration: invalid config file ".*config.json": decode failed: .*'
+stderr '^test.org@v0: import failed: cannot find package "example.com/e": cannot fetch example.com/e@v0.0.1: module example.com/e@v0.0.1: cannot do HTTP request: Get ".*": cannot load OCI auth configuration: invalid config file ".*config.json": decode failed: .*'
 -- dockerconfig/config.json --
 should be JSON but isn't
 -- expect-stdout --

cmd/cue/cmd/testdata/script/registry_module_not_found.txtar

@@ -5,7 +5,7 @@
 cmp stderr expect-stderr
 
 -- expect-stderr --
-import failed: cannot find package "example.com/e": cannot fetch example.com/e@v0.0.2: module example.com/e@v0.0.2: module not found:
+test.org@v0: import failed: cannot find package "example.com/e": cannot fetch example.com/e@v0.0.2: module example.com/e@v0.0.2: module not found:
     ./main.cue:2:8
 -- main.cue --
 package main

cmd/cue/cmd/testdata/script/registry_nofallback.txtar

@@ -4,7 +4,7 @@ env CUE_REGISTRY=foo.com=$CUE_REGISTRY1,none
 cmp stderr expect-stderr
 
 -- expect-stderr --
-import failed: cannot find package "example.com@v0": cannot fetch example.com@v0.0.1: module not found:
+main.org@v0: import failed: cannot find package "example.com@v0": cannot fetch example.com@v0.0.1: module not found:
     ./main.cue:2:8
 -- cue.mod/module.cue --
 module: "main.org@v0"

cmd/cue/cmd/testdata/script/registry_with_pkg_conflict.txtar

@@ -6,7 +6,7 @@
 cmp stderr expect-stderr
 
 -- expect-stderr --
-import failed: cannot find package "example.com/e": ambiguous import: found package example.com/e in multiple modules:
+test.org@v0: import failed: cannot find package "example.com/e": ambiguous import: found package example.com/e in multiple modules:
 	example.com/e@v0 v0.0.1 (.)
 	local (cue.mod/pkg/example.com/e):
     ./main.cue:2:8

cmd/cue/cmd/testdata/script/registry_wrong_prefix.txtar

@@ -3,7 +3,7 @@ env CUE_REGISTRY=$DEBUG_REGISTRY_HOST/something+insecure
 cmp stderr expect-stderr
 
 -- expect-stderr --
-import failed: cannot find package "example.com/e": cannot fetch example.com/e@v0.0.1: module example.com/e@v0.0.1: module not found:
+test.org@v0: import failed: cannot find package "example.com/e": cannot fetch example.com/e@v0.0.1: module example.com/e@v0.0.1: module not found:
     ./main.cue:2:8
 -- main.cue --
 package main

cue/load/config.go

@@ -20,7 +20,6 @@ import (
 	"io"
 	"os"
 	"path/filepath"
-	"strings"
 
 	"cuelang.org/go/cue/ast"
 	"cuelang.org/go/cue/build"
@@ -306,25 +305,15 @@ type importPath string
 type fsPath string
 
 func addImportQualifier(pkg importPath, name string) (importPath, errors.Error) {
-	if name != "" {
-		// TODO use module.ParseImportPath
-		s := string(pkg)
-		if i := strings.LastIndexByte(s, '/'); i >= 0 {
-			s = s[i+1:]
-		}
-		if i := strings.LastIndexByte(s, ':'); i >= 0 {
-			// should never happen, but just in case.
-			s = s[i+1:]
-			if s != name {
-				return "", errors.Newf(token.NoPos,
-					"non-matching package names (%s != %s)", s, name)
-			}
-		} else if s != name {
-			pkg += importPath(":" + name)
-		}
+	if name == "" {
+		return pkg, nil
 	}
-
-	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)
+	}
+	ip.Qualifier = name
+	return importPath(ip.String()), nil
 }
 
 // Complete updates the configuration information. After calling complete,

cue/load/import.go

@@ -343,7 +343,10 @@ func (l *loader) importPathFromAbsDir(absDir fsPath, key string) (importPath, er
 		return "", errors.Newf(token.NoPos,
 			"cannot determine import path for %q (no module)", key)
 	default:
-		pkg = l.cfg.Module + pkg
+		impPath := module.ParseImportPath(l.cfg.Module)
+		impPath.Path += pkg
+		impPath.Qualifier = ""
+		pkg = impPath.String()
 	}
 
 	name := l.cfg.Package
@@ -374,6 +377,9 @@ func (l *loader) newInstance(pos token.Pos, p importPath) *build.Instance {
 //
 // The returned directory may not exist.
 func (l *loader) absDirFromImportPath(pos token.Pos, p importPath) (absDir, name string, err errors.Error) {
+	if p == "" {
+		return "", "", errors.Newf(pos, "empty package name in import path %q", p)
+	}
 	if l.cfg.ModuleRoot == "" {
 		return "", "", errors.Newf(pos, "cannot import %q (root undefined)", p)
 	}
@@ -405,10 +411,10 @@ func (l *loader) absDirFromImportPath(pos token.Pos, p importPath) (absDir, name
 		// and hence it's available by that name via Pkg.
 		pkg := l.pkgs.Pkg(string(origp))
 		if pkg == nil {
-			return "", name, errors.Newf(pos, "no dependency found for package %q", p)
+			return "", name, l.errPkgf([]token.Pos{pos}, "no dependency found for package %q", p)
 		}
 		if err := pkg.Error(); err != nil {
-			return "", name, errors.Newf(pos, "cannot find package %q: %v", p, err)
+			return "", name, l.errPkgf([]token.Pos{pos}, "cannot find package %q: %v", p, err)
 		}
 		if mv := pkg.Mod(); mv.IsLocal() {
 			// It's a local package that's present inside one or both of the gen, usr or pkg
@@ -419,12 +425,15 @@ func (l *loader) absDirFromImportPath(pos token.Pos, p importPath) (absDir, name
 		} else {
 			locs := pkg.Locations()
 			if len(locs) > 1 {
-				return "", "", errors.Newf(pos, "package %q unexpectedly found in multiple locations", p)
+				return "", "", l.errPkgf([]token.Pos{pos}, "package %q unexpectedly found in multiple locations", p)
+			}
+			if len(locs) == 0 {
+				return "", "", l.errPkgf([]token.Pos{pos}, "no location found for package %q", p)
 			}
 			var err error
 			absDir, err = absPathForSourceLoc(locs[0])
 			if err != nil {
-				return "", name, errors.Newf(pos, "cannot determine source directory for package %q: %v", p, err)
+				return "", name, l.errPkgf([]token.Pos{pos}, "cannot determine source directory for package %q: %v", p, err)
 			}
 		}
 		return absDir, name, nil

cue/load/instances.go

@@ -82,7 +82,7 @@ func Instances(args []string, c *Config) []*build.Instance {
 	// logic resolves such paths without consulting pkgs.
 	pkgs, err := loadPackages(ctx, c, synCache, pkgArgs, otherFiles)
 	if err != nil {
-		return []*build.Instance{c.newErrInstance(fmt.Errorf("xxx: %v", err))}
+		return []*build.Instance{c.newErrInstance(err)}
 	}
 	tg := newTagger(c)
 	l := newLoader(c, tg, synCache, pkgs)

cue/load/loader_test.go

@@ -67,12 +67,14 @@ func TestLoad(t *testing.T) {
 		Dir: testdata("badmod"),
 	}
 	type loadTest struct {
+		name string
 		cfg  *Config
 		args []string
 		want string
 	}
 
 	testCases := []loadTest{{
+		name: "BadModuleFile",
 		cfg:  badModCfg,
 		args: []string{"."},
 		want: `err:    module: cannot use value 123 (type int) as string:
@@ -83,6 +85,7 @@ root:   ""
 dir:    ""
 display:""`,
 	}, {
+		name: "DefaultPackage",
 		// Even though the directory is called testdata, the last path in
 		// the module is test. So "package test" is correctly the default
 		// package of this directory.
@@ -97,6 +100,7 @@ files:
     $CWD/testdata/testmod/test.cue
 imports:
     mod.test/test/sub: $CWD/testdata/testmod/sub/sub.cue`}, {
+		name: "DefaultPackageWithExplicitDotArgument",
 		// Even though the directory is called testdata, the last path in
 		// the module is test. So "package test" is correctly the default
 		// package of this directory.
@@ -111,8 +115,7 @@ files:
     $CWD/testdata/testmod/test.cue
 imports:
     mod.test/test/sub: $CWD/testdata/testmod/sub/sub.cue`}, {
-		// TODO:
-		// - path incorrect, should be mod.test/test/other:main.
+		name: "RelativeImportPathWildcard",
 		cfg:  dirCfg,
 		args: []string{"./other/..."},
 		want: `err:    import failed: relative import paths not allowed ("./file"):
@@ -122,6 +125,7 @@ module: mod.test/test
 root:   $CWD/testdata/testmod
 dir:    ""
 display:""`}, {
+		name: "NoPackageName",
 		cfg:  dirCfg,
 		args: []string{"./anon"},
 		want: `err:    build constraints exclude all CUE files in ./anon:
@@ -131,24 +135,22 @@ module: mod.test/test
 root:   $CWD/testdata/testmod
 dir:    $CWD/testdata/testmod/anon
 display:./anon`}, {
-		// TODO:
-		// - paths are incorrect, should be mod.test/test/other:main.
+		name: "RelativeImportPathSingle",
 		cfg:  dirCfg,
 		args: []string{"./other"},
 		want: `err:    import failed: relative import paths not allowed ("./file"):
     $CWD/testdata/testmod/other/main.cue:6:2
-path:   mod.test/test/other:main
+path:   mod.test/test/other
 module: mod.test/test
 root:   $CWD/testdata/testmod
 dir:    $CWD/testdata/testmod/other
 display:./other
 files:
     $CWD/testdata/testmod/other/main.cue`}, {
-		// TODO:
-		// - incorrect path, should be mod.test/test/hello:test
+		name: "RelativePathSuccess",
 		cfg:  dirCfg,
 		args: []string{"./hello"},
-		want: `path:   mod.test/test/hello:test
+		want: `path:   mod.test/test/hello
 module: mod.test/test
 root:   $CWD/testdata/testmod
 dir:    $CWD/testdata/testmod/hello
@@ -158,8 +160,7 @@ files:
     $CWD/testdata/testmod/hello/test.cue
 imports:
     mod.test/test/sub: $CWD/testdata/testmod/sub/sub.cue`}, {
-		// TODO:
-		// - incorrect path, should be mod.test/test/hello:test
+		name: "ExplicitPackageIdentifier",
 		cfg:  dirCfg,
 		args: []string{"mod.test/test/hello:test"},
 		want: `path:   mod.test/test/hello:test
@@ -172,8 +173,7 @@ files:
     $CWD/testdata/testmod/hello/test.cue
 imports:
     mod.test/test/sub: $CWD/testdata/testmod/sub/sub.cue`}, {
-		// TODO:
-		// - incorrect path, should be mod.test/test/hello:test
+		name: "NoPackageName",
 		cfg:  dirCfg,
 		args: []string{"mod.test/test/hello:nonexist"},
 		want: `err:    build constraints exclude all CUE files in mod.test/test/hello:nonexist:
@@ -185,6 +185,7 @@ module: mod.test/test
 root:   $CWD/testdata/testmod
 dir:    $CWD/testdata/testmod/hello
 display:mod.test/test/hello:nonexist`}, {
+		name: "ExplicitNonPackageFiles",
 		cfg:  dirCfg,
 		args: []string{"./anon.cue", "./other/anon.cue"},
 		want: `path:   ""
@@ -195,7 +196,8 @@ display:command-line-arguments
 files:
     $CWD/testdata/testmod/anon.cue
     $CWD/testdata/testmod/other/anon.cue`}, {
-		cfg: dirCfg,
+		name: "AbsoluteFileIsNormalized", // TODO(rogpeppe) what is this actually testing?
+		cfg:  dirCfg,
 		// Absolute file is normalized.
 		args: []string{filepath.Join(cwd, testdata("testmod", "anon.cue"))},
 		want: `path:   ""
@@ -205,6 +207,7 @@ dir:    $CWD/testdata/testmod
 display:command-line-arguments
 files:
     $CWD/testdata/testmod/anon.cue`}, {
+		name: "StandardInput",
 		cfg:  dirCfg,
 		args: []string{"-"},
 		want: `path:   ""
@@ -214,6 +217,7 @@ dir:    $CWD/testdata/testmod
 display:command-line-arguments
 files:
     -`}, {
+		name: "BadIdentifier",
 		cfg:  dirCfg,
 		args: []string{"foo.com/bad-identifier"},
 		want: `err:    implied package identifier "bad-identifier" from import path "foo.com/bad-identifier" is not valid
@@ -223,6 +227,7 @@ root:   $CWD/testdata/testmod
 dir:    $CWD/testdata/testmod/cue.mod/gen/foo.com/bad-identifier
 display:foo.com/bad-identifier`,
 	}, {
+		name: "NonexistentStdlibImport",
 		cfg:  dirCfg,
 		args: []string{"nonexisting"},
 		want: `err:    standard library import path "nonexisting" cannot be imported as a CUE package
@@ -232,6 +237,7 @@ root:   $CWD/testdata/testmod
 dir:    ""
 display:nonexisting`,
 	}, {
+		name: "ExistingStdlibImport",
 		cfg:  dirCfg,
 		args: []string{"strconv"},
 		want: `err:    standard library import path "strconv" cannot be imported as a CUE package
@@ -241,6 +247,7 @@ root:   $CWD/testdata/testmod
 dir:    ""
 display:strconv`,
 	}, {
+		name: "EmptyPackageDirectory",
 		cfg:  dirCfg,
 		args: []string{"./empty"},
 		want: `err:    no CUE files in ./empty
@@ -250,6 +257,7 @@ root:   $CWD/testdata/testmod
 dir:    $CWD/testdata/testmod/empty
 display:./empty`,
 	}, {
+		name: "PackageWithImports",
 		cfg:  dirCfg,
 		args: []string{"./imports"},
 		want: `path:   mod.test/test/imports
@@ -262,15 +270,17 @@ files:
 imports:
     mod.test/catch: $CWD/testdata/testmod/cue.mod/pkg/mod.test/catch/catch.cue
     mod.test/helper:helper1: $CWD/testdata/testmod/cue.mod/pkg/mod.test/helper/helper1.cue`}, {
+		name: "OnlyToolFiles",
 		cfg:  dirCfg,
 		args: []string{"./toolonly"},
-		want: `path:   mod.test/test/toolonly:foo
+		want: `path:   mod.test/test/toolonly
 module: mod.test/test
 root:   $CWD/testdata/testmod
 dir:    $CWD/testdata/testmod/toolonly
 display:./toolonly
 files:
     $CWD/testdata/testmod/toolonly/foo_tool.cue`}, {
+		name: "OnlyToolFilesWithToolsDisabledInConfig",
 		cfg: &Config{
 			Dir: testdataDir,
 		},
@@ -279,11 +289,12 @@ files:
     anon.cue: no package name
     test.cue: package is test, want foo
     toolonly/foo_tool.cue: _tool.cue files excluded in non-cmd mode
-path:   mod.test/test/toolonly:foo
+path:   mod.test/test/toolonly
 module: mod.test/test
 root:   $CWD/testdata/testmod
 dir:    $CWD/testdata/testmod/toolonly
 display:./toolonly`}, {
+		name: "WithBoolTag",
 		cfg: &Config{
 			Dir:  testdataDir,
 			Tags: []string{"prod"},
@@ -296,6 +307,7 @@ dir:    $CWD/testdata/testmod/tags
 display:./tags
 files:
     $CWD/testdata/testmod/tags/prod.cue`}, {
+		name: "WithAttrValTag",
 		cfg: &Config{
 			Dir:  testdataDir,
 			Tags: []string{"prod", "foo=bar"},
@@ -308,6 +320,7 @@ dir:    $CWD/testdata/testmod/tags
 display:./tags
 files:
     $CWD/testdata/testmod/tags/prod.cue`}, {
+		name: "UnusedTag",
 		cfg: &Config{
 			Dir:  testdataDir,
 			Tags: []string{"prod"},
@@ -323,6 +336,7 @@ module: mod.test/test
 root:   $CWD/testdata/testmod
 dir:    $CWD/testdata/testmod/tagsbad
 display:./tagsbad`}, {
+		name: "ImportCycle",
 		cfg: &Config{
 			Dir: testdataDir,
 		},
@@ -397,7 +411,7 @@ func TestOverlays(t *testing.T) {
 	c := &Config{
 		Overlay: map[string]Source{
 			// Not necessary, but nice to add.
-			abs("cue.mod/module.cue"): FromString(`module: "mod.test"`),
+			abs("cue.mod/module.cue"): FromString(`module: "mod.test", language: version: "v0.9.0"`),
 
 			abs("dir/top.cue"): FromBytes([]byte(`
 			   package top

cue/load/module_test.go

@@ -57,7 +57,7 @@ func TestModuleLoadWithInvalidRegistryConfig(t *testing.T) {
 		}
 	}
 	insts = load.Instances([]string{"."}, cfg)
-	qt.Assert(t, qt.ErrorMatches(insts[0].Err, `import failed: cannot find package "example.com@v0": cannot fetch example.com@v0.0.1: bad value for \$CUE_REGISTRY: invalid registry "invalid}host:": invalid host name "invalid}host:" in registry`))
+	qt.Assert(t, qt.ErrorMatches(insts[0].Err, `import failed: .*main.cue:2:8: cannot find package "example.com@v0": cannot fetch example.com@v0.0.1: bad value for \$CUE_REGISTRY: invalid registry "invalid}host:": invalid host name "invalid}host:" in registry`))
 
 	// Try again with environment variables passed in Env.
 	// This is really just a smoke test to make sure that Env is
@@ -67,7 +67,7 @@ func TestModuleLoadWithInvalidRegistryConfig(t *testing.T) {
 		"CUE_CACHE_DIR=" + cacheDir,
 	}
 	insts = load.Instances([]string{"."}, cfg)
-	qt.Assert(t, qt.ErrorMatches(insts[0].Err, `import failed: cannot find package "example.com@v0": cannot fetch example.com@v0.0.1: bad value for \$CUE_REGISTRY: invalid registry "invalid}host2:": invalid host name "invalid}host2:" in registry`))
+	qt.Assert(t, qt.ErrorMatches(insts[0].Err, `import failed: .*main.cue:2:8: cannot find package "example.com@v0": cannot fetch example.com@v0.0.1: bad value for \$CUE_REGISTRY: invalid registry "invalid}host2:": invalid host name "invalid}host2:" in registry`))
 }
 
 func TestModuleFetch(t *testing.T) {