From 0650f77dd9defaf352f81c5ee4ee73a1eae942b7 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Mon, 10 May 2021 19:04:41 +0300 Subject: internal: remove one more immutable tree --- Cargo.lock | 4 ++-- crates/ide_db/src/helpers/merge_imports.rs | 34 +++++++++++++++++------------ crates/syntax/Cargo.toml | 2 +- crates/syntax/src/algo.rs | 35 ++++-------------------------- crates/syntax/src/ast.rs | 6 +++++ crates/syntax/src/ast/make.rs | 10 ++------- docs/dev/style.md | 22 +++++++++++++++++++ 7 files changed, 57 insertions(+), 56 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f9c34547e..a19b8339c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1295,9 +1295,9 @@ checksum = "f497285884f3fcff424ffc933e56d7cbca511def0c9831a7f9b5f6153e3cc89b" [[package]] name = "rowan" -version = "0.13.0-pre.5" +version = "0.13.0-pre.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "32a5fc82ed0b7e7fba157331f0d8f64abd73bced6e7ac2a4dfa0c4cf0ab584e8" +checksum = "82ccc04e145e9a5ab51b9c12a81d77c4a8250d87a407ab02ac650451141ff00d" dependencies = [ "countme", "hashbrown", diff --git a/crates/ide_db/src/helpers/merge_imports.rs b/crates/ide_db/src/helpers/merge_imports.rs index af2a51a4d..475ef99b5 100644 --- a/crates/ide_db/src/helpers/merge_imports.rs +++ b/crates/ide_db/src/helpers/merge_imports.rs @@ -2,8 +2,9 @@ use std::cmp::Ordering; use itertools::{EitherOrBoth, Itertools}; -use syntax::ast::{ - self, edit::AstNodeEdit, make, AstNode, AttrsOwner, PathSegmentKind, VisibilityOwner, +use syntax::{ + ast::{self, make, AstNode, AttrsOwner, PathSegmentKind, VisibilityOwner}, + ted, }; /// What type of merges are allowed. @@ -65,7 +66,7 @@ pub fn try_merge_trees( } else { (lhs.split_prefix(&lhs_prefix), rhs.split_prefix(&rhs_prefix)) }; - recursive_merge(&lhs, &rhs, merge) + recursive_merge(&lhs, &rhs, merge).map(|it| it.clone_for_update()) } /// Recursively "zips" together lhs and rhs. @@ -78,7 +79,8 @@ fn recursive_merge( .use_tree_list() .into_iter() .flat_map(|list| list.use_trees()) - // we use Option here to early return from this function(this is not the same as a `filter` op) + // We use Option here to early return from this function(this is not the + // same as a `filter` op). .map(|tree| match merge.is_tree_allowed(&tree) { true => Some(tree), false => None, @@ -111,8 +113,10 @@ fn recursive_merge( let tree_is_self = |tree: ast::UseTree| { tree.path().as_ref().map(path_is_self).unwrap_or(false) }; - // check if only one of the two trees has a tree list, and whether that then contains `self` or not. - // If this is the case we can skip this iteration since the path without the list is already included in the other one via `self` + // Check if only one of the two trees has a tree list, and + // whether that then contains `self` or not. If this is the + // case we can skip this iteration since the path without + // the list is already included in the other one via `self`. let tree_contains_self = |tree: &ast::UseTree| { tree.use_tree_list() .map(|tree_list| tree_list.use_trees().any(tree_is_self)) @@ -127,9 +131,11 @@ fn recursive_merge( _ => (), } - // glob imports arent part of the use-tree lists so we need to special handle them here as well - // this special handling is only required for when we merge a module import into a glob import of said module - // see the `merge_self_glob` or `merge_mod_into_glob` tests + // Glob imports aren't part of the use-tree lists so we need + // to special handle them here as well this special handling + // is only required for when we merge a module import into a + // glob import of said module see the `merge_self_glob` or + // `merge_mod_into_glob` tests. if lhs_t.star_token().is_some() || rhs_t.star_token().is_some() { *lhs_t = make::use_tree( make::path_unqualified(make::path_segment_self()), @@ -165,11 +171,11 @@ fn recursive_merge( } } - Some(if let Some(old) = lhs.use_tree_list() { - lhs.replace_descendant(old, make::use_tree_list(use_trees)).clone_for_update() - } else { - lhs.clone() - }) + let lhs = lhs.clone_subtree().clone_for_update(); + if let Some(old) = lhs.use_tree_list() { + ted::replace(old.syntax(), make::use_tree_list(use_trees).syntax().clone_for_update()); + } + ast::UseTree::cast(lhs.syntax().clone_subtree()) } /// Traverses both paths until they differ, returning the common prefix of both. diff --git a/crates/syntax/Cargo.toml b/crates/syntax/Cargo.toml index c0bc59918..747f0b9eb 100644 --- a/crates/syntax/Cargo.toml +++ b/crates/syntax/Cargo.toml @@ -13,7 +13,7 @@ doctest = false [dependencies] cov-mark = { version = "1.1", features = ["thread-local"] } itertools = "0.10.0" -rowan = "=0.13.0-pre.5" +rowan = "=0.13.0-pre.6" rustc_lexer = { version = "716.0.0", package = "rustc-ap-rustc_lexer" } rustc-hash = "1.1.0" arrayvec = "0.7" diff --git a/crates/syntax/src/algo.rs b/crates/syntax/src/algo.rs index 3f9b84ab9..825ea3185 100644 --- a/crates/syntax/src/algo.rs +++ b/crates/syntax/src/algo.rs @@ -337,7 +337,7 @@ enum InsertPos { } #[derive(Default)] -pub struct SyntaxRewriter<'a> { +pub(crate) struct SyntaxRewriter<'a> { //FIXME: add debug_assertions that all elements are in fact from the same file. replacements: FxHashMap, insertions: IndexMap>, @@ -354,13 +354,13 @@ impl fmt::Debug for SyntaxRewriter<'_> { } impl SyntaxRewriter<'_> { - pub fn replace>(&mut self, what: &T, with: &T) { + pub(crate) fn replace>(&mut self, what: &T, with: &T) { let what = what.clone().into(); let replacement = Replacement::Single(with.clone().into()); self.replacements.insert(what, replacement); } - pub fn rewrite(&self, node: &SyntaxNode) -> SyntaxNode { + pub(crate) fn rewrite(&self, node: &SyntaxNode) -> SyntaxNode { let _p = profile::span("rewrite"); if self.replacements.is_empty() && self.insertions.is_empty() { @@ -370,37 +370,10 @@ impl SyntaxRewriter<'_> { with_green(node, green) } - pub fn rewrite_ast(self, node: &N) -> N { + pub(crate) fn rewrite_ast(self, node: &N) -> N { N::cast(self.rewrite(node.syntax())).unwrap() } - /// Returns a node that encompasses all replacements to be done by this rewriter. - /// - /// Passing the returned node to `rewrite` will apply all replacements queued up in `self`. - /// - /// Returns `None` when there are no replacements. - pub fn rewrite_root(&self) -> Option { - let _p = profile::span("rewrite_root"); - fn element_to_node_or_parent(element: &SyntaxElement) -> Option { - match element { - SyntaxElement::Node(it) => Some(it.clone()), - SyntaxElement::Token(it) => it.parent(), - } - } - - self.replacements - .keys() - .filter_map(element_to_node_or_parent) - .chain(self.insertions.keys().filter_map(|pos| match pos { - InsertPos::FirstChildOf(it) => Some(it.clone()), - InsertPos::After(it) => element_to_node_or_parent(it), - })) - // If we only have one replacement/insertion, we must return its parent node, since `rewrite` does - // not replace the node passed to it. - .map(|it| it.parent().unwrap_or(it)) - .fold1(|a, b| least_common_ancestor(&a, &b).unwrap()) - } - fn replacement(&self, element: &SyntaxElement) -> Option { self.replacements.get(element).cloned() } diff --git a/crates/syntax/src/ast.rs b/crates/syntax/src/ast.rs index 7f472d4db..a8071b51d 100644 --- a/crates/syntax/src/ast.rs +++ b/crates/syntax/src/ast.rs @@ -47,6 +47,12 @@ pub trait AstNode { { Self::cast(self.syntax().clone_for_update()).unwrap() } + fn clone_subtree(&self) -> Self + where + Self: Sized, + { + Self::cast(self.syntax().clone_subtree()).unwrap() + } } /// Like `AstNode`, but wraps tokens rather than interior nodes. diff --git a/crates/syntax/src/ast/make.rs b/crates/syntax/src/ast/make.rs index 1998ad1f6..de04c8620 100644 --- a/crates/syntax/src/ast/make.rs +++ b/crates/syntax/src/ast/make.rs @@ -12,7 +12,7 @@ use itertools::Itertools; use stdx::{format_to, never}; -use crate::{ast, AstNode, SourceFile, SyntaxKind, SyntaxNode, SyntaxToken}; +use crate::{ast, AstNode, SourceFile, SyntaxKind, SyntaxToken}; /// While the parent module defines basic atomic "constructors", the `ext` /// module defines shortcuts for common things. @@ -601,17 +601,11 @@ fn ast_from_text(text: &str) -> N { panic!("Failed to make ast node `{}` from text {}", std::any::type_name::(), text) } }; - let node = node.syntax().clone(); - let node = unroot(node); - let node = N::cast(node).unwrap(); + let node = node.clone_subtree(); assert_eq!(node.syntax().text_range().start(), 0.into()); node } -fn unroot(n: SyntaxNode) -> SyntaxNode { - SyntaxNode::new_root(n.green().into()) -} - pub fn token(kind: SyntaxKind) -> SyntaxToken { tokens::SOURCE_FILE .tree() diff --git a/docs/dev/style.md b/docs/dev/style.md index 9b0ddec66..f22b69768 100644 --- a/docs/dev/style.md +++ b/docs/dev/style.md @@ -951,6 +951,28 @@ match p.current() { ## Documentation +Style inline code comments as proper sentences. +Start with a capital letter, end with a dot. + +```rust +// GOOD + +// Only simple single segment paths are allowed. +MergeBehavior::Last => { + tree.use_tree_list().is_none() && tree.path().map(path_len) <= Some(1) +} + +// BAD + +// only simple single segment paths are allowed +MergeBehavior::Last => { + tree.use_tree_list().is_none() && tree.path().map(path_len) <= Some(1) +} +``` + +**Rationale:** writing a sentence (or maybe even a paragraph) rather just "a comment" creates a more appropriate frame of mind. +It tricks you into writing down more of the context you keep in your head while coding. + For `.md` and `.adoc` files, prefer a sentence-per-line format, don't wrap lines. If the line is too long, you want to split the sentence in two :-) -- cgit v1.2.3