CSTTransformer can change node without actual changes.
- Dominant language
- Python
- Stars
- 1.9k
- Forks
- 229
- PR merge metrics
- No merged PRs in 30d
Description
TLDR question: how can I detect when `CSTTransformer` does not modify the tree?
## Summary
When using `CSTTransformer`, the `updated_node` parameter in `on_leave()` may be a **different object** than `original_node` even when **no changes were made** to the tree. This is **by design** — all nodes unconditionally create new objects in `_visit_and_replace_children`. However, this behavior:
1. **May surprise users** who expect `updated_node == original_node` when nothing changed
2. **Is not clearly documented**
3. **Could be optimized** to avoid unnecessary object creation (not 100% sure because it would add a condition)
## Minimal Reproduction
```python
import libcst as cst
class DebugTransformer(cst.CSTTransformer):
def __init__(self):
super().__init__()
self._root = None
def on_visit(self, node: cst.CSTNode) -> bool:
if self._root is None:
self._root = node
# Skip IfExp children to simulate a real transformer
if isinstance(node, cst.IfExp):
return False
return True
def on_leave(self, original_node: cst.CSTNode, updated_node: cst.CSTNode) -> cst.CSTNode:
same_identity = original_node is updated_node
same_equality = original_node == updated_node
deep_eq = original_node.deep_equals(updated_node)
print(f"{type(original_node).__name__}: is={same_identity}, ==={same_equality}, deep_equals={deep_eq}")
return updated_node
# Test with BinaryOperation containing two IfExp
expr = cst.parse_expression('([":a"] if cond1 else []) + ([":b"] if cond2 else [])')
expr.visit(DebugTransformer())
```
## Actual Output
```
IfExp: is=True, ===True, deep_equals=True
SimpleWhitespace: is=True, ===True, deep_equals=True
SimpleWhitespace: is=True, ===True, deep_equals=True
Add: is=False, ===False, deep_equals=True # <-- Different object even though unchanged!
IfExp: is=True, ===True, deep_equals=True
BinaryOperation: is=False, ===False, deep_equals=True # <-- Cascades up!
```
## Expected Behavior
When no modifications are made to a tree, I would expect:
- `original_node is updated_node` → `True` (same object)
- `original_node == updated_node` → `True` (equal)
This is important because the [best practices documentation](https://libcst.readthedocs.io/en/latest/best_practices.html) states:
> The original_node parameter on any leave_ method is provided for book-keeping and is guaranteed to be equal via == and is checks to the node parameter in the corresponding visit_ method.
While this guarantee is about `original_node` vs the `visit_` parameter (which holds), users naturally expect that `updated_node` should also be the same object when nothing changed.
Do we maintain this website and is this statement correct?
## Root Cause
**This is the general behavior across ALL nodes in libcst**, not a bug specific to operators. Every node's `_visit_and_replace_children()` unconditionally creates a new object:
```python
# Example from _BaseOneTokenOp (libcst/_nodes/op.py:28-37)
def _visit_and_replace_children(self, visitor: CSTVisitorT) -> "_BaseOneTokenOp":
return self.__class__( # <-- ALWAYS creates new object!
whitespace_before=visit_required(...),
whitespace_after=visit_required(...),
)
# Same pattern in BinaryOperation (libcst/_nodes/expression.py:1525-1532)
def _visit_and_replace_children(self, visitor: CSTVisitorT) -> "BinaryOperation":
return BinaryOperation( # <-- ALWAYS creates new object!
lpar=visit_sequence(...),
left=visit_required(...),
operator=visit_required(...),
right=visit_required(...),
rpar=visit_sequence(...),
)
```
The **only exception** is `BaseLeaf._visit_and_replace_children()` which returns `self` — but `BaseLeaf` has no children, so this is trivial.
This pattern is used consistently across:
- `op.py`: `_BaseOneTokenOp`, `_BaseTwoTokenOp`, `BaseUnaryOp`, `NotEqual` (~40+ operator classes)
- `expression.py`: `BinaryOperation`, `LeftSquareBracket`, `RightSquareBracket`, `LeftParen`, `RightParen`, `Name`, `Integer`, etc.
- `statement.py`: All statement nodes
## Impact
When writing a transformer that needs to detect "am I at the root of the expression I'm transforming?", users cannot rely on `updated_node == self._root`. They must use `updated_node.deep_equals(self._root)` instead, which is:
1. Not obvious from the documentation
2. More expensive (full tree comparison vs identity check)
## Proposed Fix
### Option 1: Internal optimization (minimal change)
Have `_visit_and_replace_children` return `self` when children are unchanged:
```python
def _visit_and_replace_children(self, visitor: CSTVisitorT) -> "_BaseOneTokenOp":
new_ws_before = visit_required(
self, "whitespace_before", self.whitespace_before, visitor
)
new_ws_after = visit_required(
self, "whitespace_after", self.whitespace_after, visitor
)
# Only create new object if children actually changed
if new_ws_before is self.whitespace_before and new_ws_after is self.whitespace_after:
return self
return self.__class__(
whitespace_before=new_ws_before,
whitespace_after=new_ws_after,
)
```
This preserves object identity when nothing changes, making `original_node is updated_node` reliable.
### Option 2: New sentinel (API change)
libcst already has `RemovalSentinel` (remove node) and `FlattenSentinel` (replace with multiple nodes). A `NoChangeSentinel` could signal "no changes made":
```python
class NoChangeSentinel:
NO_CHANGE = ...
# In transformer:
def on_leave(self, original_node, updated_node):
if no_modifications_needed:
return NoChangeSentinel.NO_CHANGE # Signals: keep original, skip rebuilding parents
return updated_node
```
This would allow the traversal to short-circuit parent rebuilding. However, Option 1 is simpler and doesn't require API changes.
## Environment
fbsource
Contributor guide
Assessment
This issue has not been assessed yet.