From bfd10e4c2b4d506512bf401817aaaeaf4534f090 Mon Sep 17 00:00:00 2001 From: Matt Fellenz Date: Sat, 20 Jan 2024 22:25:13 -0800 Subject: [PATCH 01/16] Use niching for Checkpoint --- src/green/builder.rs | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/src/green/builder.rs b/src/green/builder.rs index 1dbc8bb0..03d66df5 100644 --- a/src/green/builder.rs +++ b/src/green/builder.rs @@ -1,3 +1,5 @@ +use std::num::NonZeroUsize; + use crate::{ cow_mut::CowMut, green::{node_cache::NodeCache, GreenElement, GreenNode, SyntaxKind}, @@ -6,7 +8,17 @@ use crate::{ /// A checkpoint for maybe wrapping a node. See `GreenNodeBuilder::checkpoint` for details. #[derive(Clone, Copy, Debug)] -pub struct Checkpoint(usize); +pub struct Checkpoint(NonZeroUsize); + +impl Checkpoint { + fn new(inner: usize) -> Self { + Self(NonZeroUsize::new(inner + 1).unwrap()) + } + + fn into_inner(self) -> usize { + self.0.get() - 1 + } +} /// A builder for a green tree. #[derive(Default, Debug)] @@ -82,14 +94,14 @@ impl GreenNodeBuilder<'_> { /// ``` #[inline] pub fn checkpoint(&self) -> Checkpoint { - Checkpoint(self.children.len()) + Checkpoint::new(self.children.len()) } /// Wrap the previous branch marked by `checkpoint` in a new branch and /// make it current. #[inline] pub fn start_node_at(&mut self, checkpoint: Checkpoint, kind: SyntaxKind) { - let Checkpoint(checkpoint) = checkpoint; + let checkpoint = checkpoint.into_inner(); assert!( checkpoint <= self.children.len(), "checkpoint no longer valid, was finish_node called early?" From 62bd09ba8e42fb6b386657678a4b2fdd86b4aab4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jelmer=20Vernoo=C4=B3?= Date: Wed, 4 Oct 2023 11:06:32 +0100 Subject: [PATCH 02/16] Add rowan::api::SyntaxNode::new_root_mut --- src/api.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/api.rs b/src/api.rs index 605fcc50..d6411d30 100644 --- a/src/api.rs +++ b/src/api.rs @@ -98,6 +98,9 @@ impl SyntaxNode { pub fn new_root(green: GreenNode) -> SyntaxNode { SyntaxNode::from(cursor::SyntaxNode::new_root(green)) } + pub fn new_root_mut(green: GreenNode) -> SyntaxNode { + SyntaxNode::from(cursor::SyntaxNode::new_root_mut(green)) + } /// Returns a green tree, equal to the green tree this node /// belongs two, except with this node substitute. The complexity /// of operation is proportional to the depth of the tree From 20f30010f5f5e58e0626b3e922c0083161c43435 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jelmer=20Vernoo=C4=B3?= Date: Mon, 13 May 2024 09:58:15 +0100 Subject: [PATCH 03/16] Fix pointer comparison warning on newer rustc --- src/arc.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/arc.rs b/src/arc.rs index 34883129..36b7f24a 100644 --- a/src/arc.rs +++ b/src/arc.rs @@ -83,7 +83,7 @@ impl Arc { /// allocation #[inline] pub(crate) fn ptr_eq(this: &Self, other: &Self) -> bool { - this.ptr() == other.ptr() + std::ptr::addr_eq(this.ptr(), other.ptr()) } pub(crate) fn ptr(&self) -> *mut ArcInner { From 9f64e506a3f2c2751b82fd5ad8868e39d6bea6df Mon Sep 17 00:00:00 2001 From: Kevin Kuriakose Date: Sat, 18 Feb 2023 18:11:43 +0530 Subject: [PATCH 04/16] Prevent use of SyntaxNodePtr and AstPtr on mutable trees Both of these use source code locations to identify the node, which can get invalidated by syntax tree mutations. This commit adds assertions to prevent their use, and adds documentation to inform users of the issue Fixes #150 --- src/api.rs | 4 +++ src/ast.rs | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++- src/cursor.rs | 4 +++ 3 files changed, 79 insertions(+), 1 deletion(-) diff --git a/src/api.rs b/src/api.rs index d6411d30..bf85fa0f 100644 --- a/src/api.rs +++ b/src/api.rs @@ -247,6 +247,10 @@ impl SyntaxNode { SyntaxNode::from(self.raw.clone_for_update()) } + pub fn is_mutable(&self) -> bool { + self.raw.is_mutable() + } + pub fn detach(&self) { self.raw.detach() } diff --git a/src/ast.rs b/src/ast.rs index 856a9f63..e142395b 100644 --- a/src/ast.rs +++ b/src/ast.rs @@ -55,6 +55,11 @@ pub trait AstNode { } /// A "pointer" to a [`SyntaxNode`], via location in the source code. +/// +/// ## Note +/// Since the location is source code dependent, this must not be used +/// with mutable syntax trees. Any changes made in such trees causes +/// the pointed node's source location to change, invalidating the pointer. #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] pub struct SyntaxNodePtr { kind: L::Kind, @@ -63,7 +68,10 @@ pub struct SyntaxNodePtr { impl SyntaxNodePtr { /// Returns a [`SyntaxNodePtr`] for the node. + /// + /// Panics if the provided node is mutable pub fn new(node: &SyntaxNode) -> Self { + assert!(!node.is_mutable(), "tree is mutable"); Self { kind: node.kind(), range: node.text_range() } } @@ -82,10 +90,13 @@ impl SyntaxNodePtr { /// Also returns `None` if `root` is not actually a root (i.e. it has a /// parent). /// + /// NOTE: If this function is called on a mutable tree, it will panic + /// /// The complexity is linear in the depth of the tree and logarithmic in /// tree width. As most trees are shallow, thinking about this as /// `O(log(N))` in the size of the tree is not too wrong! pub fn try_to_node(&self, root: &SyntaxNode) -> Option> { + assert!(!root.is_mutable(), "tree is mutable"); if root.parent().is_some() { return None; } @@ -113,13 +124,21 @@ impl SyntaxNodePtr { } /// Like [`SyntaxNodePtr`], but remembers the type of node. +/// +/// ## Note +/// As with [`SyntaxNodePtr`], this must not be used on mutable +/// syntax trees, since any mutation can cause the pointed node's +/// source location to change, invalidating the pointer pub struct AstPtr { raw: SyntaxNodePtr, } impl AstPtr { /// Returns an [`AstPtr`] for the node. + /// + /// Panics if the provided node is mutable pub fn new(node: &N) -> Self { + // The above mentioned panic is handled by SyntaxNodePtr Self { raw: SyntaxNodePtr::new(node.syntax()) } } @@ -129,8 +148,9 @@ impl AstPtr { } /// Given the root node containing the node `n` that `self` is a pointer to, - /// returns `n` if possible. See [`SyntaxNodePtr::try_to_node`]. + /// returns `n` if possible. Panics if `root` is mutable. See [`SyntaxNodePtr::try_to_node`]. pub fn try_to_node(&self, root: &SyntaxNode) -> Option { + // The above mentioned panic is handled by SyntaxNodePtr N::cast(self.raw.try_to_node(root)?) } @@ -215,3 +235,53 @@ pub mod support { parent.children_with_tokens().filter_map(|it| it.into_token()).find(|it| it.kind() == kind) } } + +#[cfg(test)] +mod tests { + use crate::{GreenNodeBuilder, Language, SyntaxKind, SyntaxNode}; + + use super::SyntaxNodePtr; + + #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] + struct TestLanguage; + impl Language for TestLanguage { + type Kind = SyntaxKind; + + fn kind_from_raw(raw: SyntaxKind) -> Self::Kind { + raw + } + + fn kind_to_raw(kind: Self::Kind) -> SyntaxKind { + kind + } + } + + fn build_immut_tree() -> SyntaxNode { + // Creates a single-node tree + let mut builder = GreenNodeBuilder::new(); + builder.start_node(SyntaxKind(0)); + builder.finish_node(); + + SyntaxNode::::new_root(builder.finish()) + } + + #[test] + #[should_panic = "tree is mutable"] + fn ensure_mut_panic_on_create() { + // Make a mutable version + let tree = build_immut_tree().clone_for_update(); + + SyntaxNodePtr::new(&tree); + } + + #[test] + #[should_panic = "tree is mutable"] + fn ensure_mut_panic_on_deref() { + let tree = build_immut_tree(); + let tree_mut = tree.clone_for_update(); + + // Create on immutable, convert on mutable + let syn_ptr = SyntaxNodePtr::new(&tree); + syn_ptr.to_node(&tree_mut); + } +} diff --git a/src/cursor.rs b/src/cursor.rs index 671be234..9060507e 100644 --- a/src/cursor.rs +++ b/src/cursor.rs @@ -544,6 +544,10 @@ impl SyntaxNode { SyntaxNode { ptr: NodeData::new(Some(parent), index, offset, green, mutable) } } + pub fn is_mutable(&self) -> bool { + self.data().mutable + } + pub fn clone_for_update(&self) -> SyntaxNode { assert!(!self.data().mutable); match self.parent() { From 54c9c96d472c46e92c474c0cb6b8988f638cc2b3 Mon Sep 17 00:00:00 2001 From: peamaeq Date: Wed, 4 May 2022 11:30:05 +0800 Subject: [PATCH 05/16] 0503 --- src/arc.rs | 4 +--- src/cursor.rs | 18 +++++++----------- src/green/node.rs | 6 ++---- src/green/token.rs | 4 +--- src/sll.rs | 10 +++++----- 5 files changed, 16 insertions(+), 26 deletions(-) diff --git a/src/arc.rs b/src/arc.rs index 36b7f24a..76420b85 100644 --- a/src/arc.rs +++ b/src/arc.rs @@ -257,12 +257,10 @@ impl Deref for HeaderSlice { type Target = HeaderSlice; fn deref(&self) -> &Self::Target { - unsafe { let len = self.length; let fake_slice: *const [T] = ptr::slice_from_raw_parts(self as *const _ as *const T, len); - &*(fake_slice as *const HeaderSlice) - } + unsafe { &*(fake_slice as *const HeaderSlice) } } } diff --git a/src/cursor.rs b/src/cursor.rs index 9060507e..b9e55371 100644 --- a/src/cursor.rs +++ b/src/cursor.rs @@ -431,9 +431,8 @@ impl NodeData { None => return, }; - unsafe { sll::adjust(self, self.index() + 1, Delta::Sub(1)); - let parent = parent_ptr.as_ref(); + let parent = unsafe { parent_ptr.as_ref() }; sll::unlink(&parent.first, self); // Add strong ref to green @@ -449,27 +448,25 @@ impl NodeData { match parent.green() { NodeOrToken::Node(green) => { let green = green.remove_child(self.index() as usize); - parent.respine(green) + unsafe { parent.respine(green) } } NodeOrToken::Token(_) => unreachable!(), } if parent.dec_rc() { - free(parent_ptr) + unsafe { free(parent_ptr) } } - } } fn attach_child(&self, index: usize, child: &NodeData) { assert!(self.mutable && child.mutable && child.parent().is_none()); assert!(self.rc.get() > 0 && child.rc.get() > 0); - unsafe { child.index.set(index as u32); child.parent.set(Some(self.into())); self.inc_rc(); if !self.first.get().is_null() { - sll::adjust(&*self.first.get(), index as u32, Delta::Add(1)); + sll::adjust(unsafe { &*self.first.get() }, index as u32, Delta::Add(1)); } match sll::link(&self.first, child) { @@ -483,16 +480,15 @@ impl NodeData { NodeOrToken::Node(green) => { // Child is root, so it ownes the green node. Steal it! let child_green = match &child.green { - Green::Node { ptr } => GreenNode::from_raw(ptr.get()).into(), - Green::Token { ptr } => GreenToken::from_raw(*ptr).into(), + Green::Node { ptr } => unsafe { GreenNode::from_raw(ptr.get()).into() }, + Green::Token { ptr } => unsafe { GreenToken::from_raw(*ptr).into() }, }; let green = green.insert_child(index, child_green); - self.respine(green); + unsafe { self.respine(green) }; } NodeOrToken::Token(_) => unreachable!(), } - } } unsafe fn respine(&self, mut new_green: GreenNode) { let mut node = self; diff --git a/src/green/node.rs b/src/green/node.rs index e94dc01c..7753bb92 100644 --- a/src/green/node.rs +++ b/src/green/node.rs @@ -56,11 +56,9 @@ impl ToOwned for GreenNodeData { #[inline] fn to_owned(&self) -> GreenNode { - unsafe { - let green = GreenNode::from_raw(ptr::NonNull::from(self)); + let green = unsafe { GreenNode::from_raw(ptr::NonNull::from(self)) }; let green = ManuallyDrop::new(green); GreenNode::clone(&green) - } } } @@ -194,8 +192,8 @@ impl ops::Deref for GreenNode { #[inline] fn deref(&self) -> &GreenNodeData { + let repr: &Repr = &self.ptr; unsafe { - let repr: &Repr = &self.ptr; let repr: &ReprThin = &*(repr as *const Repr as *const ReprThin); mem::transmute::<&ReprThin, &GreenNodeData>(repr) } diff --git a/src/green/token.rs b/src/green/token.rs index 1a4548a4..e5ed8925 100644 --- a/src/green/token.rs +++ b/src/green/token.rs @@ -44,11 +44,9 @@ impl ToOwned for GreenTokenData { #[inline] fn to_owned(&self) -> GreenToken { - unsafe { - let green = GreenToken::from_raw(ptr::NonNull::from(self)); + let green = unsafe { GreenToken::from_raw(ptr::NonNull::from(self)) }; let green = ManuallyDrop::new(green); GreenToken::clone(&green) - } } } diff --git a/src/sll.rs b/src/sll.rs index 87d2f1f3..0dd17b87 100644 --- a/src/sll.rs +++ b/src/sll.rs @@ -84,12 +84,12 @@ pub(crate) fn unlink(head: &Cell<*const E>, elem: &E) { #[cold] pub(crate) fn link<'a, E: Elem>(head: &'a Cell<*const E>, elem: &E) -> AddToSllResult<'a, E> { + let old_head = head.get(); + // Case 1: empty head, replace it. + if old_head.is_null() { + return AddToSllResult::EmptyHead(head); + } unsafe { - let old_head = head.get(); - // Case 1: empty head, replace it. - if old_head.is_null() { - return AddToSllResult::EmptyHead(head); - } // Case 2: we are smaller than the head, replace it. if elem.key() < (*old_head).key() { From c082936255a0735ad751803ed5b7a0e8c3e5961e Mon Sep 17 00:00:00 2001 From: peamaeq Date: Thu, 16 Jun 2022 20:31:02 +0800 Subject: [PATCH 06/16] '0503' --- src/arc.rs | 7 ++-- src/cursor.rs | 86 +++++++++++++++++++++++----------------------- src/green/node.rs | 6 ++-- src/green/token.rs | 6 ++-- src/sll.rs | 1 - 5 files changed, 52 insertions(+), 54 deletions(-) diff --git a/src/arc.rs b/src/arc.rs index 76420b85..7adf45e7 100644 --- a/src/arc.rs +++ b/src/arc.rs @@ -257,10 +257,9 @@ impl Deref for HeaderSlice { type Target = HeaderSlice; fn deref(&self) -> &Self::Target { - let len = self.length; - let fake_slice: *const [T] = - ptr::slice_from_raw_parts(self as *const _ as *const T, len); - unsafe { &*(fake_slice as *const HeaderSlice) } + let len = self.length; + let fake_slice: *const [T] = ptr::slice_from_raw_parts(self as *const _ as *const T, len); + unsafe { &*(fake_slice as *const HeaderSlice) } } } diff --git a/src/cursor.rs b/src/cursor.rs index b9e55371..3b849582 100644 --- a/src/cursor.rs +++ b/src/cursor.rs @@ -431,64 +431,64 @@ impl NodeData { None => return, }; - sll::adjust(self, self.index() + 1, Delta::Sub(1)); - let parent = unsafe { parent_ptr.as_ref() }; - sll::unlink(&parent.first, self); - - // Add strong ref to green - match self.green().to_owned() { - NodeOrToken::Node(it) => { - GreenNode::into_raw(it); - } - NodeOrToken::Token(it) => { - GreenToken::into_raw(it); - } + sll::adjust(self, self.index() + 1, Delta::Sub(1)); + let parent = unsafe { parent_ptr.as_ref() }; + sll::unlink(&parent.first, self); + + // Add strong ref to green + match self.green().to_owned() { + NodeOrToken::Node(it) => { + GreenNode::into_raw(it); } - - match parent.green() { - NodeOrToken::Node(green) => { - let green = green.remove_child(self.index() as usize); - unsafe { parent.respine(green) } - } - NodeOrToken::Token(_) => unreachable!(), + NodeOrToken::Token(it) => { + GreenToken::into_raw(it); } + } - if parent.dec_rc() { - unsafe { free(parent_ptr) } + match parent.green() { + NodeOrToken::Node(green) => { + let green = green.remove_child(self.index() as usize); + unsafe { parent.respine(green) } } + NodeOrToken::Token(_) => unreachable!(), + } + + if parent.dec_rc() { + unsafe { free(parent_ptr) } + } } fn attach_child(&self, index: usize, child: &NodeData) { assert!(self.mutable && child.mutable && child.parent().is_none()); assert!(self.rc.get() > 0 && child.rc.get() > 0); - child.index.set(index as u32); - child.parent.set(Some(self.into())); - self.inc_rc(); + child.index.set(index as u32); + child.parent.set(Some(self.into())); + self.inc_rc(); - if !self.first.get().is_null() { - sll::adjust(unsafe { &*self.first.get() }, index as u32, Delta::Add(1)); - } + if !self.first.get().is_null() { + sll::adjust(unsafe { &*self.first.get() }, index as u32, Delta::Add(1)); + } - match sll::link(&self.first, child) { - sll::AddToSllResult::AlreadyInSll(_) => { - panic!("Child already in sorted linked list") - } - it => it.add_to_sll(child), + match sll::link(&self.first, child) { + sll::AddToSllResult::AlreadyInSll(_) => { + panic!("Child already in sorted linked list") } + it => it.add_to_sll(child), + } - match self.green() { - NodeOrToken::Node(green) => { - // Child is root, so it ownes the green node. Steal it! - let child_green = match &child.green { - Green::Node { ptr } => unsafe { GreenNode::from_raw(ptr.get()).into() }, - Green::Token { ptr } => unsafe { GreenToken::from_raw(*ptr).into() }, - }; + match self.green() { + NodeOrToken::Node(green) => { + // Child is root, so it ownes the green node. Steal it! + let child_green = match &child.green { + Green::Node { ptr } => unsafe { GreenNode::from_raw(ptr.get()).into() }, + Green::Token { ptr } => unsafe { GreenToken::from_raw(*ptr).into() }, + }; - let green = green.insert_child(index, child_green); - unsafe { self.respine(green) }; - } - NodeOrToken::Token(_) => unreachable!(), + let green = green.insert_child(index, child_green); + unsafe { self.respine(green) }; } + NodeOrToken::Token(_) => unreachable!(), + } } unsafe fn respine(&self, mut new_green: GreenNode) { let mut node = self; diff --git a/src/green/node.rs b/src/green/node.rs index 7753bb92..69b1cf76 100644 --- a/src/green/node.rs +++ b/src/green/node.rs @@ -56,9 +56,9 @@ impl ToOwned for GreenNodeData { #[inline] fn to_owned(&self) -> GreenNode { - let green = unsafe { GreenNode::from_raw(ptr::NonNull::from(self)) }; - let green = ManuallyDrop::new(green); - GreenNode::clone(&green) + let green = unsafe { GreenNode::from_raw(ptr::NonNull::from(self)) }; + let green = ManuallyDrop::new(green); + GreenNode::clone(&green) } } diff --git a/src/green/token.rs b/src/green/token.rs index e5ed8925..81e298c7 100644 --- a/src/green/token.rs +++ b/src/green/token.rs @@ -44,9 +44,9 @@ impl ToOwned for GreenTokenData { #[inline] fn to_owned(&self) -> GreenToken { - let green = unsafe { GreenToken::from_raw(ptr::NonNull::from(self)) }; - let green = ManuallyDrop::new(green); - GreenToken::clone(&green) + let green = unsafe { GreenToken::from_raw(ptr::NonNull::from(self)) }; + let green = ManuallyDrop::new(green); + GreenToken::clone(&green) } } diff --git a/src/sll.rs b/src/sll.rs index 0dd17b87..4a298390 100644 --- a/src/sll.rs +++ b/src/sll.rs @@ -90,7 +90,6 @@ pub(crate) fn link<'a, E: Elem>(head: &'a Cell<*const E>, elem: &E) -> AddToSllR return AddToSllResult::EmptyHead(head); } unsafe { - // Case 2: we are smaller than the head, replace it. if elem.key() < (*old_head).key() { return AddToSllResult::SmallerThanHead(head); From ba6b8e4cb42fcb7bc3fd24a9f5a6299620525c6e Mon Sep 17 00:00:00 2001 From: Alona Enraght-Moony Date: Fri, 31 May 2024 16:19:09 +0100 Subject: [PATCH 07/16] Remove `memoffset` dependency. `std::mem::offset` has been availible on stable since 1.77.0. `memoffset` uses this when compiled against a recent enough rustc. Moving to the implementation in `std` lets us drop both `memoffset` and `autocfg` from our dependencies. --- Cargo.toml | 3 +-- src/arc.rs | 4 +--- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 5c5bba7f..879c77d6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -6,7 +6,7 @@ repository = "https://github.com/rust-analyzer/rowan" license = "MIT OR Apache-2.0" description = "Library for generic lossless syntax trees" edition = "2021" - +rust-version = "1.77.0" exclude = [".github/", "bors.toml", "rustfmt.toml"] [workspace] @@ -18,7 +18,6 @@ hashbrown = { version = "0.14.3", features = [ "inline-more", ], default-features = false } text-size = "1.1.0" -memoffset = "0.9" countme = "3.0.0" serde = { version = "1.0.89", optional = true, default-features = false } diff --git a/src/arc.rs b/src/arc.rs index 7adf45e7..cb6758e6 100644 --- a/src/arc.rs +++ b/src/arc.rs @@ -4,7 +4,7 @@ use std::{ cmp::Ordering, hash::{Hash, Hasher}, marker::PhantomData, - mem::{self, ManuallyDrop}, + mem::{self, offset_of, ManuallyDrop}, ops::Deref, ptr, sync::atomic::{ @@ -13,8 +13,6 @@ use std::{ }, }; -use memoffset::offset_of; - /// A soft limit on the amount of references that may be made to an `Arc`. /// /// Going above this limit will abort your program (although not From e2d2e93e16c5104b136d0bc738a0d48346922200 Mon Sep 17 00:00:00 2001 From: Alona Enraght-Moony Date: Mon, 3 Jun 2024 15:38:43 +0100 Subject: [PATCH 08/16] v0.15.16 --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 879c77d6..fcee9656 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "rowan" -version = "0.15.15" +version = "0.15.16" authors = ["Aleksey Kladov "] repository = "https://github.com/rust-analyzer/rowan" license = "MIT OR Apache-2.0" From a87bcd2494d62c110ba9bbe4752948a4c9d2e0a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jelmer=20Vernoo=C4=B3?= Date: Fri, 16 Aug 2024 16:16:20 +0100 Subject: [PATCH 09/16] Fix typo in docstring: two => to Also, rephrase slightly. --- src/api.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/api.rs b/src/api.rs index bf85fa0f..03b2cc23 100644 --- a/src/api.rs +++ b/src/api.rs @@ -101,9 +101,10 @@ impl SyntaxNode { pub fn new_root_mut(green: GreenNode) -> SyntaxNode { SyntaxNode::from(cursor::SyntaxNode::new_root_mut(green)) } + /// Returns a green tree, equal to the green tree this node - /// belongs two, except with this node substitute. The complexity - /// of operation is proportional to the depth of the tree + /// belongs to, except with this node substituted. The complexity + /// of the operation is proportional to the depth of the tree. pub fn replace_with(&self, replacement: GreenNode) -> GreenNode { self.raw.replace_with(replacement) } @@ -263,8 +264,8 @@ impl SyntaxNode { impl SyntaxToken { /// Returns a green tree, equal to the green tree this token - /// belongs two, except with this token substitute. The complexity - /// of operation is proportional to the depth of the tree + /// belongs to, except with this token substituted. The complexity + /// of the operation is proportional to the depth of the tree. pub fn replace_with(&self, new_token: GreenToken) -> GreenNode { self.raw.replace_with(new_token) } From f45924cdca24bac7c617d1ac3981f457795c5889 Mon Sep 17 00:00:00 2001 From: John Vandenberg Date: Sat, 28 Sep 2024 07:11:54 +0800 Subject: [PATCH 10/16] Fix typos --- src/api.rs | 4 ++-- src/cursor.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/api.rs b/src/api.rs index 03b2cc23..ccfb0a58 100644 --- a/src/api.rs +++ b/src/api.rs @@ -214,7 +214,7 @@ impl SyntaxNode { } /// Find a token in the subtree corresponding to this node, which covers the offset. - /// Precondition: offset must be withing node's range. + /// Precondition: offset must be within node's range. pub fn token_at_offset(&self, offset: TextSize) -> TokenAtOffset> { self.raw.token_at_offset(offset).map(SyntaxToken::from) } @@ -222,7 +222,7 @@ impl SyntaxNode { /// Return the deepest node or token in the current subtree that fully /// contains the range. If the range is empty and is contained in two leaf /// nodes, either one can be returned. Precondition: range must be contained - /// withing the current node + /// within the current node pub fn covering_element(&self, range: TextRange) -> SyntaxElement { NodeOrToken::from(self.raw.covering_element(range)) } diff --git a/src/cursor.rs b/src/cursor.rs index 3b849582..f7faf27a 100644 --- a/src/cursor.rs +++ b/src/cursor.rs @@ -478,7 +478,7 @@ impl NodeData { match self.green() { NodeOrToken::Node(green) => { - // Child is root, so it ownes the green node. Steal it! + // Child is root, so it owns the green node. Steal it! let child_green = match &child.green { Green::Node { ptr } => unsafe { GreenNode::from_raw(ptr.get()).into() }, Green::Token { ptr } => unsafe { GreenToken::from_raw(*ptr).into() }, From 2f4371a21123dd62dcfb9ec2ee127612747a807c Mon Sep 17 00:00:00 2001 From: Martin Huschenbett Date: Mon, 7 Oct 2024 07:55:49 +0200 Subject: [PATCH 11/16] Remove needless .into() --- examples/math.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/examples/math.rs b/examples/math.rs index 930c591b..e3c35056 100644 --- a/examples/math.rs +++ b/examples/math.rs @@ -111,7 +111,7 @@ impl> Parser { } fn print(indent: usize, element: SyntaxElement) { - let kind: SyntaxKind = element.kind().into(); + let kind: SyntaxKind = element.kind(); print!("{:indent$}", "", indent = indent); match element { NodeOrToken::Node(node) => { From f06a2c903caa270e977a73ef5b9091f99e8fbe6c Mon Sep 17 00:00:00 2001 From: Theodore Luo Wang Date: Sun, 3 Oct 2021 15:03:23 -0400 Subject: [PATCH 12/16] Reduce allocations when iterating over Syntax{Node,Element} children This allows users of the API to apply a filter on the SyntaxKind before materializing concrete SyntaxNode/Token objects, which require a memory allocation for the NodeData. For slint, this removes ~400k allocations when parsing a largish project, about 50% of all rowan allocations (850k down to 450k). --- src/api.rs | 93 +++++++++++++++++++++++ src/cursor.rs | 202 +++++++++++++++++++++++++++++++++++++++++++++++--- src/lib.rs | 3 +- 3 files changed, 288 insertions(+), 10 deletions(-) diff --git a/src/api.rs b/src/api.rs index ccfb0a58..72ef0a96 100644 --- a/src/api.rs +++ b/src/api.rs @@ -148,6 +148,13 @@ impl SyntaxNode { pub fn first_child(&self) -> Option> { self.raw.first_child().map(Self::from) } + + pub fn first_child_by_kind(&self, matcher: &impl Fn(L::Kind) -> bool) -> Option> { + self.raw + .first_child_by_kind(&|raw_kind| matcher(L::kind_from_raw(raw_kind))) + .map(Self::from) + } + pub fn last_child(&self) -> Option> { self.raw.last_child().map(Self::from) } @@ -155,6 +162,16 @@ impl SyntaxNode { pub fn first_child_or_token(&self) -> Option> { self.raw.first_child_or_token().map(NodeOrToken::from) } + + pub fn first_child_or_token_by_kind( + &self, + matcher: &impl Fn(L::Kind) -> bool, + ) -> Option> { + self.raw + .first_child_or_token_by_kind(&|raw_kind| matcher(L::kind_from_raw(raw_kind))) + .map(NodeOrToken::from) + } + pub fn last_child_or_token(&self) -> Option> { self.raw.last_child_or_token().map(NodeOrToken::from) } @@ -162,6 +179,16 @@ impl SyntaxNode { pub fn next_sibling(&self) -> Option> { self.raw.next_sibling().map(Self::from) } + + pub fn next_sibling_by_kind( + &self, + matcher: &impl Fn(L::Kind) -> bool, + ) -> Option> { + self.raw + .next_sibling_by_kind(&|raw_kind| matcher(L::kind_from_raw(raw_kind))) + .map(Self::from) + } + pub fn prev_sibling(&self) -> Option> { self.raw.prev_sibling().map(Self::from) } @@ -169,6 +196,16 @@ impl SyntaxNode { pub fn next_sibling_or_token(&self) -> Option> { self.raw.next_sibling_or_token().map(NodeOrToken::from) } + + pub fn next_sibling_or_token_by_kind( + &self, + matcher: &impl Fn(L::Kind) -> bool, + ) -> Option> { + self.raw + .next_sibling_or_token_by_kind(&|raw_kind| matcher(L::kind_from_raw(raw_kind))) + .map(NodeOrToken::from) + } + pub fn prev_sibling_or_token(&self) -> Option> { self.raw.prev_sibling_or_token().map(NodeOrToken::from) } @@ -403,6 +440,34 @@ impl Iterator for SyntaxNodeChildren { } } +impl SyntaxNodeChildren { + pub fn by_kind<'a>( + self, + matcher: &'a dyn Fn(SyntaxKind) -> bool, + ) -> SyntaxNodeChildrenByKind<'a, L> { + SyntaxNodeChildrenByKind { raw: self.raw.by_kind(matcher), _p: PhantomData } + } +} + +#[derive(Clone)] +pub struct SyntaxNodeChildrenByKind<'a, L: Language> { + raw: cursor::SyntaxNodeChildrenByKind<&'a dyn Fn(SyntaxKind) -> bool>, + _p: PhantomData, +} + +impl<'a, L: Language> Iterator for SyntaxNodeChildrenByKind<'a, L> { + type Item = SyntaxNode; + fn next(&mut self) -> Option { + self.raw.next().map(SyntaxNode::from) + } +} + +impl<'a, L: Language> fmt::Debug for SyntaxNodeChildrenByKind<'a, L> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("SyntaxNodeChildrenByKind").finish() + } +} + #[derive(Debug, Clone)] pub struct SyntaxElementChildren { raw: cursor::SyntaxElementChildren, @@ -416,6 +481,34 @@ impl Iterator for SyntaxElementChildren { } } +impl SyntaxElementChildren { + pub fn by_kind<'a>( + self, + matcher: &'a dyn Fn(SyntaxKind) -> bool, + ) -> SyntaxElementChildrenByKind<'a, L> { + SyntaxElementChildrenByKind { raw: self.raw.by_kind(matcher), _p: PhantomData } + } +} + +#[derive(Clone)] +pub struct SyntaxElementChildrenByKind<'a, L: Language> { + raw: cursor::SyntaxElementChildrenByKind<&'a dyn Fn(SyntaxKind) -> bool>, + _p: PhantomData, +} + +impl<'a, L: Language> Iterator for SyntaxElementChildrenByKind<'a, L> { + type Item = SyntaxElement; + fn next(&mut self) -> Option { + self.raw.next().map(NodeOrToken::from) + } +} + +impl<'a, L: Language> fmt::Debug for SyntaxElementChildrenByKind<'a, L> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("SyntaxElementChildrenByKind").finish() + } +} + pub struct Preorder { raw: cursor::Preorder, _p: PhantomData, diff --git a/src/cursor.rs b/src/cursor.rs index f7faf27a..4681c214 100644 --- a/src/cursor.rs +++ b/src/cursor.rs @@ -376,11 +376,26 @@ impl NodeData { } fn next_sibling(&self) -> Option { - let mut siblings = self.green_siblings().enumerate(); + let siblings = self.green_siblings().enumerate(); + let index = self.index() as usize; + + siblings.skip(index + 1).find_map(|(index, child)| { + child.as_ref().into_node().and_then(|green| { + let parent = self.parent_node()?; + let offset = parent.offset() + child.rel_offset(); + Some(SyntaxNode::new_child(green, parent, index as u32, offset)) + }) + }) + } + + fn next_sibling_by_kind(&self, matcher: &impl Fn(SyntaxKind) -> bool) -> Option { + let siblings = self.green_siblings().enumerate(); let index = self.index() as usize; - siblings.nth(index); - siblings.find_map(|(index, child)| { + siblings.skip(index + 1).find_map(|(index, child)| { + if !matcher(child.as_ref().kind()) { + return None; + } child.as_ref().into_node().and_then(|green| { let parent = self.parent_node()?; let offset = parent.offset() + child.rel_offset(); @@ -388,12 +403,12 @@ impl NodeData { }) }) } + fn prev_sibling(&self) -> Option { - let mut rev_siblings = self.green_siblings().enumerate().rev(); - let index = rev_siblings.len().checked_sub(self.index() as usize + 1)?; + let rev_siblings = self.green_siblings().enumerate().rev(); + let index = rev_siblings.len().checked_sub(self.index() as usize)?; - rev_siblings.nth(index); - rev_siblings.find_map(|(index, child)| { + rev_siblings.skip(index + 1).find_map(|(index, child)| { child.as_ref().into_node().and_then(|green| { let parent = self.parent_node()?; let offset = parent.offset() + child.rel_offset(); @@ -412,6 +427,24 @@ impl NodeData { Some(SyntaxElement::new(child.as_ref(), parent, index as u32, offset)) }) } + + fn next_sibling_or_token_by_kind( + &self, + matcher: &impl Fn(SyntaxKind) -> bool, + ) -> Option { + let siblings = self.green_siblings().enumerate(); + let index = self.index() as usize; + + siblings.skip(index + 1).find_map(|(index, child)| { + if !matcher(child.as_ref().kind()) { + return None; + } + let parent = self.parent_node()?; + let offset = parent.offset() + child.rel_offset(); + Some(SyntaxElement::new(child.as_ref(), parent, index as u32, offset)) + }) + } + fn prev_sibling_or_token(&self) -> Option { let mut siblings = self.green_siblings().enumerate(); let index = self.index().checked_sub(1)? as usize; @@ -647,6 +680,23 @@ impl SyntaxNode { }) }) } + + pub fn first_child_by_kind(&self, matcher: &impl Fn(SyntaxKind) -> bool) -> Option { + self.green_ref().children().raw.enumerate().find_map(|(index, child)| { + if !matcher(child.as_ref().kind()) { + return None; + } + child.as_ref().into_node().map(|green| { + SyntaxNode::new_child( + green, + self.clone(), + index as u32, + self.offset() + child.rel_offset(), + ) + }) + }) + } + pub fn last_child(&self) -> Option { self.green_ref().children().raw.enumerate().rev().find_map(|(index, child)| { child.as_ref().into_node().map(|green| { @@ -665,6 +715,24 @@ impl SyntaxNode { SyntaxElement::new(child.as_ref(), self.clone(), 0, self.offset() + child.rel_offset()) }) } + + pub fn first_child_or_token_by_kind( + &self, + matcher: &impl Fn(SyntaxKind) -> bool, + ) -> Option { + self.green_ref().children().raw.enumerate().find_map(|(index, child)| { + if !matcher(child.as_ref().kind()) { + return None; + } + Some(SyntaxElement::new( + child.as_ref(), + self.clone(), + index as u32, + self.offset() + child.rel_offset(), + )) + }) + } + pub fn last_child_or_token(&self) -> Option { self.green_ref().children().raw.enumerate().next_back().map(|(index, child)| { SyntaxElement::new( @@ -679,6 +747,14 @@ impl SyntaxNode { pub fn next_sibling(&self) -> Option { self.data().next_sibling() } + + pub fn next_sibling_by_kind( + &self, + matcher: &impl Fn(SyntaxKind) -> bool, + ) -> Option { + self.data().next_sibling_by_kind(matcher) + } + pub fn prev_sibling(&self) -> Option { self.data().prev_sibling() } @@ -686,6 +762,14 @@ impl SyntaxNode { pub fn next_sibling_or_token(&self) -> Option { self.data().next_sibling_or_token() } + + pub fn next_sibling_or_token_by_kind( + &self, + matcher: &impl Fn(SyntaxKind) -> bool, + ) -> Option { + self.data().next_sibling_or_token_by_kind(matcher) + } + pub fn prev_sibling_or_token(&self) -> Option { self.data().prev_sibling_or_token() } @@ -910,6 +994,14 @@ impl SyntaxToken { pub fn next_sibling_or_token(&self) -> Option { self.data().next_sibling_or_token() } + + pub fn next_sibling_or_token_by_kind( + &self, + matcher: &impl Fn(SyntaxKind) -> bool, + ) -> Option { + self.data().next_sibling_or_token_by_kind(matcher) + } + pub fn prev_sibling_or_token(&self) -> Option { self.data().prev_sibling_or_token() } @@ -1028,6 +1120,17 @@ impl SyntaxElement { NodeOrToken::Token(it) => it.next_sibling_or_token(), } } + + pub fn next_sibling_or_token_by_kind( + &self, + matcher: &impl Fn(SyntaxKind) -> bool, + ) -> Option { + match self { + NodeOrToken::Node(it) => it.next_sibling_or_token_by_kind(matcher), + NodeOrToken::Token(it) => it.next_sibling_or_token_by_kind(matcher), + } + } + pub fn prev_sibling_or_token(&self) -> Option { match self { NodeOrToken::Node(it) => it.prev_sibling_or_token(), @@ -1133,18 +1236,41 @@ impl From for SyntaxElement { #[derive(Clone, Debug)] pub struct SyntaxNodeChildren { + parent: SyntaxNode, next: Option, + next_initialized: bool, } impl SyntaxNodeChildren { fn new(parent: SyntaxNode) -> SyntaxNodeChildren { - SyntaxNodeChildren { next: parent.first_child() } + SyntaxNodeChildren { parent, next: None, next_initialized: false } + } + + pub fn by_kind bool>(self, matcher: F) -> SyntaxNodeChildrenByKind { + if !self.next_initialized { + SyntaxNodeChildrenByKind { next: self.parent.first_child_by_kind(&matcher), matcher } + } else { + SyntaxNodeChildrenByKind { + next: self.next.and_then(|node| { + if matcher(node.kind()) { + Some(node) + } else { + node.next_sibling_by_kind(&matcher) + } + }), + matcher, + } + } } } impl Iterator for SyntaxNodeChildren { type Item = SyntaxNode; fn next(&mut self) -> Option { + if !self.next_initialized { + self.next = self.parent.first_child(); + self.next_initialized = true; + } self.next.take().map(|next| { self.next = next.next_sibling(); next @@ -1152,20 +1278,62 @@ impl Iterator for SyntaxNodeChildren { } } +#[derive(Clone, Debug)] +pub struct SyntaxNodeChildrenByKind bool> { + next: Option, + matcher: F, +} + +impl bool> Iterator for SyntaxNodeChildrenByKind { + type Item = SyntaxNode; + fn next(&mut self) -> Option { + self.next.take().map(|next| { + self.next = next.next_sibling_by_kind(&self.matcher); + next + }) + } +} + #[derive(Clone, Debug)] pub struct SyntaxElementChildren { + parent: SyntaxNode, next: Option, + next_initialized: bool, } impl SyntaxElementChildren { fn new(parent: SyntaxNode) -> SyntaxElementChildren { - SyntaxElementChildren { next: parent.first_child_or_token() } + SyntaxElementChildren { parent, next: None, next_initialized: false } + } + + pub fn by_kind bool>(self, matcher: F) -> SyntaxElementChildrenByKind { + if !self.next_initialized { + SyntaxElementChildrenByKind { + next: self.parent.first_child_or_token_by_kind(&matcher), + matcher, + } + } else { + SyntaxElementChildrenByKind { + next: self.next.and_then(|node| { + if matcher(node.kind()) { + Some(node) + } else { + node.next_sibling_or_token_by_kind(&matcher) + } + }), + matcher, + } + } } } impl Iterator for SyntaxElementChildren { type Item = SyntaxElement; fn next(&mut self) -> Option { + if !self.next_initialized { + self.next = self.parent.first_child_or_token(); + self.next_initialized = true; + } self.next.take().map(|next| { self.next = next.next_sibling_or_token(); next @@ -1173,6 +1341,22 @@ impl Iterator for SyntaxElementChildren { } } +#[derive(Clone, Debug)] +pub struct SyntaxElementChildrenByKind bool> { + next: Option, + matcher: F, +} + +impl bool> Iterator for SyntaxElementChildrenByKind { + type Item = SyntaxElement; + fn next(&mut self) -> Option { + self.next.take().map(|next| { + self.next = next.next_sibling_or_token_by_kind(&self.matcher); + next + }) + } +} + pub struct Preorder { start: SyntaxNode, next: Option>, diff --git a/src/lib.rs b/src/lib.rs index bb8f30d0..066d8c52 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -30,7 +30,8 @@ pub use text_size::{TextLen, TextRange, TextSize}; pub use crate::{ api::{ - Language, SyntaxElement, SyntaxElementChildren, SyntaxNode, SyntaxNodeChildren, SyntaxToken, + Language, SyntaxElement, SyntaxElementChildren, SyntaxElementChildrenByKind, SyntaxNode, + SyntaxNodeChildren, SyntaxNodeChildrenByKind, SyntaxToken, }, green::{ Checkpoint, Children, GreenNode, GreenNodeBuilder, GreenNodeData, GreenToken, From 0b1a6ec04ae9aa60413c28ddee0c344415129260 Mon Sep 17 00:00:00 2001 From: Milian Wolff Date: Sun, 27 Oct 2024 19:39:28 +0100 Subject: [PATCH 13/16] Optimize iteration over sibling nodes by reusing NodeData if possible When possible, reuse the allocated NodeData instead of allocating a new one for each iteration. This can be done as long as the refcount is 1 - we can then just rewire the values in NodeData to point to the new item. This removes ~220k allocations when compiling a largish slint file, about half of all rowan allocations that happen during iteration, i.e. we go from 450k down to 230k. --- src/cursor.rs | 134 +++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 126 insertions(+), 8 deletions(-) diff --git a/src/cursor.rs b/src/cursor.rs index 4681c214..aa66be05 100644 --- a/src/cursor.rs +++ b/src/cursor.rs @@ -597,6 +597,20 @@ impl SyntaxNode { unsafe { self.ptr.as_ref() } } + #[inline] + fn can_take_ptr(&self) -> bool { + self.data().rc.get() == 1 && !self.data().mutable + } + + #[inline] + fn take_ptr(self) -> ptr::NonNull { + assert!(self.can_take_ptr()); + let ret = self.ptr; + // don't change the refcount when self gets dropped + std::mem::forget(self); + ret + } + pub fn replace_with(&self, replacement: GreenNode) -> GreenNode { assert_eq!(self.kind(), replacement.kind()); match &self.parent() { @@ -744,6 +758,43 @@ impl SyntaxNode { }) } + // if possible (i.e. unshared), consume self and advance it to point to the next sibling + // this way, we can reuse the previously allocated buffer + pub fn to_next_sibling(self) -> Option { + if !self.can_take_ptr() { + // cannot mutate in-place + return self.next_sibling(); + } + + let mut ptr = self.take_ptr(); + let data = unsafe { ptr.as_mut() }; + assert!(data.rc.get() == 1); + + let parent = data.parent_node()?; + let parent_offset = parent.offset(); + let siblings = parent.green_ref().children().raw.enumerate(); + let index = data.index() as usize; + + siblings + .skip(index + 1) + .find_map(|(index, child)| { + child + .as_ref() + .into_node() + .and_then(|green| Some((green, index as u32, child.rel_offset()))) + }) + .and_then(|(green, index, rel_offset)| { + data.index.set(index); + data.offset = parent_offset + rel_offset; + data.green = Green::Node { ptr: Cell::new(green.into()) }; + Some(SyntaxNode { ptr }) + }) + .or_else(|| { + unsafe { free(ptr) }; + None + }) + } + pub fn next_sibling(&self) -> Option { self.data().next_sibling() } @@ -937,6 +988,20 @@ impl SyntaxToken { unsafe { self.ptr.as_ref() } } + #[inline] + fn can_take_ptr(&self) -> bool { + self.data().rc.get() == 1 && !self.data().mutable + } + + #[inline] + fn take_ptr(self) -> ptr::NonNull { + assert!(self.can_take_ptr()); + let ret = self.ptr; + // don't change the refcount when self gets dropped + std::mem::forget(self); + ret + } + pub fn replace_with(&self, replacement: GreenToken) -> GreenNode { assert_eq!(self.kind(), replacement.kind()); let parent = self.parent().unwrap(); @@ -1121,6 +1186,59 @@ impl SyntaxElement { } } + fn can_take_ptr(&self) -> bool { + match self { + NodeOrToken::Node(it) => it.can_take_ptr(), + NodeOrToken::Token(it) => it.can_take_ptr(), + } + } + + fn take_ptr(self) -> ptr::NonNull { + match self { + NodeOrToken::Node(it) => it.take_ptr(), + NodeOrToken::Token(it) => it.take_ptr(), + } + } + + // if possible (i.e. unshared), consume self and advance it to point to the next sibling + // this way, we can reuse the previously allocated buffer + pub fn to_next_sibling_or_token(self) -> Option { + if !self.can_take_ptr() { + // cannot mutate in-place + return self.next_sibling_or_token(); + } + + let mut ptr = self.take_ptr(); + let data = unsafe { ptr.as_mut() }; + + let parent = data.parent_node()?; + let parent_offset = parent.offset(); + let siblings = parent.green_ref().children().raw.enumerate(); + let index = data.index() as usize; + + siblings + .skip(index + 1) + .find_map(|(index, green)| { + data.index.set(index as u32); + data.offset = parent_offset + green.rel_offset(); + + match green.as_ref() { + NodeOrToken::Node(node) => { + data.green = Green::Node { ptr: Cell::new(node.into()) }; + Some(SyntaxElement::Node(SyntaxNode { ptr })) + } + NodeOrToken::Token(token) => { + data.green = Green::Token { ptr: token.into() }; + Some(SyntaxElement::Token(SyntaxToken { ptr })) + } + } + }) + .or_else(|| { + unsafe { free(ptr) }; + None + }) + } + pub fn next_sibling_or_token_by_kind( &self, matcher: &impl Fn(SyntaxKind) -> bool, @@ -1270,11 +1388,11 @@ impl Iterator for SyntaxNodeChildren { if !self.next_initialized { self.next = self.parent.first_child(); self.next_initialized = true; + } else { + self.next = self.next.take().and_then(|next| next.to_next_sibling()); } - self.next.take().map(|next| { - self.next = next.next_sibling(); - next - }) + + self.next.clone() } } @@ -1333,11 +1451,11 @@ impl Iterator for SyntaxElementChildren { if !self.next_initialized { self.next = self.parent.first_child_or_token(); self.next_initialized = true; + } else { + self.next = self.next.take().and_then(|next| next.to_next_sibling_or_token()); } - self.next.take().map(|next| { - self.next = next.next_sibling_or_token(); - next - }) + + self.next.clone() } } From ab5463e2749330be6846886c21e98c83caca8598 Mon Sep 17 00:00:00 2001 From: Milian Wolff Date: Sun, 27 Oct 2024 22:06:08 +0100 Subject: [PATCH 14/16] Use iterators for by_kind instead of introducing separate structs This makes this API actually useful from the outside - there is no lifetime problem with the matcher callback, and we can remap the raw Kind to the Language::Kind on the fly. --- src/api.rs | 56 +++++++++--------------------------------------------- src/lib.rs | 3 +-- 2 files changed, 10 insertions(+), 49 deletions(-) diff --git a/src/api.rs b/src/api.rs index 72ef0a96..cd86d9d8 100644 --- a/src/api.rs +++ b/src/api.rs @@ -441,30 +441,11 @@ impl Iterator for SyntaxNodeChildren { } impl SyntaxNodeChildren { - pub fn by_kind<'a>( - self, - matcher: &'a dyn Fn(SyntaxKind) -> bool, - ) -> SyntaxNodeChildrenByKind<'a, L> { - SyntaxNodeChildrenByKind { raw: self.raw.by_kind(matcher), _p: PhantomData } - } -} - -#[derive(Clone)] -pub struct SyntaxNodeChildrenByKind<'a, L: Language> { - raw: cursor::SyntaxNodeChildrenByKind<&'a dyn Fn(SyntaxKind) -> bool>, - _p: PhantomData, -} - -impl<'a, L: Language> Iterator for SyntaxNodeChildrenByKind<'a, L> { - type Item = SyntaxNode; - fn next(&mut self) -> Option { - self.raw.next().map(SyntaxNode::from) - } -} - -impl<'a, L: Language> fmt::Debug for SyntaxNodeChildrenByKind<'a, L> { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.debug_struct("SyntaxNodeChildrenByKind").finish() + pub fn by_kind(self, matcher: impl Fn(L::Kind) -> bool) -> impl Iterator> { + self.raw + .by_kind(move |raw_kind| matcher(L::kind_from_raw(raw_kind))) + .into_iter() + .map(SyntaxNode::from) } } @@ -482,30 +463,11 @@ impl Iterator for SyntaxElementChildren { } impl SyntaxElementChildren { - pub fn by_kind<'a>( + pub fn by_kind( self, - matcher: &'a dyn Fn(SyntaxKind) -> bool, - ) -> SyntaxElementChildrenByKind<'a, L> { - SyntaxElementChildrenByKind { raw: self.raw.by_kind(matcher), _p: PhantomData } - } -} - -#[derive(Clone)] -pub struct SyntaxElementChildrenByKind<'a, L: Language> { - raw: cursor::SyntaxElementChildrenByKind<&'a dyn Fn(SyntaxKind) -> bool>, - _p: PhantomData, -} - -impl<'a, L: Language> Iterator for SyntaxElementChildrenByKind<'a, L> { - type Item = SyntaxElement; - fn next(&mut self) -> Option { - self.raw.next().map(NodeOrToken::from) - } -} - -impl<'a, L: Language> fmt::Debug for SyntaxElementChildrenByKind<'a, L> { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.debug_struct("SyntaxElementChildrenByKind").finish() + matcher: impl Fn(L::Kind) -> bool, + ) -> impl Iterator> { + self.raw.by_kind(move |raw_kind| matcher(L::kind_from_raw(raw_kind))).map(NodeOrToken::from) } } diff --git a/src/lib.rs b/src/lib.rs index 066d8c52..bb8f30d0 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -30,8 +30,7 @@ pub use text_size::{TextLen, TextRange, TextSize}; pub use crate::{ api::{ - Language, SyntaxElement, SyntaxElementChildren, SyntaxElementChildrenByKind, SyntaxNode, - SyntaxNodeChildren, SyntaxNodeChildrenByKind, SyntaxToken, + Language, SyntaxElement, SyntaxElementChildren, SyntaxNode, SyntaxNodeChildren, SyntaxToken, }, green::{ Checkpoint, Children, GreenNode, GreenNodeBuilder, GreenNodeData, GreenToken, From 32949733eeaa62ff91b08dfbe51e55fa0c737834 Mon Sep 17 00:00:00 2001 From: Milian Wolff Date: Thu, 31 Oct 2024 11:01:59 +0100 Subject: [PATCH 15/16] Decrement refcount before calling free in to_next_sibling Fixes an assertion in debug builds which I accidentally introduced when attending the review comments for [1] in [2] - instead of only removing the increment, I also removed the decrement which was wrong - `std::mem::forget` only allows us to remove the increment, but the decrement is still needed before free since we are in a place of code that is by definition only run when the rc value is set to 1. See also `can_take_ptr`. I did not spot this earlier since I ran the integration test on a release build, where the assertion was disabled. It's sad that the rowan repo itself doesn't have any big test coverage in this repo itself, but rather relies on external repos for testing purposes... [1]: https://github.com/rust-analyzer/rowan/pull/171#discussion_r1818509336 [2]: https://github.com/rust-analyzer/rowan/compare/60a632ad984ab451e32058169193511154c675a9..ab5463e2749330be6846886c21e98c83caca8598 Fixes: https://github.com/rust-analyzer/rowan/issues/172 --- src/cursor.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/cursor.rs b/src/cursor.rs index aa66be05..a6d84c68 100644 --- a/src/cursor.rs +++ b/src/cursor.rs @@ -790,6 +790,7 @@ impl SyntaxNode { Some(SyntaxNode { ptr }) }) .or_else(|| { + data.dec_rc(); unsafe { free(ptr) }; None }) @@ -1234,6 +1235,7 @@ impl SyntaxElement { } }) .or_else(|| { + data.dec_rc(); unsafe { free(ptr) }; None }) From ac472cfdd960dd92d8e52817431e2bd7216c33b1 Mon Sep 17 00:00:00 2001 From: Martin Huschenbett Date: Thu, 7 Nov 2024 21:59:17 +0100 Subject: [PATCH 16/16] Implement Eq and Hash for GreenTokenData `GreenToken` implements all traits required to use it as the key type of a hash map. It also implements `Borrow`. Thus is would be nice if `GreenTokenData` also implements `Eq` and `Hash` such that the result of `SyntaxToken::green` can be used for lookups in such a hash map without additional overhead in terms of sytntax and runtime. --- src/green/token.rs | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/green/token.rs b/src/green/token.rs index 81e298c7..18e1434c 100644 --- a/src/green/token.rs +++ b/src/green/token.rs @@ -1,6 +1,7 @@ use std::{ borrow::Borrow, fmt, + hash::{Hash, Hasher}, mem::{self, ManuallyDrop}, ops, ptr, }; @@ -21,6 +22,7 @@ struct GreenTokenHead { type Repr = HeaderSlice; type ReprThin = HeaderSlice; +#[derive(Eq)] #[repr(transparent)] pub struct GreenTokenData { data: ReprThin, @@ -32,6 +34,12 @@ impl PartialEq for GreenTokenData { } } +impl Hash for GreenTokenData { + fn hash(&self, state: &mut H) { + (*self.data).hash(state); + } +} + /// Leaf node in the immutable tree. #[derive(PartialEq, Eq, Hash, Clone)] #[repr(transparent)] @@ -141,3 +149,18 @@ impl ops::Deref for GreenToken { } } } + +#[cfg(test)] +mod tests { + use std::hash::{BuildHasher, RandomState}; + + use super::*; + + #[test] + fn hash_borrow() { + let owned = GreenToken::new(SyntaxKind(42), "foobar"); + let borrowed: &GreenTokenData = owned.borrow(); + let s = RandomState::new(); + assert_eq!(s.hash_one(&owned), s.hash_one(borrowed)); + } +}