Add `qname` attribute to `AssignAttr` to prevent crashes #1373

areveny · 2022-01-28T06:11:56Z

This change prevents a crash that occurs when reassigning module names and retrieving their
qname. In some constructions an AssignAttr is inferred and treated as a ClassDef.

Steps

For new features or bug fixes, add a ChangeLog entry describing what your PR does.
Write a good description on what the PR does.

Description

The testing package freezegun includes this construction, which overrides a module name.

        datetime.datetime = FakeDatetime
        datetime.date = FakeDate

This can cause an AssignAttr to become part of an inference for a class definition and appear where we normally expect only ClassDef.

This causes the crash in the linked issue.

The below file also creates a related crash in pylint_django.

from freezegun import freeze_time
import datetime

# Change the modules
datetime.datetime = FakeDatetime
datetime.date = FakeDate

with freeze_time():
# with freeze_time(localized_utcnow()):
    pass

from django.db.models.fields import DateField

from model_utils.fields import (
    AutoCreatedField,
)
# created = AutoCreatedField()
DateField()

  File "/code/open_source/astroid/astroid/transforms.py", line 45, in _transform
    if predicate is None or predicate(node):
  File "/code/open_source/license-manager/.venv/lib/python3.9/site-packages/pylint_django/transforms/foreignkey.py", line 17, in is_foreignkey_in_class
    is_in_django_model_class = node_is_subclass(node.parent.parent, "django.db.models.base.Model", ".Model")
  File "/code/open_source/license-manager/.venv/lib/python3.9/site-packages/pylint_django/utils.py", line 26, in node_is_subclass
    if inf.qname() in subclass_names:

The most straightforward fix is to give AssignAttr a qname method like ClassDef have. For the AssignAttr datetime, the second one, this would be datetime.datetime.

This fixes the crashes described above and passes the Astroid tests and Pylint tests on Python 3.9.

I have not been able to create a concise reproduction example, so I am not sure how to test it. The examples we have seem to involve the intersection of Django and Freezegun. I have already spent a decent effort trying to figure out exactly what causes the crash without much headway, but the fix is so straightforward and seems safe enough that I wanted to just submit it and see what we think about merging it anyways.

Type of Changes

	Type
✓	🐛 Bug fix

Related Issue

Closes pylint-dev/pylint#5717

This change prevents a crash that occurs when reassigning module names and retrieving their `qname`. In some constructions an `AssignAttr` is inferred and treated as a `ClassDef`.

for more information, see https://pre-commit.ci

DanielNoord · 2022-01-28T08:22:25Z

I have brought the crashing code down to. Class names resemble those found in the original offending code:
test.py:

import datetime

# Change the modules
datetime.date = FakeDate

from django.db.models.sql.query import Query


class Lookup:
    def __init__(self, param):
        if isinstance(param, Query):
            raise NotImplementedError


Lookup()

Where django.db.models.sql.query is:

class Query:
    ...

This crashes with pylint-django 2.5.0.

The crash doesn't necessarily seem to be in astroid, but actually in pylint-django. This is the stacktrace:

pylint crashed with a ``AttributeError`` and with the following stacktrace:

