splitModule should conform to Go spec

Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>

cmd/packagesfuncs.go

@@ -49,20 +49,28 @@ func cmdUpgrade(fl Flags) (int, error) {
 func splitModule(arg string) (module, version string, err error) {
 	const versionSplit = "@"
 
-	// accommodate module paths that have @ in them, but we can only tolerate that if there's also
-	// a version, otherwise we don't know if it's a version separator or part of the file path
-	lastVersionSplit := strings.LastIndex(arg, versionSplit)
-	if lastVersionSplit < 0 {
+	// Per https://go.dev/ref/mod#go-mod-file-ident, module paths consist of
+	// ASCII letters, digits, and `-`, `.`, `_`, `~` only, so `@` is never a
+	// valid path character. That makes the split unambiguous: at most one
+	// `@` may appear, and it separates the module path from the version.
+	switch strings.Count(arg, versionSplit) {
+	case 0:
 		module = arg
-	} else {
-		module, version = arg[:lastVersionSplit], arg[lastVersionSplit+1:]
+	case 1:
+		idx := strings.Index(arg, versionSplit)
+		module, version = arg[:idx], arg[idx+1:]
+		if version == "" {
+			return "", "", fmt.Errorf("version is required after '@'")
+		}
+	default:
+		return "", "", fmt.Errorf("module path must not contain '@'")
 	}
 
 	if module == "" {
-		err = fmt.Errorf("module name is required")
+		return "", "", fmt.Errorf("module name is required")
 	}
 
-	return module, version, err
+	return module, version, nil
 }
 
 func cmdAddPackage(fl Flags) (int, error) {

cmd/packagesfuncs_test.go

@@ -105,27 +105,21 @@ func TestSplitModule(t *testing.T) {
 			expectError:     false,
 		},
 		{
-			name:            "module with @ in path and version",
+			// Per go.dev/ref/mod#go-mod-file-ident, '@' is not a valid module
+			// path character, so any input with more than one '@' is invalid.
+			name:            "module with @ in path is rejected",
 			input:           "github.com/@user/module@v1.0.0",
-			expectedModule:  "github.com/@user/module",
-			expectedVersion: "v1.0.0",
-			expectError:     false,
+			expectedModule:  "",
+			expectedVersion: "",
+			expectError:     true,
 		},
 		{
-			name:            "module with multiple @ in path",
+			name:            "module with multiple @ in path is rejected",
 			input:           "github.com/@org/@user/module@v2.3.4",
-			expectedModule:  "github.com/@org/@user/module",
-			expectedVersion: "v2.3.4",
-			expectError:     false,
+			expectedModule:  "",
+			expectedVersion: "",
+			expectError:     true,
 		},
-		// TODO: decide on the behavior for this case; it fails currently
-		// {
-		// 	name:            "module with @ in path but no version",
-		// 	input:           "github.com/@user/module",
-		// 	expectedModule:  "github.com/@user/module",
-		// 	expectedVersion: "",
-		// 	expectError:     false,
-		// },
 		{
 			name:            "empty string",
 			input:           "",
@@ -144,22 +138,22 @@ func TestSplitModule(t *testing.T) {
 			name:            "@ at start",
 			input:           "@v1.0.0",
 			expectedModule:  "",
-			expectedVersion: "v1.0.0",
+			expectedVersion: "",
 			expectError:     true,
 		},
 		{
-			name:            "@ at end",
+			name:            "trailing @ with no version is rejected",
 			input:           "github.com/user/module@",
-			expectedModule:  "github.com/user/module",
+			expectedModule:  "",
 			expectedVersion: "",
-			expectError:     false,
+			expectError:     true,
 		},
 		{
-			name:            "multiple consecutive @",
+			name:            "multiple consecutive @ is rejected",
 			input:           "github.com/user/module@@v1.0.0",
-			expectedModule:  "github.com/user/module@",
-			expectedVersion: "v1.0.0",
-			expectError:     false,
+			expectedModule:  "",
+			expectedVersion: "",
+			expectError:     true,
 		},
 		{
 			name:            "version with latest tag",