Allow `is` and `is` not for direct type comparisons by charliermarsh · Pull Request #7905 · astral-sh/ruff

charliermarsh · 2023-10-11T00:33:33Z

Summary

This PR updates our E721 implementation and semantics to match the updated pycodestyle logic, which I think is an improvement. Specifically, we now allow type(obj) is int for exact type comparisons, which were previously impossible. So now, we're largely just linting against code like type(obj) == int.

This change is gated to preview mode.

Closes #7904.

Test Plan

Updated the test fixture and ensured parity with latest Flake8.

charliermarsh · 2023-10-11T00:34:41Z

crates/ruff_linter/src/rules/pycodestyle/rules/type_comparison.rs

    {
-        if !matches!(op, CmpOp::Is | CmpOp::IsNot | CmpOp::Eq | CmpOp::NotEq) {
-            continue;
+        if is_type(left, checker.semantic()) || is_type(right, checker.semantic()) {


Unlike pycodestyle, our rule is symmetric... so, e.g., we flag both type(x) == int and int == type(x).

is is commutative, but == isn't in Python:

[ins] In [14]: (a == b) == (b == a) Out[14]: False [ins] In [15]: (a is b) == (b is a) Out[15]: True

But I don't think that matters for this linter rule. :)

github-actions · 2023-10-11T00:47:39Z

PR Check Results

Ecosystem

ℹ️ ecosystem check detected changes. (+3, -1, 0 error(s))

rotki (+3, -1)

- [*] 11 fixable with the `--fix` option (225 hidden fixes can be enabled with the `--unsafe-fixes` option).
+ [*] 13 fixable with the `--fix` option (225 hidden fixes can be enabled with the `--unsafe-fixes` option).
+ rotkehlchen/tests/api/test_settings.py:146:81: RUF100 [*] Unused `noqa` directive (unused: `E721`)
+ rotkehlchen/tests/api/test_settings.py:150:80: RUF100 [*] Unused `noqa` directive (unused: `E721`)

Rules changed: 1

Rule	Changes	Additions	Removals
RUF100	2	2	0

dhruvmanila · 2023-10-11T04:32:19Z

crates/ruff_linter/src/rules/pycodestyle/rules/type_comparison.rs

 /// ```python
-/// if type(obj) is type(1):
+/// if type(obj) == type(1):
 ///     pass


I think that the "Use instead" section needs to contain the following:

if type(obj) is type(1): pass

dhruvmanila · 2023-10-11T04:34:52Z

crates/ruff_linter/src/rules/pycodestyle/rules/type_comparison.rs

-            Expr::Attribute(ast::ExprAttribute { value, .. }) => {
-                // Ex) `type(obj) is types.NoneType`
-                if checker
-                    .semantic()
-                    .resolve_call_path(value.as_ref())
-                    .is_some_and(|call_path| matches!(call_path.as_slice(), ["types", ..]))
-                {
-                    checker
-                        .diagnostics
-                        .push(Diagnostic::new(TypeComparison, compare.range()));
-                }
-            }


This isn't present in the refactored code, is that an expected behavior?

The following doesn't get violated then:

if type(foo) == types.LambdaType: ...

Yeah, they removed this from pycodestyle with the understanding that a user-defined types would be a lot more common than accessing the standard library's types. I'm torn on it but figured it's fine to follow suit.

charliermarsh · 2023-10-17T04:38:41Z

This may need to go out under preview \cc @zanieb

charliermarsh · 2023-10-20T18:50:20Z

@zanieb - What do you think of this?

zanieb · 2023-10-20T19:21:17Z

I agree preview makes sense for this change.

Perhaps we should clarify in the versioning policy that we will confine changes in the scope of rules to minor releases.

…` (`E721`) (#11220) ## Summary Stabilizes `E721` behavior implemented in #7905. The functionality change in `E721` was implemented in #7905, released in [v0.1.2](https://github.com/astral-sh/ruff/releases/tag/v0.1.2). And seems functionally stable since #9676, without an explicit release but would correspond to [v0.2.0](https://github.com/astral-sh/ruff/releases/tag/v0.2.0). So the deprecated functionally should be removable in the next minor release. resolves: #6465

## Summary I don't fully understand the purpose of this. In #7905, it was just copied over from the previous non-preview implementation. But it means that (e.g.) we don't treat `type(self.foo)` as a type -- which is wrong. Closes #12290.

charliermarsh requested a review from zanieb October 11, 2023 00:33

charliermarsh commented Oct 11, 2023

View reviewed changes

dhruvmanila approved these changes Oct 11, 2023

View reviewed changes

charliermarsh mentioned this pull request Oct 20, 2023

add autofix for E721 #7965

Closed

charliermarsh force-pushed the charlie/E721 branch 4 times, most recently from 7fa0918 to dfcb2f7 Compare October 20, 2023 23:15

Allow is and is not for type comparisons

3ec2ee0

charliermarsh force-pushed the charlie/E721 branch from dfcb2f7 to 3ec2ee0 Compare October 20, 2023 23:16

charliermarsh added the rule Implementing or modifying a lint rule label Oct 20, 2023

charliermarsh enabled auto-merge (squash) October 20, 2023 23:16

charliermarsh merged commit df807ff into main Oct 20, 2023

charliermarsh deleted the charlie/E721 branch October 20, 2023 23:27

p-linnane mentioned this pull request Oct 24, 2023

ruff 0.1.2 Homebrew/homebrew-core#152273

Merged

hauntsaninja mentioned this pull request Nov 6, 2023

Flag all comparisons against builtin types in E721 #8491

Merged

dhruvmanila mentioned this pull request Apr 30, 2024

misleading E721 Do not compare types, use isinstance() for bool #10948

Closed

augustelalande mentioned this pull request Apr 30, 2024

[pycodestyle] Remove deprecated functionality from type-comparison (E721) #11220

Merged

maresb mentioned this pull request Jul 1, 2024

[pre-commit.ci] pre-commit autoupdate pymc-devs/pymc#7398

Merged

charliermarsh mentioned this pull request Jul 12, 2024

Remove 'non-obvious' allowance for E721 #12300

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow `is` and `is` not for direct type comparisons#7905

Allow `is` and `is` not for direct type comparisons#7905
charliermarsh merged 1 commit intomainfrom
charlie/E721

charliermarsh commented Oct 11, 2023 •

edited

Loading

Uh oh!

charliermarsh Oct 11, 2023

Uh oh!

CharString Aug 22, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Oct 11, 2023 •

edited

Loading

Uh oh!

dhruvmanila Oct 11, 2023

Uh oh!

dhruvmanila Oct 11, 2023

Uh oh!

charliermarsh Oct 12, 2023

Uh oh!

charliermarsh commented Oct 17, 2023

Uh oh!

charliermarsh commented Oct 20, 2023

Uh oh!

zanieb commented Oct 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

charliermarsh commented Oct 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

charliermarsh Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

CharString Aug 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Check Results

Ecosystem

Uh oh!

dhruvmanila Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

dhruvmanila Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

charliermarsh Oct 12, 2023

Choose a reason for hiding this comment

Uh oh!

charliermarsh commented Oct 17, 2023

Uh oh!

charliermarsh commented Oct 20, 2023

Uh oh!

zanieb commented Oct 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

charliermarsh commented Oct 11, 2023 •

edited

Loading

CharString Aug 22, 2024 •

edited

Loading

github-actions bot commented Oct 11, 2023 •

edited

Loading