fix: backward compatibility fix for changed source positions in 3.12.5 (#82) #83
Conversation
executing/_position_node_finder.py
Outdated
| # where `iter(l)` would be otherwise the resulting node for the `iter()` call and the __iter__ call of the for implementation. | ||
| # keeping the old behaviour makes it possible to distinguish both cases. | ||
|
|
||
| return self.result.parent |
There was a problem hiding this comment.
It's weird that this uses self.result instead of node.
There was a problem hiding this comment.
yes you are right.
executing/_position_node_finder.py
Outdated
| if ( | ||
| sys.version_info >= (3, 12, 5) | ||
| and instruction.opname in ("GET_ITER", "FOR_ITER") | ||
| and isinstance(node, ast.For) |
There was a problem hiding this comment.
We want to return an ast.For for these instructions, right? So isn't node correct here? Why return its parent? I feel like I'm misunderstanding.
There was a problem hiding this comment.
sorry for the confusion. It was late in the evening and I messed something up while I copied the code from the other branch.
it should be isinstance(node.parent, ast.For)
I tested probably with the wrong 3.12 version. I think it would be useful to include all patch versions to the CI tests (3.8.0, ..., 3.12.0, 3.12.1, ...) but test only the short running tests. What do you think?
| if ( | ||
| sys.version_info >= (3, 12, 5) | ||
| and instruction.opname in ("GET_ITER", "FOR_ITER") | ||
| and isinstance(node.parent, ast.For) |
There was a problem hiding this comment.
| and isinstance(node.parent, ast.For) | |
| and isinstance(node.parent, ast.For) | |
| and node is node.parent.iter |
What about comprehensions?
There was a problem hiding this comment.
They node positions for comprehensions are not affected.
class Foo:
def __iter__(self):
assert False
a = [x for x in Foo()]output (Python 3.12.5):
Traceback (most recent call last):
File "/home/frank/projects/executing/codi.py", line 7, in <module>
a = [x for x in Foo()]
^^^^^^^^^^^^^^^^^^
File "/home/frank/projects/executing/codi.py", line 5, in __iter__
assert False
^^^^^
AssertionErrorThere was a problem hiding this comment.
Based on the apparently desired behaviour for For, that seems like a Python bug lol
There was a problem hiding this comment.
I don't know, For is usually a multiline statement and comprehensions are usually on a single line. I will ask 😄
The patch/PR has been approved by the upstream maintainer: alexmojaki/executing#83 (review) Example error log: https://hydra.nixos.org/build/270409937/nixlog/2/tail
|
@alexmojaki can we merge this? The 3.13 branch is failing because the missing fix in 3.12. |
|
Yes. You should be able to merge yourself. So I will generally approve and leave you to do the merge in case you change your mind about something at the last minute. Please use "Squash and merge". |
fixes #82