aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorbors[bot] <26634292+bors[bot]@users.noreply.github.com>2020-09-30 23:06:16 +0100
committerGitHub <[email protected]>2020-09-30 23:06:16 +0100
commit43253c508d990cf9d65a1c68cf5dbec8206db164 (patch)
tree5355f982aa2c2386672e21824d38227d74d4dfb4
parent7c9ae771bca39d511a0ea7395da2b4b91b44ee12 (diff)
parent0671bf2d734c6d579896a8f976a2c65e5a7f405e (diff)
Merge #6102
6102: Fix MergingBehaviour::Last creating unintuitive import trees r=jonas-schievink a=Veykril The way this behaviour currently works is actually a bit weird. Imagine the following three imports get requested for insertion in the given order: - `winapi::um::d3d11::ID3D11Device` - `winapi::shared::dxgiformat::DXGI_FORMAT` - `winapi::um::d3d11::D3D11_FILTER` After the first two you will have the following tree: ```rust use winapi::{shared::dxgiformat::DXGI_FORMAT, um::d3d11::ID3D11Device}; ``` which is to be expected as they arent nested this kind of merging is allowed, but now importing the third one will result in: ```rust use winapi::{shared::dxgiformat::DXGI_FORMAT, um::d3d11::ID3D11Device, um::d3d11::D3D11_FILTER}; ``` which is still fine according to the rules, but it looks weird(at least in my eyes) due to the long paths that are quite similar. The changes in this PR will change the criteria for when to reject `Last` merging, it still disallows multiple nesting but it also only allows single segment paths inside of the `UseTreeList`. With this change you get the following tree after the first two imports: ```rust use winapi::um::d3d11::ID3D11Device; use winapi::shared::dxgiformat::DXGI_FORMAT; ``` and after the third: ```rust use winapi::shared::dxgiformat::DXGI_FORMAT; use winapi::um::d3d11::{ID3D11Device, D3D11_FILTER}; ``` Which I believe looks more like what you would expect. Co-authored-by: Lukas Wirth <[email protected]>
-rw-r--r--crates/assists/src/utils/insert_use.rs73
1 files changed, 52 insertions, 21 deletions
diff --git a/crates/assists/src/utils/insert_use.rs b/crates/assists/src/utils/insert_use.rs
index 5719b06af..8dd4fe607 100644
--- a/crates/assists/src/utils/insert_use.rs
+++ b/crates/assists/src/utils/insert_use.rs
@@ -174,7 +174,7 @@ pub(crate) fn try_merge_trees(
174 let (lhs_prefix, rhs_prefix) = common_prefix(&lhs_path, &rhs_path)?; 174 let (lhs_prefix, rhs_prefix) = common_prefix(&lhs_path, &rhs_path)?;
175 let lhs = lhs.split_prefix(&lhs_prefix); 175 let lhs = lhs.split_prefix(&lhs_prefix);
176 let rhs = rhs.split_prefix(&rhs_prefix); 176 let rhs = rhs.split_prefix(&rhs_prefix);
177 recursive_merge(&lhs, &rhs, merge).map(|(merged, _)| merged) 177 recursive_merge(&lhs, &rhs, merge)
178} 178}
179 179
180/// Recursively "zips" together lhs and rhs. 180/// Recursively "zips" together lhs and rhs.
@@ -182,20 +182,22 @@ fn recursive_merge(
182 lhs: &ast::UseTree, 182 lhs: &ast::UseTree,
183 rhs: &ast::UseTree, 183 rhs: &ast::UseTree,
184 merge: MergeBehaviour, 184 merge: MergeBehaviour,
185) -> Option<(ast::UseTree, bool)> { 185) -> Option<ast::UseTree> {
186 let mut use_trees = lhs 186 let mut use_trees = lhs
187 .use_tree_list() 187 .use_tree_list()
188 .into_iter() 188 .into_iter()
189 .flat_map(|list| list.use_trees()) 189 .flat_map(|list| list.use_trees())
190 // check if any of the use trees are nested, if they are and the behaviour is `last` we are not allowed to merge this 190 // we use Option here to early return from this function(this is not the same as a `filter` op)
191 // so early exit the iterator by using Option's Intoiterator impl 191 .map(|tree| match merge.is_tree_allowed(&tree) {
192 .map(|tree| match merge == MergeBehaviour::Last && tree.use_tree_list().is_some() { 192 true => Some(tree),
193 true => None, 193 false => None,
194 false => Some(tree),
195 }) 194 })
196 .collect::<Option<Vec<_>>>()?; 195 .collect::<Option<Vec<_>>>()?;
197 use_trees.sort_unstable_by(|a, b| path_cmp_opt(a.path(), b.path())); 196 use_trees.sort_unstable_by(|a, b| path_cmp_opt(a.path(), b.path()));
198 for rhs_t in rhs.use_tree_list().into_iter().flat_map(|list| list.use_trees()) { 197 for rhs_t in rhs.use_tree_list().into_iter().flat_map(|list| list.use_trees()) {
198 if !merge.is_tree_allowed(&rhs_t) {
199 return None;
200 }
199 let rhs_path = rhs_t.path(); 201 let rhs_path = rhs_t.path();
200 match use_trees.binary_search_by(|p| path_cmp_opt(p.path(), rhs_path.clone())) { 202 match use_trees.binary_search_by(|p| path_cmp_opt(p.path(), rhs_path.clone())) {
201 Ok(idx) => { 203 Ok(idx) => {
@@ -239,17 +241,9 @@ fn recursive_merge(
239 } 241 }
240 let lhs = lhs_t.split_prefix(&lhs_prefix); 242 let lhs = lhs_t.split_prefix(&lhs_prefix);
241 let rhs = rhs_t.split_prefix(&rhs_prefix); 243 let rhs = rhs_t.split_prefix(&rhs_prefix);
242 let this_has_children = use_trees.len() > 0;
243 match recursive_merge(&lhs, &rhs, merge) { 244 match recursive_merge(&lhs, &rhs, merge) {
244 Some((_, has_multiple_children)) 245 Some(use_tree) => use_trees[idx] = use_tree,
245 if merge == MergeBehaviour::Last 246 None => return None,
246 && this_has_children
247 && has_multiple_children =>
248 {
249 return None
250 }
251 Some((use_tree, _)) => use_trees[idx] = use_tree,
252 None => use_trees.insert(idx, rhs_t),
253 } 247 }
254 } 248 }
255 Err(_) 249 Err(_)
@@ -264,8 +258,7 @@ fn recursive_merge(
264 } 258 }
265 } 259 }
266 } 260 }
267 let has_multiple_children = use_trees.len() > 1; 261 Some(lhs.with_use_tree_list(make::use_tree_list(use_trees)))
268 Some((lhs.with_use_tree_list(make::use_tree_list(use_trees)), has_multiple_children))
269} 262}
270 263
271/// Traverses both paths until they differ, returning the common prefix of both. 264/// Traverses both paths until they differ, returning the common prefix of both.
@@ -308,6 +301,10 @@ fn segment_iter(path: &ast::Path) -> impl Iterator<Item = ast::PathSegment> + Cl
308 successors(first_segment(path), |p| p.parent_path().parent_path().and_then(|p| p.segment())) 301 successors(first_segment(path), |p| p.parent_path().parent_path().and_then(|p| p.segment()))
309} 302}
310 303
304fn path_len(path: ast::Path) -> usize {
305 segment_iter(&path).count()
306}
307
311/// Orders paths in the following way: 308/// Orders paths in the following way:
312/// the sole self token comes first, after that come uppercase identifiers, then lowercase identifiers 309/// the sole self token comes first, after that come uppercase identifiers, then lowercase identifiers
313// FIXME: rustfmt sort lowercase idents before uppercase, in general we want to have the same ordering rustfmt has 310// FIXME: rustfmt sort lowercase idents before uppercase, in general we want to have the same ordering rustfmt has
@@ -352,6 +349,19 @@ pub enum MergeBehaviour {
352 Last, 349 Last,
353} 350}
354 351
352impl MergeBehaviour {
353 #[inline]
354 fn is_tree_allowed(&self, tree: &ast::UseTree) -> bool {
355 match self {
356 MergeBehaviour::Full => true,
357 // only simple single segment paths are allowed
358 MergeBehaviour::Last => {
359 tree.use_tree_list().is_none() && tree.path().map(path_len) <= Some(1)
360 }
361 }
362 }
363}
364
355#[derive(Eq, PartialEq, PartialOrd, Ord)] 365#[derive(Eq, PartialEq, PartialOrd, Ord)]
356enum ImportGroup { 366enum ImportGroup {
357 // the order here defines the order of new group inserts 367 // the order here defines the order of new group inserts
@@ -676,6 +686,11 @@ use std::io;",
676 } 686 }
677 687
678 #[test] 688 #[test]
689 fn merge_last_into_self() {
690 check_last("foo::bar::baz", r"use foo::bar;", r"use foo::bar::{self, baz};");
691 }
692
693 #[test]
679 fn merge_groups_full() { 694 fn merge_groups_full() {
680 check_full( 695 check_full(
681 "std::io", 696 "std::io",
@@ -819,8 +834,24 @@ use std::io;",
819 } 834 }
820 835
821 #[test] 836 #[test]
822 fn merge_last_too_long() { 837 fn skip_merge_last_too_long() {
823 check_last("foo::bar", r"use foo::bar::baz::Qux;", r"use foo::bar::{self, baz::Qux};"); 838 check_last(
839 "foo::bar",
840 r"use foo::bar::baz::Qux;",
841 r"use foo::bar;
842use foo::bar::baz::Qux;",
843 );
844 }
845
846 #[test]
847 #[ignore] // FIXME: Order changes when switching lhs and rhs
848 fn skip_merge_last_too_long2() {
849 check_last(
850 "foo::bar::baz::Qux",
851 r"use foo::bar;",
852 r"use foo::bar;
853use foo::bar::baz::Qux;",
854 );
824 } 855 }
825 856
826 #[test] 857 #[test]