aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorbors[bot] <26634292+bors[bot]@users.noreply.github.com>2020-09-11 15:01:20 +0100
committerGitHub <[email protected]>2020-09-11 15:01:20 +0100
commit568dc38b7b6474e225533ae63718c24b0561f48d (patch)
tree8cacf0d45fa3d33e48a1f8126d54f29eadb168ee
parent96e988fcc3be11acc89dc2c1957bc14e8f39c911 (diff)
parent74b755d23366bcfa5437df25b023f5a2451e1ea6 (diff)
Merge #5955
5955: Remove merge import code duplication r=jonas-schievink a=Veykril This removes the code duplication caused by #5935, this also allows the assist to merge imports that have equal visibility and prevents merges of unequal visibility. This PR also fixes an iteration mistake in the mentioned PR: Turns out I made a mistake when writing the `segment_iter` function, I was assuming that the `children` of a path will just be the segments, which is obviously not the case. This also brings insertion order of shorter paths in line with how `rustfmt` orders them. Co-authored-by: Lukas Wirth <[email protected]>
-rw-r--r--crates/assists/src/handlers/merge_imports.rs161
-rw-r--r--crates/assists/src/utils/insert_use.rs35
2 files changed, 114 insertions, 82 deletions
diff --git a/crates/assists/src/handlers/merge_imports.rs b/crates/assists/src/handlers/merge_imports.rs
index 35b884206..0bd679260 100644
--- a/crates/assists/src/handlers/merge_imports.rs
+++ b/crates/assists/src/handlers/merge_imports.rs
@@ -1,14 +1,14 @@
1use std::iter::successors;
2
3use syntax::{ 1use syntax::{
4 algo::{neighbor, skip_trivia_token, SyntaxRewriter}, 2 algo::{neighbor, SyntaxRewriter},
5 ast::{self, edit::AstNodeEdit, make}, 3 ast, AstNode,
6 AstNode, Direction, InsertPosition, SyntaxElement, T,
7}; 4};
8 5
9use crate::{ 6use crate::{
10 assist_context::{AssistContext, Assists}, 7 assist_context::{AssistContext, Assists},
11 utils::next_prev, 8 utils::{
9 insert_use::{try_merge_imports, try_merge_trees},
10 next_prev, MergeBehaviour,
11 },
12 AssistId, AssistKind, 12 AssistId, AssistKind,
13}; 13};
14 14
@@ -30,23 +30,22 @@ pub(crate) fn merge_imports(acc: &mut Assists, ctx: &AssistContext) -> Option<()
30 let mut offset = ctx.offset(); 30 let mut offset = ctx.offset();
31 31
32 if let Some(use_item) = tree.syntax().parent().and_then(ast::Use::cast) { 32 if let Some(use_item) = tree.syntax().parent().and_then(ast::Use::cast) {
33 let (merged, to_delete) = next_prev() 33 let (merged, to_delete) =
34 .filter_map(|dir| neighbor(&use_item, dir)) 34 next_prev().filter_map(|dir| neighbor(&use_item, dir)).find_map(|use_item2| {
35 .filter_map(|it| Some((it.clone(), it.use_tree()?))) 35 try_merge_imports(&use_item, &use_item2, MergeBehaviour::Full).zip(Some(use_item2))
36 .find_map(|(use_item, use_tree)| {
37 Some((try_merge_trees(&tree, &use_tree)?, use_item))
38 })?; 36 })?;
39 37
40 rewriter.replace_ast(&tree, &merged); 38 rewriter.replace_ast(&use_item, &merged);
41 rewriter += to_delete.remove(); 39 rewriter += to_delete.remove();
42 40
43 if to_delete.syntax().text_range().end() < offset { 41 if to_delete.syntax().text_range().end() < offset {
44 offset -= to_delete.syntax().text_range().len(); 42 offset -= to_delete.syntax().text_range().len();
45 } 43 }
46 } else { 44 } else {
47 let (merged, to_delete) = next_prev() 45 let (merged, to_delete) =
48 .filter_map(|dir| neighbor(&tree, dir)) 46 next_prev().filter_map(|dir| neighbor(&tree, dir)).find_map(|use_tree| {
49 .find_map(|use_tree| Some((try_merge_trees(&tree, &use_tree)?, use_tree.clone())))?; 47 try_merge_trees(&tree, &use_tree, MergeBehaviour::Full).zip(Some(use_tree))
48 })?;
50 49
51 rewriter.replace_ast(&tree, &merged); 50 rewriter.replace_ast(&tree, &merged);
52 rewriter += to_delete.remove(); 51 rewriter += to_delete.remove();
@@ -67,66 +66,6 @@ pub(crate) fn merge_imports(acc: &mut Assists, ctx: &AssistContext) -> Option<()
67 ) 66 )
68} 67}
69 68
70fn try_merge_trees(old: &ast::UseTree, new: &ast::UseTree) -> Option<ast::UseTree> {
71 let lhs_path = old.path()?;
72 let rhs_path = new.path()?;
73
74 let (lhs_prefix, rhs_prefix) = common_prefix(&lhs_path, &rhs_path)?;
75
76 let lhs = old.split_prefix(&lhs_prefix);
77 let rhs = new.split_prefix(&rhs_prefix);
78
79 let should_insert_comma = lhs
80 .use_tree_list()?
81 .r_curly_token()
82 .and_then(|it| skip_trivia_token(it.prev_token()?, Direction::Prev))
83 .map(|it| it.kind() != T![,])
84 .unwrap_or(true);
85
86 let mut to_insert: Vec<SyntaxElement> = Vec::new();
87 if should_insert_comma {
88 to_insert.push(make::token(T![,]).into());
89 to_insert.push(make::tokens::single_space().into());
90 }
91 to_insert.extend(
92 rhs.use_tree_list()?
93 .syntax()
94 .children_with_tokens()
95 .filter(|it| it.kind() != T!['{'] && it.kind() != T!['}']),
96 );
97 let use_tree_list = lhs.use_tree_list()?;
98 let pos = InsertPosition::Before(use_tree_list.r_curly_token()?.into());
99 let use_tree_list = use_tree_list.insert_children(pos, to_insert);
100 Some(lhs.with_use_tree_list(use_tree_list))
101}
102
103fn common_prefix(lhs: &ast::Path, rhs: &ast::Path) -> Option<(ast::Path, ast::Path)> {
104 let mut res = None;
105 let mut lhs_curr = first_path(&lhs);
106 let mut rhs_curr = first_path(&rhs);
107 loop {
108 match (lhs_curr.segment(), rhs_curr.segment()) {
109 (Some(lhs), Some(rhs)) if lhs.syntax().text() == rhs.syntax().text() => (),
110 _ => break,
111 }
112 res = Some((lhs_curr.clone(), rhs_curr.clone()));
113
114 match (lhs_curr.parent_path(), rhs_curr.parent_path()) {
115 (Some(lhs), Some(rhs)) => {
116 lhs_curr = lhs;
117 rhs_curr = rhs;
118 }
119 _ => break,
120 }
121 }
122
123 res
124}
125
126fn first_path(path: &ast::Path) -> ast::Path {
127 successors(Some(path.clone()), |it| it.qualifier()).last().unwrap()
128}
129
130#[cfg(test)] 69#[cfg(test)]
131mod tests { 70mod tests {
132 use crate::tests::{check_assist, check_assist_not_applicable}; 71 use crate::tests::{check_assist, check_assist_not_applicable};
@@ -189,6 +128,78 @@ use std::{fmt::{Display, self}};
189 } 128 }
190 129
191 #[test] 130 #[test]
131 fn skip_pub1() {
132 check_assist_not_applicable(
133 merge_imports,
134 r"
135pub use std::fmt<|>::Debug;
136use std::fmt::Display;
137",
138 );
139 }
140
141 #[test]
142 fn skip_pub_last() {
143 check_assist_not_applicable(
144 merge_imports,
145 r"
146use std::fmt<|>::Debug;
147pub use std::fmt::Display;
148",
149 );
150 }
151
152 #[test]
153 fn skip_pub_crate_pub() {
154 check_assist_not_applicable(
155 merge_imports,
156 r"
157pub(crate) use std::fmt<|>::Debug;
158pub use std::fmt::Display;
159",
160 );
161 }
162
163 #[test]
164 fn skip_pub_pub_crate() {
165 check_assist_not_applicable(
166 merge_imports,
167 r"
168pub use std::fmt<|>::Debug;
169pub(crate) use std::fmt::Display;
170",
171 );
172 }
173
174 #[test]
175 fn merge_pub() {
176 check_assist(
177 merge_imports,
178 r"
179pub use std::fmt<|>::Debug;
180pub use std::fmt::Display;
181",
182 r"
183pub use std::fmt::{Debug, Display};
184",
185 )
186 }
187
188 #[test]
189 fn merge_pub_crate() {
190 check_assist(
191 merge_imports,
192 r"
193pub(crate) use std::fmt<|>::Debug;
194pub(crate) use std::fmt::Display;
195",
196 r"
197pub(crate) use std::fmt::{Debug, Display};
198",
199 )
200 }
201
202 #[test]
192 fn test_merge_nested() { 203 fn test_merge_nested() {
193 check_assist( 204 check_assist(
194 merge_imports, 205 merge_imports,
diff --git a/crates/assists/src/utils/insert_use.rs b/crates/assists/src/utils/insert_use.rs
index 8a4c8520d..98553b2e0 100644
--- a/crates/assists/src/utils/insert_use.rs
+++ b/crates/assists/src/utils/insert_use.rs
@@ -138,13 +138,23 @@ pub(crate) fn insert_use(
138 algo::insert_children(scope.as_syntax_node(), insert_position, to_insert) 138 algo::insert_children(scope.as_syntax_node(), insert_position, to_insert)
139} 139}
140 140
141fn try_merge_imports( 141fn eq_visibility(vis0: Option<ast::Visibility>, vis1: Option<ast::Visibility>) -> bool {
142 match (vis0, vis1) {
143 (None, None) => true,
144 // FIXME: Don't use the string representation to check for equality
145 // spaces inside of the node would break this comparison
146 (Some(vis0), Some(vis1)) => vis0.to_string() == vis1.to_string(),
147 _ => false,
148 }
149}
150
151pub(crate) fn try_merge_imports(
142 old: &ast::Use, 152 old: &ast::Use,
143 new: &ast::Use, 153 new: &ast::Use,
144 merge_behaviour: MergeBehaviour, 154 merge_behaviour: MergeBehaviour,
145) -> Option<ast::Use> { 155) -> Option<ast::Use> {
146 // don't merge into re-exports 156 // don't merge imports with different visibilities
147 if old.visibility().and_then(|vis| vis.pub_token()).is_some() { 157 if !eq_visibility(old.visibility(), new.visibility()) {
148 return None; 158 return None;
149 } 159 }
150 let old_tree = old.use_tree()?; 160 let old_tree = old.use_tree()?;
@@ -161,7 +171,7 @@ fn use_tree_list_is_nested(tl: &ast::UseTreeList) -> bool {
161} 171}
162 172
163// FIXME: currently this merely prepends the new tree into old, ideally it would insert the items in a sorted fashion 173// FIXME: currently this merely prepends the new tree into old, ideally it would insert the items in a sorted fashion
164pub fn try_merge_trees( 174pub(crate) fn try_merge_trees(
165 old: &ast::UseTree, 175 old: &ast::UseTree,
166 new: &ast::UseTree, 176 new: &ast::UseTree,
167 merge_behaviour: MergeBehaviour, 177 merge_behaviour: MergeBehaviour,
@@ -278,7 +288,8 @@ fn first_path(path: &ast::Path) -> ast::Path {
278} 288}
279 289
280fn segment_iter(path: &ast::Path) -> impl Iterator<Item = ast::PathSegment> + Clone { 290fn segment_iter(path: &ast::Path) -> impl Iterator<Item = ast::PathSegment> + Clone {
281 path.syntax().children().flat_map(ast::PathSegment::cast) 291 // cant make use of SyntaxNode::siblings, because the returned Iterator is not clone
292 successors(first_segment(path), |p| p.parent_path().parent_path().and_then(|p| p.segment()))
282} 293}
283 294
284#[derive(PartialEq, Eq)] 295#[derive(PartialEq, Eq)]
@@ -684,8 +695,18 @@ use std::io;",
684 check_last( 695 check_last(
685 "foo::bar", 696 "foo::bar",
686 r"use foo::bar::baz::Qux;", 697 r"use foo::bar::baz::Qux;",
687 r"use foo::bar::baz::Qux; 698 r"use foo::bar;
688use foo::bar;", 699use foo::bar::baz::Qux;",
700 );
701 }
702
703 #[test]
704 fn insert_short_before_long() {
705 check_none(
706 "foo::bar",
707 r"use foo::bar::baz::Qux;",
708 r"use foo::bar;
709use foo::bar::baz::Qux;",
689 ); 710 );
690 } 711 }
691 712