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

Fix invalid type false positive #8206

Merged
merged 14 commits into from
Feb 14, 2023
Merged

Fix invalid type false positive #8206

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
e0d32da
Fix invalid type false positive
nickdrozd Feb 7, 2023
4741057
Add docstring and more cases
nickdrozd Feb 7, 2023
07d595e
Use OR instead of AND
nickdrozd Feb 7, 2023
8e42987
Move binop check before inference
nickdrozd Feb 7, 2023
dc8f72b
Add news
nickdrozd Feb 7, 2023
bad4805
Add inferred union type check
nickdrozd Feb 7, 2023
95cfe9f
Cut extra quotes
nickdrozd Feb 10, 2023
e4df864
Add version check
nickdrozd Feb 10, 2023
e6b70c2
Merge branch 'main' into isinstance-second-argument-not-valid-type-fa…
nickdrozd Feb 10, 2023
3d0ebe7
Define PY310_PLUS in constants
nickdrozd Feb 12, 2023
1c62667
Merge branch 'main' into isinstance-second-argument-not-valid-type-fa…
nickdrozd Feb 12, 2023
673fa05
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Feb 12, 2023
bb38e7c
Add confidence=INFERENCE
nickdrozd Feb 14, 2023
66d21ac
Update test output
nickdrozd Feb 14, 2023
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
4 changes: 4 additions & 0 deletions pylint/checkers/typecheck.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 == "|":
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't we use the new UnionType here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe _alt_union_syntax_enabled ?

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

return _is_invalid_isinstance_type(arg.left) and _is_invalid_isinstance_type(
arg.right
)
return True


Expand Down
5 changes: 5 additions & 0 deletions tests/functional/i/isinstance_second_argument_py310.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# pylint: disable = missing-docstring
isinstance(0, int | str)
isinstance(0, int | int | int)
isinstance(0, int | str | list | float)
isinstance(0, (int | str) | (list | float))
2 changes: 2 additions & 0 deletions tests/functional/i/isinstance_second_argument_py310.rc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
[testoptions]
min_pyver=3.10
Comment on lines +1 to +2
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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+.

Empty file added tests/functional/i/isinstance_second_argument_py310.txt
View file
Open in desktop
Empty file.