``pyreverse`` should check that ``klass`` is still ``ClassDef``

bjmc commented 1 month ago

Ran into this when trying to use pyreverse, but I think this might be an astroid bug. It seems like the issue is that is_stdlib_module() expects a string module name, but in cases where the node is Uninferable then the special UninferableBase object returns self for almost all attribute access, including .name. This leads to confusing exceptions that are hard for a user to interpret.

Steps to reproduce

from astroid.util import Uninferable
from astroid.modutils import is_stdlib_module

# Example from pyreverse/diadefslib.py line 75:
# https://github.com/pylint-dev/pylint/blob/main/pylint/pyreverse/diadefslib.py#L75
node = Uninferable
is_stdlib_module(node.root().name)

Current behavior

Traceback (most recent call last):
  File "/home/bjmc/Sandbox/example.py", line 7, in <module>
    is_stdlib_module(node.root().name)
  File "/home/bjmc/.cache/pypoetry/virtualenvs/example-zPbyUnCb-py3.10/lib/python3.10/site-packages/astroid/modutils.py", line 524, in is_stdlib_module
    return modname.split(".")[0] in stdlib_module_names
TypeError: 'UninferableBase' object is not subscriptable

Expected behavior

return False (UninferableBase is not known to be in stdlib)

Astroid version

$ python -c "from astroid import __pkginfo__; print(__pkginfo__.version)"
3.2.3

DanielNoord commented 1 month ago

This bug makes sense. Where did you run into this? Can we just escape early in that path when we encounter an UniferableBase? Instead of adding more special cases to that __getattribute__.

bjmc commented 1 month ago

I ran into it trying to run pyreverse on our codebase to generate some PlantUML.

The full traceback is as follows:

$ pyreverse -a 0 -o plantuml src/ska_oso_pdm/ -c ska_oso_pdm.sb_definition.DishAllocation
Traceback (most recent call last):
  File "/home/bjmc/.cache/pypoetry/virtualenvs/ska-oso-pdm-zPbyUnCb-py3.10/bin/pyreverse", line 8, in <module>
    sys.exit(run_pyreverse())
  File "/home/bjmc/.cache/pypoetry/virtualenvs/ska-oso-pdm-zPbyUnCb-py3.10/lib/python3.10/site-packages/pylint/__init__.py", line 56, in run_pyreverse
    PyreverseRun(argv or sys.argv[1:])
  File "/home/bjmc/.cache/pypoetry/virtualenvs/ska-oso-pdm-zPbyUnCb-py3.10/lib/python3.10/site-packages/pylint/pyreverse/main.py", line 297, in __init__
    sys.exit(self.run(args))
  File "/home/bjmc/.cache/pypoetry/virtualenvs/ska-oso-pdm-zPbyUnCb-py3.10/lib/python3.10/site-packages/pylint/pyreverse/main.py", line 316, in run
    diadefs = handler.get_diadefs(project, linker)
  File "/home/bjmc/.cache/pypoetry/virtualenvs/ska-oso-pdm-zPbyUnCb-py3.10/lib/python3.10/site-packages/pylint/pyreverse/diadefslib.py", line 230, in get_diadefs
    diagrams.append(generator.class_diagram(project, klass))
  File "/home/bjmc/.cache/pypoetry/virtualenvs/ska-oso-pdm-zPbyUnCb-py3.10/lib/python3.10/site-packages/pylint/pyreverse/diadefslib.py", line 202, in class_diagram
    self.extract_classes(klass, anc_level, association_level)
  File "/home/bjmc/.cache/pypoetry/virtualenvs/ska-oso-pdm-zPbyUnCb-py3.10/lib/python3.10/site-packages/pylint/pyreverse/diadefslib.py", line 117, in extract_classes
    if self.classdiagram.has_node(klass_node) or not self.show_node(klass_node):
  File "/home/bjmc/.cache/pypoetry/virtualenvs/ska-oso-pdm-zPbyUnCb-py3.10/lib/python3.10/site-packages/pylint/pyreverse/diadefslib.py", line 76, in show_node
    if is_stdlib_module(node.root().name):
  File "/home/bjmc/.cache/pypoetry/virtualenvs/ska-oso-pdm-zPbyUnCb-py3.10/lib/python3.10/site-packages/astroid/modutils.py", line 524, in is_stdlib_module
    return modname.split(".")[0] in stdlib_module_names
TypeError: 'UninferableBase' object is not subscriptable

Can we just escape early in that path when we encounter an UniferableBase?

On the face of it, that sounds like a decent idea to me, but I wouldn't have any idea where to start making those changes. I was hoping to treat pyreverse as a black box tool.

Instead of adding more special cases to that __getattribute__

Another alternative to avoid added complexity in __getattribute__ might be to define an UninferableBase.name property? My understanding is that __getattribute__() is only called as a fallback when an attribute doesn't exist on the object.

DanielNoord commented 1 month ago

The fix should be made here: https://github.com/pylint-dev/pylint/blob/d5bc17026d2668f93b300c6b3b0eedfb4f3eab9c/pylint/pyreverse/diadefslib.py#L200

pyreverse incorrectly assumes that klass = next(module.ilookup(klass)) still gives klass a type of ClassDef while that is clearly not the case.

I'm moving this issue to the pylint repository and renaming it.

pylint-dev / pylint