Stable API for all node name

[Pierre-Sassoulas]

Current behavior

We often have the following code in pylint:

if isinstance(node, nodes.Attribute):
    name = node.attrname
else:
    name = node.name

This is the reason for a lot of crashes too.

Expected behavior

Maybe it would be a good thing to have a name attribute that return attrname if required ? (I don't think if we have nodes who have both name and attrname.)

[DanielNoord]

I looked at ast and thought it might derive from there, but they actually use attr.

I think it could make sense, but on the other hand: how often would this save an isinstance? Isn't it more common to have:

def func(node: nodes.NodeNG):
    if isinstance(node, nodes.Name):
        name = node.name
    elif isinstance(node, nodes.Attribute):
        name = node.attrname
    else:
        # do something for other types.

How often do we have a situation where a node is nodes.Attribute or nodes.Name without a preceding isinstance check.
This would only allow to refactor code that resembles the following pattern:

def func(node: nodes.NodeNG):
    if isinstance(node, (nodes.Name, nodes.Attribute)):
        if isinstance(node, nodes.Attribute):
            name = node.attrname
        else:
            name = node.name
    else:
        # do something for other types.

I'm not sure that happens often?

If this allows many simplifications I would support it, if it's easier to just refactor some isinstance checks in pylint I don't think we should expand on the already quite large API of nodes.

[clavedeluna]

What about an API requirement that every kind of node has an

@property
def name(self):
     return ... # self.attrname, etc

so no need to do isinstance checks and instead it relies on inheritance. For nodes that already have a name attr, great, for those that don't, it's an alias for attrname

[DanielNoord]

What would be the name of a nodes.With? Or nodes.TryExcept?

[clavedeluna]

What would be the name of a nodes.With? Or nodes.TryExcept?

Good point. I can see those two don't have something like a name attribute. One option would be to

@property
def name(self):
     return "" #or None

to stick to the API as an initial iteration.

[DanielNoord]

That doesn't really make a lot of sense and would only be necessary to make them conform to an API. As we're still able to design that API we shouldn't introduce such weird edge cases into it.

[Pierre-Sassoulas]

I'm not sure that happens often?

I'm under the impression we had multiple crash in release because of that (especially since we want to make better error message). We can also remember that everything that is a variable can be a class attribute too and ask for more functional test during review in pylint. But the cost of forgetting is a crash, so "fixing it" in astroid directly might make sense.

[DanielNoord]

I'm not sure that happens often?

I'm under the impression we had multiple crash in release because of that (especially since we want to make better error message). We can also remember that everything that is a variable can be a class attribute too and ask for more functional test during review in pylint. But the cost of forgetting is a crash, so "fixing it" in astroid directly might make sense.

There are some:
https://github.com/PyCQA/pylint/issues?q=%22%27Attribute%27+object+has+no+attribute+%27name%27%22+is%3Aissue+is%3Aclosed

But with correct typing and a py.typed in astroid this should be resolved. I think it's better to focus there than add something to our API that could be resolved by things we are already working on.

[Pierre-Sassoulas]

Closing in favor of #1287