squiid/rust
1
0
Fork 0
Code Issues Pull requests Projects Activity

Merge #5935

Browse source 
5935: Rewrite import insertion r=matklad a=Veykril

This is my attempt at refactoring the import insertion #3947. I hope what I created here is somewhat in line with what was requested, it wouldn't surprise me .

`common_prefix` is a copy from `merge_imports.rs` so those should be unified somewhere, `try_merge_trees` is also copied from there but slighly modified to take the `MergeBehaviour` enum into account.
`MergeBehaviour` should in the end become a configuration option, and the order if `ImportGroup` probably as well?

I'm not too familiar with the assist stuff and the like which is why I dont know what i have to do with `insert_use_statement` and `find_insert_use_container` for now.

I will most likely add more test cases in the end as well as I currently only tried to hit every path in `find_insert_position`. 
Some of the merge tests also fail atm due to them not sorting what they insert. There is also this test case I'm not sure if we want to support it. I would assume we want to? https://github.com/rust-analyzer/rust-analyzer/pull/5935/files#diff-6923916dd8bdd2f1ab4b984adacd265fR540-R547

The entire module was rewritten so looking at the the file itself is probably better than looking at the diff.

Regarding the sub issues of #3947:
- #3301: This is fixed with the rewrite, what this implementation does is that it scans through the first occurence of groupings and picks the appropriate one out. This means the user can actually rearrange the groupings on a per file basis to their liking. If a group isnt being found it is inserted according to the `ImportGroup` variant order(Would be nice if this was configurable I imagine).
- #3831: This should be fixed with the introduced `MergeBehaviour` enum and it's `Last` variant.
- #3946: This should also be [fixed](https://github.com/rust-analyzer/rust-analyzer/pull/5935/files#diff-6923916dd8bdd2f1ab4b984adacd265fR87)
- #5795: This is fixed in the sense that the grouping search picks the first group that is of the same kind as the import that is being added. So if  there is a random import in the middle of the program it should only be considered if there is no group of the same kind in the file already present.
- the last point in the list I havent checked yet, tho I got the feeling that it's not gonna be too simple as that will require knowledge of whether in this example `ast` is a crate or the module that is already imported.

Co-authored-by: Lukas Wirth <lukastw97@gmail.com>
This commit is contained in:
bors[bot] 2020-09-04 13:28:05 +00:00 committed by GitHub
commit 9dfa69a44a
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
6 changed files with 772 additions and 563 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

20
crates/assists/src/handlers/auto_import.rs
View file

