gh-93351: Ensure the position information in AST nodes created by the parser is always consistent #93352
Conversation
I think we should also do the same in the AST validator, otherwise this would lead an interpreter crash on the debug mode when the user tries to compile a malformed AST node object.
>>> tree = ast.parse("a = 1")
>>> tree.body[0].lineno = 0
>>> compile(tree, "<test>", "exec")
python: Python/Python-ast.c:2118: _PyAST_Assign: Assertion `p->lineno != 0 && p->lineno <= p->end_lineno' failed.
[1] 26880 IOT instruction (core dumped) ./python
|
Or maybe we should only limit this to nodes created by the parser. WDYT @pablogsal? Otherwise, I feel like it might break some code (even though it is clearly invalid). For example this now works, but if we start validating the line numbers for user created ASTs it will stop working: |
I'm preparing the other PR as we speak :) I still prefer to keep these separated as the other PR fixes a reported issue with pytest |
|
@isidentical Hummmmm there is an ordering problem here. We create the C nodes before we validate them, so the assert will trigger before we can raise a nice exception. Maybe we should add the check only on the validation step..... what do you think? There is a separate problem which is that end_line_number defaults to 0, but if the user is setting just the line number currently then that leaves the node as invalid. I plan to add a check in the AST constructor to correct this, but we can just say "well, that's invalid now". |
|
I have changed to PR to just adding the check in the AST validator. |
I think that's definietly better (and since we are running the validator on debug mode even for the regular parser outputs, I think it should cover all the bases). |
|
Should we drop the backport labels since this is theoritically a breaking change @pablogsal? Otherwise it LGTM (with a news entry). |
LGTM (though there might be some missing cases for the validation , like excepthandler?)
Python/ast.c
Outdated
| return 0; \ | ||
| } \ | ||
| if ((node->lineno < 0 && node->end_lineno != node->lineno) || \ | ||
| (node->col_offset < 0 && node->col_offset != node->end_col_offset)) { \ |
There was a problem hiding this comment.
NIT:
| (node->col_offset < 0 && node->col_offset != node->end_col_offset)) { \ | |
| (node->col_offset < 0 && node->col_offset != node->end_col_offset)) { \ |
For 3.10 yes, but for 3.11 no because failing to do this causes the compiler to break. |
I added two more in the last commit. |
|
Thanks @pablogsal for the PR |
|
GH-93360 is a backport of this pull request to the 3.11 branch. |
…by the parser is always consistent (pythonGH-93352) (cherry picked from commit 5893b5d) Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>
Closes: #93351