fix(version): require a comma between version constraints

CHANGELOG.md

@@ -9,6 +9,51 @@ mistaken for a safe patch upgrade.
 
 ## [Unreleased]
 
+### Breaking
+
+- A comma is required between version constraints in a specifier set. `>=1.0<2.0`,
+  `>= 7 < 10` and `>= 1.0 != 1.3.4.* < 2.0` previously parsed and are now rejected, as
+  PEP 440 requires and as upstream does.
+
+  They did not merely parse. `validConstraintRegexp` made the separator optional, and
+  combined with the deliberately-admitted empty operator (a bare version is a valid
+  constraint) it **rendered a comma the input never contained**: `==0.1dev10.3` became the
+  constraint `==0.1dev10` plus a bare-version constraint `3`, rendering `==0.1dev10,3`. That
+  is a fabricated constraint boundary, the same defect class as the tokenizer fix in 0.3.0
+  reached by a different route, and it completes that fix.
+
+  **Measured over the production PyPI snapshot** (2,804,136 distinct requirement strings):
+  exactly **21 newly rejected, 0 newly accepted**. Every one is a version operand whose
+  match is a proper prefix leaving a valid second token, so every one previously rendered a
+  comma from nowhere. All sampled forms were confirmed rejected by `pypa/packaging` 26.2, so
+  this closes a divergence rather than creating one.
+
+  **Measured over real CRAN data for the R path** (`NewRSpecifiers`, which PPM uses for R
+  constraint parsing): **no change at all** — 58,656 constraint occurrences, 2,939 distinct
+  expressions, identical accept/reject before and after. R constraints in practice are a
+  single operator and version; the corpus contains no comma-less pair. The change was
+  verified to actually reject six adjacency shapes and to leave twelve load-bearing forms
+  untouched, so that null result reflects the data rather than an inert change.
+
+  **Deliberately unchanged:** a bare version is still a valid constraint (PPM passes one
+  through `NewRSpecifiers`; narrowing it is tracked separately in
+  rstudio/package-manager#18634), and a trailing comma is still tolerated, which upstream
+  also accepts. Narrowing one thing at a time is what keeps the blast radius measurable.
+
+  ⚠️ The Python and R entry points diverge on dash-containing input, by design:
+  `newRSpecifiers` rewrites `-` to `.` before validating, so `>=0-12.1` is the single
+  constraint `>=0.12.1` there, while on the Python path PEP 440 reads `0-12` as the
+  post-release `0.post12` and the trailing `.1` makes it an adjacent pair. There is a test
+  pinning both halves.
+
+### Fixed
+
+- Five `TestVersion_Check` cases were written space-separated (`>= 1.0 != 1.3.4.* < 2.0`)
+  and asserted as valid, pinning a leniency the reference implementation does not have.
+  Measured against `pypa/packaging` 26.2, that form raises `InvalidSpecifier` while the
+  comma form parses. Rewritten to the comma-separated form PEP 440 actually uses; the intent
+  of each case (the conjunction) is unchanged.
+
 ### Fixed
 
 - No method on a zero-value `Version` panics. Eight of the thirteen exported methods did:

version/adjacent_constraint_test.go

@@ -0,0 +1,129 @@
+// SPDX-License-Identifier: Apache-2.0 OR MIT
+
+package version
+
+import (
+	"strings"
+	"testing"
+)
+
+func identitySanitizer(s string) string { return s }
+
+// Two constraints written with no comma between them are rejected.
+//
+// validConstraintRegexp made the separator optional, so adjacency validated and
+// then re-rendered WITH a comma the input never contained. Combined with the
+// deliberately-admitted empty operator (a bare version is a valid constraint),
+// that turned "==0.1dev10.3" into "==0.1dev10" plus a bare-version constraint
+// "3" and rendered "==0.1dev10,3" -- a fabricated constraint boundary, the same
+// defect class as the tokenizer fix in #22 reached by a different route.
+//
+// Every case here was verified REJECTED by pypa/packaging 26.2.
+func TestAdjacentConstraintsRejected(t *testing.T) {
+	for _, spec := range []string{
+		">=1.0<2.0",
+		">= 7 < 10",
+		">= 1.0 != 1.3.4.* < 2.0",
+		">= 3.5.0 >= 4.0.0",
+		"> 1.0 2.0", // operator form followed by a bare version
+
+		// Drawn from the production PyPI snapshot. Each is a single operand whose
+		// version match is a PROPER PREFIX, leaving a valid second token behind:
+		// the fabricated comma appears between the two.
+		"==0.1dev10.3",     // -> "==0.1dev10" + "3"
+		">=9.0.0a1.0",      // -> ">=9.0.0a1"  + "0"
+		"==1.1b02.1",       // -> "==1.1b02"   + "1"
+		">=0.2.1dev4.dev0", // -> ">=0.2.1dev4" + ".dev0"
+	} {
+		t.Run(spec, func(t *testing.T) {
+			if _, err := NewSpecifiers(spec); err == nil {
+				t.Errorf("NewSpecifiers(%q) accepted an adjacent pair with no comma", spec)
+			}
+			if _, err := NewRSpecifiers(spec, identitySanitizer); err == nil {
+				t.Errorf("NewRSpecifiers(%q) accepted an adjacent pair with no comma", spec)
+			}
+		})
+	}
+}
+
+// ⚠️ The two entry points DIVERGE on dash-containing input, by design, and it is
+// easy to write a test that conflates them.
+//
+// newRSpecifiers replaces "-" with "." BEFORE validating, because that is R's
+// version convention. So ">=0-12.1" is not an adjacent pair on the R path at
+// all: it becomes ">=0.12.1", a single valid constraint. On the Python path the
+// same string IS an adjacent pair -- PEP 440 reads "0-12" as the post-release
+// "0.post12", leaving ".1" behind -- and is now rejected, which matches
+// pypa/packaging 26.2.
+//
+// Asserting both paths reject it would have been wrong, and asserting neither
+// does would have missed the Python fix.
+func TestDashInputDivergesBetweenPythonAndRPaths(t *testing.T) {
+	const spec = ">=0-12.1"
+
+	if _, err := NewSpecifiers(spec); err == nil {
+		t.Errorf("NewSpecifiers(%q) should reject: PEP 440 reads it as "+
+			"\">=0.post12\" plus a bare \"1\"", spec)
+	}
+	if _, err := NewRSpecifiers(spec, identitySanitizer); err != nil {
+		t.Errorf("NewRSpecifiers(%q) should accept: the dash becomes a dot before "+
+			"validation, making it the single constraint \">=0.12.1\": %v", spec, err)
+	}
+}
+
+// The narrowing must not reach anything else. These are the forms measured as
+// load-bearing: real CRAN constraint syntax, PPM's R inputs, and the two
+// leniencies deliberately left in place.
+func TestCommaRequirementDoesNotNarrowValidForms(t *testing.T) {
+	cases := []struct{ spec, why string }{
+		{">= 3.5.0", "the overwhelmingly common CRAN form (99.8% of real occurrences)"},
+		{">= 1.2-18", "R dash form"},
+		{">= 1.1-18-1", "real CRAN two-dash form"},
+		{">= 2016-12-12", "real CRAN date-shaped version"},
+		{"1.2-18", "bare R version"},
+		{"3", "bare version: PPM's ?version= passes one, tracked in #18634"},
+		{">= 1.0, != 1.3.4.*, < 2.0", "PEP 440's own comma-separated example"},
+		{"!=3.2-2, >3.0.0", "PPM's R multi-constraint test shape"},
+		{"> 2.0.0, < 3.0.0, != 2.4.2", "three comma-separated constraints"},
+		{">= 1.0,", "trailing comma, which upstream also accepts"},
+		{"*", "the any-version shorthand"},
+		{"===lolwat", "arbitrary equality takes an opaque operand"},
+		{">=1.0 , <2.0", "spaces around the comma"},
+		{">=2.3 || < 3", "OR groups"},
+	}
+	for _, c := range cases {
+		t.Run(c.spec, func(t *testing.T) {
+			if _, err := NewRSpecifiers(c.spec, identitySanitizer); err != nil {
+				t.Errorf("NewRSpecifiers(%q) rejected a valid form (%s): %v", c.spec, c.why, err)
+			}
+		})
+	}
+}
+
+// ⚠️ The empty operator stays admissible. It is what makes a bare version a
+// valid constraint, PPM depends on it, and narrowing it is separately tracked in
+// rstudio/package-manager#18634. A comma requirement must not remove it by
+// accident -- this pins the two changes apart.
+func TestBareVersionStillAcceptedAfterCommaRequirement(t *testing.T) {
+	for _, spec := range []string{"2.0", "3", "1.2.3", "1!2.0", "1.0+local"} {
+		if _, err := NewSpecifiers(spec); err != nil {
+			t.Errorf("NewSpecifiers(%q) rejected a bare version: %v", spec, err)
+		}
+	}
+}
+
+// No fabricated comma survives in rendered output. This is the actual harm the
+// change prevents, so it is asserted on the RENDERING rather than only on
+// accept/reject: a constraint set must never render a comma its input lacked.
+func TestNoFabricatedCommaInRenderedOutput(t *testing.T) {
+	// Inputs with no comma must either be rejected or render without one.
+	for _, spec := range []string{">=1.26", "1.0", ">= 3.5.0", "===lolwat"} {
+		specs, err := NewSpecifiers(spec)
+		if err != nil {
+			t.Fatalf("NewSpecifiers(%q): unexpected error %v", spec, err)
+		}
+		if got := specs.String(); strings.Contains(got, ",") {
+			t.Errorf("NewSpecifiers(%q).String() = %q, which invents a comma", spec, got)
+		}
+	}
+}

version/specifier.go

@@ -76,13 +76,40 @@ func init() {
 		),
 	)
 
