-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Fix syntax_editor duplicated changed element #21023
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -150,6 +150,35 @@ pub(super) fn apply_edits(editor: SyntaxEditor) -> SyntaxEdit { | |
| // Map change targets to the correct syntax nodes | ||
| let tree_mutator = TreeMutator::new(&root); | ||
| let mut changed_elements = vec![]; | ||
| let mut changed_elements_set = rustc_hash::FxHashSet::default(); | ||
| let mut deduplicate_node = |node_or_token: &mut SyntaxElement| { | ||
| let node; | ||
| let node = match node_or_token { | ||
| SyntaxElement::Token(token) => match token.parent() { | ||
| None => return, | ||
| Some(parent) => { | ||
| node = parent; | ||
| &node | ||
| } | ||
| }, | ||
| SyntaxElement::Node(node) => node, | ||
| }; | ||
| if changed_elements_set.contains(node) { | ||
| let new_node = node.clone_subtree().clone_for_update(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops, looks like I've merged without enough checks 😅
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's fiiiine 😆 |
||
| match node_or_token { | ||
| SyntaxElement::Node(node) => *node = new_node, | ||
| SyntaxElement::Token(token) => { | ||
| *token = new_node | ||
| .children_with_tokens() | ||
| .filter_map(SyntaxElement::into_token) | ||
| .find(|it| it.kind() == token.kind() && it.text() == token.text()) | ||
| .unwrap(); | ||
| } | ||
| } | ||
| } else { | ||
| changed_elements_set.insert(node.clone()); | ||
| } | ||
| }; | ||
|
|
||
| for index in independent_changes { | ||
| match &mut changes[index as usize] { | ||
|
|
@@ -180,6 +209,18 @@ pub(super) fn apply_edits(editor: SyntaxEditor) -> SyntaxEdit { | |
| } | ||
| } | ||
|
|
||
| match &mut changes[index as usize] { | ||
| Change::Insert(_, element) | Change::Replace(_, Some(element)) => { | ||
| deduplicate_node(element); | ||
| } | ||
| Change::InsertAll(_, elements) | ||
| | Change::ReplaceWithMany(_, elements) | ||
| | Change::ReplaceAll(_, elements) => { | ||
| elements.iter_mut().for_each(&mut deduplicate_node); | ||
| } | ||
| Change::Replace(_, None) => (), | ||
| } | ||
|
|
||
| // Collect changed elements | ||
| match &changes[index as usize] { | ||
| Change::Insert(_, element) => changed_elements.push(element.clone()), | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_subtreebreaks 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huge thanks for your clear explanation!