mod/modfile: always allow missing major version in module field

See issue #3217 for background context.

This updates the module logic to always allow a missing major version in
the cue.mod module field, and changes `cue mod tidy` and `cue mod fix`
so that they don't add one if not already present. When the version is
missing, it's assumed to be `v0`.

Note that this now allows `cue mod publish` to publish a module that
does not contain a major version suffix in its module field.

This is a breaking change to the `mod/modfile` API for the reasons
explained below.

The current invariant for `modfile.File` is that the `Module` field is
either empty or a well-formed module path including the major version.
This change breaks that invariant, but our judgement is that
this is relatively new API and that it should affect very few clients
if any (and the fix is easy: change `.Module` to `.QualifiedModule`).

Given that this is relatively new API, this should hopefully not break
too many clients (and the fix is trivial - just change `.Module` to
`.Module()`).

Note: a more comprehensive change, and one we should consider for the
future, is to use an approach like Go's modfile package
(https://pkg.go.dev/golang.org/x/mod/modfile#File) and distinguish the
parsed version from the syntax that it was created from.

Fixes #3217.

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

cmd/cue/cmd/modinit.go

@@ -22,7 +22,6 @@ import (
 
 	"github.com/spf13/cobra"
 
-	"cuelang.org/go/internal/cueexperiment"
 	"cuelang.org/go/internal/cueversion"
 	"cuelang.org/go/mod/modfile"
 	"cuelang.org/go/mod/module"
@@ -64,10 +63,6 @@ func runModInit(cmd *Command, args []string) (err error) {
 				}
 				return fmt.Errorf("invalid module name %q: %v", modulePath, err1)
 			}
-			// Default major version to v0 if the modules experiment is enabled.
-			if cueexperiment.Flags.Modules {
-				modulePath += "@v0"
-			}
 		}
 	}
 

cmd/cue/cmd/modpublish.go

