Skip to content

[flake8-simplify] Preserve operand order in SIM109 fix - #27824

Merged
ntBre merged 3 commits into
astral-sh:mainfrom
alzeph:fix-sim109-order-preservation
Sep 15, 2026
Merged

ntBre merged 3 commits into
astral-sh:mainfrom
alzeph:fix-sim109-order-preservation

Conversation

@alzeph

@alzeph alzeph commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #18945.

compare-with-tuple (SIM109) merges repeated equality checks against the same target into a single in comparison, e.g. a == b or a == c becomes a in (b, c). When the boolean expression also contains unrelated operands, the fix always placed the merged in comparison first and appended the unmatched operands after it, regardless of where they originally appeared. That reorders the expression and can change its value under short-circuit evaluation:

x = None
y = "y"
z = "z"
w = "w"
print(x or y == z or y == w)  # False

was fixed to:

print(y in (z, w) or x)  # None

Fix

The merged in comparison is now anchored to the position of its earliest matched comparator, and sorted together with the unmatched operands by position instead of always being placed first. This preserves the original order for operands that appear before or after the group of merged comparisons.

There's one case this doesn't fully solve: if an unmatched operand sits between two merged comparisons (e.g. a == b or None or a == c), there's no reordering that keeps both the merged comparison contiguous and every operand in its original position, so it's still moved after the merged comparison. I've documented this as a known limitation in the rule's ## Fix safety section, along with a regression test that pins down the current (documented) behavior.

This follows the approach @ntBre sketched in review on the earlier attempt at this fix (#19753, closed for inactivity).

Test plan

  • Extended SIM109.py with a regression case for the exact scenario from the issue (unmatched operand before the merged comparisons) and kept/annotated the existing case that demonstrates the documented limitation (unmatched operand between merged comparisons).
  • Reviewed the updated snapshot diff.
  • Ran the flake8_simplify test suite, clippy, and cargo dev generate-all (doc comment change).

The compare-with-tuple fix always placed the merged `in` comparison
first, so an unmatched operand appearing before the merged comparisons
in the original expression ended up after them instead, changing the
expression's value under short-circuit evaluation (e.g. `x or y == z
or y == w` became `y in (z, w) or x`).

The merged comparison is now anchored to the position of its earliest
matched comparator and sorted together with the unmatched operands, so
the original order is preserved except when an unmatched operand sits
between two merged comparisons, which is now documented as a known
limitation of the fix.

Fixes astral-sh#18945
@astral-sh-bot
astral-sh-bot Bot requested a review from ntBre August 18, 2026 08:48
@ntBre ntBre added bug Something isn't working fixes Related to suggested fixes for violations labels Aug 18, 2026

@ntBre ntBre left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! And sorry for the delay. I resolved the merge conflicts and then applied a very minor simplification with itertools, but this looked good to me!

@ntBre ntBre changed the title [flake8-simplify] Preserve operand order in SIM109 fix [flake8-simplify] Preserve operand order in SIM109 fix Sep 15, 2026
@ntBre
ntBre enabled auto-merge (squash) September 15, 2026 19:57
@ntBre
ntBre merged commit 42d9f30 into astral-sh:main Sep 15, 2026
49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working fixes Related to suggested fixes for violations

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[flake8-simplify] SIM109 fix reorders statements incorrectly

2 participants