GH-94694: Fix column offsets for multi-line method lookups #94697
Conversation
brandtbucher
added
interpreter-core
brandtbucher
added
the
🔨 test-with-buildbots
|
🤖 New build scheduled with the buildbot fleet by @brandtbucher for commit 33dc4d8 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
bedevere-bot
removed
the
🔨 test-with-buildbots
|
|
Lib/test/test_compile.py
Outdated
| @@ -1181,6 +1181,27 @@ def test_complex_single_line_expression(self): | |||
| self.assertOpcodeSourcePositionIs(compiled_code, 'BINARY_OP', | |||
| line=1, end_line=1, column=0, end_column=27, occurrence=4) | |||
|
|
|||
| def test_multiline_assert_rewritten_as_method_call(self): | |||
| # GH-94694: Copying location information from a "real" node to a | |||
| # handwritten one should always be valid! | |||
There was a problem hiding this comment.
Should it really always be valid?
There was a problem hiding this comment.
I can remove the comment if it's too controversial. It just seems to me that the utility/safety of ast.copy_location and ast.fix_missing_locations is dramatically reduced if they can't at least promise crash-free behavior.
|
Is a bit unfortunate that we cannot just propagate correctly the offsets from the AST nodes. The disadvantage of a method relying on inspecting the source (here through the method name) is that ASTs created from abstract sources (or no source at all) may fall through the cracks in these kind of checks. What's preventing us from correctly grab the original column offsets from the multi line method call? (I know this has more to be with the original issue, but I would like to fully understand this before adding more band aids :) ) |
|
The issue here is that we are flattening a multi line location into a single line. So we are trying to construct the location info for the attribute name only (which is not available directly in the AST, it seems). |
For our own nodes, I can fix that in the parser so our nodes make those available. For random AST nodes it may still be a problem. |
I think we almost have it. The location of |
The pytest nodes are not completely random, if we copy to them the location of the attr name from the source code the traceback will probably look right - the source line from the file will appear with the right thing hilighted. |
The root of the issue is that
Unfortunately, This also creates a weird (but probably fine) inconsistency. If the attribute access is all on one line, the whole thing is underlined in the traceback. If it spans multiple lines, only the attribute name is underlined. All this just comes from our need to trace the line that the attribute name is on, and trying (after the fact) to figure out what columns that should include. :( |
This is exactly what I'm referring to. We could modify the AST to add this information so we don't need to guess. This way, it should also have work with arbitrary AST nodes. The price is that although this is something we can do, it will force everyone to adapt. |
|
Yeah. I think it would be an acceptable change for 3.12. While some consumers of the AST would need to change their code slightly, it wouldn't be removing any functionality that's currently present. If anything, it's adding functionality. |
|
Thanks @brandtbucher for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11. |
|
Sorry, @brandtbucher, I could not cleanly backport this to |
|
I think this one's gonna need a manual backport. |
…ps (pythonGH-94697). (cherry picked from commit 264b3dd) Co-authored-by: Brandt Bucher <[email protected]>
|
GH-94721 is a backport of this pull request to the 3.11 branch. |
If we're given an AST that doesn't make much sense here, just wipe out the column info instead of crashing later.
Also, don't try to include the dot in the column span.
column >= -1assertion failure with pytest rewriting #94694