@@ -134,7 +134,7 @@ func runModUpload(cmd *Command, args []string) error {
 	if err != nil {
 		return err
 	}
-	mv, err := module.NewVersion(mf.Module, args[0])
+	mv, err := module.NewVersion(mf.QualifiedModule(), args[0])
 	if err != nil {
 		return fmt.Errorf("cannot form module version: %v", err)
 	}

cmd/cue/cmd/testdata/script/modfix_initial.txtar

@@ -12,7 +12,7 @@ cmp cue.mod/module.cue want-module
 module: "foo.com"
 foo: "bar"
 -- want-module --
-module: "foo.com@v0"
+module: "foo.com"
 language: {
 	version: "v0.9.0"
 }

cmd/cue/cmd/testdata/script/modget_major_version.txtar

@@ -7,7 +7,7 @@ exec cue export .
 cmp stdout want-stdout
 
 -- want-module --
-module: "main.example@v0"
+module: "main.example"
 language: {
 	version: "v0.8.0"
 }

cmd/cue/cmd/testdata/script/modinit_nomajorversion.txtar

@@ -8,7 +8,9 @@ cmp cue.mod/module.cue want-module.cue
 exists cue.mod/usr
 exists cue.mod/pkg
 
-# With the experiment, we add the major version v0 by default.
+# With the experiment, although the major version will be implied
+# as v0, it's still omitted so that there's a possibility of compatibility
+# with earlier cue versions.
 env CUE_EXPERIMENT=modules
 rm cue.mod
 exec cue mod init foo.com/bar
@@ -22,7 +24,7 @@ language: {
 	version: "$CUE_LANGUAGE_VERSION"
 }
 -- want-module-experiment.cue --
-module: "foo.com/bar@v0"
+module: "foo.com/bar"
 language: {
 	version: "$CUE_LANGUAGE_VERSION"
 }

cmd/cue/cmd/testdata/script/modinit_without_version.txtar

@@ -10,7 +10,7 @@ exec cue mod tidy
 cmp cue.mod/module.cue want-module
 
 -- want-module --
-module: "foo.example@v0"
+module: "foo.example"
 language: {
 	version: "$CUE_LANGUAGE_VERSION"
 }

cmd/cue/cmd/testdata/script/modinit_withsource.txtar

@@ -19,15 +19,15 @@ cd $WORK/test2
 cmp stderr $WORK/want-stderr
 
 -- want-module.cue-0 --
-module: "test.example@v0"
+module: "test.example"
 language: {
 	version: "$CUE_LANGUAGE_VERSION"
 }
 source: {
 	kind: "self"
 }
 -- want-module.cue-1 --
-module: "test.example@v0"
+module: "test.example"
 language: {
 	version: "$CUE_LANGUAGE_VERSION"
 }

cmd/cue/cmd/testdata/script/modpublish_no_major_suffix.txtar

@@ -1,5 +1,4 @@
-# Test that cue mod publish fails when the module path lacks a major version suffix.
-# The module path can be fixed up with `cue mod fix` and then published successfully.
+# Test that cue mod publish succeeds when the module path lacks a major version suffix.
 # Using or developing the module locally works otherwise, even `cue mod tidy`.
 
 memregistry MEMREGISTRY
@@ -11,27 +10,21 @@ exec cue mod tidy --check
 
 cp cue.mod/module.cue cue.mod/module.cue.original
 
-! exec cue mod publish v0.0.1
-# TODO(mvdan): this error message should not use an absolute path.
-# TODO(mvdan): we should suggest what command to run to the user based on the version to publish.
-stderr 'module path "main.example" in '${WORK@R}\${/}cue.mod\${/}'module.cue does not contain major version'
+exec cue mod publish v0.0.1
 
-# Note that module commands like `cue mod tidy` work even without a major version suffix
-# as one is not required to fully use the current module locally, even with dependencies.
-exec cue mod tidy --check
+# The inferred version is v0. Using any other major version will fail.
+! exec cue mod publish v1.0.0
+stderr 'mismatched major version suffix'
+! exec cue mod publish v2.0.0
+stderr 'mismatched major version suffix'
 
-# `cue mod fix` adds a major version suffix "v0" when missing.
-# The user can use `cue mod edit --module` if they want a different major version.
+# `cue mod fix` will not add add a major version suffix when missing.
 exec cue mod fix
 cmp cue.mod/module.cue cue.mod/module.cue.fixed
 
-# TODO(mvdan): we cannot edit the module to add a missing major version suffix, which seems like a bug.
 cp cue.mod/module.cue.original cue.mod/module.cue
-! exec cue mod edit --module main.example@v0
-stderr 'module.cue does not contain major version'
-
-# Continue the test with the fixed v0 form.
-cp cue.mod/module.cue.fixed cue.mod/module.cue
+exec cue mod edit --module main.example@v0
+cmp cue.mod/module.cue cue.mod/module.cue.edited-with-v0
 
 # Trying to publish with the wrong major version will fail.
 ! exec cue mod publish v1.0.0
@@ -40,7 +33,7 @@ stderr 'mismatched major version suffix'
 stderr 'mismatched major version suffix'
 
 # Publishing with the right major version works.
-exec cue mod publish v0.0.1
+exec cue mod publish v0.0.2
 
 -- export.stdout --
 {
@@ -51,6 +44,14 @@ module: "main.example"
 language: version: "v0.9.0"
 source: kind: "self"
 -- cue.mod/module.cue.fixed --
+module: "main.example"
+language: {
+	version: "v0.9.0"
+}
+source: {
+	kind: "self"
+}
+-- cue.mod/module.cue.edited-with-v0 --
 module: "main.example@v0"
 language: {
 	version: "v0.9.0"

cmd/cue/cmd/testdata/script/modtidy_missing_major_version.txtar

@@ -0,0 +1,18 @@
+# Check that cue mod tidy does not add a major
+# version suffix to the module field unless it is
+# already present.
+
+exec cue mod tidy --check
+exec cue mod tidy
+cmp cue.mod/module.cue want-cue.mod
+-- want-cue.mod --
+module: "main.org"
+language: {
+	version: "v0.8.100"
+}
+-- cue.mod/module.cue --
+module: "main.org"
+language: version: "v0.8.100"
+
+-- main.cue --
+package main

cmd/cue/cmd/testdata/script/module_compatibility_backwards.txtar

@@ -97,7 +97,7 @@ downstream: root
 -- premodules/cue.mod/pkg/premodules.example/cue.mod/module.cue --
 module: "premodules.example"
 -- premodules-module.cue.fixed --
-module: "premodules.example@v0"
+module: "premodules.example"
 language: {
 	version: "v0.9.0"
 }

cue/load/config.go

@@ -420,15 +420,15 @@ func (c *Config) loadModule() error {
 		return err
 	}
 	c.modFile = mf
-	if mf.Module == "" {
+	if mf.QualifiedModule() == "" {
 		// Backward compatibility: allow empty module.cue file.
 		// TODO maybe check that the rest of the fields are empty too?
 		return nil
 	}
 	if c.Module != "" && c.Module != mf.Module {
 		return errors.Newf(token.NoPos, "inconsistent modules: got %q, want %q", mf.Module, c.Module)
 	}
-	c.Module = mf.Module
+	c.Module = mf.QualifiedModule()
 	return nil
 }
 

cue/load/instances.go

@@ -156,7 +156,7 @@ func loadPackages(ctx context.Context, cfg *Config, synCache *syntaxCache, pkgs
 		return nil, nil
 	}
 	reqs := modrequirements.NewRequirements(
-		cfg.modFile.Module,
+		cfg.modFile.QualifiedModule(),
 		cfg.Registry,
 		cfg.modFile.DepVersions(),
 		cfg.modFile.DefaultMajorVersions(),

internal/mod/modload/tidy.go

@@ -63,7 +63,7 @@ func tidy(ctx context.Context, fsys fs.FS, modRoot string, reg Registry, checkTi
 		return nil, err
 	}
 	// TODO check that module path is well formed etc
-	origRs := modrequirements.NewRequirements(mf.Module, reg, mf.DepVersions(), mf.DefaultMajorVersions())
+	origRs := modrequirements.NewRequirements(mf.QualifiedModule(), reg, mf.DepVersions(), mf.DefaultMajorVersions())
 	rootPkgPaths, err := modimports.AllImports(modimports.AllModuleFiles(fsys, modRoot))
 	if err != nil {
 		return nil, err
@@ -134,9 +134,9 @@ func readModuleFile(fsys fs.FS, modRoot string) (module.Version, *modfile.File,
 	if err != nil {
 		return module.Version{}, nil, err
 	}
-	mainModuleVersion, err := module.NewVersion(mf.Module, "")
+	mainModuleVersion, err := module.NewVersion(mf.QualifiedModule(), "")
 	if err != nil {
-		return module.Version{}, nil, fmt.Errorf("invalid module path %q: %v", mf.Module, err)
+		return module.Version{}, nil, fmt.Errorf("invalid module path %q: %v", mf.QualifiedModule(), err)
 	}
 	return mainModuleVersion, mf, nil
 }

internal/mod/modload/update.go

@@ -33,7 +33,7 @@ func UpdateVersions(ctx context.Context, fsys fs.FS, modRoot string, reg Registr
 	if err != nil {
 		return nil, err
 	}
-	rs := modrequirements.NewRequirements(mf.Module, reg, mf.DepVersions(), mf.DefaultMajorVersions())
+	rs := modrequirements.NewRequirements(mf.QualifiedModule(), reg, mf.DepVersions(), mf.DefaultMajorVersions())
 	mversions, err := resolveUpdateVersions(ctx, reg, rs, mainModuleVersion, versions)
 	if err != nil {
 		return nil, err
@@ -73,7 +73,7 @@ func UpdateVersions(ctx context.Context, fsys fs.FS, modRoot string, reg Registr
 	for _, v := range mversionsMap {
 		newVersions = append(newVersions, v)
 	}
-	rs = modrequirements.NewRequirements(mf.Module, reg, newVersions, mf.DefaultMajorVersions())
+	rs = modrequirements.NewRequirements(mf.QualifiedModule(), reg, newVersions, mf.DefaultMajorVersions())
 	g, err = rs.Graph(ctx)
 	if err != nil {
 		return nil, fmt.Errorf("cannot determine new module graph: %v", err)
@@ -92,7 +92,7 @@ func UpdateVersions(ctx context.Context, fsys fs.FS, modRoot string, reg Registr
 			finalVersions = append(finalVersions, v)
 		}
 	}
-	rs = modrequirements.NewRequirements(mf.Module, reg, finalVersions, mf.DefaultMajorVersions())
+	rs = modrequirements.NewRequirements(mf.QualifiedModule(), reg, finalVersions, mf.DefaultMajorVersions())
 	return modfileFromRequirements(mf, rs), nil
 }
 

internal/registrytest/registry.go

@@ -490,16 +490,12 @@ func (c *moduleContent) init(versDir string) error {
 		if found {
 			return fmt.Errorf("multiple module.cue files")
 		}
-		modp, _, ok := module.SplitPathVersion(modf.Module)
-		if !ok {
-			return fmt.Errorf("module %q does not contain major version", modf.Module)
-		}
-		mod := strings.ReplaceAll(modp, "/", "_") + "_"
+		mod := strings.ReplaceAll(modf.ModulePath(), "/", "_") + "_"
 		vers := strings.TrimPrefix(versDir, mod)
 		if len(vers) == len(versDir) {
-			return fmt.Errorf("module path %q in module.cue does not match directory %q", modf.Module, versDir)
+			return fmt.Errorf("module path %q in module.cue does not match directory %q", modf.QualifiedModule(), versDir)
 		}
-		v, err := module.NewVersion(modf.Module, vers)
+		v, err := module.NewVersion(modf.QualifiedModule(), vers)
 		if err != nil {
 			return fmt.Errorf("cannot make module version: %v", err)
 		}

mod/modfile/modfile.go

@@ -48,6 +48,11 @@ const schemaFile = "cuelang.org/go/mod/modfile/schema.cue"
 
 // File represents the contents of a cue.mod/module.cue file.
 type File struct {
+	// Module holds the module path, which may
+	// not contain a major version suffix.
+	// Use the [File.QualifiedModule] method to obtain a module
+	// path that's always qualified. See also the
+	// [File.ModulePath] and [File.MajorVersion] methods.
 	Module   string                    `json:"module"`
 	Language *Language                 `json:"language,omitempty"`
 	Source   *Source                   `json:"source,omitempty"`
@@ -64,6 +69,39 @@ type File struct {
 	actualSchemaVersion string
 }
 
+// Module returns the fully qualified module path
+// if is one. It returns the empty string when [ParseLegacy]
+// is used and the module field is empty.
+//
+// Note that when the module field does not contain a major
+// version suffix, "@v0" is assumed.
+func (f *File) QualifiedModule() string {
+	if strings.Contains(f.Module, "@") {
+		return f.Module
+	}
+	if f.Module == "" {
+		return ""
+	}
+	return f.Module + "@v0"
+}
+
+// ModulePath returns the path part of the module without
+// its major version suffix.
+func (f *File) ModulePath() string {
+	path, _, _ := module.SplitPathVersion(f.QualifiedModule())
+	return path
+}
+
+// MajorVersion returns the major version of the module,
+// not including the "@".
+// If there is no module (which can happen when [ParseLegacy]
+// is used or if Module is explicitly set to an empty string),
+// it returns the empty string.
+func (f *File) MajorVersion() string {
+	_, vers, _ := module.SplitPathVersion(f.QualifiedModule())
+	return vers
+}
+
 // baseFileVersion is used to decode the language version
 // to decide how to decode the rest of the file.
 type baseFileVersion struct {
@@ -266,7 +304,7 @@ func ParseNonStrict(modfile []byte, filename string) (*File, error) {
 // into a format suitable for parsing with [Parse]. It adds a language.version
 // field and moves all unrecognized fields into custom.legacy.
 //
-// If there is no module field or it is empty, it is set to "test.example@v0".
+// If there is no module field or it is empty, it is set to "test.example".
 //
 // If the file already parses OK with [ParseNonStrict], it returns the
 // result of that.
@@ -289,7 +327,7 @@ func FixLegacy(modfile []byte, filename string) (*File, error) {
 	if err := v.Decode(&allFields); err != nil {
 		return nil, err
 	}
-	mpath := "test.example@v0"
+	mpath := "test.example"
 	if m, ok := allFields["module"]; ok {
 		if mpath1, ok := m.(string); ok && mpath1 != "" {
 			mpath = mpath1
@@ -408,9 +446,6 @@ func parse(modfile []byte, filename string, strict bool) (*File, error) {
 		return nil, err
 	}
 	mainPath, mainMajor, ok := module.SplitPathVersion(mf.Module)
-	if strict && !ok {
-		return nil, fmt.Errorf("module path %q in %s does not contain major version", mf.Module, filename)
-	}
 	if ok {
 		if semver.Major(mainMajor) != mainMajor {
 			return nil, fmt.Errorf("module path %s in %q should contain the major version only", mf.Module, filename)
@@ -421,8 +456,6 @@ func parse(modfile []byte, filename string, strict bool) (*File, error) {
 		}
 		// There's no main module major version: default to v0.
 		mainMajor = "v0"
-		// TODO perhaps we'd be better preserving the original?
-		mf.Module += "@v0"
 	}
 	if mf.Language != nil {
 		vers := mf.Language.Version