[ruff] Implement redundant-bool-literal (RUF038)
#14319
Conversation
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| RUF038 | 7 | 7 | 0 | 0 | 0 |
| RUF036 | 1 | 1 | 0 | 0 | 0 |
There was a problem hiding this comment.
LGTM. Thank you and I especially like the detailed fix safety documentation. @AlexWaygood would you mind signing off the rule?
MichaReiser
added
rule
|
What do you think of |
|
I've got no strong naming preference. I considered |
There was a problem hiding this comment.
Hmm, how common is this antipattern in practice? I see a few ecosystem hits here, but not enough to make me think that this is something that a lot of users are doing. Has this rule been requested by any of our userse?
For context: we've had lots of conversations over at flake8-pyi over whether we should implement more rules that detect redundant unions (PyCQA/flake8-pyi#283, PyCQA/flake8-pyi#414, etc.). Our conclusion every time has been that without very sophisticated multifile type inference, a generalised way of detecting redundant union elements is impossible, so we should only implement rules that detect common mistakes that we've actually encountered in real code.
It's a fair concern, but I would argue to give it a try and see how it turns out for users. I've searched before implementing this rule, and it's less rare than I expected (see list below). For the nested unions and literals, having the rules in place can simplify the auto fixes of other rules, since we can reduce the problem to one that we've already solved. We also should remind ourselves that the ecosystem results are published code, it could be that while developing/refactoring, such rules become more relevant. For instance saving users time when running It's good to consider that is that if you expect typed Python to be more common with the amazing tooling that is coming available (type checkers, auto typing etc.), the number of users of these features also increase, and so will their need for learning new concepts such as One final and exciting motivation is that these rules add to the diversity of test cases too, which can help testing red-knot! Some more results:
|
Absolutely! I'm sure that this rule would help some people. My concern is just that there's sort of an endless number of rules like this, regarding redundant union members, that you could plausibly add to Ruff. It's not clear where to draw the line, and they all add a certain amount of maintenance burden for us. When we adopt red-knot as the new backend, moreover, we should be able to consolidate them all into a single rule that is implemented with much less code and has much greater accuracy. So I'm not against adding these rules when they address very common user issues or when we've been explicitly asked for a rule like this. If I were dead-set against it, I wouldn't have added rules like this to flake8-pyi in the first place! But I do think the bar needs to be ~reasonably high for this kind of rule. |
There was a problem hiding this comment.
I chatted with @MichaReiser offline and he convinced me that this would be an especially helpful rule for Python beginners. I'm still a bit concerned that there'll be a lot of churn for users if we end up deprecating all these rules and replacing them with a single, generalised rule when we switch to using red-knot as the backend. But that's probably a long way off.
Just a few docs nitpicks, then I think we're good to go 👍
| /// The `bool` type has exactly two constant instances: `True` and `False`. Writing | ||
| /// `Literal[True, False]` in the context of type annotations could be replaced by a | ||
| /// bare `bool` annotations. Static type checkers such as [mypy] treat them as | ||
| /// equivalent. |
There was a problem hiding this comment.
Start off with a short sentence that clearly states that this is a rule concerned with readability and style rather than correctness:
| /// The `bool` type has exactly two constant instances: `True` and `False`. Writing | |
| /// `Literal[True, False]` in the context of type annotations could be replaced by a | |
| /// bare `bool` annotations. Static type checkers such as [mypy] treat them as | |
| /// equivalent. | |
| /// `Literal[True, False]` can be replaced with `bool` in type annotations, | |
| /// which means the same thing but is more concise and more readable. | |
| /// | |
| /// This is because the `bool` type has exactly two constant instances: | |
| /// `True` and `False`. Static type checkers such as [mypy] treat | |
| /// `Literal[True, False]` as equivalent to `bool` in a type annotation. |
| /// Literal[True, False] | ||
| /// Literal[True, False, "hello", "world"] |
There was a problem hiding this comment.
| /// Literal[True, False] | |
| /// Literal[True, False, "hello", "world"] | |
| /// from typing import Literal | |
| /// | |
| /// x: Literal[True, False] | |
| /// y: Literal[True, False, "hello", "world"] |
| /// bool | ||
| /// Literal["hello", "world"] | bool |
There was a problem hiding this comment.
| /// bool | |
| /// Literal["hello", "world"] | bool | |
| /// from typing import Literal | |
| /// | |
| /// x: bool | |
| /// y: Literal["hello", "world"] | bool |
| /// - `bool` is not strictly equivalent to `Literal[True, False]`, as `bool` is | ||
| /// a subclass of `int`, and this rule might not apply if the type annotations are used | ||
| /// in a numerical context. | ||
| /// - The `Literal` might contain comments. |
There was a problem hiding this comment.
| /// - The `Literal` might contain comments. | |
| /// - The `Literal` slice might contain trailing-line comments which the fix would | |
| /// remove. |
| /// treat `bool` as equivalent when overloading boolean arguments with `Literal[True]` | ||
| /// and `Literal[False]`, e.g. see [#14764] and [#5421]. | ||
| /// - `bool` is not strictly equivalent to `Literal[True, False]`, as `bool` is | ||
| /// a subclass of `int`, and this rule might not apply if the type annotations are used | ||
| /// in a numerical context. |
There was a problem hiding this comment.
| /// treat `bool` as equivalent when overloading boolean arguments with `Literal[True]` | |
| /// and `Literal[False]`, e.g. see [#14764] and [#5421]. | |
| /// - `bool` is not strictly equivalent to `Literal[True, False]`, as `bool` is | |
| /// a subclass of `int`, and this rule might not apply if the type annotations are used | |
| /// in a numerical context. | |
| /// treat `bool` as equivalent when overloading boolean arguments with `Literal[True]` | |
| /// and `Literal[False]`, e.g. see [#14764] and [#5421]. | |
| /// - `bool` is not strictly equivalent to `Literal[True, False]`, as `bool` is | |
| /// a subclass of `int`, and this rule might not apply if the type annotations are used | |
| /// in a numerical context. |
| /// - `bool` is not strictly equivalent to `Literal[True, False]`, as `bool` is | ||
| /// a subclass of `int`, and this rule might not apply if the type annotations are used | ||
| /// in a numerical context. |
There was a problem hiding this comment.
(arguably it's a type-checker bug if they don't understand that <object of type Literal[True]> + <object of type Literal[True]> == <object of type Literal[2]> in a numeric context. But hey.)
charliermarsh
force-pushed
the
redundant-bool-literal
branch
from
8d98cb9 to
4cf2d4e
Compare
|
Applying @AlexWaygood comments, then merging. |
## Summary Implements `redundant-bool-literal` ## Test Plan <!-- How was it tested? --> `cargo test` The ecosystem results are all correct, but for `Airflow` the rule is not relevant due to the use of overloading (and is marked as unsafe correctly). --------- Co-authored-by: Charlie Marsh <[email protected]>
Summary
Implements
redundant-bool-literalTest Plan
cargo testThe ecosystem results are all correct, but for
Airflowthe rule is not relevant due to the use of overloading (and is marked as unsafe correctly).