Add position attribute for nodes

[cdce8p]

Description

Supersedes #1390
Ref pylint-dev/pylint#5466

Opened a new PR to preserve the original idea and discussion.

This PR adds the new (optional) attribute position to NodeNG. It should be used to highlight the position / range of keyword(s) and name for block nodes such as ClassDef and FunctionDef where the AST nodes don't include good enough positional information.

class HelloWorld(Parent):
^^^^^^^^^^^^^^^^

    async def func(var: int = 2):
    ^^^^^^^^^^^^^^
        ...

Additional block nodes for which it could make sense include Match, MatchCase, ExceptHandler, (and later WithItem). To a limited extend, If, For, and While might also work.

Type of Changes

	Type
✓	✨ New feature

[Pierre-Sassoulas]

LGTM, the tests in particular look great.

[Pierre-Sassoulas]

We're going to remove python 3.6 support in 2.11, I guess we'll just have to remove that part when we actually do it ?

[cdce8p]

Yeah. For a moment I thought about just ignoring the error in Python 3.6. Then again, that would be cheating. In the end, when updating we're gonna search for PY36 anyway. It's also trivial to update.

[Pierre-Sassoulas]

Great utility class !

[DanielNoord]

Looks very good @cdce8p

One day I'll write such good first-try PRs as well 😄

[DanielNoord]

Is this a false positive? Or will mypy also require this in the future?

[cdce8p]

One of the rare instances where mypy is smarter than pyright. I'll probably report that error soon. So might as well remove it now.

[DanielNoord]

Shall we add one test with a decorator? I tested and it works fine currently, but it might be good to have it in there as well.

[cdce8p]

Depends on

• Fix ClassDef.fromlineno for Python < 3.8 #1395

[cdce8p]

Some last open questions

1. Should the attribute be named position or something else? Normally, range would describe it better, but that feels wrong in this context somehow. Any (other) ideas?
2. Should the NamedTuple be renamed to Position. Range feels better, but it could get confusing with an attribute named position.
3. Should the NamedTuple be included in the scoped_nodes package __all__? I'm 50/50 on that one. Usually it shouldn't be necessary; it doesn't hurt though.
4. Should we add the "performance hack" for tokenize in astroid? Since we would be overwriting the default implementation, this would also apply to pylint. We already import functools, so I don't think there is be a downside to doing that.
   https://bugs.python.org/issue43014
   https://github.com/PyCQA/pycodestyle/pull/993/files
   4.2. If we want to do it, where should it be placed? It should be executed before any tokenize call happens. So ideally at import (?)

[DanielNoord]

Some last open questions

1. Should the attribute be named position or something else? Normally, range would describe it better, but that feels wrong in this context somehow. Any (other) ideas?

I think position makes sense. Perhaps keyword_position?

2. Should the NamedTuple be renamed to Position. Range feels better, but it could get confusing with an attribute named position.

What about PositionRange? 😄 I mean, would we ever use this range for anything other than conveying a position?

3. Should the NamedTuple be included in the scoped_nodes package __all__? I'm 50/50 on that one. Usually it shouldn't be necessary; it doesn't hurt though.

Shall we add a todo and do so in 2.11? If we find out in pylint that we would like another attribute we would have a little from freedom to change stuff around.
I agree that it wouldn't hurt, but based on the feedback we're getting people seem to have quite different opinions about this. Being flexible for 1 or 2 releases might be welcome.

4. Should we add the "performance hack" for tokenize in astroid? Since we would be overwriting the default implementation, this would also apply to pylint. We already import functools, so I don't think there is be a downside to doing that.
   https://bugs.python.org/issue43014
   https://github.com/PyCQA/pycodestyle/pull/993/files
   4.2. If we want to do it, where should it be placed? It should be executed before any tokenize call happens. So ideally at import (?)

I think it makes sense to do so. The change is significant and we would likely forget about it later on. At import seems fine? Not sure what the official guideslines are but 1) imports, 2) conditional redefinitions, 3) typing variables seem like a logical pattern for the file start.

[Pierre-Sassoulas]

