Fix invalid type false positive #8206

nickdrozd · 2023-02-07T03:02:42Z

Type of Changes

	Type
✓	🐛 Bug fix

Description

Fixes false positive for isinstance-second-argument-not-valid-type with union types.

Closes #8205

codecov · 2023-02-07T03:12:46Z

Codecov Report

Merging #8206 (66d21ac) into main (81ed650) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #8206   +/-   ##
=======================================
  Coverage   95.46%   95.46%           
=======================================
  Files         177      177           
  Lines       18698    18704    +6     
=======================================
+ Hits        17850    17856    +6     
  Misses        848      848

Impacted Files	Coverage Δ
pylint/checkers/typecheck.py	`96.35% <100.00%> (+0.01%)`	⬆️
pylint/constants.py	`100.00% <100.00%> (ø)`

DanielNoord

This will need a changelog entry

tests/functional/i/isinstance_second_argument_py310.py

DanielNoord · 2023-02-07T08:09:11Z

pylint/checkers/typecheck.py

@@ -806,6 +806,10 @@ def _is_invalid_isinstance_type(arg: nodes.NodeNG) -> bool:
        return False
    if isinstance(inferred, astroid.Instance) and inferred.qname() == BUILTIN_TUPLE:
        return False
+    if isinstance(arg, nodes.BinOp) and arg.op == "|":


Can't we use the new UnionType here?

Because of what I assume are inference failures, I couldn't cover all the test cases without checking both the inferred value and the superficial value. Is there a better way to do this?

orSolocate · 2023-02-07T09:12:45Z

pylint/checkers/typecheck.py

@@ -806,6 +806,10 @@ def _is_invalid_isinstance_type(arg: nodes.NodeNG) -> bool:
        return False
    if isinstance(inferred, astroid.Instance) and inferred.qname() == BUILTIN_TUPLE:
        return False
+    if isinstance(arg, nodes.BinOp) and arg.op == "|":


I reckon this check should only be done for python version that support PEP604 and the | operand for type annotations. I think this condition should only be checked when supported, similar code in pylint/extensions/typing.py

Suggested change

if isinstance(arg, nodes.BinOp) and arg.op == "|":

if _should_check_alternative_union_syntax and isinstance(arg, nodes.BinOp) and arg.op == "|":

where:

