Skip to content

Add broken Callable check #5891

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
merged 2 commits into from
Mar 10, 2022
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
4 changes: 4 additions & 0 deletions ChangeLog
Original file line number Diff line number Diff line change
Expand Up @@ -406,6 +406,10 @@ Release date: TBA
if ``py-version`` is set to Python ``3.7.1`` or below.
https://bugs.python.org/issue34921

* Added new check ``broken-collections-callable`` to detect broken uses of ``collections.abc.Callable``
if ``py-version`` is set to Python ``3.9.1`` or below.
https://bugs.python.org/issue42965

* The ``testutils`` for unittests now accept ``end_lineno`` and ``end_column``. Tests
without these will trigger a ``DeprecationWarning``.

Expand Down
4 changes: 4 additions & 0 deletions doc/whatsnew/2.13.rst
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,10 @@ Extensions
if ``py-version`` is set to Python ``3.7.1`` or below.
https://bugs.python.org/issue34921

* Added new check ``broken-collections-callable`` to detect broken uses of ``collections.abc.Callable``
if ``py-version`` is set to Python ``3.9.1`` or below.
https://bugs.python.org/issue42965

* ``DocstringParameterChecker``

* Fixed incorrect classification of Numpy-style docstring as Google-style docstring for
Expand Down
104 changes: 97 additions & 7 deletions pylint/extensions/typing.py
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ class DeprecatedTypingAliasMsg(NamedTuple):
node: Union[nodes.Name, nodes.Attribute]
qname: str
alias: str
parent_subscript: bool
parent_subscript: bool = False


class TypingChecker(BaseChecker):
Expand Down Expand Up @@ -118,6 +118,14 @@ class TypingChecker(BaseChecker):
"use string annotation instead. E.g. "
"``Callable[..., 'NoReturn']``. https://bugs.python.org/issue34921",
),
"E6005": (
"'collections.abc.Callable' inside Optional and Union is broken in "
"3.9.0 / 3.9.1 (use 'typing.Callable' instead)",
"broken-collections-callable",
"``collections.abc.Callable`` inside Optional and Union is broken in "
"Python 3.9.0 and 3.9.1. Use ``typing.Callable`` for these cases instead. "
"https://bugs.python.org/issue42965",
),
}
options = (
(
Expand Down Expand Up @@ -152,7 +160,9 @@ class TypingChecker(BaseChecker):
def __init__(self, linter: "PyLinter") -> None:
"""Initialize checker instance."""
super().__init__(linter=linter)
self._found_broken_callable_location: bool = False
self._alias_name_collisions: Set[str] = set()
self._deprecated_typing_alias_msgs: List[DeprecatedTypingAliasMsg] = []
self._consider_using_alias_msgs: List[DeprecatedTypingAliasMsg] = []

def open(self) -> None:
Expand All @@ -169,8 +179,9 @@ def open(self) -> None:
)

self._should_check_noreturn = py_version < (3, 7, 2)
self._should_check_callable = py_version < (3, 9, 2)

def _msg_postponed_eval_hint(self, node) -> str:
def _msg_postponed_eval_hint(self, node: nodes.NodeNG) -> str:
"""Message hint if postponed evaluation isn't enabled."""
if self._py310_plus or "annotations" in node.root().future_imports:
return ""
Expand All @@ -181,6 +192,7 @@ def _msg_postponed_eval_hint(self, node) -> str:
"consider-using-alias",
"consider-alternative-union-syntax",
"broken-noreturn",
"broken-collections-callable",
)
def visit_name(self, node: nodes.Name) -> None:
if self._should_check_typing_alias and node.name in ALIAS_NAMES:
Expand All @@ -189,12 +201,15 @@ def visit_name(self, node: nodes.Name) -> None:
self._check_for_alternative_union_syntax(node, node.name)
if self._should_check_noreturn and node.name == "NoReturn":
self._check_broken_noreturn(node)
if self._should_check_callable and node.name == "Callable":
self._check_broken_callable(node)

@check_messages(
"deprecated-typing-alias",
"consider-using-alias",
"consider-alternative-union-syntax",
"broken-noreturn",
"broken-collections-callable",
)
def visit_attribute(self, node: nodes.Attribute) -> None:
if self._should_check_typing_alias and node.attrname in ALIAS_NAMES:
Expand All @@ -203,6 +218,8 @@ def visit_attribute(self, node: nodes.Attribute) -> None:
self._check_for_alternative_union_syntax(node, node.attrname)
if self._should_check_noreturn and node.attrname == "NoReturn":
self._check_broken_noreturn(node)
if self._should_check_callable and node.attrname == "Callable":
self._check_broken_callable(node)