Traceback (most recent call last):
  File "/Users/daniel/.pyenv/versions/3.10.0/envs/astroid-3.10.0/lib/python3.10/site-packages/pylint/lint/pylinter.py", line 1034, in _check_files
    self._check_file(get_ast, check_astroid_module, file)
  File "/Users/daniel/.pyenv/versions/3.10.0/envs/astroid-3.10.0/lib/python3.10/site-packages/pylint/lint/pylinter.py", line 1069, in _check_file
    check_astroid_module(ast_node)
  File "/Users/daniel/.pyenv/versions/3.10.0/envs/astroid-3.10.0/lib/python3.10/site-packages/pylint/lint/pylinter.py", line 1203, in check_astroid_module
    retval = self._check_astroid_module(
  File "/Users/daniel/.pyenv/versions/3.10.0/envs/astroid-3.10.0/lib/python3.10/site-packages/pylint/lint/pylinter.py", line 1250, in _check_astroid_module
    walker.walk(node)
  File "/Users/daniel/.pyenv/versions/3.10.0/envs/astroid-3.10.0/lib/python3.10/site-packages/pylint/utils/ast_walker.py", line 75, in walk
    self.walk(child)
  File "/Users/daniel/.pyenv/versions/3.10.0/envs/astroid-3.10.0/lib/python3.10/site-packages/pylint/utils/ast_walker.py", line 72, in walk
    callback(astroid)
  File "/Users/daniel/.pyenv/versions/3.10.0/envs/astroid-3.10.0/lib/python3.10/site-packages/pylint/checkers/variables.py", line 1339, in visit_importfrom
    module = self._check_module_attrs(node, module, name_parts[1:])
  File "/Users/daniel/.pyenv/versions/3.10.0/envs/astroid-3.10.0/lib/python3.10/site-packages/pylint/checkers/variables.py", line 2168, in _check_module_attrs
    module = next(module.getattr(name)[0].infer())
  File "/Users/daniel/.pyenv/versions/3.10.0/envs/astroid-3.10.0/lib/python3.10/site-packages/astroid/nodes/scoped_nodes/scoped_nodes.py", line 622, in getattr
    result = [self.import_module(name, relative_only=True)]
  File "/Users/daniel/.pyenv/versions/3.10.0/envs/astroid-3.10.0/lib/python3.10/site-packages/astroid/nodes/scoped_nodes/scoped_nodes.py", line 739, in import_module
    return AstroidManager().ast_from_module_name(absmodname)
  File "/Users/daniel/.pyenv/versions/3.10.0/envs/astroid-3.10.0/lib/python3.10/site-packages/astroid/manager.py", line 211, in ast_from_module_name
    return self.ast_from_file(found_spec.location, modname, fallback=False)
  File "/Users/daniel/.pyenv/versions/3.10.0/envs/astroid-3.10.0/lib/python3.10/site-packages/astroid/manager.py", line 124, in ast_from_file
    return AstroidBuilder(self).file_build(filepath, modname)
  File "/Users/daniel/.pyenv/versions/3.10.0/envs/astroid-3.10.0/lib/python3.10/site-packages/astroid/builder.py", line 151, in file_build
    return self._post_build(module, encoding)
  File "/Users/daniel/.pyenv/versions/3.10.0/envs/astroid-3.10.0/lib/python3.10/site-packages/astroid/builder.py", line 168, in _post_build
    self.add_from_names_to_locals(from_node)
  File "/Users/daniel/.pyenv/versions/3.10.0/envs/astroid-3.10.0/lib/python3.10/site-packages/astroid/builder.py", line 224, in add_from_names_to_locals
    imported = node.do_import_module()
  File "/Users/daniel/.pyenv/versions/3.10.0/envs/astroid-3.10.0/lib/python3.10/site-packages/astroid/mixins.py", line 108, in do_import_module
    return mymodule.import_module(
  File "/Users/daniel/.pyenv/versions/3.10.0/envs/astroid-3.10.0/lib/python3.10/site-packages/astroid/nodes/scoped_nodes/scoped_nodes.py", line 739, in import_module
    return AstroidManager().ast_from_module_name(absmodname)
  File "/Users/daniel/.pyenv/versions/3.10.0/envs/astroid-3.10.0/lib/python3.10/site-packages/astroid/manager.py", line 211, in ast_from_module_name
    return self.ast_from_file(found_spec.location, modname, fallback=False)
  File "/Users/daniel/.pyenv/versions/3.10.0/envs/astroid-3.10.0/lib/python3.10/site-packages/astroid/manager.py", line 124, in ast_from_file
    return AstroidBuilder(self).file_build(filepath, modname)
  File "/Users/daniel/.pyenv/versions/3.10.0/envs/astroid-3.10.0/lib/python3.10/site-packages/astroid/builder.py", line 151, in file_build
    return self._post_build(module, encoding)
  File "/Users/daniel/.pyenv/versions/3.10.0/envs/astroid-3.10.0/lib/python3.10/site-packages/astroid/builder.py", line 175, in _post_build
    module = self._manager.visit_transforms(module)
  File "/Users/daniel/.pyenv/versions/3.10.0/envs/astroid-3.10.0/lib/python3.10/site-packages/astroid/manager.py", line 101, in visit_transforms
    return self._transform.visit(node)
  File "/Users/daniel/.pyenv/versions/3.10.0/envs/astroid-3.10.0/lib/python3.10/site-packages/astroid/transforms.py", line 96, in visit
    return self._visit(module)
  File "/Users/daniel/.pyenv/versions/3.10.0/envs/astroid-3.10.0/lib/python3.10/site-packages/astroid/transforms.py", line 61, in _visit
    visited = self._visit_generic(value)
  File "/Users/daniel/.pyenv/versions/3.10.0/envs/astroid-3.10.0/lib/python3.10/site-packages/astroid/transforms.py", line 68, in _visit_generic
    return [self._visit_generic(child) for child in node]
  File "/Users/daniel/.pyenv/versions/3.10.0/envs/astroid-3.10.0/lib/python3.10/site-packages/astroid/transforms.py", line 68, in <listcomp>
    return [self._visit_generic(child) for child in node]
  File "/Users/daniel/.pyenv/versions/3.10.0/envs/astroid-3.10.0/lib/python3.10/site-packages/astroid/transforms.py", line 74, in _visit_generic
    return self._visit(node)
  File "/Users/daniel/.pyenv/versions/3.10.0/envs/astroid-3.10.0/lib/python3.10/site-packages/astroid/transforms.py", line 61, in _visit
    visited = self._visit_generic(value)
  File "/Users/daniel/.pyenv/versions/3.10.0/envs/astroid-3.10.0/lib/python3.10/site-packages/astroid/transforms.py", line 68, in _visit_generic
    return [self._visit_generic(child) for child in node]
  File "/Users/daniel/.pyenv/versions/3.10.0/envs/astroid-3.10.0/lib/python3.10/site-packages/astroid/transforms.py", line 68, in <listcomp>
    return [self._visit_generic(child) for child in node]
  File "/Users/daniel/.pyenv/versions/3.10.0/envs/astroid-3.10.0/lib/python3.10/site-packages/astroid/transforms.py", line 74, in _visit_generic
    return self._visit(node)
  File "/Users/daniel/.pyenv/versions/3.10.0/envs/astroid-3.10.0/lib/python3.10/site-packages/astroid/transforms.py", line 61, in _visit
    visited = self._visit_generic(value)
  File "/Users/daniel/.pyenv/versions/3.10.0/envs/astroid-3.10.0/lib/python3.10/site-packages/astroid/transforms.py", line 74, in _visit_generic
    return self._visit(node)
  File "/Users/daniel/.pyenv/versions/3.10.0/envs/astroid-3.10.0/lib/python3.10/site-packages/astroid/transforms.py", line 64, in _visit
    return self._transform(node)
  File "/Users/daniel/.pyenv/versions/3.10.0/envs/astroid-3.10.0/lib/python3.10/site-packages/astroid/transforms.py", line 45, in _transform
    if predicate is None or predicate(node):
  File "/Users/daniel/.pyenv/versions/3.10.0/envs/astroid-3.10.0/lib/python3.10/site-packages/pylint_django/transforms/foreignkey.py", line 17, in is_foreignkey_in_class
    is_in_django_model_class = node_is_subclass(node.parent.parent, "django.db.models.base.Model", ".Model")
  File "/Users/daniel/.pyenv/versions/3.10.0/envs/astroid-3.10.0/lib/python3.10/site-packages/pylint_django/utils.py", line 24, in node_is_subclass
    if inf.qname() in subclass_names:
AttributeError: 'AssignAttr' object has no attribute 'qname'

The offending line seems to be in node_is_subclass in utils of pylint-django. I think it would be good to raise this against them before committing anything to astroid. @areveny Do you want to do this? Feel free to tag me in the new issue.

areveny · 2022-01-28T14:59:17Z

Thanks! This is really great.

I find that the node_is_subclass issue isn't causing the linked pylint crash, and that node_is_subclass is a separate but related issue. We will have to change _is_metaclass in scoped_nodes.py of astroid as well.

So our approach seems to be:

Guard the checks to qname in _is_metaclass and node_is_subclass (and anywhere else we can find it) against AssignAttr.
Also give AssignAttr a qname method. Is this still valid? It's not necessary for fixing the crashes assuming we do point 1, and would expose a new interface on AssignAttr.

Let me know what you think.

DanielNoord · 2022-01-28T15:07:28Z

Thanks! This is really great.

I find that the node_is_subclass issue isn't causing the linked pylint crash, and that node_is_subclass is a separate but related issue. We will have to change _is_metaclass in scoped_nodes.py of astroid as well.

Oh wow, have I just found another crash while trying to reproduce the crash of this issue? 😅
I didn't bother checking the stack traces, but they are indeed different.. Wow...

So our approach seems to be:

Guard the checks to qname in _is_metaclass and node_is_subclass (and anywhere else we can find it) against AssignAttr.

I think this is the best approach, although I would like a small investigation as to why AssignAttr can be passed there anyway. It feels like there should be a short-circuit before we enter those functions.

Also give AssignAttr a qname method. Is this still valid? It's not necessary for fixing the crashes assuming we do point 1, and would expose a new interface on AssignAttr.

I don't think that is the right way forward. That seems to add a method just so we don't crash. qname doesn't make a lot of sense on AssignAttr right?

areveny · 2022-01-28T15:25:18Z

Okay, great. Agree on AssignAttr.qname. I will close this PR, and open an issue in astroid for tracking the qname fix here, and another issue in pylint_django for theirs.

I can look again into how AssignAttr enters into the inference list, but it's not easy. The stack trace is quite tall by the time we ever hit any of these issues... Any fix should be straightforward but it would be good to investigate this thoroughly to get the right fix.

That investigation can be discussed in the astroid issue.

areveny and others added 2 commits January 27, 2022 23:54

Add qname attribute to AssignAttr

aac4fd4

This change prevents a crash that occurs when reassigning module names and retrieving their `qname`. In some constructions an `AssignAttr` is inferred and treated as a `ClassDef`.

[pre-commit.ci] auto fixes from pre-commit.com hooks

dd7c47f

for more information, see https://pre-commit.ci

areveny force-pushed the assign-attr-qname branch from a0cac85 to dd7c47f Compare January 28, 2022 06:22

DanielNoord mentioned this pull request Jan 28, 2022

Crash 'AssignAttr' object has no attribute 'qname' pylint-dev/pylint#5717

Closed

areveny closed this Jan 28, 2022

This was referenced Jan 28, 2022

Crash when AssignAttr is inferred as a class #1374

Closed

Crash when AssignAttr is inferred as a class pylint-dev/pylint-django#349

Closed

jacobtylerwalls mentioned this pull request Jan 17, 2023

Fixed a crash when inferring a value and using its qname on a slice pylint-dev/pylint#8070

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `qname` attribute to `AssignAttr` to prevent crashes #1373

Add `qname` attribute to `AssignAttr` to prevent crashes #1373

areveny commented Jan 28, 2022 •

edited

Loading

DanielNoord commented Jan 28, 2022

areveny commented Jan 28, 2022 •

edited

Loading

DanielNoord commented Jan 28, 2022

areveny commented Jan 28, 2022

Add qname attribute to AssignAttr to prevent crashes #1373

Add qname attribute to AssignAttr to prevent crashes #1373

Conversation

areveny commented Jan 28, 2022 • edited Loading

Steps

Description

Type of Changes

Related Issue

DanielNoord commented Jan 28, 2022

areveny commented Jan 28, 2022 • edited Loading

DanielNoord commented Jan 28, 2022

areveny commented Jan 28, 2022

Add `qname` attribute to `AssignAttr` to prevent crashes #1373

Add `qname` attribute to `AssignAttr` to prevent crashes #1373

areveny commented Jan 28, 2022 •

edited

Loading

areveny commented Jan 28, 2022 •

edited

Loading