Should the NamedTuple be included in the scoped_nodes package all? I'm 50/50 on that one. Usually it shouldn't be necessary; it doesn't hurt though.

At first I was thinking that we could use it in pylint too. I think it's possible but this is one more thing to keep track off and would increase the coupling between pylint and astroid so I did not suggest it. Also increasing what we consider the astroid's API is also increasing the chance of having to deal with breaking changes later.

Should we add the "performance hack" for tokenize in astroid?

I think it's safe to do, especially considering that pycodestyle also does it.

[DanielNoord]

@Pierre-Sassoulas I see you requested a review, but there are some open questions from @cdce8p that still need to be solved.

@cdce8p Do you know if you'll have the time to look into this this week? If not, I could offer some help? I really think we should try and land this and then release 2.10 asap (postponing all other PRs). We're limiting ourselves quite a bit and development of pylint has completely died down. We are probably also losing the interest of new contributors that joined over the last couple of months.

[cdce8p]

@cdce8p Do you know if you'll have the time to look into this this week? If not, I could offer some help? I really think we should try and land this and then release 2.10 asap (postponing all other PRs). We're limiting ourselves quite a bit and development of pylint has completely died down. We are probably also losing the interest of new contributors that joined over the last couple of months.

@DanielNoord I looked at a few PRs today tagged for 2.10 and left commends there or moved them to 2.11. Still have about 4 remaining. Hope I get to them tomorrow. Any other I should know about?

Once 2.10 is released, pylint needs to be updated to use the new position attribute. Which reminds me that I missed renaming it to keyword_position which you suggested 🤦🏻‍♂️ Will open a PR for it later. (Edit: The position also include the class / function name, so keyword_position wouldn't be ideal either. Let's keep position.) Would you like to take care of that? Don't know if I'll have the time to do so.

Some things to keep in mind

• It might make sense to either opt-in or have an option to opt-out of the new position via add_message. Some errors should indeed be raised for the whole function / class.
• Updating the tests is probably just running the test script once. If you use VS Code, you could check the error positions in the editor directly.

[DanielNoord]

@DanielNoord I looked at a few PRs today tagged for 2.10 and left commends there or moved them to 2.11. Still have about 4 remaining. Hope I get to them tomorrow. Any other I should know about?

Personally I'd really like to land #1306 as it is blocking some work I'd like to do on NamedTuple inference in astroid. But it isn't necessary for 2.10 and pylint right now.

There is also the TypeVar PR(s) in pylint. For astroid I don't think there is anything that's crucial for pylint 2.13. This was the most important one.

Would you like to take care of that? Don't know if I'll have the time to do so.

Yes I will.

• It might make sense to either opt-in or have an option to opt-out of the new position via add_message. Some errors should indeed be raised for the whole function / class.

I'm going to add Position to the __init__. That way we can add a keyword only argument position to add_message and allow messages to pass their own Position.
The code in pylint would then be:

if position:
    ...
elif node.position:
    ...
elif node.lineno:
    ...
elif ...:
    ...

That would work right?

• Updating the tests is probably just running the test script once. If you use VS Code, you could check the error positions in the editor directly.

Yeah I'll do a quick inspection in VSC. Test updating should be easy.

[cdce8p]

Personally I'd really like to land #1306 as it is blocking some work I'd like to do on NamedTuple inference in astroid. But it isn't necessary for 2.10 and pylint right now.

There is also the TypeVar PR(s) in pylint. For astroid I don't think there is anything that's crucial for pylint 2.13. This was the most important one.

👌🏻 The TypeVar PR is still on my mind. Trying to catch up with astroid at the moment.

I'm going to add Position to the __init__. That way we can add a keyword only argument position to add_message and allow messages to pass their own Position. The code in pylint would then be:

Not sure if that would actually be used. Usually errors are bound to node. No harm in adding Position to __init__ though. What I was referring to primarily was an option to choice if node.position´ or node.lineno` should be used. And what the default for that should be.

• Updating the tests is probably just running the test script once. If you use VS Code, you could check the error positions in the editor directly.

Yeah I'll do a quick inspection in VSC. Test updating should be easy.

Thanks!