py_version = self.linter.config.py_version self._py37_plus = py_version >= (3, 7) self._py310_plus = py_version >= (3, 10) self._should_check_alternative_union_syntax = self._py310_plus or ( self._py37_plus and self.linter.config.runtime_typing is False

Agreed, I'll add that once the rest is figured out. And maybe a shorter name would be nice 😆

maybe _alt_union_syntax_enabled ?

You can also rename it in pylint/extensions/typing.py

orSolocate · 2023-02-07T09:16:03Z

tests/functional/i/isinstance_second_argument_py310.rc

+[testoptions]
+min_pyver=3.10


maybe you could allow the test to run also for earlier versions like was done in redundant-typehint-argument:

Suggested change

[testoptions]

min_pyver=3.10

[testoptions]

min_pyver=3.7

[TYPING]

runtime-typing=no

What does the runtime-typing option do? Also, is there a need to specify 3.7 as the min version, since that's already the min version being used?

Now that I think about it, this check is different because it doesn't deal with type annotations. In this case the types involved will actually be called, so I think this option won't apply.

you don't have to explicitly write min_pyver=3.7 but it would be just clearer for the test reader to understand why runtime-typing was used. There is some documentation runtime-typing at pylint/extensions/typing.py. It will identify type annotations if it can for python versions 3.7-3.9 with the | operation.

Now that I think about it, this check is different because it doesn't deal with type annotations. In this case the types involved will actually be called, so I think this option won't apply.

I think you are right. These are not annotations it the actual code. so if someone uses the | operation they can only do that in py3.10+.

nickdrozd · 2023-02-07T15:38:19Z

I included test cases for false negatives from #8213.

pylint/checkers/typecheck.py

tests/functional/i/isinstance_second_argument_py310.py

…lse-positive

pylint/checkers/typecheck.py

orSolocate · 2023-02-11T07:09:03Z

tests/functional/i/isinstance_second_argument_py310.rc

+[testoptions]
+min_pyver=3.10


you don't have to explicitly write min_pyver=3.7 but it would be just clearer for the test reader to understand why runtime-typing was used. There is some documentation runtime-typing at pylint/extensions/typing.py. It will identify type annotations if it can for python versions 3.7-3.9 with the | operation.

Now that I think about it, this check is different because it doesn't deal with type annotations. In this case the types involved will actually be called, so I think this option won't apply.

I think you are right. These are not annotations it the actual code. so if someone uses the | operation they can only do that in py3.10+.

orSolocate · 2023-02-11T07:13:22Z

pylint/checkers/typecheck.py

@@ -806,6 +813,10 @@ def _is_invalid_isinstance_type(arg: nodes.NodeNG) -> bool:
        return False
    if isinstance(inferred, astroid.Instance) and inferred.qname() == BUILTIN_TUPLE:
        return False
+    if PY310_PLUS and isinstance(inferred, bases.UnionType):


Suggested change

if PY310_PLUS and isinstance(inferred, bases.UnionType):

if isinstance(inferred, bases.UnionType):

Is the bases.UnionType the legacy Union/Optional type? if so it is supported from 3.7+ I think.

No, Union[int, str] is an instance of ClassDef and not UnionType. I'm not sure what exactly UnionType corresponds to.

…lse-positive

for more information, see https://pre-commit.ci

Pierre-Sassoulas

👍

nickdrozd · 2023-02-13T19:03:16Z

@DanielNoord Anything else for this one?

DanielNoord

Could you add INFERENCE as confidence to the message?>

github-actions · 2023-02-14T16:21:53Z

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit 66d21ac

DanielNoord

Thanks for this! 😄

github-actions · 2023-02-14T21:40:16Z

The backport to maintenance/2.16.x failed:

The process '/usr/bin/git' failed with exit code 1

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/2.16.x maintenance/2.16.x
# Navigate to the new working tree
cd .worktrees/backport-maintenance/2.16.x
# Create a new branch
git switch --create backport-8206-to-maintenance/2.16.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 e64f0437388298c7f8514a6755c3c27a0d9a35f2
# Push it to GitHub
git push --set-upstream origin backport-8206-to-maintenance/2.16.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-maintenance/2.16.x

Then, create a pull request where the base branch is maintenance/2.16.x and the compare/head branch is backport-8206-to-maintenance/2.16.x.

(cherry picked from commit e64f043)

(cherry picked from commit e64f043) Co-authored-by: Nick Drozd <nicholasdrozd@gmail.com>

Fix invalid type false positive

e0d32da

This comment has been minimized.

Sign in to view

DanielNoord requested changes Feb 7, 2023

View reviewed changes

DanielNoord added typing False Positive 🦟 A message is emitted but nothing is wrong with the code labels Feb 7, 2023

DanielNoord added this to the 2.16.2 milestone Feb 7, 2023

orSolocate reviewed Feb 7, 2023

View reviewed changes

nickdrozd added 5 commits February 7, 2023 09:58

Add docstring and more cases

4741057

Use OR instead of AND

07d595e

Move binop check before inference

8e42987

Add news

dc8f72b

Add inferred union type check

bad4805

This comment has been minimized.

Sign in to view

Pierre-Sassoulas reviewed Feb 7, 2023

View reviewed changes

pylint/checkers/typecheck.py Outdated Show resolved Hide resolved

tests/functional/i/isinstance_second_argument_py310.py Show resolved Hide resolved

Pierre-Sassoulas added the backport maintenance/2.16.x label Feb 8, 2023

Pierre-Sassoulas previously approved these changes Feb 9, 2023

View reviewed changes

nickdrozd added 2 commits February 10, 2023 10:50

Cut extra quotes

95cfe9f

Add version check

e4df864

nickdrozd dismissed Pierre-Sassoulas’s stale review via e4df864 February 10, 2023 16:33

nickdrozd requested a review from cdce8p as a code owner February 10, 2023 16:33

nickdrozd requested a review from DanielNoord February 10, 2023 16:37

Merge branch 'main' into isinstance-second-argument-not-valid-type-fa…

e6b70c2

…lse-positive

This comment has been minimized.

Sign in to view

orSolocate reviewed Feb 11, 2023

View reviewed changes

nickdrozd and others added 3 commits February 12, 2023 11:18

Define PY310_PLUS in constants

3d0ebe7

Merge branch 'main' into isinstance-second-argument-not-valid-type-fa…

1c62667

…lse-positive

[pre-commit.ci] auto fixes from pre-commit.com hooks

673fa05

for more information, see https://pre-commit.ci

nickdrozd requested a review from Pierre-Sassoulas February 12, 2023 17:20

nickdrozd requested a review from orSolocate February 12, 2023 17:20

Pierre-Sassoulas previously approved these changes Feb 12, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

Pierre-Sassoulas modified the milestones: 2.16.2, 2.16.3 Feb 13, 2023

DanielNoord reviewed Feb 13, 2023

View reviewed changes

Add confidence=INFERENCE

bb38e7c

nickdrozd dismissed Pierre-Sassoulas’s stale review via bb38e7c February 14, 2023 14:52

nickdrozd requested review from DanielNoord and removed request for orSolocate February 14, 2023 14:52

Update test output

66d21ac

Pierre-Sassoulas approved these changes Feb 14, 2023

View reviewed changes

DanielNoord approved these changes Feb 14, 2023

View reviewed changes

Pierre-Sassoulas merged commit e64f043 into pylint-dev:main Feb 14, 2023

nickdrozd mentioned this pull request Mar 4, 2023

Union type as second arg for isinstance raises W1116 #8381

Closed

cdce8p pushed a commit to cdce8p/pylint that referenced this pull request Mar 6, 2023

Fix invalid type false positive (pylint-dev#8206)

90f43f2

(cherry picked from commit e64f043)

cdce8p modified the milestones: 2.16.3, 2.16.4 Mar 6, 2023

cdce8p added a commit that referenced this pull request Mar 6, 2023

Fix invalid type false positive (#8206) (#8386)

fb4512c

(cherry picked from commit e64f043) Co-authored-by: Nick Drozd <nicholasdrozd@gmail.com>

Pierre-Sassoulas added Backported and removed backport maintenance/2.16.x labels Mar 8, 2023

ytoku mentioned this pull request Mar 10, 2023

isinstance-second-argument-not-valid-type false positive for union types with None #8424

Closed

nickdrozd deleted the isinstance-second-argument-not-valid-type-false-positive branch October 7, 2024 14:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix invalid type false positive #8206

Fix invalid type false positive #8206

nickdrozd commented Feb 7, 2023

codecov bot commented Feb 7, 2023 •

edited

Loading

This comment has been minimized.

DanielNoord left a comment

DanielNoord Feb 7, 2023

nickdrozd Feb 7, 2023

orSolocate Feb 7, 2023

nickdrozd Feb 7, 2023

orSolocate Feb 9, 2023

orSolocate Feb 7, 2023

nickdrozd Feb 10, 2023

nickdrozd Feb 10, 2023

orSolocate Feb 11, 2023

nickdrozd commented Feb 7, 2023

This comment has been minimized.

This comment has been minimized.

orSolocate Feb 11, 2023

orSolocate Feb 11, 2023

nickdrozd Feb 12, 2023

Pierre-Sassoulas left a comment

This comment has been minimized.

nickdrozd commented Feb 13, 2023

DanielNoord left a comment

github-actions bot commented Feb 14, 2023

DanielNoord left a comment

github-actions bot commented Feb 14, 2023

	if isinstance(arg, nodes.BinOp) and arg.op == "\|":
	if _should_check_alternative_union_syntax and isinstance(arg, nodes.BinOp) and arg.op == "\|":

	if PY310_PLUS and isinstance(inferred, bases.UnionType):
	if isinstance(inferred, bases.UnionType):

Fix invalid type false positive #8206

Fix invalid type false positive #8206

Conversation

nickdrozd commented Feb 7, 2023

Type of Changes

Description

codecov bot commented Feb 7, 2023 • edited Loading

Codecov Report

This comment has been minimized.

DanielNoord left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickdrozd commented Feb 7, 2023

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

This comment has been minimized.

nickdrozd commented Feb 13, 2023

DanielNoord left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 14, 2023

DanielNoord left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 14, 2023

codecov bot commented Feb 7, 2023 •

edited

Loading