def _check_for_alternative_union_syntax(
self,
Expand Down Expand Up @@ -255,10 +272,16 @@ def _check_for_typing_alias(
return

if self._py39_plus:
self.add_message(
"deprecated-typing-alias",
node=node,
args=(inferred.qname(), alias.name),
if inferred.qname() == "typing.Callable" and self._broken_callable_location(
node
):
self._found_broken_callable_location = True
self._deprecated_typing_alias_msgs.append(
DeprecatedTypingAliasMsg(
node,
inferred.qname(),
alias.name,
)
)
return

Expand All @@ -284,7 +307,20 @@ def leave_module(self, node: nodes.Module) -> None:
'consider-using-alias' check. Make sure results are safe
to recommend / collision free.
"""
if self._py37_plus and not self._py39_plus:
if self._py39_plus:
for msg in self._deprecated_typing_alias_msgs:
if (
self._found_broken_callable_location
and msg.qname == "typing.Callable"
):
continue
self.add_message(
"deprecated-typing-alias",
node=msg.node,
args=(msg.qname, msg.alias),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
args=(msg.qname, msg.alias),
args=(msg.qname, msg.alias),
confidence=interfaces.INFERENCE,

AFAIK because of inferred.qname() line 275 / 282 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's do that separately. It would change existing tests.

)

elif self._py37_plus:
msg_future_import = self._msg_postponed_eval_hint(node)
for msg in self._consider_using_alias_msgs:
if msg.qname in self._alias_name_collisions:
Expand All @@ -298,7 +334,10 @@ def leave_module(self, node: nodes.Module) -> None:
msg_future_import if msg.parent_subscript else "",
),
)

# Clear all module cache variables
self._found_broken_callable_location = False
self._deprecated_typing_alias_msgs.clear()
self._alias_name_collisions.clear()
self._consider_using_alias_msgs.clear()

Expand Down Expand Up @@ -328,6 +367,57 @@ def _check_broken_noreturn(self, node: Union[nodes.Name, nodes.Attribute]) -> No
self.add_message("broken-noreturn", node=node, confidence=INFERENCE)
break

def _check_broken_callable(self, node: Union[nodes.Name, nodes.Attribute]) -> None:
"""Check for 'collections.abc.Callable' inside Optional and Union."""
inferred = safe_infer(node)
if not (
isinstance(inferred, nodes.ClassDef)
and inferred.qname() == "_collections_abc.Callable"
and self._broken_callable_location(node)
):
return

self.add_message("broken-collections-callable", node=node, confidence=INFERENCE)

def _broken_callable_location( # pylint: disable=no-self-use
self, node: Union[nodes.Name, nodes.Attribute]
) -> bool:
"""Check if node would be a broken location for collections.abc.Callable."""
if is_postponed_evaluation_enabled(node) and is_node_in_type_annotation_context(
node
):
return False

# Check first Callable arg is a list of arguments -> Callable[[int], None]
if not (
isinstance(node.parent, nodes.Subscript)
and isinstance(node.parent.slice, nodes.Tuple)
and len(node.parent.slice.elts) == 2
and isinstance(node.parent.slice.elts[0], nodes.List)
):
return False

# Check nested inside Optional or Union
parent_subscript = node.parent.parent
if isinstance(parent_subscript, nodes.BaseContainer):
parent_subscript = parent_subscript.parent
if not (
isinstance(parent_subscript, nodes.Subscript)
and isinstance(parent_subscript.value, (nodes.Name, nodes.Attribute))
):
return False
Copy link
Member

Choose a reason for hiding this comment

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

This line is not covered by the tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Saw that too. Seems like my previous filter steps are quite good already. Took me a moment to find something that would run this line.


inferred_parent = safe_infer(parent_subscript.value)
if not (
isinstance(inferred_parent, nodes.FunctionDef)
and inferred_parent.qname() in {"typing.Optional", "typing.Union"}
or isinstance(inferred_parent, astroid.bases.Instance)
and inferred_parent.qname() == "typing._SpecialForm"
):
return False

return True


def register(linter: "PyLinter") -> None:
linter.register_checker(TypingChecker(linter))
28 changes: 28 additions & 0 deletions tests/functional/ext/typing/typing_broken_callable.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
"""
'collections.abc.Callable' is broken inside Optional and Union types for Python 3.9.0
https://bugs.python.org/issue42965

Use 'typing.Callable' instead.
"""
# pylint: disable=missing-docstring,unsubscriptable-object
import collections.abc
from collections.abc import Callable
from typing import Optional, Union

Alias1 = Optional[Callable[[int], None]] # [broken-collections-callable]
Alias2 = Union[Callable[[int], None], None] # [broken-collections-callable]

Alias3 = Optional[Callable[..., None]]
Alias4 = Union[Callable[..., None], None]
Alias5 = list[Callable[..., None]]
Alias6 = Callable[[int], None]


def func1() -> Optional[Callable[[int], None]]: # [broken-collections-callable]
...

def func2() -> Optional["Callable[[int], None]"]:
...

def func3() -> Union[collections.abc.Callable[[int], None], None]: # [broken-collections-callable]
...
6 changes: 6 additions & 0 deletions tests/functional/ext/typing/typing_broken_callable.rc
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
[master]
py-version=3.9
load-plugins=pylint.extensions.typing

[testoptions]
min_pyver=3.7
4 changes: 4 additions & 0 deletions tests/functional/ext/typing/typing_broken_callable.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
broken-collections-callable:12:18:12:26::'collections.abc.Callable' inside Optional and Union is broken in 3.9.0 / 3.9.1 (use 'typing.Callable' instead):INFERENCE
broken-collections-callable:13:15:13:23::'collections.abc.Callable' inside Optional and Union is broken in 3.9.0 / 3.9.1 (use 'typing.Callable' instead):INFERENCE
broken-collections-callable:21:24:21:32:func1:'collections.abc.Callable' inside Optional and Union is broken in 3.9.0 / 3.9.1 (use 'typing.Callable' instead):INFERENCE
broken-collections-callable:27:21:27:45:func3:'collections.abc.Callable' inside Optional and Union is broken in 3.9.0 / 3.9.1 (use 'typing.Callable' instead):INFERENCE
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
"""
'collections.abc.Callable' is broken inside Optional and Union types for Python 3.9.0
https://bugs.python.org/issue42965

Use 'typing.Callable' instead.

Don't emit 'deprecated-typing-alias' for 'Callable' if at least one replacement
would create broken instances.
"""
# pylint: disable=missing-docstring,unsubscriptable-object
from typing import Callable, Optional, Union

Alias1 = Optional[Callable[[int], None]]
Alias2 = Union[Callable[[int], None], None]

Alias3 = Optional[Callable[..., None]]
Alias4 = Union[Callable[..., None], None]
Alias5 = list[Callable[[int], None]]
Alias6 = Callable[[int], None]


def func1() -> Optional[Callable[[int], None]]:
...

def func2() -> Optional["Callable[[int], None]"]:
...
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
[master]
py-version=3.9
load-plugins=pylint.extensions.typing

[testoptions]
min_pyver=3.7
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
"""
'collections.abc.Callable' is broken inside Optional and Union types for Python 3.9.0
https://bugs.python.org/issue42965

Use 'typing.Callable' instead.
"""
# pylint: disable=missing-docstring,unsubscriptable-object
from __future__ import annotations

import collections.abc
from collections.abc import Callable
from typing import Optional, Union

Alias1 = Optional[Callable[[int], None]] # [broken-collections-callable]
Alias2 = Union[Callable[[int], None], None] # [broken-collections-callable]

Alias3 = Optional[Callable[..., None]]
Alias4 = Union[Callable[..., None], None]
Alias5 = list[Callable[[int], None]]
Alias6 = Callable[[int], None]


def func1() -> Optional[Callable[[int], None]]:
...

def func2() -> Optional["Callable[[int], None]"]:
...

def func3() -> Union[collections.abc.Callable[[int], None], None]:
...
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
[master]
py-version=3.9
load-plugins=pylint.extensions.typing

[testoptions]
min_pyver=3.7
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
broken-collections-callable:14:18:14:26::'collections.abc.Callable' inside Optional and Union is broken in 3.9.0 / 3.9.1 (use 'typing.Callable' instead):INFERENCE
broken-collections-callable:15:15:15:23::'collections.abc.Callable' inside Optional and Union is broken in 3.9.0 / 3.9.1 (use 'typing.Callable' instead):INFERENCE