From 2785f167789002fb8ba024b7694938cd986a20f7 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Sun, 28 Aug 2022 12:35:33 +0200 Subject: [PATCH 1/6] refactor(rome_formatter): Return reference from `context.comments()` `context.comments` used to return a `Rc` so that the lifetime of `comments` isn't bound to the lifetime of the `FormatContext`. This is necessary to support writing to the formatter while holding to a reference of comments: (Pseudo code): ```rust let comments = f.context().comments(); for comment in comments.leading_comments(node) { write!(f, [comment])?; } ``` The downside of returning an `Rc` is that every code path using `comments` pays for the overhead of incrementing the `Rc` counter. This PR changes `Comments` to use an `Rc` internally to enable cheap cloning, similar to how `SyntaxNode` works. This allows `context.comments()` to return a reference to `Comments` instead and only code paths that need to write to the formatter have to pay the overhead of cloning the comments. ## Tests `cargo test` as this PR isn't adding any new functionality --- crates/rome_formatter/src/comments.rs | 46 ++++++++++++++++--------- crates/rome_formatter/src/lib.rs | 3 +- crates/rome_js_formatter/src/context.rs | 4 +-- 3 files changed, 32 insertions(+), 21 deletions(-) diff --git a/crates/rome_formatter/src/comments.rs b/crates/rome_formatter/src/comments.rs index 93912040d8d..2d21c411fec 100644 --- a/crates/rome_formatter/src/comments.rs +++ b/crates/rome_formatter/src/comments.rs @@ -5,6 +5,7 @@ use rome_rowan::{ #[cfg(debug_assertions)] use std::cell::RefCell; use std::collections::HashSet; +use std::rc::Rc; #[derive(Copy, Clone, Eq, PartialEq, Debug)] pub enum CommentKind { @@ -138,21 +139,11 @@ pub trait CommentStyle { /// * whether a node should be formatted as is because it has a leading suppression comment. /// * a node's leading and trailing comments /// * the dangling comments of a token +/// +/// Cloning `comments` is cheap as it only involves bumping a reference counter. #[derive(Debug, Default, Clone)] pub struct Comments { - /// Stores the nodes that have at least one leading suppression comment. - suppressed_nodes: HashSet>, - - /// Stores all nodes for which [Comments::is_suppressed] has been called. - /// This index of nodes that have been checked if they have a suppression comments is used to - /// detect format implementations that manually format a child node without previously checking if - /// the child has a suppression comment. - /// - /// The implementation refrains from snapshotting the checked nodes because a node gets formatted - /// as verbatim if its formatting fails which has the same result as formatting it as suppressed node - /// (thus, guarantees that the formatting isn't changed). - #[cfg(debug_assertions)] - checked_suppressions: RefCell>>, + data: Rc>, } impl Comments { @@ -202,10 +193,14 @@ impl Comments { } } - Self { + let data = CommentsData { suppressed_nodes, #[cfg(debug_assertions)] checked_suppressions: RefCell::default(), + }; + + Self { + data: Rc::new(data), } } @@ -225,7 +220,7 @@ impl Comments { /// call expression is nested inside of the expression statement. pub fn is_suppressed(&self, node: &SyntaxNode) -> bool { self.mark_suppression_checked(node); - self.suppressed_nodes.contains(node) + self.data.suppressed_nodes.contains(node) } /// Marks that it isn't necessary for the given node to check if it has been suppressed or not. @@ -233,7 +228,7 @@ impl Comments { pub fn mark_suppression_checked(&self, node: &SyntaxNode) { cfg_if::cfg_if! { if #[cfg(debug_assertions)] { - let mut checked_nodes = self.checked_suppressions.borrow_mut(); + let mut checked_nodes = self.data.checked_suppressions.borrow_mut(); checked_nodes.insert(node.clone()); } else { let _ = node; @@ -250,7 +245,7 @@ impl Comments { pub(crate) fn assert_checked_all_suppressions(&self, root: &SyntaxNode) { cfg_if::cfg_if! { if #[cfg(debug_assertions)] { - let checked_nodes = self.checked_suppressions.borrow(); + let checked_nodes = self.data.checked_suppressions.borrow(); for node in root.descendants() { if node.kind().is_list() || node.kind().is_root() { continue; @@ -274,3 +269,20 @@ Node: } } } + +#[derive(Debug, Default)] +struct CommentsData { + /// Stores the nodes that have at least one leading suppression comment. + suppressed_nodes: HashSet>, + + /// Stores all nodes for which [Comments::is_suppressed] has been called. + /// This index of nodes that have been checked if they have a suppression comments is used to + /// detect format implementations that manually format a child node without previously checking if + /// the child has a suppression comment. + /// + /// The implementation refrains from snapshotting the checked nodes because a node gets formatted + /// as verbatim if its formatting fails which has the same result as formatting it as suppressed node + /// (thus, guarantees that the formatting isn't changed). + #[cfg(debug_assertions)] + checked_suppressions: RefCell>>, +} diff --git a/crates/rome_formatter/src/lib.rs b/crates/rome_formatter/src/lib.rs index 6560be18ad3..58c2108c1f5 100644 --- a/crates/rome_formatter/src/lib.rs +++ b/crates/rome_formatter/src/lib.rs @@ -65,7 +65,6 @@ use rome_rowan::{ }; use std::error::Error; use std::num::ParseIntError; -use std::rc::Rc; use std::str::FromStr; #[derive(Debug, Eq, PartialEq, Clone, Copy, Hash)] @@ -248,7 +247,7 @@ pub trait CstFormatContext: FormatContext { /// |- Mutably borrows the formatter, state, context (and comments, if they aren't wrapped by a Rc) /// } /// ``` - fn comments(&self) -> Rc>; + fn comments(&self) -> &Comments; } #[derive(Debug, Default, Eq, PartialEq)] diff --git a/crates/rome_js_formatter/src/context.rs b/crates/rome_js_formatter/src/context.rs index 7c78aa2007a..b6816fe5471 100644 --- a/crates/rome_js_formatter/src/context.rs +++ b/crates/rome_js_formatter/src/context.rs @@ -59,8 +59,8 @@ impl CstFormatContext for JsFormatContext { JsCommentStyle } - fn comments(&self) -> Rc> { - self.comments.clone() + fn comments(&self) -> &Comments { + &self.comments } } From 7816bce0aa8eb1df14b45179434eed88f0bad248 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Sun, 28 Aug 2022 12:42:37 +0200 Subject: [PATCH 2/6] Documentation --- crates/rome_formatter/src/comments.rs | 15 +++++++++++++++ crates/rome_formatter/src/lib.rs | 16 +--------------- 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/crates/rome_formatter/src/comments.rs b/crates/rome_formatter/src/comments.rs index 2d21c411fec..fc8cf99a829 100644 --- a/crates/rome_formatter/src/comments.rs +++ b/crates/rome_formatter/src/comments.rs @@ -143,6 +143,21 @@ pub trait CommentStyle { /// Cloning `comments` is cheap as it only involves bumping a reference counter. #[derive(Debug, Default, Clone)] pub struct Comments { + /// The use of a [Rc] is necessary to achieve that [Comments] has a lifetime that is independent of the [crate::Formatter]. + /// Having independent lifetimes is necessary to support the use case where a (formattable object)[Format] + /// iterates over all comments and writes them into the [crate::Formatter] (mutably borrowing the [crate::Formatter] and in turn its context). + /// + /// ```block + /// for leading in f.context().comments().leading_comments(node) { + /// ^ + /// |- Borrows comments + /// write!(f, [comment(leading.piece.text())])?; + /// ^ + /// |- Mutably borrows the formatter, state, context, and comments (if comments aren't cloned) + /// } + /// ``` + /// + /// Using an `Rc` here allows cheaply cloning [Comments] for these use cases. data: Rc>, } diff --git a/crates/rome_formatter/src/lib.rs b/crates/rome_formatter/src/lib.rs index 58c2108c1f5..1d51260165a 100644 --- a/crates/rome_formatter/src/lib.rs +++ b/crates/rome_formatter/src/lib.rs @@ -232,21 +232,7 @@ pub trait CstFormatContext: FormatContext { #[deprecated(note = "Prefer FormatLanguage::comment_style")] fn comment_style(&self) -> Self::Style; - /// Returns a ref counted [Comments]. - /// - /// The use of a [Rc] is necessary to achieve that [Comments] has a lifetime that is independent of the [crate::Formatter]. - /// Having independent lifetimes is necessary to support the use case where a (formattable object)[Format] - /// iterates over all comments and writes them into the [crate::Formatter] (mutably borrowing the [crate::Formatter] and in turn this context). - /// - /// ```block - /// for leading in f.context().comments().leading_comments(node) { - /// ^ - /// |- Borrows comments - /// write!(f, [comment(leading.piece.text())])?; - /// ^ - /// |- Mutably borrows the formatter, state, context (and comments, if they aren't wrapped by a Rc) - /// } - /// ``` + /// Returns a reference to the program's comments. fn comments(&self) -> &Comments; } From de7cb0499358f4a6e6abfe2220c14dc3bdca8fcf Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Mon, 29 Aug 2022 08:14:07 +0200 Subject: [PATCH 3/6] Clippy --- crates/rome_js_formatter/src/js/statements/block_statement.rs | 2 +- crates/rome_js_formatter/src/utils/member_chain/mod.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/rome_js_formatter/src/js/statements/block_statement.rs b/crates/rome_js_formatter/src/js/statements/block_statement.rs index 0ebd548b2ca..3b1ad2389d2 100644 --- a/crates/rome_js_formatter/src/js/statements/block_statement.rs +++ b/crates/rome_js_formatter/src/js/statements/block_statement.rs @@ -18,7 +18,7 @@ impl FormatNodeRule for FormatJsBlockStatement { r_curly_token, } = node.as_fields(); - if is_non_collapsable_empty_block(node, &f.context().comments()) { + if is_non_collapsable_empty_block(node, f.context().comments()) { for stmt in statements .iter() .filter_map(|stmt| JsEmptyStatement::cast(stmt.into_syntax())) diff --git a/crates/rome_js_formatter/src/utils/member_chain/mod.rs b/crates/rome_js_formatter/src/utils/member_chain/mod.rs index 32c80a95046..e6b3dbdd4dc 100644 --- a/crates/rome_js_formatter/src/utils/member_chain/mod.rs +++ b/crates/rome_js_formatter/src/utils/member_chain/mod.rs @@ -201,7 +201,7 @@ pub(crate) fn get_member_chain( let root = flatten_member_chain( &mut chain_members, call_expression.clone().into(), - &f.context().comments(), + f.context().comments(), )?; chain_members.push(root); From 9a81d1eabe2a511cf99338341f184c673f63badf Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Mon, 29 Aug 2022 08:39:17 +0200 Subject: [PATCH 4/6] Documentation --- crates/rome_formatter/src/comments.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/rome_formatter/src/comments.rs b/crates/rome_formatter/src/comments.rs index fc8cf99a829..fe41f300d1c 100644 --- a/crates/rome_formatter/src/comments.rs +++ b/crates/rome_formatter/src/comments.rs @@ -144,7 +144,7 @@ pub trait CommentStyle { #[derive(Debug, Default, Clone)] pub struct Comments { /// The use of a [Rc] is necessary to achieve that [Comments] has a lifetime that is independent of the [crate::Formatter]. - /// Having independent lifetimes is necessary to support the use case where a (formattable object)[Format] + /// Having independent lifetimes is necessary to support the use case where a (formattable object)[crate::Format] /// iterates over all comments and writes them into the [crate::Formatter] (mutably borrowing the [crate::Formatter] and in turn its context). /// /// ```block From 84691328ed6fc01edfce4cce926c78ce0b9aa005 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Mon, 29 Aug 2022 11:44:15 +0200 Subject: [PATCH 5/6] Update crates/rome_formatter/src/comments.rs Co-authored-by: Emanuele Stoppa --- crates/rome_formatter/src/comments.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/rome_formatter/src/comments.rs b/crates/rome_formatter/src/comments.rs index fe41f300d1c..acc01f08f9a 100644 --- a/crates/rome_formatter/src/comments.rs +++ b/crates/rome_formatter/src/comments.rs @@ -143,7 +143,7 @@ pub trait CommentStyle { /// Cloning `comments` is cheap as it only involves bumping a reference counter. #[derive(Debug, Default, Clone)] pub struct Comments { - /// The use of a [Rc] is necessary to achieve that [Comments] has a lifetime that is independent of the [crate::Formatter]. + /// The use of a [Rc] is necessary to achieve that [Comments] has a lifetime that is independent from the [crate::Formatter]. /// Having independent lifetimes is necessary to support the use case where a (formattable object)[crate::Format] /// iterates over all comments and writes them into the [crate::Formatter] (mutably borrowing the [crate::Formatter] and in turn its context). /// From e1cd151145af2bb8f17b10d433e5ae8862aebb84 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Mon, 29 Aug 2022 11:44:36 +0200 Subject: [PATCH 6/6] Apply suggestions from code review Co-authored-by: Emanuele Stoppa --- crates/rome_formatter/src/comments.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/rome_formatter/src/comments.rs b/crates/rome_formatter/src/comments.rs index acc01f08f9a..d6c34cad742 100644 --- a/crates/rome_formatter/src/comments.rs +++ b/crates/rome_formatter/src/comments.rs @@ -145,7 +145,7 @@ pub trait CommentStyle { pub struct Comments { /// The use of a [Rc] is necessary to achieve that [Comments] has a lifetime that is independent from the [crate::Formatter]. /// Having independent lifetimes is necessary to support the use case where a (formattable object)[crate::Format] - /// iterates over all comments and writes them into the [crate::Formatter] (mutably borrowing the [crate::Formatter] and in turn its context). + /// iterates over all comments, and writes them into the [crate::Formatter] (mutably borrowing the [crate::Formatter] and in turn its context). /// /// ```block /// for leading in f.context().comments().leading_comments(node) { @@ -157,7 +157,7 @@ pub struct Comments { /// } /// ``` /// - /// Using an `Rc` here allows cheaply cloning [Comments] for these use cases. + /// Using an `Rc` here allows to cheaply clone [Comments] for these use cases. data: Rc>, }