Markers redux by dimbleby · Pull Request #939 · python-poetry/poetry-core

dimbleby · 2026-05-03T23:09:24Z

Revisiting #645 (comment). Best reviewed commit by commit

the first two are some preparatory pedantry noting that a VersionRange might be empty
- at some point in the drafting of this pull request this case was actually hit
- seem fairly clearly correct, even if we have managed without this logic until now
next is the main event: when we see a version constraint <V, PEP440 rules re pre-releases imply that really what this means is <V.dev0
next fix the issue noted back at Fix for handling of version ranges #645

(>1).union(!=2) ... becomes >1,<2 || >2. ... that's correct as an internal representation, but becomes wrong once re-parsed - it really does want to be written eg in lockfiles as >1,!=2

finally opt out of this when parsing python versions. Also as mentioned back in Fix for handling of version ranges #645:

there's a distinction between version ranges as used in version requirements, and versions as used in markers / python versions. So as not to break the world it's probably unwise to make changes for the latter two

sourcery-ai

Hey - I've found 2 issues, and left some high level feedback:

There are now two separate helpers around the <V dev0 semantics (_canonical_strict_max and _display_max_text / _is_canonical_strict_max); consider consolidating or co-locating the canonicalization/display rules so they can’t silently drift apart over time.
The _range_or_empty helper is only wired into some VersionRange arithmetic paths (e.g. parts of intersect and difference); it’s worth double-checking whether other operations or constructors that can synthesize inverted/coincident bounds should also be routed through it to avoid leaving behind non-normalized empty ranges.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- There are now two separate helpers around the `<V` dev0 semantics (`_canonical_strict_max` and `_display_max_text` / `_is_canonical_strict_max`); consider consolidating or co-locating the canonicalization/display rules so they can’t silently drift apart over time.
- The `_range_or_empty` helper is only wired into some `VersionRange` arithmetic paths (e.g. parts of `intersect` and `difference`); it’s worth double-checking whether other operations or constructors that can synthesize inverted/coincident bounds should also be routed through it to avoid leaving behind non-normalized empty ranges.

## Individual Comments

### Comment 1
<location path="src/poetry/core/constraints/version/version_union.py" line_range="355-357" />
<code_context>
+        # constructor doesn't enforce that, so sort defensively.
+        ranges = sorted(self._ranges)  # type: ignore[type-var]
+
+        groups: list[list[VersionRangeConstraint]] = [[ranges[0]]]
+        for prev, cur in pairwise(ranges):
+            if prev.max is not None and prev.max == cur.min:
+                groups[-1].append(cur)
+            else:
</code_context>
<issue_to_address>
**issue (bug_risk):** Seam detection for punctured ranges ignores inclusivity, which can change semantics (e.g. `<=2 || >2` becomes `!=2`).

The grouping in `_punctured_range_string` currently collapses any adjacent ranges where `prev.max is not None and prev.max == cur.min`, so cases like `<=2 || >2` or `<2 || >=2` are treated as a single punctured range. That makes `_render_punctured_range` emit `!=2`, which wrongly excludes 2 even though the union should include it. To preserve the intended “single-point exclusion” behavior, the grouping condition should also check `not prev.include_max and not cur.include_min`, so only `<V` followed by `>V` is collapsed into a `!=V` puncture.
</issue_to_address>

### Comment 2
<location path="tests/test_factory.py" line_range="1280" />
<code_context>
     dep = Factory.create_dependency("foo", constraint)
     assert dep.python_versions == exp_python
-    assert dep.python_constraint == parse_constraint(exp_python)
+    assert dep.python_constraint == parse_marker_version_constraint(exp_python)
     assert str(dep.marker) == exp_marker

</code_context>
<issue_to_address>
**issue (testing):** Missing targeted tests for `parse_marker_version_constraint` and marker-specific `<V` handling

Switching to `parse_marker_version_constraint` changes behaviour for Python markers, but the tests don’t currently distinguish marker parsing from standard `parse_constraint`. Please add tests that:

