[pylint] Flag chained associative operators + Explicitly account for unary ops (non-augmented-assignment, PLR6104) - #27188
Open
Avasam wants to merge 2 commits into
Conversation
Only two-operand assignments were matched, so `x = x * 2 * y` and `x = 2 * 3 * x` were missed. Python parses `a * b * c` as `(a * b) * c`, so the target is found by walking the chain's left spine. Chains are only rewritten for operators that are both commutative and associative. `@` is associative but excluded: `x @= a @ b` builds the full matrix-matrix product first, which can cost orders of magnitude more than the pair of matrix-vector products it replaces. A multi-line operand may only have been valid because the whole assigned value was parenthesized, and those parentheses are dropped with the rest of the statement, so the fix adds its own. This also corrects existing broken output for implicitly concatenated strings. Documents three fix hazards that were previously unstated: comment loss, float regrouping, and NumPy in-place operators rejecting shape or dtype changes that the plain form allows. Closes astral-sh#11656 Relates to astral-sh#27051 Expand rule safety recommendations for astral-sh#12890 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| non-augmented-assignment | 37 | 37 | 0 | 0 | 0 |
Contributor
|
Thanks for looking into this, but I think it may be another needs-decision case. To me, expanding the rule to these more niche cases isn't really worth the complications in the rule implementation and explanation, but I'm open to other perspectives. I also wonder if this could make the problem tracked in #12890 even more common. That seems to be the case from a quick look at the new pandas ecosystem hit. |
ntBre
added
rule
Contributor
Author
|
The PR did reveal additional complexities I didn't initially think of before trying to implement. Even if it doesn't get merged, it did have exploratory value. Maybe it's worth just waiting for types to be available to Ruff? To properly separate safe vs unsafe fixes (and reducing false positives) before expanding the rule. Also open to more opinions perspectives. (I would take the changes I did, but that's just me and my usages) |
Avasam
commented
Jul 28, 2026
Avasam
commented
Jul 28, 2026
| /// [`ast::UnaryOp::Not`] is left out because it can only appear here parenthesized, as in | ||
| /// `x = (not 0) + x`; `not 0 + x` parses as `not (0 + x)` instead. The extra case isn't worth | ||
| /// supporting for a form nobody writes. | ||
| fn is_numeric_constant(expr: &Expr) -> bool { |
Contributor
Author
There was a problem hiding this comment.
Extracted to #27251 (and simplified since that doesn't care about chained operators yet)
Which would reduce the amount of new tests here.
Avasam
changed the title
pylint] Flag chained associative operators + Explicitly account for unary ops (PLR6104)pylint] Flag chained associative operators + Explicitly account for unary ops (non-augmented-assignment, PLR6104)
ntBre
pushed a commit
that referenced
this pull request
Jul 28, 2026
…nt` (`PLR6104`) (#27250) ## Summary Add missing fix safety gotchas for [non-augmented-assignment (PLR6104)](https://docs.astral.sh/ruff/rules/non-augmented-assignment/#non-augmented-assignment-plr6104) Extracted from #27188 ## Test Plan Look at generated doc
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
non-augmented-assignment(PLR6104): Can we reduce the number of unsafe recommendations from this rule? #12890 )I kept addition and multiplication chains for floating point numbers despite them not being associative due to floating point precision (
(0.1 + 0.2) + 0.3 != 0.1 + (0.2 + 0.3)). The fix is already marked as unsafe and this isn't any more unsafe than it already is for mutable types. (see #12890 ). But opinions may differ.Open question
Should I migrate the snapshot to the newer mdtest format ?
Claude did flag the following:
If you think it's worth fixing / accounting for.
Test Plan
cargo run -p ruff -- check --no-cache --select=PLR6104 --previewon a few of the projects I maintain.Coding Agent disclaimer
Code was fully written by Claude Opus 5 with a few passes of caveman-review. I did a glancing review of code changes given my limited Rust knowledge. PR description fully handwritten. Docs and tests human reviewed.