mod/module: improve handling of Qualifier in ImportPath

This change fixes a couple of issues with `module.ImportPath`:
- it only sets `Qualifier` when it's a valid identifier
- it always includes the package qualifier when it's needed.

This exposes an issue with the cue/load logic: it was ignoring an error
when it should not be. We fix that with a minor refactoring of the
`absDirFromImportPath` logic. As the error from determining the name
from the import path seems independent of the error from trying to get
the package, it feels like it's better exposed as an extra error.

For #3155.

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

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

@@ -12,9 +12,6 @@ cmp stdout stdout.golden
 # Without a qualifier, it's an error because the chosen package is ambiguous
 # (the package clause in CUE has to declare a valid CUE identifier).
 ! exec cue eval ./test2/root.cue
-# Note: the errors here could use improvement. Specifically it says:
-#	package is x, want 1x:
-# but it's not possible for a package to be 1x because it's not a valid CUE identifier.
 cmp stderr test2-import-stderr.golden
 ! exec cue eval mod.com/1x
 cmp stderr test2-abs-stderr.golden
@@ -44,9 +41,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 determine package name for "mod.com/1x"; set it explicitly with ':':
     ./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 determine package name for "mod.com/1x"; set it explicitly with ':'

cue/load/import.go

@@ -24,7 +24,6 @@ import (
 	"slices"
 	"strings"
 
-	"cuelang.org/go/cue/ast"
 	"cuelang.org/go/cue/build"
 	"cuelang.org/go/cue/errors"
 	"cuelang.org/go/cue/token"
@@ -361,15 +360,20 @@ func (l *loader) importPathFromAbsDir(absDir fsPath, key string) (importPath, er
 }
 
 func (l *loader) newInstance(pos token.Pos, p importPath) *build.Instance {
-	dir, name, err := l.absDirFromImportPath(pos, p)
+	dir, err := l.absDirFromImportPath(pos, p)
 	i := l.cfg.Context.NewInstance(dir, l.loadFunc)
+	i.Err = errors.Append(i.Err, err)
 	i.Dir = dir
-	i.PkgName = name
+
+	parts := module.ParseImportPath(string(p))
+	i.PkgName = parts.Qualifier
+	if i.PkgName == "" {
+		i.Err = errors.Append(i.Err, l.errPkgf([]token.Pos{pos}, "cannot determine package name for %q; set it explicitly with ':'", p))
+	}
 	i.DisplayPath = string(p)
 	i.ImportPath = string(p)
 	i.Root = l.cfg.ModuleRoot
 	i.Module = l.cfg.Module
-	i.Err = errors.Append(i.Err, err)
 
 	return i
 }
@@ -378,41 +382,33 @@ func (l *loader) newInstance(pos token.Pos, p importPath) *build.Instance {
 // and a package name. The root directory must be set.
 //
 // The returned directory may not exist.
-func (l *loader) absDirFromImportPath(pos token.Pos, p importPath) (absDir, name string, err errors.Error) {
-	dir, name, err0 := l.absDirFromImportPath1(pos, p)
-	if err0 != nil {
+func (l *loader) absDirFromImportPath(pos token.Pos, p importPath) (string, errors.Error) {
+	dir, err := l.absDirFromImportPath1(pos, p)
+	if err != nil {
 		// Any error trying to determine the package location
 		// is a PackageError.
-		err = l.errPkgf([]token.Pos{pos}, "%s", err0.Error())
+		return "", l.errPkgf([]token.Pos{pos}, "%s", err.Error())
 	}
-	return dir, name, err
+	return dir, nil
 }
 
-func (l *loader) absDirFromImportPath1(pos token.Pos, p importPath) (absDir, name string, err error) {
+func (l *loader) absDirFromImportPath1(pos token.Pos, p importPath) (absDir string, err error) {
 	if p == "" {
-		return "", "", fmt.Errorf("empty package name in import path %q", p)
+		return "", fmt.Errorf("empty import path")
 	}
 	if l.cfg.ModuleRoot == "" {
-		return "", "", fmt.Errorf("cannot import %q (root undefined)", p)
+		return "", fmt.Errorf("cannot import %q (root undefined)", p)
 	}
 	if isStdlibPackage(string(p)) {
-		return "", "", fmt.Errorf("standard library import path %q cannot be imported as a CUE package", p)
+		return "", fmt.Errorf("standard library import path %q cannot be imported as a CUE package", p)
 	}
 	origp := p
 	// Extract the package name.
 	parts := module.ParseImportPath(string(p))
-	name = parts.Qualifier
 	p = importPath(parts.Unqualified().String())
-	if name == "" {
-		err = fmt.Errorf("empty package name in import path %q", p)
-	} else if strings.IndexByte(name, '.') >= 0 {
-		err = fmt.Errorf("cannot determine package name for %q (set explicitly with ':')", p)
-	} else if !ast.IsValidIdent(name) {
-		err = fmt.Errorf("implied package identifier %q from import path %q is not valid", name, p)
-	}
 	if l.cfg.Registry != nil {
 		if l.pkgs == nil {
-			return "", name, fmt.Errorf("imports are unavailable because there is no cue.mod/module.cue file")
+			return "", fmt.Errorf("imports are unavailable because there is no cue.mod/module.cue file")
 		}
 		// TODO predicate registry-aware lookup on module.cue-declared CUE version?
 
@@ -421,10 +417,10 @@ func (l *loader) absDirFromImportPath1(pos token.Pos, p importPath) (absDir, nam
 		// and hence it's available by that name via Pkg.
 		pkg := l.pkgs.Pkg(string(origp))
 		if pkg == nil {
-			return "", name, fmt.Errorf("no dependency found for package %q", p)
+			return "", fmt.Errorf("no dependency found for package %q", p)
 		}
 		if err := pkg.Error(); err != nil {
-			return "", name, fmt.Errorf("cannot find package %q: %v", p, err)
+			return "", fmt.Errorf("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
@@ -435,18 +431,18 @@ func (l *loader) absDirFromImportPath1(pos token.Pos, p importPath) (absDir, nam
 		} else {
 			locs := pkg.Locations()
 			if len(locs) > 1 {
-				return "", "", fmt.Errorf("package %q unexpectedly found in multiple locations", p)
+				return "", fmt.Errorf("package %q unexpectedly found in multiple locations", p)
 			}
 			if len(locs) == 0 {
-				return "", "", fmt.Errorf("no location found for package %q", p)
+				return "", fmt.Errorf("no location found for package %q", p)
 			}
 			var err error
 			absDir, err = absPathForSourceLoc(locs[0])
 			if err != nil {
-				return "", name, fmt.Errorf("cannot determine source directory for package %q: %v", p, err)
+				return "", fmt.Errorf("cannot determine source directory for package %q: %v", p, err)
 			}
 		}
-		return absDir, name, nil
+		return absDir, nil
 	}
 
 	// Determine the directory without using the registry.
@@ -462,7 +458,7 @@ func (l *loader) absDirFromImportPath1(pos token.Pos, p importPath) (absDir, nam
 	default:
 		absDir = filepath.Join(GenPath(l.cfg.ModuleRoot), sub)
 	}
-	return absDir, name, err
+	return absDir, err
 }
 
 func absPathForSourceLoc(loc module.SourceLoc) (string, error) {

cue/load/loader_test.go

@@ -227,7 +227,8 @@ files:
 		name: "BadIdentifier",
 		cfg:  dirCfg,
 		args: []string{"foo.com/bad-identifier"},
-		want: `err:    cannot find package "foo.com/bad-identifier": cannot find module providing package foo.com/bad-identifier
+		want: `err:    cannot determine package name for "foo.com/bad-identifier"; set it explicitly with ':'
+cannot find package "foo.com/bad-identifier": cannot find module providing package foo.com/bad-identifier
 path:   foo.com/bad-identifier
 module: mod.test/test@v0
 root:   $CWD/testdata/testmod

mod/module/module_test.go

@@ -260,7 +260,7 @@ var parseImportPathTests = []struct {
 	want: ImportPath{
 		Path:      "main.test",
 		Version:   "v0",
-		Qualifier: "main.test",
+		Qualifier: "",
 	},
 }, {
 	testName: "WithMajorVersionAndExplicitQualifier",
@@ -306,7 +306,7 @@ var parseImportPathTests = []struct {
 		Path:              "foo.com/bar/...",
 		Version:           "",
 		ExplicitQualifier: false,
-		Qualifier:         "...",
+		Qualifier:         "",
 	},
 	wantCanonical: "foo.com/bar/...",
 }, {
@@ -326,7 +326,7 @@ var parseImportPathTests = []struct {
 		Path:              "foo.com/bar/#foo",
 		Version:           "",
 		ExplicitQualifier: false,
-		Qualifier:         "#foo",
+		Qualifier:         "",
 	},
 	wantCanonical: "foo.com/bar/#foo",
 }}
@@ -359,5 +359,5 @@ func TestImportPathStringAddsQualifierWhenNoVersion(t *testing.T) {
 		Path:      "foo.com/bar",
 		Qualifier: "baz",
 	}
-	qt.Assert(t, qt.Equals(ip.String(), "foo.com/bar"))
+	qt.Assert(t, qt.Equals(ip.String(), "foo.com/bar:baz"))
 }

mod/module/path.go

@@ -8,6 +8,7 @@ import (
 	"unicode"
 	"unicode/utf8"
 
+	"cuelang.org/go/cue/ast"
 	"cuelang.org/go/internal/mod/semver"
 )
 
@@ -443,7 +444,7 @@ func (parts ImportPath) String() string {
 			needQualifier = true
 		}
 	}
-	if parts.Version == "" && !parts.ExplicitQualifier {
+	if parts.Version == "" && !needQualifier {
 		// Fast path.
 		return parts.Path
 	}
@@ -461,6 +462,7 @@ func (parts ImportPath) String() string {
 }
 
 // ParseImportPath returns the various components of an import path.
+// It does not check the result for validity.
 func ParseImportPath(p string) ImportPath {
 	var parts ImportPath
 	pathWithoutQualifier := p
@@ -480,6 +482,9 @@ func ParseImportPath(p string) ImportPath {
 		} else {
 			parts.Qualifier = parts.Path
 		}
+		if !ast.IsValidIdent(parts.Qualifier) || strings.HasPrefix(parts.Qualifier, "#") {
+			parts.Qualifier = ""
+		}
 	}
 	return parts
 }