Add end_lineno and end_col_offset to nodes

[cdce8p]

Description

In Python 3.8, end_lineno and end_col_offset where added to all AST nodes which already had lineno and col_offset information. This PR adds them to the astoid nodes. Part of pylint-dev/pylint#5336

Type of Changes

	Type
✓	✨ New feature

Todo list

• Add docstrings for end_lineno, end_col_offset
• Write changelog entry
• Write tests for all nodes
• Decide if sys.version_info >= (3, 8) checks are ok, or if getattr(node, 'end_lineno', None) should be used.
  Note: getattr can't be checked with mypy.
• Resolve open TODOs in code
• ...

[Pierre-Sassoulas]

Looks great, I only have two minor remarks.

[Pierre-Sassoulas]

As discussed in the other issue, I don't think it's a false positive.

[cdce8p]

Even if not a false-positive, I would recommend to keep the pylint: disable as this preserves the symmetry with all other cases and will make removing the check once 3.8 is the default much easier.

[Pierre-Sassoulas]

Huhu. I'm going to stop suggesting for every test then. But why though ? I'd say it's a good thing to use PY38+ if only for the performance increase of not having to recalculate the bool everytime ?

[cdce8p]

True, I don't really like the performance impact either. On the positive side sys.version_info is special cased by type checkers so they know how to infer it. That doesn't work with PY38_PLUS.

If performance is a real concern, the only other option, I think, would be to conditional define the visit_xxx methods itself. That would mean a lot of code duplications though. Not sure we could maintain that, at least not easily.

In the end, it's only the node tree. So the performance impact might be acceptable.

--

    if sys.version_info >= (3, 8):
        def visit_constant(self, node: "ast.Constant", parent: NodeNG) -> nodes.Const:
            """visit a Constant node by returning a fresh instance of Const"""
            return nodes.Const(
                value=node.value,
                kind=node.kind,
                lineno=node.lineno,
                col_offset=node.col_offset,
                end_lineno=node.end_lineno,
                end_col_offset=node.end_col_offset,
                parent=parent,
            )
    else:
        def visit_constant(self, node: "ast.Constant", parent: NodeNG) -> nodes.Const:
            """visit a Constant node by returning a fresh instance of Const"""
            return nodes.Const(
                node.value,
                node.lineno,
                node.col_offset,
                parent,
                node.kind,
            )

[DanielNoord]

Based on the documentation of ast I think only nodes.Statement and subclasses can have an end_lineno. nodes.Expr is a subclass so that is covered as well. Does it make sense to make 1) nodes.NodeNG have end_lineno = None, 2) add an __init__ to nodes.Statement and 3) handle adding end_lineno there?
This would give better IntelliSense as nodes.Module will have end_lineno: None instead of end_lineno: Optional[int]. Because of the way we visit_ nodes in pylint this information would be accessible to use. I'm not sure if we will need but as it seems to reflect reality it might be good to adhere to ast here.

(A similar point can be made for lineno and col_offset but let's not worry about that now)

[cdce8p]

@DanielNoord

Based on the documentation of ast I think only nodes.Statement and subclasses can have an end_lineno. nodes.Expr is a subclass so that is covered as well.

nodes.Expr is, but what about all ast.expr nodes? BoolOp, Set, Dict, List, just to name a few.

Does it make sense to make 1) nodes.NodeNG have end_lineno = None, 2) add an __init__ to nodes.Statement and 3) handle adding end_lineno there? This would give better IntelliSense as nodes.Module will have end_lineno: None instead of end_lineno: Optional[int]. Because of the way we visit_ nodes in pylint this information would be accessible to use. I'm not sure if we will need but as it seems to reflect reality it might be good to adhere to ast here.

