Fix syntax_editor duplicated changed element #21023
Conversation
Example
---
```rust
let arg_list = make::arg_list([make::expr_literal("1").into(), make::expr_literal("2").into()]);
let mut editor = SyntaxEditor::new(arg_list.syntax().clone());
let target_expr = make::expr_literal("3").clone_for_update();
for arg in arg_list.args() {
editor.replace(arg.syntax(), target_expr.syntax());
}
let edit = editor.finish();
let expect = expect![["(3, 3)"]];
expect.assert_eq(&edit.new_root.to_string());
```
**Before this PR**
```text
(, )3
```
**After this PR**
```text
(3, 3)
```
rustbot
added
the
S-waiting-on-review
There was a problem hiding this comment.
The dedup logic looks correct to me, but I’m not sure whether inserting or replacing with the same syntax node multiple times is part of the intended design. Should we automatically handle this by cloning the duplicated node (as done here), or should we reject duplicates and require the caller to handle it instead?
@DropDemBits, I’d appreciate your thoughts on this 😅
There was a problem hiding this comment.
Oof, it's been a year but I managed to recollect enough context to form an opinion 😆
The current SyntaxEditor design admits that SyntaxNodes do have a form of identity which at the moment is tracked by SyntaxNodes sharing the same NodeData1. At the same time however, the implementation detail being mutable syntax trees necessarily means that shared nodes get mutated (causing this issue), while using clone_subtree breaks this identity.
For now, I am okay with letting this through to fix this issue, but I've opened #21155 for myself to fix the broader issue.
Footnotes
-
This doesn't necessarily lock us in to having SyntaxNodes be reference counted, just that they need to carry through some identity token when we need it, e.g. some u32 derived from a global AtomicU32 counter. In fact, lifting the concept of node identity into rowan proper should let us remove SyntaxMappings, annoying as they are! ↩
There was a problem hiding this comment.
Huge thanks for your clear explanation!
| SyntaxElement::Node(node) => node, | ||
| }; | ||
| if changed_elements_set.contains(node) { | ||
| let new_node = node.clone_subtree().clone_for_update(); |
There was a problem hiding this comment.
This unfortunately does break node identity at the moment, but it doesn't seem like there's any test depending on it at the moment. There's also not really a good way to fix this at the moment.
There was a problem hiding this comment.
We should also be broadcasting source nodes for dependent changes, but that's trickier because we still need to preserve the mapping in that case. Again, not really a good way to fix that at the moment.
There was a problem hiding this comment.
Oops, looks like I've merged without enough checks 😅
rustbot
removed
the
S-waiting-on-review
4 participants
Fixes #21020
Example
Before this PR
After this PR