* parse a marker constraint like `"<3.8"` via `parse_marker_version_constraint` and assert it renders exactly as `"<3.8"`, and
* confirm that a non-marker constraint with the same text still uses the stricter PEP 440 semantics (i.e. effectively `<3.8.dev0`).

This will lock in the intended non-canonicalizing behaviour for markers and guard against regressions.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

radoering

LGTM. Just some minor things. (I can change them myself; I would just like to make sure beforehand that I have not overlooked anything.)

dimbleby · 2026-06-06T12:05:12Z

@radoering I pushed a couple of commits: one with simplifications per your suggestion and one with a small simplification / caching win of my own.

I see that you have a couple of conversations with the bot where I am not sure whether you are looking to make further changes or not: either way is fine with me.

…ython-poetry#939) A range with coincident min and max is only inhabited when both bounds are inclusive (the single-point range [V, V]). Any of [V, V), (V, V], (V, V) is the empty set. Previously is_empty unconditionally returned False, so callers that constructed (or computed) such a degenerate range would treat it as a non-empty constraint -- silently producing nonsense in operations that rely on emptiness as a base case. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…nt (python-poetry#939) `VersionRange.__init__` cannot return a different class. Now that construction canonicalizes a strict upper bound `<V` to `max=V.dev0`, arithmetic on otherwise-non-empty inputs can produce a `VersionRange` that is empty (e.g. `VersionRange(V, V, True, False)` becomes `[V, V.dev0)`). The arithmetic methods only emitted `EmptyConstraint` from explicit guards on the *inputs* — so a result that becomes empty only after canonicalization leaked out as an `is_empty()` `VersionRange`. Downstream consumers (poetry's solver) then take the non-Empty type branch and silently misbehave. Add a small `_range_or_empty` factory that returns `EmptyConstraint()` if the constructed range is empty, and use it at the tail of every `VersionRange` arithmetic site whose result can be emptied by canonicalization (intersect of ranges, intersect with a `Version` for a local-bearing min, the three difference-by-Version arms, and both arms of difference-by-range). Also fix the `intersect` early-out: `if other.is_empty(): return other` returned the empty `VersionRange` as-is. Return an `EmptyConstraint` so the result type is the canonical empty representation regardless of how the empty input was spelt. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

