Skip to content

Fix invalid type false positive #8206

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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
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/8205.false_positive
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Fix false positive for isinstance-second-argument-not-valid-type with union types.

Closes #8205
15 changes: 14 additions & 1 deletion pylint/checkers/typecheck.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
supports_membership_test,
supports_setitem,
)
from pylint.constants import PY310_PLUS
from pylint.interfaces import HIGH, INFERENCE
from pylint.typing import MessageDefinitionTuple

Expand Down Expand Up @@ -796,6 +797,10 @@ def _is_c_extension(module_node: InferenceResult) -> bool:

def _is_invalid_isinstance_type(arg: nodes.NodeNG) -> bool:
# Return True if we are sure that arg is not a type
if PY310_PLUS and isinstance(arg, nodes.BinOp) and arg.op == "|":
return _is_invalid_isinstance_type(arg.left) or _is_invalid_isinstance_type(
arg.right
)
inferred = utils.safe_infer(arg)
if not inferred:
# Cannot infer it so skip it.
Expand All @@ -806,6 +811,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):
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

return _is_invalid_isinstance_type(
inferred.left
) or _is_invalid_isinstance_type(inferred.right)
return True


Expand Down Expand Up @@ -1398,7 +1407,11 @@ def _check_isinstance_args(self, node: nodes.Call) -> None:

second_arg = node.args[1]
if _is_invalid_isinstance_type(second_arg):
self.add_message("isinstance-second-argument-not-valid-type", node=node)
self.add_message(
"isinstance-second-argument-not-valid-type",
node=node,
confidence=INFERENCE,
)

# pylint: disable = too-many-branches, too-many-locals, too-many-statements
def visit_call(self, node: nodes.Call) -> None:
Expand Down
1 change: 1 addition & 0 deletions pylint/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

PY38_PLUS = sys.version_info[:2] >= (3, 8)
PY39_PLUS = sys.version_info[:2] >= (3, 9)
PY310_PLUS = sys.version_info[:2] >= (3, 10)

IS_PYPY = platform.python_implementation() == "PyPy"

Expand Down
2 changes: 1 addition & 1 deletion tests/functional/d/dataclass/dataclass_typecheck.txt
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,4 @@ unsupported-delete-operation:72:4:72:13::'obj.attr1' does not support item delet
not-context-manager:97:0:98:8::Context manager 'str' doesn't implement __enter__ and __exit__.:UNDEFINED
invalid-metaclass:105:0:105:11:Test2:Invalid metaclass 'Instance of builtins.int' used:UNDEFINED
unhashable-member:111:0:111:2::'obj.attr5' is unhashable and can't be used as a key in a dict:INFERENCE
isinstance-second-argument-not-valid-type:121:6:121:30::Second argument of isinstance is not a type:UNDEFINED
isinstance-second-argument-not-valid-type:121:6:121:30::Second argument of isinstance is not a type:INFERENCE
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
""""Checks for redundant Union typehints in assignments"""
"""Checks for redundant Union typehints in assignments"""
# pylint: disable=deprecated-typing-alias,consider-alternative-union-syntax,consider-using-alias,invalid-name,unused-argument,missing-function-docstring

from __future__ import annotations
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
""""Checks for redundant Union typehints in assignments"""
"""Checks for redundant Union typehints in assignments"""
# pylint: disable=deprecated-typing-alias,consider-alternative-union-syntax,consider-using-alias,invalid-name,unused-argument,missing-function-docstring

from __future__ import annotations
Expand Down
10 changes: 5 additions & 5 deletions tests/functional/i/isinstance_second_argument.txt
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
isinstance-second-argument-not-valid-type:27:0:27:23::Second argument of isinstance is not a type:UNDEFINED
isinstance-second-argument-not-valid-type:28:0:28:19::Second argument of isinstance is not a type:UNDEFINED
isinstance-second-argument-not-valid-type:29:0:29:34::Second argument of isinstance is not a type:UNDEFINED
isinstance-second-argument-not-valid-type:30:0:30:54::Second argument of isinstance is not a type:UNDEFINED
isinstance-second-argument-not-valid-type:31:0:31:18::Second argument of isinstance is not a type:UNDEFINED
isinstance-second-argument-not-valid-type:27:0:27:23::Second argument of isinstance is not a type:INFERENCE
isinstance-second-argument-not-valid-type:28:0:28:19::Second argument of isinstance is not a type:INFERENCE
isinstance-second-argument-not-valid-type:29:0:29:34::Second argument of isinstance is not a type:INFERENCE
isinstance-second-argument-not-valid-type:30:0:30:54::Second argument of isinstance is not a type:INFERENCE
isinstance-second-argument-not-valid-type:31:0:31:18::Second argument of isinstance is not a type:INFERENCE
27 changes: 27 additions & 0 deletions tests/functional/i/isinstance_second_argument_py310.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
'''Tests for invalid isinstance with compound types'''

# True negatives
isinstance(0, int | str)
isinstance(0, int | int | int)
isinstance(0, int | str | list | float)
isinstance(0, (int | str) | (list | float))

IntOrStr = int | str
isinstance(0, IntOrStr)
ListOrDict = list | dict
isinstance(0, (float | ListOrDict) | IntOrStr)

# True positives
isinstance(0, int | 5) # [isinstance-second-argument-not-valid-type]
isinstance(0, str | 5 | int) # [isinstance-second-argument-not-valid-type]
INT = 5
isinstance(0, INT | int) # [isinstance-second-argument-not-valid-type]


# FALSE NEGATIVES

# Parameterized generics will raise type errors at runtime.
# Warnings should be raised, but aren't (yet).
isinstance(0, list[int])
ListOfInts = list[int]
isinstance(0, ListOfInts)
2 changes: 2 additions & 0 deletions tests/functional/i/isinstance_second_argument_py310.rc
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+.

3 changes: 3 additions & 0 deletions tests/functional/i/isinstance_second_argument_py310.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
isinstance-second-argument-not-valid-type:15:0:15:22::Second argument of isinstance is not a type:INFERENCE
isinstance-second-argument-not-valid-type:16:0:16:28::Second argument of isinstance is not a type:INFERENCE
isinstance-second-argument-not-valid-type:18:0:18:24::Second argument of isinstance is not a type:INFERENCE