[sakshichitnis27]

Fixes python-poetry/poetry#10978

Summary

• Avoid merging same-name markers when their constraints use incompatible types.
• Add a regression case for combining a version comparison with a swapped string comparison for platform_release.

Testing

• pytest tests/version/test_markers.py
• pytest
• ruff check src/poetry/core/version/markers.py tests/version/test_markers.py
• mypy src/poetry/core/version/markers.py tests/version/test_markers.py

[sourcery-ai]

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

• The new type guard for constraints only distinguishes VersionConstraint from non-VersionConstraint; consider either documenting why this special case is sufficient or generalizing the check to ensure both constraints are of the same compatible type (e.g., isinstance(marker1.constraint, type(marker2.constraint))).
• It may be clearer to factor the constraint-compatibility check into a small helper (e.g., _can_merge_constraints(marker1.constraint, marker2.constraint)) to make the merge rules easier to reason about and extend as new constraint types are introduced.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The new type guard for constraints only distinguishes `VersionConstraint` from non-`VersionConstraint`; consider either documenting why this special case is sufficient or generalizing the check to ensure both constraints are of the same compatible type (e.g., `isinstance(marker1.constraint, type(marker2.constraint))`).
- It may be clearer to factor the constraint-compatibility check into a small helper (e.g., `_can_merge_constraints(marker1.constraint, marker2.constraint)`) to make the merge rules easier to reason about and extend as new constraint types are introduced.

## Individual Comments

### Comment 1
<location path="src/poetry/core/version/markers.py" line_range="1289-1290" />
<code_context>
     if marker1.name != marker2.name:
         return None

+    if isinstance(marker1.constraint, VersionConstraint) != isinstance(
+        marker2.constraint, VersionConstraint
+    ):
+        return None
</code_context>
<issue_to_address>
**suggestion:** Clarify the type check for VersionConstraint to avoid the non-obvious boolean comparison.

This boolean inequality works but is hard to read. Please use a more explicit check that “exactly one is a VersionConstraint”, e.g. an XOR:

```python
if isinstance(marker1.constraint, VersionConstraint) ^ isinstance(marker2.constraint, VersionConstraint):
    return None
```

or an equivalent `and`/`not` formulation to make the intent clearer for future readers and refactors.
</issue_to_address>

### Comment 2
<location path="tests/version/test_markers.py" line_range="76" />
<code_context>
         '"tegra" not in platform_release',
         '"tegra" in platform_release or "rpi-v8" in platform_release',
         '"tegra" not in platform_release and "rpi-v8" not in platform_release',
+        'platform_release != "1" and "a" not in platform_release',
         # extra starting with "in"
         'extra == "in1" or extra == "in2"',
</code_context>
<issue_to_address>
**suggestion (testing):** Add an explicit test that asserts incompatible marker constraint types are not merged, instead of only relying on this regression example in the aggregate list.

The regression is currently only covered indirectly via the large marker-expression list, while `_merge_single_markers` specifically guards against merging markers whose constraints differ in type (VersionConstraint vs non-VersionConstraint). Please add a dedicated test that builds two markers with the same name but different constraint types and asserts that merging them returns `None`/leaves them unmerged, so this behavior is verified explicitly and remains safe through future refactors of `_merge_single_markers`.

Suggested implementation:

```python
        '"tegra" not in platform_release',
        '"tegra" in platform_release or "rpi-v8" in platform_release',
        '"tegra" not in platform_release and "rpi-v8" not in platform_release',
        'platform_release != "1" and "a" not in platform_release',
        # extra starting with "in"
        'extra == "in1" or extra == "in2"',
        'extra == "in1" and extra == "in2"',


def test_merge_single_markers_does_not_merge_incompatible_constraint_types():
    """
    `_merge_single_markers` must refuse to merge markers with the same name
    but different constraint types (VersionConstraint vs non-VersionConstraint).
    """

    # `platform_release != "1"` is parsed as a version-like constraint,
    # while `"a" not in platform_release` is parsed as a generic/non-version constraint.
    version_like_marker = parse_marker('platform_release != "1"')
    generic_marker = parse_marker('"a" not in platform_release')

    # Both markers must refer to the same name to exercise the incompatible-type guard.
    # Depending on how `parse_marker()` structures the result, we extract the
    # concrete single-markers that share the "platform_release" name.
    version_single_marker = next(
        m for m in version_like_marker.markers
        if getattr(m, "name", None) == "platform_release"
    )
    generic_single_marker = next(
        m for m in generic_marker.markers
        if getattr(m, "name", None) == "platform_release"
    )

    merged = _merge_single_markers(version_single_marker, generic_single_marker)
    merged_reverse = _merge_single_markers(generic_single_marker, version_single_marker)

    assert merged is None
    assert merged_reverse is None

```

To fully integrate this test you will likely need to:

1. Ensure `parse_marker` and `_merge_single_markers` are imported at the top of `tests/version/test_markers.py`, matching existing conventions. For example, if not already present:
   - `from poetry.core.version.markers import parse_marker, _merge_single_markers`
2. Adjust attribute access and container traversal to match the actual marker representation used in this test file:
   - If `parse_marker()` returns a `Marker` with a different attribute than `.markers`, adapt the extraction of `version_single_marker` and `generic_single_marker` to whatever structure is already used in nearby tests (e.g., `.single_markers`, `.items`, or nested tuples).
   - If single markers are represented by a specific class (e.g., `SingleMarker`), and you already construct them directly in other tests, you may prefer to construct:
     `SingleMarker("platform_release", <version_constraint>)` and `SingleMarker("platform_release", <generic_constraint>)` explicitly instead of going through `parse_marker()`.
3. Confirm that the expressions `'platform_release != "1"'` and `'"a" not in platform_release'` indeed produce different underlying constraint types (VersionConstraint vs non-VersionConstraint) in your implementation:
   - If your marker implementation uses different literals to distinguish version vs non-version constraints, update the test expressions accordingly while keeping the same logical intent (same marker name, different constraint types).
4. Place the new `test_merge_single_markers_does_not_merge_incompatible_constraint_types` function at top-level scope in the file (not nested inside another test or helper) and adjust indentation so it matches the rest of the pytest tests once you integrate it into the full file context.
</issue_to_address>

---

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

• X
• Mastodon
• LinkedIn
• Facebook

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