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?

[DanielNoord]

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.

Okay! I just wonder if there are differences between assigning with = and class that we need to think about. Things I can think of now are stuff like the parent attribute which might also be of a specific type for AssignName and could be different for class and def. Not sure if this is a real problem, but just something to consider. If there are real differences between the two we might need to make another subclass.

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.

👍

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?

Shouldn't be too much work right? Just this in add_message:

if isinstance(node, nodes.ClassDef):
    node.end_lineno = node.lineno
    node.end_colnum = node.colnum + 2

Or whatever the right attribute names are.

Imo, that could go in 2.13 with "better highlighting of class and function definitions messages" as a new function in 2.14 or 2.x.

[astrowonk]

A possible workaround, at least for VS Code highlighting issue, would be some command line argument that would disable sending endLine and endColumn keys in the JSON.

[Pierre-Sassoulas]

I'm thinking about moving this to 2.14 because of the amount of work involved and the number of duplicate issues we must handle that are already fixed in main and could be released :)

[Pierre-Sassoulas]

@kovla would an highlight of class ClassName and def function_name instead of the beginning of the first line of the definition would be acceptable to you ? There's been a discussion about it here : pylint-dev/astroid#1390

[kovla]

@kovla would an highlight of class ClassName and def function_name instead of the beginning of the first line of the definition would be acceptable to you ? There's been a discussion about it here : PyCQA/astroid#1390

@Pierre-Sassoulas Thanks for mentioning this. If the idea is to simply convey that there is a mistake in that line/section, underlining the first symbol (previous behavior) is the most minimalistic solution. As a user, I will explore the issue once, decide if I want to correct it, and after that all communication about it becomes merely a distraction. Hope this way of explaining it makes sense.

Sometimes I would look at unresolved issues "in batch" for the entire script, e.g. a sort of final review before a commit. In that case, I typically pay more attention to the code map highlighting, or simply to the textual list of issues. Again, extensive highlighting is not offering much advantage in this scenario.

All in all, it might be a matter of subjective preference, and, pragmatically, both approaches work. My personal preference, not that it should matter, would be to go for the minimalism (it may also help keeping underscoring consistent for different issues).

[cdce8p]

The main issue, and I think we agree on that, is highlighting the whole class / function (without reason). That will be resolved either way.

would an highlight of class ClassName and def function_name instead of the beginning of the first line of the definition would be acceptable to you ?

That's a subjective preference IMO. Maybe something a dedicated VS Code addon can explore? #5796
For now, outputting a range would be consistent with other errors which is why that would be my preference.

[cdce8p]

Reopening it to collect feedback after we release 2.13. The current changes are definitely a good start, but we might still be missing some things.

[Michaelzhouisnotwhite]

Have this feature added?

[Michaelzhouisnotwhite]

How to install pylint-2.13-dev?

[Pierre-Sassoulas]

With pip you can do pip install git+git://github.com/PyCQA/pylint.git@main#egg=pylint -U. We're also going to release 2.13, soon ™️

[cdce8p]

With pip you can do pip install git+git://github.com/PyCQA/pylint.git@main#egg=pylint -U.

The git protocol is no longer supported. You can use https instead.

pip install -U git+https://github.com/PyCQA/pylint.git@main

[Michaelzhouisnotwhite]

Thanks a lot!!

[Pierre-Sassoulas]

@cdce8p should we close now ? It seems the change was good enough that we did not get any more feedback :)

[DanielNoord]

I'm going to close this as we're nearing the end of the 2.14 milestone after 48 very productive hours. Thanks for everybody's input and feedback. Please let us know if there is anything we can improve on!

[DanielNoord]

I'm going to close this as we're nearing the end of the 2.14 milestone after 48 very productive hours. Thanks for everybody's input and feedback. Please let us know if there is anything we can improve on!

[cdce8p]

@cdce8p should we close now ? It seems the change was good enough that we did not get any more feedback :)

There are still some edge cases which can be improved, like #6606.
I've created a new label for it to track related issues / PRs: https://github.com/PyCQA/pylint/labels/Error%20range