marker: report which comparisons could not be decided

CHANGELOG.md

@@ -9,6 +9,37 @@ mistaken for a safe patch upgrade.
 
 ## [Unreleased]
 
+### Added
+
+- `marker`: `Marker.EvaluateUndecidable` reports whether a marker is satisfied
+  **and** which of its comparisons could not be decided, and `Marker.Variables`
+  reports the environment variables a marker references.
+
+  `Evaluate` returns a bare `bool`, so a caller cannot distinguish "false" from
+  "could not tell". Two things produce the latter: `~=` and `===` reaching the
+  generic string-operator table, which has no semantics for them (pypa/packaging
+  raises `UndefinedComparison`), and an environment variable that resolves to
+  `""`, which `EnvironmentFromTarget` legitimately does for `platform_release`
+  and `platform_version`, since a *declared* target has no kernel to report.
+
+  A consumer that must not discard a dependency edge (building a mirror or an
+  offline bundle, where a dropped edge means a missing package and no fallback)
+  previously had to scan `Marker.String()` for those operators and variable
+  names, with false positives on quoted literals and a token list to keep in
+  sync by hand.
+
+  `Variables` covers the half this library cannot decide for the caller: a
+  declared `3.13` forces the caller to invent a `PythonFullVersion`, and an
+  invented value is decidable-but-arbitrary (`>= "3.13.2"` is false at `3.13.0`,
+  true at `3.13.99`). Only the caller knows which fields it fabricated; it just
+  needs to ask which variables a marker touches.
+
+  Ordered comparisons on string operands are deliberately **not** undecidable:
+  `<`/`>` returning false and `<=`/`>=` collapsing to equality is faithful to
+  packaging, whose operator table is the same. Verified against 26.3.
+
+  `Evaluate` is now a wrapper and its behaviour is unchanged.
+
 ### Fixed
 
 - `tags`: `riscv64` and `loongarch64` targets now claim the same manylinux

marker/evaluate.go