@ -1,11 +1,13 @@
use std::collections::BTreeSet;
use ast::make;
use either::Either;
use hir::{
AsAssocItem, AssocItemContainer, ModPath, Module, ModuleDef, PathResolution, Semantics, Trait,
Type,
};
use ide_db::{imports_locator, RootDatabase};
use insert_use::ImportScope;
use rustc_hash::FxHashSet;
use syntax::{
ast::{self, AstNode},
@ -13,7 +15,8 @@ use syntax::{
};
use crate::{
utils::insert_use_statement, AssistContext, AssistId, AssistKind, Assists, GroupLabel,
utils::{insert_use, MergeBehaviour},
AssistContext, AssistId, AssistKind, Assists, GroupLabel,
};
// Assist: auto_import
@ -44,6 +47,9 @@ pub(crate) fn auto_import(acc: &mut Assists, ctx: &AssistContext) -> Option<()>
let range = ctx.sema.original_range(&auto_import_assets.syntax_under_caret).range;
let group = auto_import_assets.get_import_group_message();
let scope =
ImportScope::find_insert_use_container(&auto_import_assets.syntax_under_caret, ctx)?;
let syntax = scope.as_syntax_node();
for import in proposed_imports {
acc.add_group(
&group,
@ -51,12 +57,12 @@ pub(crate) fn auto_import(acc: &mut Assists, ctx: &AssistContext) -> Option<()>
format!("Import `{}`", &import),
range,
|builder| {
insert_use_statement(
&auto_import_assets.syntax_under_caret,
&import.to_string(),
ctx,
builder.text_edit_builder(),
let new_syntax = insert_use(
&scope,
make::path_from_text(&import.to_string()),
Some(MergeBehaviour::Full),
);
builder.replace(syntax.text_range(), new_syntax.to_string())
},
);
}
@ -358,7 +364,7 @@ mod tests {
}
",
r"
use PubMod::{PubStruct2, PubStruct1};
use PubMod::{PubStruct1, PubStruct2};
struct Test {
test: PubStruct2<u8>,

33
crates/assists/src/handlers/extract_struct_from_enum_variant.rs
View file

@ -10,9 +10,12 @@ use syntax::{
};
use crate::{
assist_context::AssistBuilder, utils::insert_use_statement, AssistContext, AssistId,
AssistKind, Assists,
assist_context::AssistBuilder,
utils::{insert_use, MergeBehaviour},
AssistContext, AssistId, AssistKind, Assists,
};
use ast::make;
use insert_use::ImportScope;
// Assist: extract_struct_from_enum_variant
//
@ -94,6 +97,7 @@ fn existing_struct_def(db: &RootDatabase, variant_name: &str, variant: &EnumVari
.any(|(name, _)| name.to_string() == variant_name.to_string())
}
#[allow(dead_code)]
fn insert_import(
ctx: &AssistContext,
builder: &mut AssistBuilder,
@ -107,12 +111,16 @@ fn insert_import(
if let Some(mut mod_path) = mod_path {
mod_path.segments.pop();
mod_path.segments.push(variant_hir_name.clone());
insert_use_statement(
path.syntax(),
&mod_path.to_string(),
ctx,
builder.text_edit_builder(),
let scope = ImportScope::find_insert_use_container(path.syntax(), ctx)?;
let syntax = scope.as_syntax_node();
let new_syntax = insert_use(
&scope,
make::path_from_text(&mod_path.to_string()),
Some(MergeBehaviour::Full),
);
// FIXME: this will currently panic as multiple imports will have overlapping text ranges
builder.replace(syntax.text_range(), new_syntax.to_string())
}
Some(())
}
@ -167,9 +175,9 @@ fn update_reference(
builder: &mut AssistBuilder,
reference: Reference,
source_file: &SourceFile,
enum_module_def: &ModuleDef,
variant_hir_name: &Name,
visited_modules_set: &mut FxHashSet<Module>,
_enum_module_def: &ModuleDef,
_variant_hir_name: &Name,
_visited_modules_set: &mut FxHashSet<Module>,
) -> Option<()> {
let path_expr: ast::PathExpr = find_node_at_offset::<ast::PathExpr>(
source_file.syntax(),
@ -178,13 +186,14 @@ fn update_reference(
let call = path_expr.syntax().parent().and_then(ast::CallExpr::cast)?;
let list = call.arg_list()?;
let segment = path_expr.path()?.segment()?;
let module = ctx.sema.scope(&path_expr.syntax()).module()?;
let _module = ctx.sema.scope(&path_expr.syntax()).module()?;
let list_range = list.syntax().text_range();
let inside_list_range = TextRange::new(
list_range.start().checked_add(TextSize::from(1))?,
list_range.end().checked_sub(TextSize::from(1))?,
);
builder.edit_file(reference.file_range.file_id);
/* FIXME: this most likely requires AST-based editing, see `insert_import`
if !visited_modules_set.contains(&module) {
if insert_import(ctx, builder, &path_expr, &module, enum_module_def, variant_hir_name)
.is_some()
@ -192,6 +201,7 @@ fn update_reference(
visited_modules_set.insert(module);
}
}
*/
builder.replace(inside_list_range, format!("{}{}", segment, list));
Some(())
}
@ -250,6 +260,7 @@ pub enum A { One(One) }"#,
}
#[test]
#[ignore] // FIXME: this currently panics if `insert_import` is used
fn test_extract_struct_with_complex_imports() {
check_assist(
extract_struct_from_enum_variant,

68
crates/assists/src/handlers/replace_qualified_name_with_use.rs
View file

@ -2,9 +2,10 @@ use syntax::{algo::SyntaxRewriter, ast, match_ast, AstNode, SyntaxNode, TextRang
use test_utils::mark;
use crate::{
utils::{find_insert_use_container, insert_use_statement},
utils::{insert_use, ImportScope, MergeBehaviour},
AssistContext, AssistId, AssistKind, Assists,
};
use ast::make;
// Assist: replace_qualified_name_with_use
//
@ -32,7 +33,7 @@ pub(crate) fn replace_qualified_name_with_use(
mark::hit!(dont_import_trivial_paths);
return None;
}
let path_to_import = path.to_string().clone();
let path_to_import = path.to_string();
let path_to_import = match path.segment()?.generic_arg_list() {
Some(generic_args) => {
let generic_args_start =
@ -43,28 +44,26 @@ pub(crate) fn replace_qualified_name_with_use(
};
let target = path.syntax().text_range();
let scope = ImportScope::find_insert_use_container(path.syntax(), ctx)?;
let syntax = scope.as_syntax_node();
acc.add(
AssistId("replace_qualified_name_with_use", AssistKind::RefactorRewrite),
"Replace qualified path with use",
target,
|builder| {
let container = match find_insert_use_container(path.syntax(), ctx) {
Some(c) => c,
None => return,
};
insert_use_statement(
path.syntax(),
&path_to_import.to_string(),
ctx,
builder.text_edit_builder(),
);
// Now that we've brought the name into scope, re-qualify all paths that could be
// affected (that is, all paths inside the node we added the `use` to).
let mut rewriter = SyntaxRewriter::default();
let syntax = container.either(|l| l.syntax().clone(), |r| r.syntax().clone());
shorten_paths(&mut rewriter, syntax, path);
builder.rewrite(rewriter);
shorten_paths(&mut rewriter, syntax.clone(), path);
let rewritten_syntax = rewriter.rewrite(&syntax);
if let Some(ref import_scope) = ImportScope::from(rewritten_syntax) {
let new_syntax = insert_use(
import_scope,
make::path_from_text(path_to_import),
Some(MergeBehaviour::Full),
);
builder.replace(syntax.text_range(), new_syntax.to_string())
}
},
)
}
@ -220,9 +219,10 @@ impl std::fmt::Debug<|> for Foo {
}
",
r"
use stdx;
use std::fmt::Debug;
use stdx;
impl Debug for Foo {
}
",
@ -274,7 +274,7 @@ impl std::io<|> for Foo {
}
",
r"
use std::{io, fmt};
use std::{fmt, io};
impl io for Foo {
}
@ -293,7 +293,7 @@ impl std::fmt::Debug<|> for Foo {
}
",
r"
use std::fmt::{self, Debug, };
use std::fmt::{self, Debug};
impl Debug for Foo {
}
@ -312,7 +312,7 @@ impl std::fmt<|> for Foo {
}
",
r"
use std::fmt::{self, Debug};
use std::fmt::{Debug, self};
impl fmt for Foo {
}
@ -330,8 +330,9 @@ use std::fmt::{Debug, nested::{Display}};
impl std::fmt::nested<|> for Foo {
}
",
// FIXME(veykril): should be nested::{self, Display} here
r"
use std::fmt::{Debug, nested::{Display, self}};
use std::fmt::{Debug, nested::{Display}, nested};
impl nested for Foo {
}
@ -349,8 +350,9 @@ use std::fmt::{Debug, nested::{self, Display}};
impl std::fmt::nested<|> for Foo {
}
",
// FIXME(veykril): nested is duplicated now
r"
use std::fmt::{Debug, nested::{self, Display}};
use std::fmt::{Debug, nested::{self, Display}, nested};
impl nested for Foo {
}
@ -369,7 +371,7 @@ impl std::fmt::nested::Debug<|> for Foo {
}
",
r"
use std::fmt::{Debug, nested::{Display, Debug}};
use std::fmt::{Debug, nested::{Display}, nested::Debug};
impl Debug for Foo {
}
@ -388,7 +390,7 @@ impl std::fmt::nested::Display<|> for Foo {
}
",
r"
use std::fmt::{nested::Display, Debug};
use std::fmt::{Debug, nested::Display};
impl Display for Foo {
}
@ -407,7 +409,7 @@ impl std::fmt::Display<|> for Foo {
}
",
r"
use std::fmt::{Display, nested::Debug};
use std::fmt::{nested::Debug, Display};
impl Display for Foo {
}
@ -427,11 +429,12 @@ use crate::{
fn foo() { crate::ty::lower<|>::trait_env() }
",
// FIXME(veykril): formatting broke here
r"
use crate::{
ty::{Substs, Ty, lower},
ty::{Substs, Ty},
AssocItem,
};
ty::lower};
fn foo() { lower::trait_env() }
",
@ -451,6 +454,8 @@ impl foo::Debug<|> for Foo {
r"
use std::fmt as foo;
use foo::Debug;
impl Debug for Foo {
}
",
@ -515,6 +520,7 @@ fn main() {
",
r"
#![allow(dead_code)]
use std::fmt::Debug;
fn main() {
@ -627,7 +633,7 @@ fn main() {
}
",
r"
use std::fmt::{self, Display};
use std::fmt::{Display, self};
fn main() {
fmt;
@ -647,9 +653,8 @@ impl std::io<|> for Foo {
}
",
r"
use std::io;
pub use std::fmt;
use std::io;
impl io for Foo {
}
@ -668,9 +673,8 @@ impl std::io<|> for Foo {
}
",
r"
use std::io;
pub(crate) use std::fmt;
use std::io;
impl io for Foo {
}

2
crates/assists/src/utils.rs
View file

@ -16,7 +16,7 @@ use syntax::{
use crate::assist_config::SnippetCap;
pub(crate) use insert_use::{find_insert_use_container, insert_use_statement};
pub(crate) use insert_use::{insert_use, ImportScope, MergeBehaviour};
pub(crate) fn unwrap_trivial_block(block: ast::BlockExpr) -> ast::Expr {
extract_trivial_expression(&block)

1200
crates/assists/src/utils/insert_use.rs
View file

File diff suppressed because it is too large Load diff

12
crates/syntax/src/ast/make.rs
View file

@ -339,7 +339,7 @@ pub mod tokens {
use crate::{ast, AstNode, Parse, SourceFile, SyntaxKind::*, SyntaxToken};
pub(super) static SOURCE_FILE: Lazy<Parse<SourceFile>> =
Lazy::new(|| SourceFile::parse("const C: <()>::Item = (1 != 1, 2 == 2, !true)\n;"));
Lazy::new(|| SourceFile::parse("const C: <()>::Item = (1 != 1, 2 == 2, !true)\n;\n\n"));
pub fn single_space() -> SyntaxToken {
SOURCE_FILE
@ -379,6 +379,16 @@ pub mod tokens {
.unwrap()
}
pub fn blank_line() -> SyntaxToken {
SOURCE_FILE
.tree()
.syntax()
.descendants_with_tokens()
.filter_map(|it| it.into_token())
.find(|it| it.kind() == WHITESPACE && it.text().as_str() == "\n\n")
.unwrap()
}
pub struct WsBuilder(SourceFile);
impl WsBuilder {