Skip to content
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

Incorrect scenario of unidiomatic-typecheck #10161 #10170

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+24 −18
Open

Incorrect scenario of unidiomatic-typecheck #10161 #10170

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0040ef0
(bugfix) Issue 10161: Incorrect scenario of unidiomatic-typecheck.
Jan 7, 2025
226c110
Merge branch 'pylint-dev:main' into main
nedelcu-ioan Jan 7, 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 @@
Fix a false positive for `unidiomatic-typecheck` when comparing two direct types.

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 @@ -339,15 +339,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?

self.add_message("unidiomatic-typecheck", node=node)
26 changes: 19 additions & 7 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,10 +61,22 @@ def deliberate_subclass_check_negatives(b):
type(42) is type(b)
type(42) is not 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([])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't the issue say that we don't want to suggest this but only exclude the is and is not comparisons?

Copy link
Member

Choose a reason for hiding this comment

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

I think @zenlyj wanted to treat == similarly to is.

type(a) != type([])
type(a) == type({})
type(a) != type({})
type(a) == type("")
type(a) != type("")

def double_type_check_negatives(a, b):
type(a) == type(b)
type(a) != type(b)
type(a) is type(b)
type(a) is not type(b)
Comment on lines +78 to +82
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of adding a new set of tests here, perhaps we can extend deliberate_subclass_check_negatives()? by adding the == / != test cases in that block

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 @@ -6,9 +6,3 @@ unidiomatic-typecheck:12:4:12:20:simple_inference_positives:Use isinstance() rat
unidiomatic-typecheck:13:4:13:24:simple_inference_positives:Use isinstance() rather than type() for a typecheck.:UNDEFINED
unidiomatic-typecheck:14:4:14:20:simple_inference_positives:Use isinstance() rather than type() for a typecheck.:UNDEFINED
unidiomatic-typecheck:15:4:15:20:simple_inference_positives:Use isinstance() rather than type() for a typecheck.:UNDEFINED
unidiomatic-typecheck:65:4:65:23:type_of_literals_positives:Use isinstance() rather than type() for a typecheck.:UNDEFINED
unidiomatic-typecheck:66:4:66:27:type_of_literals_positives:Use isinstance() rather than type() for a typecheck.:UNDEFINED
unidiomatic-typecheck:67:4:67:23:type_of_literals_positives:Use isinstance() rather than type() for a typecheck.:UNDEFINED
unidiomatic-typecheck:68:4:68:27:type_of_literals_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
Loading