@@ -26,11 +26,44 @@ var versionTypedVars = map[string]struct{}{
 // set of active extras bound to the `extra` variable. Pass nil extras for a
 // metadata-context evaluation with no active extras.
 func (m Marker) Evaluate(env Environment, extraList []string) bool {
+	result, _ := m.EvaluateUndecidable(env, extraList)
+	return result
+}
+
+// EvaluateUndecidable reports whether the marker is satisfied in env, and which
+// of its comparisons could not be decided. When the returned slice is empty the
+// bool is authoritative.
+//
+// Two things make a comparison undecidable, and neither is recoverable from
+// Evaluate's bare bool:
+//
+//   - An environment variable resolves to "". EnvironmentFromTarget legitimately
+//     cannot know PlatformRelease or PlatformVersion for a DECLARED target and
+//     leaves them empty, and comparing "" against anything is meaningless.
+//
+//   - `~=` or `===` reaches the generic string-operator table, which has no
+//     semantics for them (evalStringOp returns false; packaging raises
+//     UndefinedComparison). Note this cannot happen when either operand is
+//     version-typed and both sides parse as PEP 440 versions, because the
+//     specifier path answers first and answers correctly.
+//
+// Ordered comparisons on string operands are NOT undecidable: `<` and `>`
+// returning false and `<=`/`>=` collapsing to equality is faithful to
+// pypa/packaging, whose operator table is the same.
+//
+// A caller that must not discard a dependency edge -- building a mirror, an
+// offline bundle -- should treat a non-empty slice as "include regardless of the
+// bool". Short-circuiting means only the comparisons actually reached are
+// reported, which is what the caller wants: an `and` that was decided false by
+// its first operand needs no further explanation.
+func (m Marker) EvaluateUndecidable(env Environment, extraList []string) (bool, []Undecidable) {
 	if m.ast == nil {
-		return true
+		return true, nil
 	}
 	active := normalizeExtraSet(extraList)
-	return evalExpr(m.ast, env, active)
+	var und []Undecidable
+	result := evalExpr(m.ast, env, active, &und)
+	return result, und
 }
 
 // normalizeExtraSet normalizes each active extra name (extras.Normalize) at
@@ -48,30 +81,30 @@ func normalizeExtraSet(extraList []string) map[string]struct{} {
 
 // evalExpr walks the marker AST: a *pep508.BoolExpr short-circuits its
 // "and"/"or" operands, a *pep508.CompareExpr evaluates a single comparison.
-func evalExpr(e pep508.Expr, env Environment, active map[string]struct{}) bool {
+func evalExpr(e pep508.Expr, env Environment, active map[string]struct{}, und *[]Undecidable) bool {
 	switch n := e.(type) {
 	case *pep508.BoolExpr:
-		return evalBool(n, env, active)
+		return evalBool(n, env, active, und)
 	case *pep508.CompareExpr:
-		return evalCompare(n, env, active)
+		return evalCompare(n, env, active, und)
 	default:
 		// Unreachable: pep508.Expr has exactly these two implementations.
 		return false
 	}
 }
 
-func evalBool(n *pep508.BoolExpr, env Environment, active map[string]struct{}) bool {
+func evalBool(n *pep508.BoolExpr, env Environment, active map[string]struct{}, und *[]Undecidable) bool {
 	switch n.Op {
 	case pep508.And:
 		for _, operand := range n.Operands {
-			if !evalExpr(operand, env, active) {
+			if !evalExpr(operand, env, active, und) {
 				return false
 			}
 		}
 		return true
 	case pep508.Or:
 		for _, operand := range n.Operands {
-			if evalExpr(operand, env, active) {
+			if evalExpr(operand, env, active, und) {
 				return true
 			}
 		}
@@ -87,7 +120,7 @@ func evalBool(n *pep508.BoolExpr, env Environment, active map[string]struct{}) b
 // one of the four version-typed variables (rule 1), falling back on any
 // failure - or immediately, for any other variable - to a single generic
 // string-operator table (rule 2).
-func evalCompare(n *pep508.CompareExpr, env Environment, active map[string]struct{}) bool {
+func evalCompare(n *pep508.CompareExpr, env Environment, active map[string]struct{}, und *[]Undecidable) bool {
 	if isExtraVar(n.Lhs) {
 		return evalExtra(n.Op, n.Rhs, env, active)
 	}
@@ -98,14 +131,45 @@ func evalCompare(n *pep508.CompareExpr, env Environment, active map[string]struc
 	lhsVal := resolveOperand(n.Lhs, env)
 	rhsVal := resolveOperand(n.Rhs, env)
 
+	// An environment variable with no value cannot be compared meaningfully. This
+	// is reported rather than silently answered, because the empty value is often
+	// the caller's own gap (a declared target has no kernel version to supply)
+	// rather than a property of the environment.
+	noteEmptyEnvVar(n, n.Lhs, lhsVal, und)
+	noteEmptyEnvVar(n, n.Rhs, rhsVal, und)
+
 	if isVersionTypedOperand(n.Lhs) || isVersionTypedOperand(n.Rhs) {
 		if result, ok := tryVersionCompare(n.Op, lhsVal, rhsVal); ok {
 			return result
 		}
 	}
+	if n.Op == "~=" || n.Op == "===" {
+		note(und, Undecidable{
+			Expr:   n.String(),
+			Reason: "operator has no string semantics",
+		})
+	}
 	return evalStringOp(n.Op, lhsVal, rhsVal)
 }
 
+// noteEmptyEnvVar records an undecidable when operand names an environment
+// variable that resolved to the empty string. A literal "" is not reported: the
+// marker author wrote it deliberately.
+func noteEmptyEnvVar(n *pep508.CompareExpr, operand pep508.Operand, value string, und *[]Undecidable) {
+	if value != "" {
+		return
+	}
+	v, ok := operand.(pep508.EnvVar)
+	if !ok {
+		return
+	}
+	note(und, Undecidable{
+		Expr:   n.String(),
+		Reason: "environment variable is empty",
+		Var:    v.Name,
+	})
+}
+
 // evalExtra evaluates a comparison where one side is the `extra` variable
 // and other is the opposite operand (the value side, whichever operand
 // order the marker was written in). Only == and != are meaningful for

marker/undecidable.go

@@ -0,0 +1,74 @@
+// SPDX-License-Identifier: Apache-2.0 OR MIT
+
+package marker
+
+import (
+	"github.com/posit-dev/go-python-packaging/internal/pep508"
+)
+
+// Undecidable names one comparison that Evaluate answered without being able to
+// decide it, and why. See Marker.EvaluateUndecidable.
+type Undecidable struct {
+	// Expr is the comparison as rendered by the marker's own String(), so it is
+	// canonical rather than the caller's original spelling.
+	Expr string
+	// Reason is a short, stable description suitable for a log line.
+	Reason string
+	// Var is the environment variable involved, when the reason concerns one.
+	// Empty otherwise.
+	Var string
+}
+
+// note appends u to *und, allocating on first use. A nil und disables
+// collection, which is what Evaluate's bare-bool path relies on being cheap.
+func note(und *[]Undecidable, u Undecidable) {
+	if und == nil {
+		return
+	}
+	*und = append(*und, u)
+}
+
+// Variables returns the distinct environment variables this marker references,
+// in first-appearance order. An empty marker returns nil.
+//
+// This exists for callers that construct an Environment with fields they had to
+// invent. A declared target names an interpreter as "3.13", so
+// PythonFullVersion must be given some patch level; the value is then decidable
+// but arbitrary, and `python_full_version >= "3.13.2"` answers false at an
+// invented 3.13.0 and true at an invented 3.13.99. EvaluateUndecidable cannot
+// help there -- nothing is empty and no operator is undefined -- because only
+// the caller knows which fields it fabricated. Variables lets it ask whether a
+// marker depends on one of them, without pattern-matching String().
+func (m Marker) Variables() []string {
+	if m.ast == nil {
+		return nil
+	}
+	seen := make(map[string]struct{})
+	var out []string
+	collectVars(m.ast, seen, &out)
+	return out
+}
+
+func collectVars(e pep508.Expr, seen map[string]struct{}, out *[]string) {
+	switch n := e.(type) {
+	case *pep508.BoolExpr:
+		for _, operand := range n.Operands {
+			collectVars(operand, seen, out)
+		}
+	case *pep508.CompareExpr:
+		collectVarOperand(n.Lhs, seen, out)
+		collectVarOperand(n.Rhs, seen, out)
+	}
+}
+
+func collectVarOperand(operand pep508.Operand, seen map[string]struct{}, out *[]string) {
+	v, ok := operand.(pep508.EnvVar)
+	if !ok {
+		return
+	}
+	if _, dup := seen[v.Name]; dup {
+		return
+	}
+	seen[v.Name] = struct{}{}
+	*out = append(*out, v.Name)
+}