+	// One constraint, either arbitrary equality or an operator plus a version.
+	//
+	// ⚠️ The operator alternation admits the EMPTY operator, so a bare version
+	// ("2.0") is a valid constraint. That is deliberate and separately tracked
+	// (rstudio/package-manager#18634), and it is load-bearing: PPM's
+	// GET /repos/:repo/packages/:key/doc?version=<raw> passes a bare version
+	// through NewRSpecifiers. Do not remove it while tightening the comma.
+	constraint := fmt.Sprintf(
+		`(?:===\s*%s|(%s)\s*(%s(\.\*)?))`,
+		arbitraryOperand,
+		strings.Join(ops, "|"),
+		regex,
+	)
+
+	// A COMMA IS REQUIRED BETWEEN CONSTRAINTS.
+	//
+	// This previously ended each repetition with `\s*\,?`, making the separator
+	// optional, so two constraints written adjacently validated and then
+	// re-rendered WITH a comma the input never contained. Combined with the
+	// empty operator above, that turned "==0.1dev10.3" into the constraint
+	// "==0.1dev10" plus a bare-version constraint "3", rendering
+	// "==0.1dev10,3" -- a fabricated constraint boundary, the same defect class
+	// as #22 by a different route.
+	//
+	// A trailing comma is still tolerated, deliberately: that is a separate
+	// leniency from the missing separator, nothing observed depends on
+	// tightening it, and narrowing one thing at a time keeps the blast radius
+	// measurable.
+	//
+	// Go's regexp permits a capture-group name to repeat, so embedding the
+	// version pattern twice needs no name stripping. Verified rather than
+	// assumed -- it is the opposite of Python's rule.
 	validConstraintRegexp = regexp.MustCompile(
-		fmt.Sprintf(
-			`^\s*(\s*(?:===\s*%s|(%s)\s*(%s(\.\*)?))\s*\,?)*\s*$`,
-			arbitraryOperand,
-			strings.Join(ops, "|"),
-			regex,
-		),
+		fmt.Sprintf(`^\s*(?:%s\s*(?:,\s*%s\s*)*,?)?\s*$`, constraint, constraint),
 	)
 
 	prefixRegexp = regexp.MustCompile(`^([0-9]+)((?:a|b|c|rc)[0-9]+)$`)

