Matchers trigger when original node matches even if current version doesn't
- Dominant language
- Python
- Stars
- 1.9k
- Forks
- 229
- PR merge metrics
- No merged PRs in 30d
Description
I was debugging https://github.com/Zac-HD/shed/issues/84 where shed's libcst based codemods were producing wrong output, and tracked it down to an interaction between two of the rules in shed interacting badly with each other, due to some unintuitive (to me at least!) behaviour in libcst, which is that whether a function annotated with `leave` fires depends on whether the *original* node matches, not the current updated version of the node at the point of entry. In Shed's case this resulted in transformations being called with the node in a completely different state than expected.
It's not mysterious why this happens, this is just literally what the code checks for: https://github.com/Instagram/LibCST/blob/f9536b522f58d2b70ae8beb6b607b45ef08620e6/libcst/matchers/_visitors.py#L526
For posterity, here's a reproducible example of how this caused shed problems:
```python
import libcst as cst
import libcst.matchers as m
from libcst.codemod import VisitorBasedCodemodCommand
import sys
class ShedFixers(VisitorBasedCodemodCommand):
@m.leave(
m.BooleanOperation(
left=m.Call(m.Name("isinstance"), [m.Arg(), m.Arg()]),
operator=m.Or(),
right=m.Call(m.Name("isinstance"), [m.Arg(), m.Arg()]),
)
)
def collapse_isinstance_checks(self, _, updated_node):
left_target, left_type = updated_node.left.args
right_target, right_type = updated_node.right.args
if left_target.deep_equals(right_target):
merged_type = cst.Arg(
cst.Tuple([cst.Element(left_type.value), cst.Element(right_type.value)])
)
return updated_node.left.with_changes(args=[left_target, merged_type])
return updated_node
@m.leave(m.If(test=m.BooleanOperation()))
def remove_unnecessary_call_test(self, _, updated_node):
assert m.matches(updated_node.test, m.BooleanOperation())
return updated_node
CODE = """
x = 1
if isinstance(x, int) or isinstance(x, float):
x = 2
"""
if __name__ == '__main__':
context = cst.codemod.CodemodContext()
mod = cst.parse_module(CODE)
mod = ShedFixers(context).transform_module(mod)
print(mod.code)
```
This triggers the assertion (in the original it triggers a code rewrite that produces nonsensical results because it expects this property to be true and it isn't, but that bit's mostly distracting so I replaced it with an assertion) because although the two `isinstance` calls have been merged into a single one, so the `if` statement no longer has a `BooleanOperation` as its test, the second matcher still fires.
This strikes me as surprising behaviour though. I'm pretty new to libcst so I might be missing something obvious as to why it has to be this way, but it seems to me like the desired behaviour is always to check whether the node you're about to transform looks like you'd expect, not whether a previous version did, and this makes it very hard to write correct code without basically duplicating the check inside your method.
If this behaviour is necessary/intended, ideally the documentation would include some warning about it, as it appears to be a bit of a trap for the unwary.
Contributor guide
Assessment
This issue has not been assessed yet.