From 704d2c39a242257071446db80fe01fde31a15cfb Mon Sep 17 00:00:00 2001 From: Jonathan Yoder Date: Thu, 27 Aug 2026 11:53:25 -0400 Subject: [PATCH] marker: report which comparisons could not be decided (#44) Evaluate returns a bare bool, so a caller cannot tell "this marker is false" from "this marker could not be decided". Two things produce the second: `~=` and `===` reaching the generic string-operator table, which has no semantics for them and where packaging raises UndefinedComparison; and an environment variable resolving to "", which EnvironmentFromTarget legitimately does for platform_release and platform_version because a DECLARED target has no kernel to report. EvaluateUndecidable returns both the answer and the comparisons behind it that were undecidable. A consumer that must not discard a dependency edge -- a mirror, an offline bundle, where a dropped edge means a missing package and no fallback -- can then include on a non-empty slice instead of scanning Marker.String() for operators and variable names, which has false positives on quoted literals and a token list to hand-maintain. 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 and true at 3.13.99. Only the caller knows which fields it fabricated, so it just needs to ask which variables a marker touches. TestInventedPatchLevelIsDecidableButArbitrary pins that distinction. Ordered comparisons on string operands are deliberately NOT undecidable. `<`/`>` returning false and `<=`/`>=` collapsing to equality is faithful to pypa/packaging, whose _operators table is literally the same and where `sys_platform >= "darwin"` evaluates False. Verified against 26.3, and asserted here so a future reader does not "fix" it. Short-circuiting limits reporting to the comparisons actually reached, which is the behaviour a caller wants: an `and` already decided false by its first operand needs no further explanation. Evaluate becomes a wrapper; its behaviour is unchanged and the existing tests pin that. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 31 +++++++ marker/evaluate.go | 82 +++++++++++++++-- marker/undecidable.go | 74 +++++++++++++++ marker/undecidable_test.go | 184 +++++++++++++++++++++++++++++++++++++ 4 files changed, 362 insertions(+), 9 deletions(-) create mode 100644 marker/undecidable.go create mode 100644 marker/undecidable_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 9ea3c3a..1719885 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/marker/evaluate.go b/marker/evaluate.go index 63f1830..e8dc46d 100644 --- a/marker/evaluate.go +++ b/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 diff --git a/marker/undecidable.go b/marker/undecidable.go new file mode 100644 index 0000000..dd83cac --- /dev/null +++ b/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) +} diff --git a/marker/undecidable_test.go b/marker/undecidable_test.go new file mode 100644 index 0000000..82d1a37 --- /dev/null +++ b/marker/undecidable_test.go @@ -0,0 +1,184 @@ +// SPDX-License-Identifier: Apache-2.0 OR MIT +package marker + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// linuxEnv is a declared-target environment: everything a wheel-compatibility +// target can supply, with PlatformRelease and PlatformVersion empty exactly as +// EnvironmentFromTarget leaves them, since a declared target has no kernel. +func linuxEnv() Environment { + return Environment{ + OsName: "posix", + SysPlatform: "linux", + PlatformSystem: "Linux", + PlatformMachine: "x86_64", + PlatformRelease: "", + PlatformVersion: "", + PythonVersion: "3.13", + PythonFullVersion: "3.13.0", + ImplementationName: "cpython", + PlatformPythonImplementation: "CPython", + ImplementationVersion: "3.13.0", + } +} + +func TestEvaluateUndecidable_Decidable(t *testing.T) { + for _, src := range []string{ + `sys_platform == "linux"`, + `sys_platform != "win32"`, + `python_version >= "3.11"`, + `python_full_version < "3.14"`, + // Ordered comparison on string operands: false, but FAITHFUL to + // pypa/packaging, whose operator table returns False for < and > and + // equality for <= and >=. Must not be reported as undecidable. + `sys_platform >= "darwin"`, + `platform_system > "Darwin"`, + // A version-typed operand with a parseable version answers via the + // specifier path, so even ~= is decidable here. + `python_version ~= "3.13"`, + `python_full_version ~= "3.13.0"`, + } { + t.Run(src, func(t *testing.T) { + m, err := Parse(src) + require.NoError(t, err) + _, und := m.EvaluateUndecidable(linuxEnv(), nil) + assert.Empty(t, und, "%s must be decidable", src) + }) + } +} + +func TestEvaluateUndecidable_EmptyEnvVar(t *testing.T) { + // The case that motivates the API: a declared target cannot know the kernel + // version, so this comparison is meaningless and must not read as a plain + // false. + m, err := Parse(`platform_release >= "5.4"`) + require.NoError(t, err) + + result, und := m.EvaluateUndecidable(linuxEnv(), nil) + assert.False(t, result, "the bare answer is still false") + require.Len(t, und, 1) + assert.Equal(t, "platform_release", und[0].Var) + assert.Equal(t, "environment variable is empty", und[0].Reason) + assert.Contains(t, und[0].Expr, "platform_release") + + // Evaluate must be unchanged by the collection path. + assert.False(t, m.Evaluate(linuxEnv(), nil)) +} + +func TestEvaluateUndecidable_UndefinedOperator(t *testing.T) { + // ~= and === have no string semantics; packaging raises UndefinedComparison. + // Reaching evalStringOp with them is the divergence this reports. + for _, src := range []string{ + `sys_platform ~= "linux"`, + `sys_platform === "linux"`, + } { + t.Run(src, func(t *testing.T) { + m, err := Parse(src) + require.NoError(t, err) + result, und := m.EvaluateUndecidable(linuxEnv(), nil) + assert.False(t, result) + require.NotEmpty(t, und) + assert.Equal(t, "operator has no string semantics", und[0].Reason) + }) + } +} + +func TestEvaluateUndecidable_ALiteralEmptyStringIsNotReported(t *testing.T) { + // The marker author wrote "" deliberately; only an empty ENVIRONMENT value + // is the caller's gap. + m, err := Parse(`sys_platform == ""`) + require.NoError(t, err) + result, und := m.EvaluateUndecidable(linuxEnv(), nil) + assert.False(t, result) + assert.Empty(t, und) +} + +func TestEvaluateUndecidable_ShortCircuitLimitsReporting(t *testing.T) { + // An `and` decided false by its first operand needs no further explanation, + // so the undecidable second operand is never reached. This keeps the caller + // from over-including on markers whose answer was already determined. + m, err := Parse(`sys_platform == "win32" and platform_release >= "5.4"`) + require.NoError(t, err) + result, und := m.EvaluateUndecidable(linuxEnv(), nil) + assert.False(t, result) + assert.Empty(t, und, "the undecidable operand is unreachable") + + // Reversed, the undecidable operand is reached and reported. + m2, err := Parse(`platform_release >= "5.4" and sys_platform == "win32"`) + require.NoError(t, err) + _, und2 := m2.EvaluateUndecidable(linuxEnv(), nil) + assert.Len(t, und2, 1) +} + +func TestEvaluateUndecidable_EmptyMarker(t *testing.T) { + var m Marker + result, und := m.EvaluateUndecidable(linuxEnv(), nil) + assert.True(t, result) + assert.Empty(t, und) +} + +func TestVariables(t *testing.T) { + for _, tc := range []struct { + src string + want []string + }{ + {`sys_platform == "linux"`, []string{"sys_platform"}}, + {`extra == "async"`, []string{"extra"}}, + { + `python_version >= "3.11" and sys_platform == "linux"`, + []string{"python_version", "sys_platform"}, + }, + { + // First-appearance order, deduplicated. + `sys_platform == "linux" or (sys_platform == "darwin" and python_version > "3.9")`, + []string{"sys_platform", "python_version"}, + }, + { + // The motivating case: the caller invented PythonFullVersion and needs + // to know this marker depends on it. + `python_full_version >= "3.13.2"`, + []string{"python_full_version"}, + }, + } { + t.Run(tc.src, func(t *testing.T) { + m, err := Parse(tc.src) + require.NoError(t, err) + assert.Equal(t, tc.want, m.Variables()) + }) + } +} + +func TestVariables_EmptyMarker(t *testing.T) { + var m Marker + assert.Nil(t, m.Variables()) +} + +// TestInventedPatchLevelIsDecidableButArbitrary is the reason Variables exists +// alongside EvaluateUndecidable. Nothing here is empty and no operator is +// undefined, so the comparison is decidable -- and the answer is whatever patch +// level the caller invented. Only Variables surfaces the dependency. +func TestInventedPatchLevelIsDecidableButArbitrary(t *testing.T) { + m, err := Parse(`python_full_version >= "3.13.2"`) + require.NoError(t, err) + + low := linuxEnv() + low.PythonFullVersion = "3.13.0" + resLow, undLow := m.EvaluateUndecidable(low, nil) + + high := linuxEnv() + high.PythonFullVersion = "3.13.99" + resHigh, undHigh := m.EvaluateUndecidable(high, nil) + + assert.False(t, resLow) + assert.True(t, resHigh, "the answer flips on a value the caller invented") + assert.Empty(t, undLow, "and neither evaluation is undecidable") + assert.Empty(t, undHigh) + + assert.Contains(t, m.Variables(), "python_full_version", + "Variables is the only signal a caller has here") +}