Fix ClassDef.fromlineno for Python < 3.8

[cdce8p]

Description

Another unexpected AST change. For Python < 3.8 the lineno for ClassDef nodes also includes decorator nodes. @DanielNoord noticed that one while working with builder.extract_node in #1391. I encountered the same issue in #1393.

The fix isn't perfect unfortunately. Multiline decorators without dedicated AST nodes on the last line don't quite work. That however is consistent with FunctionDef.fromlineno which has already been modified.

@decorator(
    var=42
)
class A:
    ...

# Here the reported line would be ')' instead of 'class A'.
# We can't use 'end_lineno', since it was only added in Python 3.8

--
The current property for FunctionDef:

https://github.com/PyCQA/astroid/blob/552f1c19aab0719e7b30bb02e032d4bfe655f343/astroid/nodes/scoped_nodes/scoped_nodes.py#L1708-L1722

Type of Changes

	Type
✓	🐛 Bug fix

[DanielNoord]

👍

[DanielNoord]

I'd say we can remove the discussable here? Python 3.8+ has clearly made a choice and I think we have made a choice in pylint as well.
I think this "discussion" has been solved.

[cdce8p]

👌🏻 Yeah, for ClassDef I think it is resolved. Although to be fair, pylint was fine with either option.
I'll leave the comment for FunctionDef.fromlineno in place though, since this still differs from the stdlib.

[Pierre-Sassoulas]

👍

[DanielNoord]

Just for my personal understanding:
Why don't we set lineno on a ClassDef to the value returned by fromlineno? Is there a difference between the two?

[cdce8p]

Just for my personal understanding:
Why don't we set lineno on a ClassDef to the value returned by fromlineno? Is there a difference between the two?

Usually, it's the other way round. fromlineno is set to the value of lineno. Tbh though, I'm not completely sure what the difference between them should be. Maybe that lineno is the extracted value from the AST, whereas fromlineno also works for nodes which don't have any positional information.

We could think about patching the lineno for Python < 3.8. The issue there however is that we don't always get the correct, so not sure it's worth it. If we do that, it should probably be done in TreeRebuilder.

--
I mentioned FunctionDef.lineno earlier. After looking through the code again, it's actually the case that Python 3.8+ is consistent with ClassDef.lineno. Both AST nodes don't include decorators. However, we override that behavior. Maybe something we should change for astroid 3.0 to be consistent?

Update
Just tested pylint, changing that would be a massive breaking change unfortunately. 80 failed functional tests.

https://github.com/PyCQA/astroid/blob/cfd9e74f7b4cbac08357cadec03c736501368afa/astroid/rebuilder.py#L1513-L1522

[DanielNoord]

Usually, it's the other way round. fromlineno is set to the value of lineno. Tbh though, I'm not completely sure what the difference between them should be. Maybe that lineno is the extracted value from the AST, whereas fromlineno also works for nodes which don't have any positional information.

Can a case be made that fromlineno should become lineno then? Why would you want to access the extracted value from the AST without any fixes that we might apply?

We could think about patching the lineno for Python < 3.8. The issue there however is that we don't always get the correct, so not sure it's worth it. If we do that, it should probably be done in TreeRebuilder.

-- I mentioned FunctionDef.lineno earlier. After looking through the code again, it's actually the case that Python 3.8+ is consistent with ClassDef.lineno. Both AST nodes don't include decorators. However, we override that behavior. Maybe something we should change for astroid 3.0 to be consistent?

Update Just tested pylint, changing that would be a massive breaking change unfortunately. 80 failed functional tests.

https://github.com/PyCQA/astroid/blob/cfd9e74f7b4cbac08357cadec03c736501368afa/astroid/rebuilder.py#L1513-L1522

Ah you already tested this. Are they failing by not giving the right messages or just requiring an updating of the position?

[cdce8p]

Usually, it's the other way round. fromlineno is set to the value of lineno. Tbh though, I'm not completely sure what the difference between them should be. Maybe that lineno is the extracted value from the AST, whereas fromlineno also works for nodes which don't have any positional information.

Can a case be made that fromlineno should become lineno then? Why would you want to access the extracted value from the AST without any fixes that we might apply?

Don't know about that. Just my opinion, but I do appreciate knowing that lineno is something concrete from the AST which is only modified for backwards compatibility. For the nodes which don't have position attributes, there is fromlineno and tolineno as fallback. Knowing that those might not be 100% correct.

Unless there is a strong reason, I would suggest to just leave it. We have may other areas to deal with.

Ah you already tested this. Are they failing by not giving the right messages or just requiring an updating of the position?

Not the right message if I remember correctly. Although I haven't checked how much work it would actual be to update the code.