Better error ranges for classes and functions

[cdce8p]

The error ranges for Class and Function nodes should be improved. At the moment they span the whole length of the corresponding block which isn't ideal, especially once error range support in VSCode is added.

Ideally, those should only highlight the actual class or function name.

def func2():
    return

class SomeCls:
    def __init__(self):
        """"""
        pass


def some_decorator_function():
    """some docstring"""

class invalid_class_name:
    pass

@some_decorator_function()
def invalidFunctionName():
    pass

[DanielNoord]

@cdce8p Do you want to fix this by changing the end_line for nodes.ClassDef in astroid or do a check in add_message like:

if isinstance(node, nodes.ClassDef):
    end_lineno = lineno

[Pierre-Sassoulas]

I think it should be done in pylint: the end lineno from astroid is accurate and useful (maybe used in other libs too). The current issue is linked to the way pylint needs to display errors for class and function.

[cdce8p]

I agree, we shouldn't change the end_line attributes itself. They are still correct. What we need is some better way to indicate at which range the actual class / function name is defined. We can't use the AST for it, unfortunately.

Thinking out loud for a moment, it should be somewhat strait forward to do some basic string parsing. Maybe it would make sense to modify the astroid/rebuilder.py, ie. add an additional field for it. That's a long shoot though.

For a start, it would already be an improvement to modify add_message so the error is only emitted for the first line. It won't be perfect but definitely an improvement.

[kovla]

Highlighting entire code block is very distracting, to be honest. A lot of unnecessary highlighting is done to communicate something very simple or something that the developer choses not to change.

Old behavior (highlight just the beginning of the affected line) was concise and effective enough. Please at least provide an option to change this behavior via settings. Thanks!

[DanielNoord]

@cdce8p Have you thought about how to go about this? I actually struggled to find an easy and quick implementation to either 1) find the end of the class or function definition or 2) find the end_column of the first line of a definition. Either would work for this I think.

[cdce8p]

Have you thought about how to go about this?

Instead of adding some new attribute for the end_lineno, wouldn't it be better if ClassDef.name / FunctionDef.name were actual nodes instead of just strings? The problem we already identified is that the Python AST only stores the string itself, ie. not enough information to create a Name node for it. Since those are fairly strict patterns (class ..., def ..., async def ...), it should however be possible to do some string parsing in order to extract the position information from the code.

The next steps and challenges

• Changing the name attribute from str to a dedicated Node would probably be a breaking change. Ideally we would need a way to update the astroid TreeRebuilder such that it doesn't immediately break libraries dependent on astroid. That will also make our own lives easier when updating pylint. I already started working on it.
  Just to be clear though, even if the tree changes aren't breaking changes for astroid, they still could be for pylint pluings. We would need to decide how to handle the pylint version number¹.
• Update the TreeRebuilder to parse the source code itself in order to detect the position for class and function names.
• Update pylint to use the new TreeRebuilder version. This will likely require a lot of work, but doable!
• Lastly, identify all error messages that are emitted on a ClassDef, FunctionDef, or AsyncFunctionDef and update them to use the name node instead.

Where do I need help?

• If we had a pre-compiled list of problematic error messages, that would certainly help when updating each of them.
• Additionally, any help with pylint will be useful. We might need to update a lot of code, but tbh I don't know that yet.
• Decide how we want to handle the pylint version updates if the TreeRebuilder changes.

Footnotes

1. It's a difficult decision, but I would argue, these are still functional changes, no breaking once, for the end user. This won't be the last time we update the tree, do we do a new major version for each small change? Even if they might only break plugins. That isn't feasible. Better to do it with a minor version change and include a noticeable warning in the What's new section. ↩

[DanielNoord]

Lot to unpack here.

Instead of adding some new attribute for the end_lineno, wouldn't it be better if ClassDef.name / FunctionDef.name were actual nodes instead of just strings? The problem we already identified is that the Python AST only stores the string itself, ie. not enough information to create a Name node for it. Since those are fairly strict patterns (class ..., def ..., async def ...), it should however be possible to do some string parsing in order to extract the position information from the code.

Although I like making the name attribute "smarter" I'm not sure it should be a Name node. Name is used to represent variables and mimics (in a way) ast.Name. def fun(param: int) -> bool: doesn't really fall jn the same category as the x in x = 1 in my opinion. Perhaps we should think about something else? A NamedTuple or another datatype that can convey information about the "assignment position" of the function or class definition.

The next steps and challenges

• Changing the name attribute from str to a dedicated Node would probably be a breaking change. Ideally we would need a way to update the astroid TreeRebuilder such that it doesn't immediately break libraries dependent on astroid. That will also make our own lives easier when updating pylint. I already started working on it.

I trust you're thinking about this carefully as you always do with breaking changes 😄

• Lastly, identify all error messages that are emitted on a ClassDef, FunctionDef, or AsyncFunctionDef and update them to use the name node instead.

Where do I need help?

• If we had a pre-compiled list of problematic error messages, that would certainly help when updating each of them.

This should be fairly easy with a simple script/print statements on the test suite. I'm happy to help here.

Footnotes

1. It's a difficult decision, but I would argue, these are still functional changes, no breaking once, for the end user. This won't be the last time we update the tree, do we do a new major version for each small change? Even if they might only break plugins. That isn't feasible. Better to do it with a minor version change and include a noticeable warning in the What's new section. ↩

Perhaps it would be good to discuss a possible longterm roadmap for astroid at some point to see if we can group or schedule these potentially breaking changes. That way we get a better overview of all of our personal mental todo lists 😄

[DanielNoord]

Btw, perhaps in the meantime we should change ClassDef and FunctionDef to only raise on the first 3 columns of the first line (that should never exceed either def or class) as not to annoy our users while we're working on this.

[cdce8p]

Although I like making the name attribute "smarter" I'm not sure it should be a Name node. Name is used to represent variables and mimics (in a way) ast.Name. def fun(param: int) -> bool: doesn't really fall jn the same category as the x in x = 1 in my opinion. Perhaps we should think about something else? A NamedTuple or another datatype that can convey information about the "assignment position" of the function or class definition.

I trust you're thinking about this carefully as you always do with breaking changes

I'm certainly still open for ideas how to do it better, maybe even without a breaking change. However, what I've found while thing about it is that contextual it would make use Name nodes for them, more specifically AssignName nodes.

The main reason is probably that we already do that in other places (ExceptHandler.name, MatchMapping.rest, MatchStar.name, MatchAs.name). We convert the string from the AST into a dedicated node. At the moment though we only copy the position information from the parent. That could certainly be improved once code is in place to manually parse the source code.

Furthermore, an AssignName node "is a name of something that is assigned to." (from the docstring). Basically the class / function definition is assigned to the class / function name.

Perhaps it would be good to discuss a possible longterm roadmap for astroid at some point to see if we can group or schedule these potentially breaking changes. That way we get a better overview of all of our personal mental todo lists

Just a rough list of the top of my head. (only the breaking changes - for plugins)

• Change ClassDef.name, FunctionDef.name into dedicated nodes.
• Replace old TryExcept and TryFinally nodes with new Try node.
• Update Import and ImportFrom. In particular with Alias nodes.

Additionally, update the position information for the AssignName nodes mentioned earlier. That wouldn't be a breaking change however.

Btw, perhaps in the meantime we should change ClassDef and FunctionDef to only raise on the first 3 columns of the first line (that should never exceed either def or class) as not to annoy our users while we're working on this.

That would only make sense if we want to do a 2.12.3 release. It could make sense doing a fix just for that, but it's also quite a bit of work. What do you think @Pierre-Sassoulas?