Consistency between is/is not and ==/!= when comparing types for unidiomatic-typecheck
#10170
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10170 +/- ##
==========================================
- Coverage 95.83% 95.83% -0.01%
==========================================
Files 174 174
Lines 18995 18993 -2
==========================================
- Hits 18204 18202 -2
Misses 791 791
|
This comment has been minimized.
This comment has been minimized.
Pierre-Sassoulas
added
False Positive 🦟
| right_arg = utils.safe_infer(right.args[0]) | ||
| if not isinstance(right_arg, LITERAL_NODE_TYPES): | ||
| # not e.g. type(x) == type([]) | ||
| return | ||
| return |
There was a problem hiding this comment.
I'm not sure I would change this.
There was a problem hiding this comment.
agree with jacob, shall we keep this guard block that checks for literal node types?
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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
==andiswe 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 asisinstance(x, type(y)), going against OP - extending
comparison-with-callableto cover== list
This comment has been minimized.
This comment has been minimized.
DanielNoord
added
the
Needs take over 🛎️
Pierre-Sassoulas
removed
the
Needs take over 🛎️
This comment has been minimized.
This comment has been minimized.
Pierre-Sassoulas
changed the title
is/is not and ==/!= when comparing types for unidiomatic-typecheck
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Zen Lee <[email protected]>
This comment has been minimized.
This comment has been minimized.
| right_arg = utils.safe_infer(right.args[0]) | ||
| if not isinstance(right_arg, LITERAL_NODE_TYPES): | ||
| # not e.g. type(x) == type([]) | ||
| return | ||
| return |
There was a problem hiding this comment.
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
==andiswe 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 asisinstance(x, type(y)), going against OP - extending
comparison-with-callableto cover== list
This comment has been minimized.
This comment has been minimized.
Pierre-Sassoulas
dismissed stale reviews from jacobtylerwalls and DanielNoord
via
74c2fa7
|
LGTM 👌, thanks! |
|
The backport to To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-maintenance/3.3.x maintenance/3.3.x
# Navigate to the new working tree
cd .worktrees/backport-maintenance/3.3.x
# Create a new branch
git switch --create backport-10170-to-maintenance/3.3.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 d396616db59977540ab32496e324c7aaca352ab2
# Push it to GitHub
git push --set-upstream origin backport-10170-to-maintenance/3.3.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-maintenance/3.3.xThen, create a pull request where the |
…g types for ``unidiomatic-typecheck`` (pylint-dev#10170) (cherry picked from commit d396616)
…g types for ``unidiomatic-typecheck`` (pylint-dev#10170) Co-authored-by: Pierre Sassoulas <[email protected]>
…g types for ``unidiomatic-typecheck`` (#10170) (#10366) (cherry picked from commit d396616) Co-authored-by: Nedelcu Ioan-Andrei <[email protected]> Co-authored-by: Pierre Sassoulas <[email protected]>
Type of Changes
Description
Closes #10161