Crash when AssignAttr is inferred as a class

[areveny]

This is a crash separately reproduced by @DanielNoord and I in this PR: pylint-dev/astroid#1373

When overwriting a module name that later gets inferred, a crash can result because we get an AssignAttr node where we normally only expect ClassDef.

There is an astroid issue tracking a very similar issue: pylint-dev/astroid#1374. The fix in pylint-django should be similar to the one for astroid, so we can wait until the astroid PR is created and merged and create an equivalent PR here.

[DanielNoord]

Just to add.
There are two possible scenario's I believe:

1. astroid is wrongly returning a AssignAttr which is then picked up by pylint and pylint-django in functions that expect a ClassDef.
2. Both pylint and pylint-django both have a function that makes a wrong assumption about parameters. I.e., the parameter can be more than only a ClassDef

We need to rule out scenario 1 before trying to fix a potential scenario 2. We'll probably do so when we investigate the issue raised against astroid itself. I'll report back when we know more about this and whether pylint-django needs to do anything.

[carlio]

The way that pylint-django uses it (as far as I can remember) is mostly for

class SomeModel(models.Model):
   ...

to know if an instance of SomeModel also gets all of the metaclass additions that the base model class does.

Therefore

class Blah():
   pass

x = Blah

class Something(x):
   pass

then pylint-django expects the node in the subclass definition to be a classdef, while the last node that x was part of is an AssignAttr.

So I think that perhaps the correct fix in node_is_subclass is to test for if it's an AssignAttr and if it is, use the right-hand side of that as the ClassDef to compare against.

However this is just speculation, I'll wait until you finish your investigation before probing further.

[areveny]

The AssignAttr nodes seem to be originating from this code in pylint_django.transforms.fields:

    elif cls.name in ("SplitDateTimeField", "DateTimeField"):
        base_nodes = MANAGER.ast_from_module_name("datetime").lookup("datetime")

datetime is looked up, and instead of only getting ClassDef for datetime.datetime, we also get an AssignAttr because the name is manually overwritten. Unfortunately I can't reproduce this with just calling lookup("datetime") on a file that overwrites datetime.datetime.

But one potential fix at this level would filter out AssignAttr from base_nodes just like we already filter out import_from... @carlio it looks like you ran into a very similar problem 6 years ago and implemented this filter!

4840b73

    # XXX: for some reason, with python3, this particular line triggers a
    # check in the StdlibChecker for deprecated methods; one of these nodes
    # is an ImportFrom which has no qname() method, causing the checker
    # to die...
    if utils.PY3:
        base_nodes = [n for n in base_nodes[1] if not isinstance(n, nodes.ImportFrom)]
        base_nodes = [n for n in base_nodes if not isinstance(n, nodes.AssignAttr)]

This prevents the crash.

[DanielNoord]

I need to see some code to precisely see what you're suggesting here, but can't we use this in combination with @carlio suggestion of taking the actual right-hand value that is being assigned in the AssignAttr? We should probably check if that right-hand value is a ClassDef but if I understand what you're saying correctly that should work right?

[areveny]

Yup, the filter was more of a proof of concept. We could do something like,

n.parent.value if isinstance(n, "AssignAttr") and isinstance(n.parent.value, "ClassDef") else n

Though n.parent.value is a Name: FakeDateTime, do we have to infer to see if it refers to aClassDef?

[DanielNoord]

I guess so? We could also return Uninferable if things start to get too complicated? If you have some WIP code you could make a PR to show me the exact situation we're dealing with. It's becoming a little too abstract for me to comment on it without seeing the exact code.

[areveny]

Added a draft PR to process nodes so they always have a qname.

[areveny]

@DanielNoord @carlio

I lost track of this bug for a bit but it would be good to close. What do we think of the PR? It follows the suggestion from @carlio above to get the value of the AssignAttr and return it instead of the AssignAttr itself.

[carlio]

It makes sense to me to do it this way, and as the tests are passing, I'm happy to merge and release :-) Thanks @areveny !