Skip to content

Consistency between is/is not and ==/!= when comparing types for unidiomatic-typecheck #10170

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
May 4, 2025
Merged

Consistency between is/is not and ==/!= when comparing types for unidiomatic-typecheck #10170

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
f30417c
(bugfix) Issue 10161: Incorrect scenario of unidiomatic-typecheck.
Jan 7, 2025
0a09606
Take zenlyj remark into account
Pierre-Sassoulas May 2, 2025
4380fbe
More descriptive fragment
Pierre-Sassoulas May 2, 2025
59a0a5d
Update pylint/checkers/base/comparison_checker.py
Pierre-Sassoulas May 3, 2025
d61b85c
Update doc/whatsnew/fragments/10161.false_positive
Pierre-Sassoulas May 3, 2025
986d1b4
Further simplification
Pierre-Sassoulas May 3, 2025
ac6e517
Revert following discussion
Pierre-Sassoulas May 3, 2025
3974ec1
Merge remote-tracking branch 'origin/main' into nedelcu-ioan-main
Pierre-Sassoulas May 3, 2025
dc13f1b
black
Pierre-Sassoulas May 3, 2025
59f975c
Update pylint/checkers/base/comparison_checker.py
Pierre-Sassoulas May 3, 2025
74c2fa7
fix functional tests
Pierre-Sassoulas May 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions doc/whatsnew/fragments/10161.false_positive
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Comparisons of types won't raise an ``unidiomatic-typecheck`` warning anymore, consistent with the behavior applied only for ``==`` previously.

Closes #10161
7 changes: 2 additions & 5 deletions pylint/checkers/base/comparison_checker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,15 +343,12 @@ def _check_type_x_is_y(
):
return

if operator in {"is", "is not"} and _is_one_arg_pos_call(right):
if operator in {"!=", "==", "is", "is not"} and _is_one_arg_pos_call(right):
right_func = utils.safe_infer(right.func)
if (
isinstance(right_func, nodes.ClassDef)
and right_func.qname() == TYPE_QNAME
):
# type(x) == type(a)
right_arg = utils.safe_infer(right.args[0])
if not isinstance(right_arg, LITERAL_NODE_TYPES):
# not e.g. type(x) == type([])
return
return
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I would change this.

Copy link
Contributor

@zenlyj zenlyj Feb 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

agree with jacob, shall we keep this guard block that checks for literal node types?

Copy link
Member

Choose a reason for hiding this comment

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

At this point we already know that the left arg is type(something), once we know the right argument is starting by type() we don't have to check what's inside anymore ? Or we want to remove the whole "type_of_literals_negatives" test block ?

Copy link
Contributor

Choose a reason for hiding this comment

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

i think it depends on whether we want to retain current behavior of raising a message for type(a) is type(LITERAL), where LITERAL: [] | {} | "" | .... These cases are covered by the type_of_literals_positives test block, introduced from #299.

Couldn't dig into the related PR as it was merged >10 years ago in a different repository, but the separation of deliberate_subclass_check_negatives and type_of_literals_positives test blocks seems to indicate that the author wanted to raise a message because type(a) is type([]) is equivalent to type(a) is list, which should be refactored to isinstance(a, list)?

Copy link
Member

Choose a reason for hiding this comment

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

I re-read the issue. Here's my context:

  • OP wanted no message for type(x) == type(y) on the theory that users know what they're doing (me: I'd probably wontfix that -- but I won't stand in the way right now)
  • then @zenlyj noticed we have a discrepancy between == and is we should fix. (me: I'm mostly interested in fixing that alone)
  • then in PR review we face the question of whether to remove the cases that check type(x) == type([]). This seems to pull even further in the wrong direction, linter should catch this.

Let's just fix the is/is not thing and keep these test cases and the open follow-up issues for:

  • suggesting type(x) is type(y) to be rewritten as isinstance(x, type(y)), going against OP
  • extending comparison-with-callable to cover == list

Copy link
Member

Choose a reason for hiding this comment

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

Opened #10364 and #10365

self.add_message("unidiomatic-typecheck", node=node)
28 changes: 20 additions & 8 deletions tests/functional/u/unidiomatic_typecheck.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,14 +61,26 @@ def parameter_shadowing_inference_negatives(type):
type(42) in [int]
type(42) not in [int]

def deliberate_subclass_check_negatives(b):
def deliberate_subclass_check_negatives(a,b):
type(42) is type(b)
type(42) is not type(b)
type(42) == type(b)
type(42) != type(b)
type(a) is type(b)
type(a) is not type(b)
type(a) == type(b)
type(a) != type(b)

def type_of_literals_positives(a):
type(a) is type([]) # [unidiomatic-typecheck]
type(a) is not type([]) # [unidiomatic-typecheck]
type(a) is type({}) # [unidiomatic-typecheck]
type(a) is not type({}) # [unidiomatic-typecheck]
type(a) is type("") # [unidiomatic-typecheck]
type(a) is not type("") # [unidiomatic-typecheck]
def type_of_literals_negatives(a):
type(a) is type([])
type(a) is not type([])
type(a) is type({})
type(a) is not type({})
type(a) is type("")
type(a) is not type("")
type(a) == type([])
type(a) != type([])
type(a) == type({})
type(a) != type({})
type(a) == type("")
type(a) != type("")
6 changes: 0 additions & 6 deletions tests/functional/u/unidiomatic_typecheck.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,3 @@ unidiomatic-typecheck:16:4:16:20:simple_inference_positives:Use isinstance() rat
unidiomatic-typecheck:17:4:17:24:simple_inference_positives:Use isinstance() rather than type() for a typecheck.:UNDEFINED
unidiomatic-typecheck:18:4:18:20:simple_inference_positives:Use isinstance() rather than type() for a typecheck.:UNDEFINED
unidiomatic-typecheck:19:4:19:20:simple_inference_positives:Use isinstance() rather than type() for a typecheck.:UNDEFINED
unidiomatic-typecheck:69:4:69:23:type_of_literals_positives:Use isinstance() rather than type() for a typecheck.:UNDEFINED
unidiomatic-typecheck:70:4:70:27:type_of_literals_positives:Use isinstance() rather than type() for a typecheck.:UNDEFINED
unidiomatic-typecheck:71:4:71:23:type_of_literals_positives:Use isinstance() rather than type() for a typecheck.:UNDEFINED
unidiomatic-typecheck:72:4:72:27:type_of_literals_positives:Use isinstance() rather than type() for a typecheck.:UNDEFINED
unidiomatic-typecheck:73:4:73:23:type_of_literals_positives:Use isinstance() rather than type() for a typecheck.:UNDEFINED
unidiomatic-typecheck:74:4:74:27:type_of_literals_positives:Use isinstance() rather than type() for a typecheck.:UNDEFINED
Loading