From fe93675e8ac2b55d051156151489dbe0496efec3 Mon Sep 17 00:00:00 2001 From: Timo Freiberg Date: Sat, 25 Apr 2020 16:57:59 +0200 Subject: New HirDisplay method for displaying sourcecode --- crates/ra_hir/src/code_model.rs | 9 +- crates/ra_hir_ty/src/display.rs | 176 +++++++++++++++++----- crates/ra_hir_ty/src/tests.rs | 17 ++- crates/ra_hir_ty/src/tests/display_source_code.rs | 23 +++ 4 files changed, 184 insertions(+), 41 deletions(-) create mode 100644 crates/ra_hir_ty/src/tests/display_source_code.rs (limited to 'crates') diff --git a/crates/ra_hir/src/code_model.rs b/crates/ra_hir/src/code_model.rs index 5f480c304..be18c845c 100644 --- a/crates/ra_hir/src/code_model.rs +++ b/crates/ra_hir/src/code_model.rs @@ -22,8 +22,11 @@ use hir_expand::{ MacroDefId, MacroDefKind, }; use hir_ty::{ - autoderef, display::HirFormatter, expr::ExprValidator, method_resolution, ApplicationTy, - Canonical, InEnvironment, Substs, TraitEnvironment, Ty, TyDefId, TypeCtor, + autoderef, + display::{HirDisplayError, HirFormatter}, + expr::ExprValidator, + method_resolution, ApplicationTy, Canonical, InEnvironment, Substs, TraitEnvironment, Ty, + TyDefId, TypeCtor, }; use ra_db::{CrateId, CrateName, Edition, FileId}; use ra_prof::profile; @@ -1319,7 +1322,7 @@ impl Type { } impl HirDisplay for Type { - fn hir_fmt(&self, f: &mut HirFormatter) -> std::fmt::Result { + fn hir_fmt(&self, f: &mut HirFormatter) -> Result<(), HirDisplayError> { self.ty.value.hir_fmt(f) } } diff --git a/crates/ra_hir_ty/src/display.rs b/crates/ra_hir_ty/src/display.rs index d03bbd5a7..f5edaea8c 100644 --- a/crates/ra_hir_ty/src/display.rs +++ b/crates/ra_hir_ty/src/display.rs @@ -6,28 +6,42 @@ use crate::{ db::HirDatabase, utils::generics, ApplicationTy, CallableDef, FnSig, GenericPredicate, Obligation, ProjectionTy, Substs, TraitRef, Ty, TypeCtor, }; -use hir_def::{generics::TypeParamProvenance, AdtId, AssocContainerId, Lookup}; +use hir_def::{ + find_path, generics::TypeParamProvenance, item_scope::ItemInNs, AdtId, AssocContainerId, + Lookup, ModuleId, +}; use hir_expand::name::Name; -pub struct HirFormatter<'a, 'b> { +pub struct HirFormatter<'a> { pub db: &'a dyn HirDatabase, - fmt: &'a mut fmt::Formatter<'b>, + fmt: &'a mut dyn fmt::Write, buf: String, curr_size: usize, pub(crate) max_size: Option, omit_verbose_types: bool, + display_target: DisplayTarget, } pub trait HirDisplay { - fn hir_fmt(&self, f: &mut HirFormatter) -> fmt::Result; + fn hir_fmt(&self, f: &mut HirFormatter) -> Result<(), HirDisplayError>; + /// Returns a `Display`able type that is human-readable. + /// Use this for showing types to the user (e.g. diagnostics) fn display<'a>(&'a self, db: &'a dyn HirDatabase) -> HirDisplayWrapper<'a, Self> where Self: Sized, { - HirDisplayWrapper(db, self, None, false) + HirDisplayWrapper { + db, + t: self, + max_size: None, + omit_verbose_types: false, + display_target: DisplayTarget::Diagnostics, + } } + /// Returns a `Display`able type that is human-readable and tries to be succinct. + /// Use this for showing types to the user where space is constrained (e.g. doc popups) fn display_truncated<'a>( &'a self, db: &'a dyn HirDatabase, @@ -36,16 +50,46 @@ pub trait HirDisplay { where Self: Sized, { - HirDisplayWrapper(db, self, max_size, true) + HirDisplayWrapper { + db, + t: self, + max_size, + omit_verbose_types: true, + display_target: DisplayTarget::Diagnostics, + } + } + + /// Returns a String representation of `self` that can be inserted into the given module. + /// Use this when generating code (e.g. assists) + fn display_source_code<'a>( + &'a self, + db: &'a dyn HirDatabase, + module_id: ModuleId, + ) -> Result { + let mut result = String::new(); + match self.hir_fmt(&mut HirFormatter { + db, + fmt: &mut result, + buf: String::with_capacity(20), + curr_size: 0, + max_size: None, + omit_verbose_types: false, + display_target: DisplayTarget::SourceCode { module_id }, + }) { + Ok(()) => {} + Err(HirDisplayError::FmtError) => panic!("Writing to String can't fail!"), + Err(HirDisplayError::DisplaySourceCodeError(e)) => return Err(e), + }; + Ok(result) } } -impl<'a, 'b> HirFormatter<'a, 'b> { +impl<'a> HirFormatter<'a> { pub fn write_joined( &mut self, iter: impl IntoIterator, sep: &str, - ) -> fmt::Result { + ) -> Result<(), HirDisplayError> { let mut first = true; for e in iter { if !first { @@ -58,14 +102,14 @@ impl<'a, 'b> HirFormatter<'a, 'b> { } /// This allows using the `write!` macro directly with a `HirFormatter`. - pub fn write_fmt(&mut self, args: fmt::Arguments) -> fmt::Result { + pub fn write_fmt(&mut self, args: fmt::Arguments) -> Result<(), HirDisplayError> { // We write to a buffer first to track output size self.buf.clear(); fmt::write(&mut self.buf, args)?; self.curr_size += self.buf.len(); // Then we write to the internal formatter from the buffer - self.fmt.write_str(&self.buf) + self.fmt.write_str(&self.buf).map_err(HirDisplayError::from) } pub fn should_truncate(&self) -> bool { @@ -81,34 +125,76 @@ impl<'a, 'b> HirFormatter<'a, 'b> { } } -pub struct HirDisplayWrapper<'a, T>(&'a dyn HirDatabase, &'a T, Option, bool); +#[derive(Clone, Copy)] +enum DisplayTarget { + /// Display types for inlays, doc popups, autocompletion, etc... + /// Showing `{unknown}` or not qualifying paths is fine here. + /// There's no reason for this to fail. + Diagnostics, + /// Display types for inserting them in source files. + /// The generated code should compile, so paths need to be qualified. + SourceCode { module_id: ModuleId }, +} + +#[derive(Debug)] +pub enum DisplaySourceCodeError { + PathNotFound, +} + +pub enum HirDisplayError { + /// Errors that can occur when generating source code + DisplaySourceCodeError(DisplaySourceCodeError), + /// `FmtError` is required to be compatible with std::fmt::Display + FmtError, +} +impl From for HirDisplayError { + fn from(_: fmt::Error) -> Self { + Self::FmtError + } +} + +pub struct HirDisplayWrapper<'a, T> { + db: &'a dyn HirDatabase, + t: &'a T, + max_size: Option, + omit_verbose_types: bool, + display_target: DisplayTarget, +} impl<'a, T> fmt::Display for HirDisplayWrapper<'a, T> where T: HirDisplay, { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - self.1.hir_fmt(&mut HirFormatter { - db: self.0, + match self.t.hir_fmt(&mut HirFormatter { + db: self.db, fmt: f, buf: String::with_capacity(20), curr_size: 0, - max_size: self.2, - omit_verbose_types: self.3, - }) + max_size: self.max_size, + omit_verbose_types: self.omit_verbose_types, + display_target: self.display_target, + }) { + Ok(()) => Ok(()), + Err(HirDisplayError::FmtError) => Err(fmt::Error), + Err(HirDisplayError::DisplaySourceCodeError(_)) => { + // This should never happen + panic!("HirDisplay failed when calling Display::fmt!") + } + } } } const TYPE_HINT_TRUNCATION: &str = "…"; impl HirDisplay for &Ty { - fn hir_fmt(&self, f: &mut HirFormatter) -> fmt::Result { + fn hir_fmt(&self, f: &mut HirFormatter) -> Result<(), HirDisplayError> { HirDisplay::hir_fmt(*self, f) } } impl HirDisplay for ApplicationTy { - fn hir_fmt(&self, f: &mut HirFormatter) -> fmt::Result { + fn hir_fmt(&self, f: &mut HirFormatter) -> Result<(), HirDisplayError> { if f.should_truncate() { return write!(f, "{}", TYPE_HINT_TRUNCATION); } @@ -191,12 +277,30 @@ impl HirDisplay for ApplicationTy { } } TypeCtor::Adt(def_id) => { - let name = match def_id { - AdtId::StructId(it) => f.db.struct_data(it).name.clone(), - AdtId::UnionId(it) => f.db.union_data(it).name.clone(), - AdtId::EnumId(it) => f.db.enum_data(it).name.clone(), - }; - write!(f, "{}", name)?; + match f.display_target { + DisplayTarget::Diagnostics => { + let name = match def_id { + AdtId::StructId(it) => f.db.struct_data(it).name.clone(), + AdtId::UnionId(it) => f.db.union_data(it).name.clone(), + AdtId::EnumId(it) => f.db.enum_data(it).name.clone(), + }; + write!(f, "{}", name)?; + } + DisplayTarget::SourceCode { module_id } => { + if let Some(path) = find_path::find_path( + f.db.upcast(), + ItemInNs::Types(def_id.into()), + module_id, + ) { + write!(f, "{}", path)?; + } else { + return Err(HirDisplayError::DisplaySourceCodeError( + DisplaySourceCodeError::PathNotFound, + )); + } + } + } + if self.parameters.len() > 0 { let mut non_default_parameters = Vec::with_capacity(self.parameters.len()); let parameters_to_write = if f.omit_verbose_types() { @@ -269,7 +373,7 @@ impl HirDisplay for ApplicationTy { } impl HirDisplay for ProjectionTy { - fn hir_fmt(&self, f: &mut HirFormatter) -> fmt::Result { + fn hir_fmt(&self, f: &mut HirFormatter) -> Result<(), HirDisplayError> { if f.should_truncate() { return write!(f, "{}", TYPE_HINT_TRUNCATION); } @@ -287,7 +391,7 @@ impl HirDisplay for ProjectionTy { } impl HirDisplay for Ty { - fn hir_fmt(&self, f: &mut HirFormatter) -> fmt::Result { + fn hir_fmt(&self, f: &mut HirFormatter) -> Result<(), HirDisplayError> { if f.should_truncate() { return write!(f, "{}", TYPE_HINT_TRUNCATION); } @@ -332,7 +436,7 @@ impl HirDisplay for Ty { fn write_bounds_like_dyn_trait( predicates: &[GenericPredicate], f: &mut HirFormatter, -) -> fmt::Result { +) -> Result<(), HirDisplayError> { // Note: This code is written to produce nice results (i.e. // corresponding to surface Rust) for types that can occur in // actual Rust. It will have weird results if the predicates @@ -394,7 +498,7 @@ fn write_bounds_like_dyn_trait( } impl TraitRef { - fn hir_fmt_ext(&self, f: &mut HirFormatter, use_as: bool) -> fmt::Result { + fn hir_fmt_ext(&self, f: &mut HirFormatter, use_as: bool) -> Result<(), HirDisplayError> { if f.should_truncate() { return write!(f, "{}", TYPE_HINT_TRUNCATION); } @@ -416,19 +520,19 @@ impl TraitRef { } impl HirDisplay for TraitRef { - fn hir_fmt(&self, f: &mut HirFormatter) -> fmt::Result { + fn hir_fmt(&self, f: &mut HirFormatter) -> Result<(), HirDisplayError> { self.hir_fmt_ext(f, false) } } impl HirDisplay for &GenericPredicate { - fn hir_fmt(&self, f: &mut HirFormatter) -> fmt::Result { + fn hir_fmt(&self, f: &mut HirFormatter) -> Result<(), HirDisplayError> { HirDisplay::hir_fmt(*self, f) } } impl HirDisplay for GenericPredicate { - fn hir_fmt(&self, f: &mut HirFormatter) -> fmt::Result { + fn hir_fmt(&self, f: &mut HirFormatter) -> Result<(), HirDisplayError> { if f.should_truncate() { return write!(f, "{}", TYPE_HINT_TRUNCATION); } @@ -452,15 +556,15 @@ impl HirDisplay for GenericPredicate { } impl HirDisplay for Obligation { - fn hir_fmt(&self, f: &mut HirFormatter) -> fmt::Result { - match self { - Obligation::Trait(tr) => write!(f, "Implements({})", tr.display(f.db)), + fn hir_fmt(&self, f: &mut HirFormatter) -> Result<(), HirDisplayError> { + Ok(match self { + Obligation::Trait(tr) => write!(f, "Implements({})", tr.display(f.db))?, Obligation::Projection(proj) => write!( f, "Normalize({} => {})", proj.projection_ty.display(f.db), proj.ty.display(f.db) - ), - } + )?, + }) } } diff --git a/crates/ra_hir_ty/src/tests.rs b/crates/ra_hir_ty/src/tests.rs index d60732e19..623d00010 100644 --- a/crates/ra_hir_ty/src/tests.rs +++ b/crates/ra_hir_ty/src/tests.rs @@ -6,6 +6,7 @@ mod patterns; mod traits; mod method_resolution; mod macros; +mod display_source_code; use std::sync::Arc; @@ -16,7 +17,7 @@ use hir_def::{ item_scope::ItemScope, keys, nameres::CrateDefMap, - AssocItemId, DefWithBodyId, LocalModuleId, Lookup, ModuleDefId, + AssocItemId, DefWithBodyId, LocalModuleId, Lookup, ModuleDefId, ModuleId, }; use hir_expand::{db::AstDatabase, InFile}; use insta::assert_snapshot; @@ -37,6 +38,18 @@ use crate::{ // update the snapshots. fn type_at_pos(db: &TestDB, pos: FilePosition) -> String { + type_at_pos_displayed(db, pos, |ty, _| ty.display(db).to_string()) +} + +fn displayed_source_at_pos(db: &TestDB, pos: FilePosition) -> String { + type_at_pos_displayed(db, pos, |ty, module_id| ty.display_source_code(db, module_id).unwrap()) +} + +fn type_at_pos_displayed( + db: &TestDB, + pos: FilePosition, + display_fn: impl FnOnce(&Ty, ModuleId) -> String, +) -> String { let file = db.parse(pos.file_id).ok().unwrap(); let expr = algo::find_node_at_offset::(file.syntax(), pos.offset).unwrap(); let fn_def = expr.syntax().ancestors().find_map(ast::FnDef::cast).unwrap(); @@ -49,7 +62,7 @@ fn type_at_pos(db: &TestDB, pos: FilePosition) -> String { if let Some(expr_id) = source_map.node_expr(InFile::new(pos.file_id.into(), &expr)) { let infer = db.infer(func.into()); let ty = &infer[expr_id]; - return ty.display(db).to_string(); + return display_fn(ty, module); } panic!("Can't find expression") } diff --git a/crates/ra_hir_ty/src/tests/display_source_code.rs b/crates/ra_hir_ty/src/tests/display_source_code.rs new file mode 100644 index 000000000..ca1748615 --- /dev/null +++ b/crates/ra_hir_ty/src/tests/display_source_code.rs @@ -0,0 +1,23 @@ +use super::displayed_source_at_pos; +use crate::test_db::TestDB; +use ra_db::fixture::WithFixture; + +#[test] +fn qualify_path_to_submodule() { + let (db, pos) = TestDB::with_position( + r#" +//- /main.rs + +mod foo { + pub struct Foo; +} + +fn bar() { + let foo: foo::Foo = foo::Foo; + foo<|> +} + +"#, + ); + assert_eq!("foo::Foo", displayed_source_at_pos(&db, pos)); +} -- cgit v1.2.3 From 64e6b8200bfceea92b0dcaab3e533a9152994e78 Mon Sep 17 00:00:00 2001 From: Timo Freiberg Date: Sat, 25 Apr 2020 16:58:28 +0200 Subject: Use new HirDisplay variant in add_function assist --- crates/ra_assists/src/handlers/add_function.rs | 174 ++++++++++++++++--------- 1 file changed, 116 insertions(+), 58 deletions(-) (limited to 'crates') diff --git a/crates/ra_assists/src/handlers/add_function.rs b/crates/ra_assists/src/handlers/add_function.rs index 6b5616aa9..95faf0f4f 100644 --- a/crates/ra_assists/src/handlers/add_function.rs +++ b/crates/ra_assists/src/handlers/add_function.rs @@ -43,16 +43,12 @@ pub(crate) fn add_function(acc: &mut Assists, ctx: &AssistContext) -> Option<()> return None; } - let target_module = if let Some(qualifier) = path.qualifier() { - if let Some(hir::PathResolution::Def(hir::ModuleDef::Module(module))) = - ctx.sema.resolve_path(&qualifier) - { - Some(module.definition_source(ctx.sema.db)) - } else { - return None; - } - } else { - None + let target_module = match path.qualifier() { + Some(qualifier) => match ctx.sema.resolve_path(&qualifier) { + Some(hir::PathResolution::Def(hir::ModuleDef::Module(module))) => Some(module), + _ => return None, + }, + None => None, }; let function_builder = FunctionBuilder::from_call(&ctx, &call, &path, target_module)?; @@ -83,25 +79,29 @@ struct FunctionBuilder { } impl FunctionBuilder { - /// Prepares a generated function that matches `call` in `generate_in` - /// (or as close to `call` as possible, if `generate_in` is `None`) + /// Prepares a generated function that matches `call`. + /// The function is generated in `target_module` or next to `call` fn from_call( ctx: &AssistContext, call: &ast::CallExpr, path: &ast::Path, - target_module: Option>, + target_module: Option, ) -> Option { - let needs_pub = target_module.is_some(); let mut file = ctx.frange.file_id; - let target = if let Some(target_module) = target_module { - let (in_file, target) = next_space_for_fn_in_module(ctx.sema.db, target_module)?; - file = in_file; - target - } else { - next_space_for_fn_after_call_site(&call)? + let target = match &target_module { + Some(target_module) => { + let module_source = target_module.definition_source(ctx.db); + let (in_file, target) = next_space_for_fn_in_module(ctx.sema.db, &module_source)?; + file = in_file; + target + } + None => next_space_for_fn_after_call_site(&call)?, }; + let needs_pub = target_module.is_some(); + let target_module = target_module.or_else(|| ctx.sema.scope(target.syntax()).module())?; let fn_name = fn_name(&path)?; - let (type_params, params) = fn_args(ctx, &call)?; + let (type_params, params) = fn_args(ctx, target_module, &call)?; + Some(Self { target, fn_name, type_params, params, file, needs_pub }) } @@ -144,6 +144,15 @@ enum GeneratedFunctionTarget { InEmptyItemList(ast::ItemList), } +impl GeneratedFunctionTarget { + fn syntax(&self) -> &SyntaxNode { + match self { + GeneratedFunctionTarget::BehindItem(it) => it, + GeneratedFunctionTarget::InEmptyItemList(it) => it.syntax(), + } + } +} + fn fn_name(call: &ast::Path) -> Option { let name = call.segment()?.syntax().to_string(); Some(ast::make::name(&name)) @@ -152,17 +161,17 @@ fn fn_name(call: &ast::Path) -> Option { /// Computes the type variables and arguments required for the generated function fn fn_args( ctx: &AssistContext, + target_module: hir::Module, call: &ast::CallExpr, ) -> Option<(Option, ast::ParamList)> { let mut arg_names = Vec::new(); let mut arg_types = Vec::new(); for arg in call.arg_list()?.args() { - let arg_name = match fn_arg_name(&arg) { + arg_names.push(match fn_arg_name(&arg) { Some(name) => name, None => String::from("arg"), - }; - arg_names.push(arg_name); - arg_types.push(match fn_arg_type(ctx, &arg) { + }); + arg_types.push(match fn_arg_type(ctx, target_module, &arg) { Some(ty) => ty, None => String::from("()"), }); @@ -218,12 +227,21 @@ fn fn_arg_name(fn_arg: &ast::Expr) -> Option { } } -fn fn_arg_type(ctx: &AssistContext, fn_arg: &ast::Expr) -> Option { +fn fn_arg_type( + ctx: &AssistContext, + target_module: hir::Module, + fn_arg: &ast::Expr, +) -> Option { let ty = ctx.sema.type_of_expr(fn_arg)?; if ty.is_unknown() { return None; } - Some(ty.display(ctx.sema.db).to_string()) + + if let Ok(rendered) = ty.display_source_code(ctx.db, target_module.into()) { + Some(rendered) + } else { + None + } } /// Returns the position inside the current mod or file @@ -252,10 +270,10 @@ fn next_space_for_fn_after_call_site(expr: &ast::CallExpr) -> Option, + module_source: &hir::InFile, ) -> Option<(FileId, GeneratedFunctionTarget)> { - let file = module.file_id.original_file(db); - let assist_item = match module.value { + let file = module_source.file_id.original_file(db); + let assist_item = match &module_source.value { hir::ModuleSource::SourceFile(it) => { if let Some(last_item) = it.items().last() { GeneratedFunctionTarget::BehindItem(last_item.syntax().clone()) @@ -599,8 +617,33 @@ fn bar(foo: impl Foo) { } #[test] - #[ignore] - // FIXME print paths properly to make this test pass + fn borrowed_arg() { + check_assist( + add_function, + r" +struct Baz; +fn baz() -> Baz { todo!() } + +fn foo() { + bar<|>(&baz()) +} +", + r" +struct Baz; +fn baz() -> Baz { todo!() } + +fn foo() { + bar(&baz()) +} + +fn bar(baz: &Baz) { + <|>todo!() +} +", + ) + } + + #[test] fn add_function_with_qualified_path_arg() { check_assist( add_function, @@ -609,10 +652,8 @@ mod Baz { pub struct Bof; pub fn baz() -> Bof { Bof } } -mod Foo { - fn foo() { - <|>bar(super::Baz::baz()) - } +fn foo() { + <|>bar(Baz::baz()) } ", r" @@ -620,14 +661,12 @@ mod Baz { pub struct Bof; pub fn baz() -> Bof { Bof } } -mod Foo { - fn foo() { - bar(super::Baz::baz()) - } +fn foo() { + bar(Baz::baz()) +} - fn bar(baz: super::Baz::Bof) { - <|>todo!() - } +fn bar(baz: Baz::Bof) { + <|>todo!() } ", ) @@ -808,6 +847,40 @@ fn foo() { ) } + #[test] + #[ignore] + // Ignored until local imports are supported. + // See https://github.com/rust-analyzer/rust-analyzer/issues/1165 + fn qualified_path_uses_correct_scope() { + check_assist( + add_function, + " +mod foo { + pub struct Foo; +} +fn bar() { + use foo::Foo; + let foo = Foo; + baz<|>(foo) +} +", + " +mod foo { + pub struct Foo; +} +fn bar() { + use foo::Foo; + let foo = Foo; + baz(foo) +} + +fn baz(foo: foo::Foo) { + <|>todo!() +} +", + ) + } + #[test] fn add_function_in_module_containing_other_items() { check_assist( @@ -919,21 +992,6 @@ fn bar(baz: ()) {} ) } - #[test] - fn add_function_not_applicable_if_function_path_not_singleton() { - // In the future this assist could be extended to generate functions - // if the path is in the same crate (or even the same workspace). - // For the beginning, I think this is fine. - check_assist_not_applicable( - add_function, - r" -fn foo() { - other_crate::bar<|>(); -} - ", - ) - } - #[test] #[ignore] fn create_method_with_no_args() { -- cgit v1.2.3