(A similar point can be made for lineno and col_offset but let's not worry about that now)

If I understand you correctly, what you would like is for IntelliSense to know that some nodes will always have end_lineno = None (same for lineno, col_offset, and end_col_offset). That can still be archived by overriding the type. E.g. for nodes.Module:

class Module(LocalsDictNodeNG):
    lineno: None
    col_offset: None
    end_lineno: None
    end_col_offset: None
    parent: None

[DanielNoord]

Ah I didn't know not all ast.Expr nodes actually subclass nodes.Expr. That makes this more difficult indeed. Your latest commit is a nice solution!

[cdce8p]

Ah I didn't know not all ast.Expr nodes actually subclass nodes.Expr.

Actually, no node subclasses ast.Expr, some subclass ast.expr but we don't represent that in astroid currently.

[cdce8p]

Took a moment to get this ready, especially writing the tests. I believe it's now ready for review.

Some additional comments regarding things I noticed while working on it:

1. end_lineno and end_col_offset are usually set regardless the annotation int | None. With the exception of the few nodes that truly don't have these information.
2. The ast module is usually considered unstable, that's even more apparent in the handling of end_lineno and end_col_offset.
3. Sometimes there are just bugs that might or might not have been fixed from one version to the next. This could make testing a bit more interesting in pylint if we require those and they vary between versions.
4. At least until 3.10 the lineno and col_offset information for JoinedStr and FormattedValue nodes don't make much sense.
5. Many AssignName nodes don't have own lineno and col_offset info. Instead they fall back to that of the parent. The reason for it is the AST sometimes only stores strings (identifier) instead of ast.Name nodes. The TreeRebuilder still interprets them as AssignName but doesn't have any individual node information. E.g. MatchMapping.rest, MatchAs.name, arg.arg (argument names).
   https://github.com/PyCQA/astroid/blob/40d556070fd566b7bb43cb3ed0e498045b88f2d0/astroid/rebuilder.py#L997-L1013
6. The handling of ast.Try nodes introduced in Python 3.3 should be adopted in TreeRebuilder. At the moment we split it into TryExcept and TryFinally the ast nodes for which have long been removed. I was thinking about adding a version argument to the TreeRebuilder initialization. That would allow us to make this a non-breaking change. Only downside being that it also adds additional maintenance and test requirements.
   https://github.com/PyCQA/astroid/blob/40d556070fd566b7bb43cb3ed0e498045b88f2d0/astroid/rebuilder.py#L1622-L1638
7. There is already a NodeNG.tolineno property. This works by looking at all astroid child nodes and using the lineno of the last one. We could consider updating it, but that isn't required for this PR.
   https://github.com/PyCQA/astroid/blob/40d556070fd566b7bb43cb3ed0e498045b88f2d0/astroid/nodes/node_ng.py#L417-L427

[Pierre-Sassoulas]

Amazing work ! Do you want to wait for 2.9.0 ? Should we use astroid 2.9.0 in pylint 2.12 ? We could bump the other 2.9.0 features in 2.10 so we can release the end column/end line.

[Pierre-Sassoulas]

I just reviewed the content of the test, not the expected column. But it comes from the ast, if it's wrong there isn't a lot we can do, right ?

[cdce8p]

True, at least not without significant effort. One area I was able to fix fairly easily was Arguments. See the snipped below. We might also be able to address the AssignName issue I mentioned by parsing the source code ourselves but that is definitely an issue for another time.

Usually the worse that can happen is that the lineno and col_offset also include the parent node. This is still correct just not as accurate as we would like it to be for error highlighting.

The true value of these tests is to know when a new version changed something. Like knowing that Keywords and Slice only include end_lineno in 3.9, or the issue with Arguments.

--
https://github.com/PyCQA/astroid/blob/7dd55fe8d96846617b2827e5272582ffc865190f/astroid/rebuilder.py#L834-L840

[DanielNoord]

Great work @cdce8p! Writing those tests must not have been an easy task.

[cdce8p]

Amazing work ! Do you want to wait for 2.9.0 ? Should we use astroid 2.9.0 in pylint 2.12 ? We could bump the other 2.9.0 features in 2.10 so we can release the end column/end line.

I would consider this a new feature that probably deserves its own minor version. I.e. release 2.9.0 next and bump the other features to 2.10.0 if that's ok with you.

[Pierre-Sassoulas]

I would consider this a new feature that probably deserves its own minor version. I.e. release 2.9.0 next and bump the other features to 2.10.0

Sounds good, do we still release 2.8.6 first ?

[cdce8p]

I would consider this a new feature that probably deserves its own minor version. I.e. release 2.9.0 next and bump the other features to 2.10.0

Sounds good, do we still release 2.8.6 first ?

Could make sense if it isn't too much effort. What is your opinion?
If we decide to, I just think we should do the release from the current main and do 2.9.0 directly afterwards with just this one. Everything currently targeted for 2.8.6 could be moved to 2.9.1.

[Pierre-Sassoulas]

Hmm, I forgot to deprecate python 3.6.0 and 3.6.1. I'm going release 2.8.7 with the deprecation and suggest astroid 2.6.6 (as it's the one working with pylint 2.9.3).