Skip to content

Refactor and deprecate resolving of interface implementations in pyreverse #6713

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 6 commits into from
May 27, 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
5 changes: 5 additions & 0 deletions ChangeLog
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,11 @@ Release date: TBA

Ref #6712

* ``pyreverse``: Resolving and displaying implemented interfaces that are defined by the ``__implements__``
attribute has been deprecated and will be removed in 3.0.

Ref #6713

* ``interfaces.implements`` has been deprecated and will be removed in 3.0. Please use standard inheritance
patterns instead of ``__implements__``.

Expand Down
13 changes: 9 additions & 4 deletions doc/whatsnew/2.14.rst
Original file line number Diff line number Diff line change
Expand Up @@ -281,10 +281,6 @@ Other Changes

Closes #6644

* ``pylint.pyreverse.ASTWalker`` has been removed, as it was only used internally by a single child class.

Ref #6712

Comment on lines -284 to -287
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is now under "Deprecations" in order to be grouped with #6713, although this is technically not a deprecation but a hard removal. Let me know if that's OK for you to put it there.


Deprecations
============
Expand Down Expand Up @@ -387,6 +383,15 @@ Deprecations

Ref #5392

* ``pylint.pyreverse.ASTWalker`` has been removed, as it was only used internally by a single child class.

Ref #6712

* ``pyreverse``: Resolving and displaying implemented interfaces that are defined by the ``__implements__``
attribute has been deprecated and will be removed in 3.0.

Comment on lines +390 to +392
Copy link
Member

Choose a reason for hiding this comment

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

Could you put it alongside the changelog for #6712, please ? There is going to be a conflict in the Changelog file in #6688 during rebase (as we burst the changelog file), it would be nice to regroup new changelog entry together until this MR is merged 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done 😊

Ref #6713

* ``is_class_subscriptable_pep585_with_postponed_evaluation_enabled`` has been deprecated.
Use ``is_postponed_evaluation_enabled(node) and is_node_in_type_annotation_context(node)``
instead.
Expand Down
41 changes: 17 additions & 24 deletions pylint/pyreverse/inspector.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import collections
import os
import traceback
import warnings
from collections.abc import Generator
from typing import Any, Callable, Optional

Expand All @@ -24,11 +25,6 @@
_WrapperFuncT = Callable[[Callable[[str], nodes.Module], str], Optional[nodes.Module]]


def _iface_hdlr(_: nodes.NodeNG | Any) -> bool:
"""Handler used by interfaces to handle suspicious interface nodes."""
return True


def _astroid_wrapper(
func: Callable[[str], nodes.Module], modname: str
) -> nodes.Module | None:
Expand All @@ -42,25 +38,21 @@ def _astroid_wrapper(
return None


def interfaces(
node: nodes.ClassDef,
herited: bool = True,
handler_func: Callable[[nodes.NodeNG | Any], bool] = _iface_hdlr,
) -> Generator[Any, None, None]:
def interfaces(node: nodes.ClassDef) -> Generator[Any, None, None]:
"""Return an iterator on interfaces implemented by the given class node."""
try:
implements = astroid.bases.Instance(node).getattr("__implements__")[0]
except astroid.exceptions.NotFoundError:
return
if not herited and implements.frame(future=True) is not node:
if implements.frame(future=True) is not node:
return
found = set()
missing = False
for iface in nodes.unpack_infer(implements):
if iface is astroid.Uninferable:
missing = True
continue
if iface not in found and handler_func(iface):
if iface not in found:
found.add(iface)
yield iface
if missing:
Expand Down Expand Up @@ -135,13 +127,9 @@ class Linker(IdGeneratorMixIn, utils.LocalsVisitor):
list of implemented interface _objects_ (only on astroid.Class nodes)
"""

def __init__(
self, project: Project, inherited_interfaces: bool = False, tag: bool = False
) -> None:
def __init__(self, project: Project, tag: bool = False) -> None:
IdGeneratorMixIn.__init__(self)
utils.LocalsVisitor.__init__(self)
# take inherited interface in consideration or not
self.inherited_interfaces = inherited_interfaces
# tag nodes or not
self.tag = tag
# visited project
Expand Down Expand Up @@ -196,8 +184,18 @@ def visit_classdef(self, node: nodes.ClassDef) -> None:
self.handle_assignattr_type(assignattr, node)
# resolve implemented interface
try:
ifaces = interfaces(node, self.inherited_interfaces)
node.implements = list(ifaces) if ifaces is not None else []
ifaces = interfaces(node)
if ifaces is not None:
node.implements = list(ifaces)
# TODO: 3.0: Remove support for __implements__
warnings.warn(
"pyreverse will drop support for resolving and displaying implemented interfaces in pylint 3.0. "
"The implementation relies on the '__implements__' attribute proposed in PEP 245, which was rejected "
"in 2006.",
Copy link
Member

Choose a reason for hiding this comment

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

Wow, 2006 😄 I was copy pasting javascript and basic for high school calculator at the time.

DeprecationWarning,
)
else:
node.implements = []
except astroid.InferenceError:
node.implements = []

Expand All @@ -213,11 +211,6 @@ def visit_functiondef(self, node: nodes.FunctionDef) -> None:
if self.tag:
node.uid = self.generate_id()

link_project = visit_project
link_module = visit_module
link_class = visit_classdef
link_function = visit_functiondef

def visit_assignname(self, node: nodes.AssignName) -> None:
"""Visit an astroid.AssignName node.

Expand Down
17 changes: 16 additions & 1 deletion tests/pyreverse/test_inspector.py
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ class Concrete23(Concrete1): pass
("Concrete0", ["MyIFace"]),
("Concrete1", ["MyIFace", "AnotherIFace"]),
("Concrete2", ["MyIFace", "AnotherIFace"]),
("Concrete23", ["MyIFace", "AnotherIFace"]),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The reason this test changed is that the unit test actually used the default value of True for the now removed herited parameter, but in production this parameter was always False. So after removing the parameter from the function signature I had to update this test case accordingly.

("Concrete23", []),
):
klass = module[klass]
assert [i.name for i in inspector.interfaces(klass)] == interfaces
Expand All @@ -130,3 +130,18 @@ def test_project_node(project: Project) -> None:
"data.suppliermodule_test",
]
assert sorted(project.keys()) == expected


def test_interface_deprecation(project: Project) -> None:
linker = inspector.Linker(project)
cls = astroid.extract_node(
'''
class IMachin: pass

class Concrete: #@
"""docstring"""
__implements__ = (IMachin,)
'''
)
with pytest.warns(DeprecationWarning):
linker.visit_classdef(cls)