version/specifier_test.go

@@ -397,12 +397,18 @@ func TestVersion_Check(t *testing.T) {
 		{"2.0", "=2.0+deadbeef", false},
 		{"2.0", "*", true},
 
-		// space separated
-		{"1.0", ">= 1.0 != 1.3.4.* < 2.0", true},
-		{"1.0", "~= 0.9 >= 1.0 != 1.3.4.* < 2.0", false},
-		{"0.9", "~= 0.9 != 1.3.4.* < 2.0", true},
-		{"2.0", ">= 1.0 != 1.3.4.* < 2.0", false},
-		{"1.3.4", ">= 1.0 != 1.3.4.* < 2.0", false},
+		// Comma-separated, which is what PEP 440 writes and what upstream
+		// accepts. These five cases were previously written SPACE-separated
+		// (">= 1.0 != 1.3.4.* < 2.0") and asserted as valid, pinning a leniency
+		// the reference implementation does not have: measured against
+		// pypa/packaging 26.2, that form raises InvalidSpecifier while the comma
+		// form parses to '!=1.3.4.*,<2.0,>=1.0'. The intent of each case is the
+		// conjunction, which the commas express without relying on the defect.
+		{"1.0", ">= 1.0, != 1.3.4.*, < 2.0", true},
+		{"1.0", "~= 0.9, >= 1.0, != 1.3.4.*, < 2.0", false},
+		{"0.9", "~= 0.9, != 1.3.4.*, < 2.0", true},
+		{"2.0", ">= 1.0, != 1.3.4.*, < 2.0", false},
+		{"1.3.4", ">= 1.0, != 1.3.4.*, < 2.0", false},
 	}
 	for _, tt := range tests {
 		t.Run(fmt.Sprintf("%s %s", tt.version, tt.spec), func(t *testing.T) {