PEP 440 specifies that the exclusive ordered comparison <V (with V a stable release) MUST NOT allow any pre-release of V. Prior to this change, the parser stored <V with max=V (raw), and a separate allowed_max property re-derived V.dev0 at every comparison site -- which was easy to bypass and produced incorrect results in the few remaining call sites that used max directly (e.g. VersionRange.allows for a pre-release with the same release tuple as max). Push the canonicalization to the parser instead: tilde, PEP-440 tilde, caret and basic <V all run through a small _canonical_strict_max helper that turns <V into <V.dev0 when V is stable. Internal range arithmetic preserves whatever max its inputs carry, so an interior split such as (>1, <3) - {2} still produces (1, 2) with raw max=2 -- correctly allowing 2.dev0 in that fragment. Marker constraints (python_version, etc.) opt out via is_marker_constraint=True: marker contexts only see concrete release strings, so the canonicalization would be a semantic no-op and would break field-equality with marker construction code. !=V continues to parse to raw <V || >V. PEP 440 != is strict equality (2.0.dev1 != 2 is True), so the upper piece must remain raw. The standard excludes_single_version recogniser then naturally renders this back as !=V because _inverted on the raw shape is a single Version. VersionRange.__str__ uses a small _display_max_text helper to render canonical <V.dev0 back as <V so user-typed input round-trips. allowed_max is removed; self.max is now used directly at the three former call sites in VersionRangeConstraint. Tests: - New behavioural tests in test_version_range.py prove the fix end-to-end via allows(): <V rejects V/V.dev0/Va1; (1,3)-{2} allows 2.dev0; !=2 allows 2.dev0 and 2.post1. - test_parse_constraint expectations updated to .first_devrelease() for the strict-max bound where the test compares parsed output against a hand-constructed expected range. - test_dependency_string_representation no longer asserts the incorrect collapse <V || >V -> !=V (those are different sets when the <V is parser-canonical). - test_dependency_from_pep_508_with_constraint reflects display canonicalization: <2.17.dev0 renders as <2.17. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…ons (python-poetry#939) After parser-only canonicalization of `<V`, an interior split such as `>=1,<10` minus `{2}` produces a union of two raw ranges `>=1,<2 || >2,<10` whose seam at 2 excludes only the single point V=2 (both bounds raw exclusive). Re-parsing the naive `||` rendering re-canonicalizes the inner `<2` to `<2.dev0`, silently shrinking the allowed set -- breaking lockfile round-trips (cf. PR python-poetry#645 discussion). Detect single-point puncture seams (`ranges[i].max == ranges[i+1].min`) and group consecutive punctures into a single `>=A,!=V1,!=V2,<B` rendering. Mixed unions (some seams puncture, others gap) partition naturally into groups joined by `||`. This also subsumes the previous `<V || >V` -> `!=V` collapse, which is itself just the singleton-group case. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

`python_versions` (on Dependency, Package, ProjectPackage) is implemented as a PEP 508 `python_version` marker. Marker comparisons treat versions as concrete environment values rather than as PEP 440 ordered comparisons, so the canonicalization of `<V` -> `<V.dev0` introduced for *requirement* parsing must not apply here -- otherwise e.g. `python = "<3.7"` no longer admits a 3.7.dev environment, with follow-on solver fallout (cf. python-poetry#645). Route every `python_versions` parse through `parse_marker_version_constraint`, which already exists for exactly this purpose (and which is also why marker parsing has its own `pep440=False` switch for non-PEP-440 fields like `platform_release`). Update one factory test that was asserting parse-equality with the canonical (requirement) parser. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…939) A range with coincident min and max is only inhabited when both bounds are inclusive (the single-point range [V, V]). Any of [V, V), (V, V], (V, V) is the empty set. Previously is_empty unconditionally returned False, so callers that constructed (or computed) such a degenerate range would treat it as a non-empty constraint -- silently producing nonsense in operations that rely on emptiness as a base case. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…nt (#939) `VersionRange.__init__` cannot return a different class. Now that construction canonicalizes a strict upper bound `<V` to `max=V.dev0`, arithmetic on otherwise-non-empty inputs can produce a `VersionRange` that is empty (e.g. `VersionRange(V, V, True, False)` becomes `[V, V.dev0)`). The arithmetic methods only emitted `EmptyConstraint` from explicit guards on the *inputs* — so a result that becomes empty only after canonicalization leaked out as an `is_empty()` `VersionRange`. Downstream consumers (poetry's solver) then take the non-Empty type branch and silently misbehave. Add a small `_range_or_empty` factory that returns `EmptyConstraint()` if the constructed range is empty, and use it at the tail of every `VersionRange` arithmetic site whose result can be emptied by canonicalization (intersect of ranges, intersect with a `Version` for a local-bearing min, the three difference-by-Version arms, and both arms of difference-by-range). Also fix the `intersect` early-out: `if other.is_empty(): return other` returned the empty `VersionRange` as-is. Return an `EmptyConstraint` so the result type is the canonical empty representation regardless of how the empty input was spelt. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

PEP 440 specifies that the exclusive ordered comparison <V (with V a stable release) MUST NOT allow any pre-release of V. Prior to this change, the parser stored <V with max=V (raw), and a separate allowed_max property re-derived V.dev0 at every comparison site -- which was easy to bypass and produced incorrect results in the few remaining call sites that used max directly (e.g. VersionRange.allows for a pre-release with the same release tuple as max). Push the canonicalization to the parser instead: tilde, PEP-440 tilde, caret and basic <V all run through a small _canonical_strict_max helper that turns <V into <V.dev0 when V is stable. Internal range arithmetic preserves whatever max its inputs carry, so an interior split such as (>1, <3) - {2} still produces (1, 2) with raw max=2 -- correctly allowing 2.dev0 in that fragment. Marker constraints (python_version, etc.) opt out via is_marker_constraint=True: marker contexts only see concrete release strings, so the canonicalization would be a semantic no-op and would break field-equality with marker construction code. !=V continues to parse to raw <V || >V. PEP 440 != is strict equality (2.0.dev1 != 2 is True), so the upper piece must remain raw. The standard excludes_single_version recogniser then naturally renders this back as !=V because _inverted on the raw shape is a single Version. VersionRange.__str__ uses a small _display_max_text helper to render canonical <V.dev0 back as <V so user-typed input round-trips. allowed_max is removed; self.max is now used directly at the three former call sites in VersionRangeConstraint. Tests: - New behavioural tests in test_version_range.py prove the fix end-to-end via allows(): <V rejects V/V.dev0/Va1; (1,3)-{2} allows 2.dev0; !=2 allows 2.dev0 and 2.post1. - test_parse_constraint expectations updated to .first_devrelease() for the strict-max bound where the test compares parsed output against a hand-constructed expected range. - test_dependency_string_representation no longer asserts the incorrect collapse <V || >V -> !=V (those are different sets when the <V is parser-canonical). - test_dependency_from_pep_508_with_constraint reflects display canonicalization: <2.17.dev0 renders as <2.17. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…ons (#939) After parser-only canonicalization of `<V`, an interior split such as `>=1,<10` minus `{2}` produces a union of two raw ranges `>=1,<2 || >2,<10` whose seam at 2 excludes only the single point V=2 (both bounds raw exclusive). Re-parsing the naive `||` rendering re-canonicalizes the inner `<2` to `<2.dev0`, silently shrinking the allowed set -- breaking lockfile round-trips (cf. PR #645 discussion). Detect single-point puncture seams (`ranges[i].max == ranges[i+1].min`) and group consecutive punctures into a single `>=A,!=V1,!=V2,<B` rendering. Mixed unions (some seams puncture, others gap) partition naturally into groups joined by `||`. This also subsumes the previous `<V || >V` -> `!=V` collapse, which is itself just the singleton-group case. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

dimbleby mentioned this pull request May 3, 2026

patch up test scripts for poetry-core#939 python-poetry/poetry#10881

Merged

sourcery-ai Bot reviewed May 3, 2026

View reviewed changes

Comment thread src/poetry/core/constraints/version/version_union.py Outdated

Comment thread tests/test_factory.py

radoering reviewed Jun 6, 2026

View reviewed changes

Comment thread src/poetry/core/constraints/version/parser.py Outdated

Comment thread src/poetry/core/constraints/version/parser.py Outdated

Comment thread tests/test_factory.py

Comment thread src/poetry/core/constraints/version/version_union.py Outdated

dimbleby and others added 5 commits June 6, 2026 18:14

radoering force-pushed the markers-redux branch from a9df6e0 to 1f1a633 Compare June 6, 2026 16:15

radoering approved these changes Jun 6, 2026

View reviewed changes

radoering merged commit e3998b0 into python-poetry:main Jun 6, 2026
18 of 19 checks passed

This was referenced Jun 6, 2026

Fix for handling of version ranges #645

Closed

poetry never resolves package combination: google-cloud-aiplatform kfp python-poetry/poetry#8475

Closed

dimbleby deleted the markers-redux branch June 6, 2026 16:31

radoering mentioned this pull request Jun 6, 2026

assert term is not None in poetry lock/update python-poetry/poetry#8405

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Markers redux#939

Markers redux#939
radoering merged 5 commits into
python-poetry:mainfrom
dimbleby:markers-redux

dimbleby commented May 3, 2026

Uh oh!

sourcery-ai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

radoering left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dimbleby commented Jun 6, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

dimbleby commented May 3, 2026

Uh oh!

sourcery-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

radoering left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dimbleby commented Jun 6, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants