feat: Add RUF065 rule for binary operator identity simplification
#19518
Conversation
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
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| RUF065 | 104 | 104 | 0 | 0 | 0 |
1 task
|
I haven't looked at the implementation. The rule itself seems reasonable as a Ruff rule because it is about simplification, or even catching potential bugs. What's less clear to me is if the rule should provide a fix because it's not clear to me whether |
|
I think we should provide the fixes since these are guaranteed mathematical identities (unlike other simplifications that might change behavior), but add a note in the rule description that developers should verify this was their intent. We could also consider a separate rule for catching suspicious identity operations without fixes, if that makes more sense. |
|
I think Micha's point was more that the user probably meant to write something like An unsafe or display-only fix might be a good compromise here so that users can apply the fix in an editor, for example, but we don't silently perform the fix without alerting them. I don't think documenting the issue would be enough when the fix is marked as safe. |
|
Got it. One thing I'm still trying to figure out is the cases for |
…NS` file This does unfortunately add a fair bit of complexity to the flow diagram. Ref astral-sh#19620 (comment)
## Summary I was a bit stuck on some snapshot differences I was seeing in astral-sh#19415, but @BurntSushi pointed out that `annotate-snippets` already normalizes tabs on its own, which was very helpful! Instead of applying this change directly to the other branch, I wanted to try applying it in `ruff_linter` first. This should very slightly reduce the number of changes in astral-sh#19415 proper. It looks like `annotate-snippets` always expands a tab to four spaces, whereas I think we were aligning to tab stops: ```diff 6 | spam(ham[1], { eggs: 2}) 7 | #: E201:1:6 - 8 | spam( ham[1], {eggs: 2}) - | ^^^ E201 + 8 | spam( ham[1], {eggs: 2}) + | ^^^^ E201 ``` ```diff 61 | #: E203:2:15 E702:2:16 62 | if x == 4: -63 | print(x, y) ; x, y = y, x - | ^ E203 +63 | print(x, y) ; x, y = y, x + | ^^^^ E203 ``` ```diff E27.py:15:6: E271 [*] Multiple spaces after keyword | -13 | True and False +13 | True and False 14 | #: E271 15 | a and b | ^^ E271 ``` I don't think this is too bad and has the major benefit of allowing us to pass the non-tab-expanded range to `annotate-snippets` in astral-sh#19415, where it's also displayed in the header. Ruff doesn't have this problem currently because it uses its own concise diagnostic output as the header for full diagnostics, where the pre-expansion range is used directly. ## Test Plan Existing tests with a few snapshot updates
…stral-sh#19434) ## Summary Fixes astral-sh#19433 --------- Co-authored-by: Brent Westbrook <36778786+ntBre@users.noreply.github.com>
## Summary Add support for `async for` loops and async iterables. part of astral-sh/ty#151 ## Ecosystem impact ```diff - boostedblob/listing.py:445:54: warning[unused-ignore-comment] Unused blanket `type: ignore` directive ``` This is correct. We now find a true positive in the `# type: ignore`'d code. All of the other ecosystem hits are of the type ```diff trio (https://github.com/python-trio/trio) + src/trio/_core/_tests/test_guest_mode.py:532:24: error[not-iterable] Object of type `MemorySendChannel[int] | MemoryReceiveChannel[int]` may not be iterable ``` The message is correct, because only `MemoryReceiveChannel` has an `__aiter__` method, but `MemorySendChannel` does not. What's not correct is our inferred type here. It should be `MemoryReceiveChannel[int]`, not the union of the two. This is due to missing unpacking support for tuple subclasses, which @AlexWaygood is working on. I don't think this should block merging this PR, because those wrong types are already there, without this PR. ## Test Plan New Markdown tests and snapshot tests for diagnostics.
…e it's necessary to solve a `TypeVar` (astral-sh#19635) ## Summary This PR improves our generics solver such that we are able to solve the `TypeVar` in this snippet to `int | str` (the union of the elements in the heterogeneous tuple) by upcasting the heterogeneous tuple to its pure-homogeneous-tuple supertype: ```py def f[T](x: tuple[T, ...]) -> T: return x[0] def g(x: tuple[int, str]): reveal_type(f(x)) ``` ## Test Plan Mdtests. Some TODOs remain in the mdtest regarding solving `TypeVar`s for mixed tuples, but I think this PR on its own is a significant step forward for our generics solver when it comes to tuple types. --------- Co-authored-by: Douglas Creager <dcreager@dcreager.net>
… ending with backslash (astral-sh#19505) Issue: astral-sh#19498 ## Summary [missing-required-import](https://docs.astral.sh/ruff/rules/missing-required-import/) inserts the missing import on the line immediately following the last line of the docstring. However, if the dosctring is immediately followed by a continuation token (i.e. backslash) then this leads to a syntax error because Python interprets the docstring and the inserted import to be on the same line. The proposed solution in this PR is to check if the first token after a file docstring is a continuation character, and if so, to advance an additional line before inserting the missing import. ## Test Plan Added a unit test, and the following example was verified manually: Given this simple test Python file: ```python "Hello, World!"\ print(__doc__) ``` and this ruff linting configuration in the `pyproject.toml` file: ```toml [tool.ruff.lint] select = ["I"] [tool.ruff.lint.isort] required-imports = ["import sys"] ``` Without the changes in this PR, the ruff linter would try to insert the missing import in line 2, resulting in a syntax error, and report the following: `error: Fix introduced a syntax error. Reverting all changes.` With the changes in this PR, ruff correctly advances one more line before adding the missing import, resulting in the following output: ```python "Hello, World!"\ import sys print(__doc__) ``` --------- Co-authored-by: Jim Hoekstra <jim.hoekstra@pacmed.nl>
…9645) ## Summary Resolves astral-sh/ty#862 by not emitting a diagnostic. ## Test Plan Add test to show we don't emit the diagnostic
## Summary Pulls in a bunch of salsa micro-optimizations.
Summary -- Fixes astral-sh#19640. I'm not sure these are the exact fixes we really want, but I reproduced the issue in a 32-bit Docker container and tracked down the causes, so I figured I'd open a PR. As I commented on the issue, the `goto_references` test depends on the iteration order of the files in an `FxHashSet` in `Indexed`. In this case, we can just sort the output in test code. Similarly, the tuple case depended on the order of overloads inserted in an `FxHashMap`. `FxIndexMap` seemed like a convenient drop-in replacement, but I don't know if that will have other detrimental effects. I did have to change the assertion for the tuple test, but I think it should now be stable across architectures. Test Plan -- Running the tests in the aforementioned Docker container
…stral-sh#19413) ## Summary Fixes astral-sh#18729 and fixes astral-sh#16802 ## Test Plan Manually verified via CLI that Ruff no longer enters an infinite loop by running: ```sh echo 1 | ruff --isolated check - --select I002,UP010 --fix ``` with `required-imports = ["from __future__ import generator_stop"]` set in the config, confirming “All checks passed!” and no snapshots were generated. --------- Co-authored-by: Brent Westbrook <brentrwestbrook@gmail.com>
…9672) <!-- Thank you for contributing to Ruff/ty! To help us out with reviewing, please consider the following: - Does this pull request include a summary of the change? (See below.) - Does this pull request include a descriptive title? (Please prefix with `[ty]` for ty pull requests.) - Does this pull request include references to any relevant issues? --> ## Summary <!-- What's the purpose of the change? What does it do, and why? --> Part of astral-sh#18972 This PR makes [meta-class-abc-meta (FURB180)](https://docs.astral.sh/ruff/rules/meta-class-abc-meta/#meta-class-abc-meta-furb180)'s example error out-of-the-box. [Old example](https://play.ruff.rs/6beca1be-45cd-4e5a-aafa-6a0584c10d64) ```py class C(metaclass=ABCMeta): pass ``` [New example](https://play.ruff.rs/bbad34da-bf07-44e6-9f34-53337e8f57d4) ```py import abc class C(metaclass=abc.ABCMeta): pass ``` The "Use instead" section as also modified similarly. ## Test Plan <!-- How was it tested? --> N/A, no functionality/tests affected
…sses (astral-sh#19440) ## Summary Fixes astral-sh#19437
We'll want to use these when implementing the "list modules" API.
These tests capture existing behavior. I added these when I stumbled upon what I thought was an oddity: we prioritize `foo.pyi` over `foo.py`, but prioritize `foo/__init__.py` over `foo.pyi`. (I plan to investigate this more closely in follow-up work. Particularly, to look at other type checkers. It seems like we may want to change this to always prioritize stubs.)
This makes it a little more flexible to call. For example, we might have a `StmtImport` and not a `StmtImportFrom`.
Previously, if the module was just `foo-stubs`, we'd skip over stripping the `-stubs` suffix which would lead to us returning `None`. This function is now a little convoluted and could be simpler if we did an intermediate allocation. But I kept the iterative approach and added a special case to handle `foo-stubs`.
This will make it easier to emit this info into snapshots for testing.
The actual implementation wasn't too bad. It's not long but pretty fiddly. I copied over the tests from the existing module resolver and adapted them to work with this API. Then I added a number of my own tests as well.
These tests were added as a regression check that a panic didn't occur. So we were asserting a bit more than necessary. In particular, these will soon return completions for modules, which creates large snapshots that we don't need. So modify these to just check there is sensible output that doesn't panic.
This makes `import <CURSOR>` and `from <CURSOR>` completions work. This also makes `import os.<CURSOR>` and `from os.<CURSOR>` completions work. In this case, we are careful to only offer submodule completions.
This ensures there is some level of consistency between the APIs. This did require exposing a couple more things on `Module` for good error messages. This also motivated a switch to an interned struct instead of a tracked struct. This ensures that `list_modules` and `resolve_modules` reuse the same `Module` values when the inputs are the same. Ref astral-sh#19883 (comment)
This basically splits `list_modules` into a higher level "aggregation" routine and a lower level "get modules for one search path" routine. This permits Salsa to cache the lower level components, e.g., many search paths refer to directories that rarely change. This saves us interaction with the system. This did require a fair bit of surgery in terms of being careful about adding file roots. Namely, now that we rely even more on file roots existing for correct handling of cache invalidation, there were several spots in our code that needed to be updated to add roots (that we weren't previously doing). This feels Not Great, and it would be better if we had some kind of abstraction that handled this for us. But it isn't clear to me at this time what that looks like.
Summary -- I noticed while working on astral-sh#20006 that we had a custom `unwrap` function for `Option`. This has been const on stable since 1.83 ([docs](https://doc.rust-lang.org/std/option/enum.Option.html#method.unwrap), [release notes](https://blog.rust-lang.org/2024/11/28/Rust-1.83.0/)), so I think it's safe to use now. I grepped a bit for related todos and found this one for `AsciiCharSet` but no others. Test Plan -- Existing tests
…20002) This commit corrects the type checker's behavior when handling `dataclass_transform` decorators that don't explicitly specify `field_specifiers`. According to [PEP 681 (Data Class Transforms)](https://peps.python.org/pep-0681/#dataclass-transform-parameters), when `field_specifiers` is not provided, it defaults to an empty tuple, meaning no field specifiers are supported and `dataclasses.field`/`dataclasses.Field` calls should be ignored. Fixes astral-sh/ty#980
…#19514) ## Summary Part of astral-sh#2331 ## Test Plan <!-- How was it tested? --> `cargo nextest run flake8_use_pathlib`
…en they are used (`UP010`) (astral-sh#19769) ## Summary Resolves astral-sh#19561 Fixes the [unnecessary-future-import (UP010)](https://docs.astral.sh/ruff/rules/unnecessary-future-import/) rule to correctly identify when imported __future__ modules are actually used in the code, preventing false positives. I assume there is no way to check usage in `analyze::statements`, because we don't have any usage bindings for imports. To determine unused imports, we have to fully scan the file to create bindings and then check usage, similar to [unused-import (F401)](https://docs.astral.sh/ruff/rules/unused-import/#unused-import-f401). So, `Rule::UnnecessaryFutureImport` was moved from the `analyze::statements` to the `analyze::deferred_scopes` stage. This caused the need to change the logic of future import handling to a bindings-based approach. Also, the diagnostic report was changed. Before ``` | 1 | from __future__ import nested_scopes, generators | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ UP010 ``` after ``` | 1 | from __future__ import nested_scopes, generators | ^^^^^^^^^^^^^ UP010 ``` I believe this is the correct way, because `generators` may be used, but `nested_scopes` is not. ### Special case I've found out about some specific case. ```python from __future__ import nested_scopes nested_scopes = 1 ``` Here we can treat `nested_scopes` as an unused import because the variable `nested_scopes` shadows it and we can safely remove the future import (my fix does it). But [F401](https://docs.astral.sh/ruff/rules/unused-import/#unused-import-f401) not triggered for such case ([sandbox](https://play.ruff.rs/296d9c7e-0f02-4659-b0c0-78cc21f3de76)) ``` from foo import print_function print_function = 1 ``` In my mind, `print_function` here is an unused import and should be deleted (my IDE highlight it). What do you think? ## Test Plan Added test cases and snapshots: - Split test file into separate _0 and _1 files for appropriate checks. - Added test cases to verify fixes when future module are used. --------- Co-authored-by: Igor Drokin <drokinii1017@gmail.com>
…zher/ruff into binary-operator-identity
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
41 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
This PR implements
RUF065(BinaryOperatorIdentity) that detects binary operations between a value and itself and suggests simplifications to their known identities. This rule is inspired by similar functionality in Sourcery and contributes to issue #11060.What it does
The rule identifies binary operations where both operands are identical and can be simplified to known mathematical identities:
x | x→x(bitwise OR identity)x & x→x(bitwise AND identity)x ^ x→0(bitwise XOR identity)x - x→0(subtraction identity)x / x→1(division identity)x // x→1(floor division identity)x % x→0(modulo identity)@ntBre, @MichaReiser, I wanted to draft this PR as a proof of concept of what this may look like as a custom
RUFrule implementation. Please critique and comment as you see fit! I'm sure there are many details to discuss in terms of where we want the rule to live, semantics, opinionatedness, etc. Thanks!