From d9b3242bcd5f4208fad707a30907633ccf081056 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20Nevyho=C5=A1t=C4=9Bn=C3=BD?= Date: Wed, 22 Dec 2021 19:48:14 +0100 Subject: [PATCH 1/2] Fix generic type substitution in impl trait with assoc type --- .../src/handlers/add_missing_impl_members.rs | 306 ++++++++++++++++++ crates/ide_db/src/path_transform.rs | 104 +++++- crates/syntax/src/ast/make.rs | 12 +- 3 files changed, 411 insertions(+), 11 deletions(-) diff --git a/crates/ide_assists/src/handlers/add_missing_impl_members.rs b/crates/ide_assists/src/handlers/add_missing_impl_members.rs index a10eca10d1..6546d8f7fb 100644 --- a/crates/ide_assists/src/handlers/add_missing_impl_members.rs +++ b/crates/ide_assists/src/handlers/add_missing_impl_members.rs @@ -942,4 +942,310 @@ impl FooB for Foo { "#, ) } + + #[test] + fn test_assoc_type_when_trait_with_same_name_in_scope() { + check_assist( + add_missing_impl_members, + r#" +pub trait Foo {} + +pub trait Types { + type Foo; +} + +pub trait Behavior { + fn reproduce(&self, foo: T::Foo); +} + +pub struct Impl; + +impl Behavior for Impl { $0 }"#, + r#" +pub trait Foo {} + +pub trait Types { + type Foo; +} + +pub trait Behavior { + fn reproduce(&self, foo: T::Foo); +} + +pub struct Impl; + +impl Behavior for Impl { + fn reproduce(&self, foo: ::Foo) { + ${0:todo!()} + } +}"#, + ); + } + + #[test] + fn test_assoc_type_on_concrete_type() { + check_assist( + add_missing_impl_members, + r#" +pub trait Types { + type Foo; +} + +impl Types for u32 { + type Foo = bool; +} + +pub trait Behavior { + fn reproduce(&self, foo: T::Foo); +} + +pub struct Impl; + +impl Behavior for Impl { $0 }"#, + r#" +pub trait Types { + type Foo; +} + +impl Types for u32 { + type Foo = bool; +} + +pub trait Behavior { + fn reproduce(&self, foo: T::Foo); +} + +pub struct Impl; + +impl Behavior for Impl { + fn reproduce(&self, foo: ::Foo) { + ${0:todo!()} + } +}"#, + ); + } + + #[test] + fn test_assoc_type_on_concrete_type_qualified() { + check_assist( + add_missing_impl_members, + r#" +pub trait Types { + type Foo; +} + +impl Types for std::string::String { + type Foo = bool; +} + +pub trait Behavior { + fn reproduce(&self, foo: T::Foo); +} + +pub struct Impl; + +impl Behavior for Impl { $0 }"#, + r#" +pub trait Types { + type Foo; +} + +impl Types for std::string::String { + type Foo = bool; +} + +pub trait Behavior { + fn reproduce(&self, foo: T::Foo); +} + +pub struct Impl; + +impl Behavior for Impl { + fn reproduce(&self, foo: ::Foo) { + ${0:todo!()} + } +}"#, + ); + } + + #[test] + fn test_assoc_type_on_concrete_type_multi_option_ambiguous() { + check_assist( + add_missing_impl_members, + r#" +pub trait Types { + type Foo; +} + +pub trait Types2 { + type Foo; +} + +impl Types for u32 { + type Foo = bool; +} + +impl Types2 for u32 { + type Foo = String; +} + +pub trait Behavior { + fn reproduce(&self, foo: ::Foo); +} + +pub struct Impl; + +impl Behavior for Impl { $0 }"#, + r#" +pub trait Types { + type Foo; +} + +pub trait Types2 { + type Foo; +} + +impl Types for u32 { + type Foo = bool; +} + +impl Types2 for u32 { + type Foo = String; +} + +pub trait Behavior { + fn reproduce(&self, foo: ::Foo); +} + +pub struct Impl; + +impl Behavior for Impl { + fn reproduce(&self, foo: ::Foo) { + ${0:todo!()} + } +}"#, + ); + } + + #[test] + fn test_assoc_type_on_concrete_type_multi_option() { + check_assist( + add_missing_impl_members, + r#" +pub trait Types { + type Foo; +} + +pub trait Types2 { + type Bar; +} + +impl Types for u32 { + type Foo = bool; +} + +impl Types2 for u32 { + type Bar = String; +} + +pub trait Behavior { + fn reproduce(&self, foo: T::Bar); +} + +pub struct Impl; + +impl Behavior for Impl { $0 }"#, + r#" +pub trait Types { + type Foo; +} + +pub trait Types2 { + type Bar; +} + +impl Types for u32 { + type Foo = bool; +} + +impl Types2 for u32 { + type Bar = String; +} + +pub trait Behavior { + fn reproduce(&self, foo: T::Bar); +} + +pub struct Impl; + +impl Behavior for Impl { + fn reproduce(&self, foo: ::Bar) { + ${0:todo!()} + } +}"#, + ); + } + + #[test] + fn test_assoc_type_on_concrete_type_multi_option_foreign() { + check_assist( + add_missing_impl_members, + r#" +mod bar { + pub trait Types2 { + type Bar; + } +} + +pub trait Types { + type Foo; +} + +impl Types for u32 { + type Foo = bool; +} + +impl bar::Types2 for u32 { + type Bar = String; +} + +pub trait Behavior { + fn reproduce(&self, foo: T::Bar); +} + +pub struct Impl; + +impl Behavior for Impl { $0 }"#, + r#" +mod bar { + pub trait Types2 { + type Bar; + } +} + +pub trait Types { + type Foo; +} + +impl Types for u32 { + type Foo = bool; +} + +impl bar::Types2 for u32 { + type Bar = String; +} + +pub trait Behavior { + fn reproduce(&self, foo: T::Bar); +} + +pub struct Impl; + +impl Behavior for Impl { + fn reproduce(&self, foo: ::Bar) { + ${0:todo!()} + } +}"#, + ); + } } diff --git a/crates/ide_db/src/path_transform.rs b/crates/ide_db/src/path_transform.rs index 1b4f793a50..5507c9cd68 100644 --- a/crates/ide_db/src/path_transform.rs +++ b/crates/ide_db/src/path_transform.rs @@ -118,14 +118,20 @@ struct Ctx<'a> { impl<'a> Ctx<'a> { fn apply(&self, item: &SyntaxNode) { - for event in item.preorder() { - let node = match event { - syntax::WalkEvent::Enter(_) => continue, - syntax::WalkEvent::Leave(it) => it, - }; - if let Some(path) = ast::Path::cast(node.clone()) { - self.transform_path(path); - } + // `transform_path` may update a node's parent and that would break the + // tree traversal. Thus all paths in the tree are collected into a vec + // so that such operation is safe. + let paths = item + .preorder() + .filter_map(|event| match event { + syntax::WalkEvent::Enter(_) => None, + syntax::WalkEvent::Leave(node) => Some(node), + }) + .filter_map(ast::Path::cast) + .collect::>(); + + for path in paths { + self.transform_path(path); } } fn transform_path(&self, path: ast::Path) -> Option<()> { @@ -145,10 +151,60 @@ impl<'a> Ctx<'a> { match resolution { hir::PathResolution::TypeParam(tp) => { if let Some(subst) = self.substs.get(&tp) { - ted::replace(path.syntax(), subst.clone_subtree().clone_for_update().syntax()) + let parent = path.syntax().parent()?; + if let Some(parent) = ast::Path::cast(parent.clone()) { + // Path inside path means that there is an associated + // type on the type parameter. It is necessary to fully + // qualify the type with `as Trait`. Even though it + // might be unnecessary if `subst` is generic type, + // always fully qualifying the path is safer because of + // potential clash of associated types from multiple + // traits + + let trait_ref = find_trait_for_assoc_type( + self.source_scope, + tp, + parent.segment()?.name_ref()?, + ) + .and_then(|trait_ref| { + let found_path = self.target_module.find_use_path( + self.source_scope.db.upcast(), + hir::ModuleDef::Trait(trait_ref), + )?; + match ast::make::ty_path(mod_path_to_ast(&found_path)) { + ast::Type::PathType(path_ty) => Some(path_ty), + _ => None, + } + }); + + let segment = ast::make::path_segment_ty(subst.clone(), trait_ref); + let qualified = + ast::make::path_from_segments(std::iter::once(segment), false); + ted::replace(path.syntax(), qualified.clone_for_update().syntax()); + } else if let Some(path_ty) = ast::PathType::cast(parent) { + ted::replace( + path_ty.syntax(), + subst.clone_subtree().clone_for_update().syntax(), + ); + } else { + ted::replace( + path.syntax(), + subst.clone_subtree().clone_for_update().syntax(), + ); + } } } hir::PathResolution::Def(def) => { + if let hir::ModuleDef::Trait(_) = def { + if matches!(path.segment()?.kind()?, ast::PathSegmentKind::Type { .. }) { + // `speculative_resolve` resolves segments like `` into `Trait`, but just the trait name should + // not be used as the replacement of the original + // segment. + return None; + } + } + let found_path = self.target_module.find_use_path(self.source_scope.db.upcast(), def)?; let res = mod_path_to_ast(&found_path).clone_for_update(); @@ -195,3 +251,33 @@ fn get_type_args_from_arg_list(generic_arg_list: ast::GenericArgList) -> Option< Some(result) } + +fn find_trait_for_assoc_type( + scope: &SemanticsScope, + type_param: hir::TypeParam, + assoc_type: ast::NameRef, +) -> Option { + let db = scope.db; + let trait_bounds = type_param.trait_bounds(db); + + let assoc_type_name = assoc_type.text(); + + for trait_ in trait_bounds { + let type_aliases = trait_.items(db).into_iter().filter_map(|item| match item { + hir::AssocItem::TypeAlias(ta) => Some(ta), + _ => None, + }); + + for type_alias in type_aliases { + if assoc_type_name.as_str() == type_alias.name(db).as_text()?.as_str() { + // It is fine to return the first match because in case of + // multiple possibilities, the exact trait must be disambiguated + // in the definition of trait being implemented, so this search + // should not be needed. + return Some(trait_); + } + } + } + + None +} diff --git a/crates/syntax/src/ast/make.rs b/crates/syntax/src/ast/make.rs index e1938307cf..1e94b4574b 100644 --- a/crates/syntax/src/ast/make.rs +++ b/crates/syntax/src/ast/make.rs @@ -166,6 +166,14 @@ pub fn path_segment(name_ref: ast::NameRef) -> ast::PathSegment { ast_from_text(&format!("use {};", name_ref)) } +pub fn path_segment_ty(type_ref: ast::Type, trait_ref: Option) -> ast::PathSegment { + let text = match trait_ref { + Some(trait_ref) => format!("fn f(x: <{} as {}>) {{}}", type_ref, trait_ref), + None => format!("fn f(x: <{}>) {{}}", type_ref), + }; + ast_from_text(&text) +} + pub fn path_segment_self() -> ast::PathSegment { ast_from_text("use self;") } @@ -196,9 +204,9 @@ pub fn path_from_segments( ) -> ast::Path { let segments = segments.into_iter().map(|it| it.syntax().clone()).join("::"); ast_from_text(&if is_abs { - format!("use ::{};", segments) + format!("fn f(x: ::{}) {{}}", segments) } else { - format!("use {};", segments) + format!("fn f(x: {}) {{}}", segments) }) } From a710b87b1bb86602e9acccbb098039f66e27d80d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20Nevyho=C5=A1t=C4=9Bn=C3=BD?= Date: Fri, 7 Jan 2022 16:41:39 +0100 Subject: [PATCH 2/2] Fix generic type substitution in impl trait with assoc const --- .../src/handlers/add_missing_impl_members.rs | 56 +++++++++++++++++++ crates/ide_db/src/path_transform.rs | 29 +++++----- 2 files changed, 71 insertions(+), 14 deletions(-) diff --git a/crates/ide_assists/src/handlers/add_missing_impl_members.rs b/crates/ide_assists/src/handlers/add_missing_impl_members.rs index 6546d8f7fb..3105b28911 100644 --- a/crates/ide_assists/src/handlers/add_missing_impl_members.rs +++ b/crates/ide_assists/src/handlers/add_missing_impl_members.rs @@ -1245,6 +1245,62 @@ impl Behavior for Impl { fn reproduce(&self, foo: ::Bar) { ${0:todo!()} } +}"#, + ); + } + + #[test] + fn test_transform_path_in_path_expr() { + check_assist( + add_missing_default_members, + r#" +pub trait Const { + const FOO: u32; +} + +pub trait Trait { + fn foo() -> bool { + match T::FOO { + 0 => true, + _ => false, + } + } +} + +impl Const for u32 { + const FOO: u32 = 1; +} + +struct Impl; + +impl Trait for Impl { $0 }"#, + r#" +pub trait Const { + const FOO: u32; +} + +pub trait Trait { + fn foo() -> bool { + match T::FOO { + 0 => true, + _ => false, + } + } +} + +impl Const for u32 { + const FOO: u32 = 1; +} + +struct Impl; + +impl Trait for Impl { + $0fn foo() -> bool { + match ::FOO { + 0 => true, + _ => false, + } + } }"#, ); } diff --git a/crates/ide_db/src/path_transform.rs b/crates/ide_db/src/path_transform.rs index 5507c9cd68..524af7fe8f 100644 --- a/crates/ide_db/src/path_transform.rs +++ b/crates/ide_db/src/path_transform.rs @@ -154,14 +154,14 @@ impl<'a> Ctx<'a> { let parent = path.syntax().parent()?; if let Some(parent) = ast::Path::cast(parent.clone()) { // Path inside path means that there is an associated - // type on the type parameter. It is necessary to fully - // qualify the type with `as Trait`. Even though it - // might be unnecessary if `subst` is generic type, - // always fully qualifying the path is safer because of - // potential clash of associated types from multiple - // traits + // type/constant on the type parameter. It is necessary + // to fully qualify the type with `as Trait`. Even + // though it might be unnecessary if `subst` is generic + // type, always fully qualifying the path is safer + // because of potential clash of associated types from + // multiple traits - let trait_ref = find_trait_for_assoc_type( + let trait_ref = find_trait_for_assoc_item( self.source_scope, tp, parent.segment()?.name_ref()?, @@ -252,24 +252,25 @@ fn get_type_args_from_arg_list(generic_arg_list: ast::GenericArgList) -> Option< Some(result) } -fn find_trait_for_assoc_type( +fn find_trait_for_assoc_item( scope: &SemanticsScope, type_param: hir::TypeParam, - assoc_type: ast::NameRef, + assoc_item: ast::NameRef, ) -> Option { let db = scope.db; let trait_bounds = type_param.trait_bounds(db); - let assoc_type_name = assoc_type.text(); + let assoc_item_name = assoc_item.text(); for trait_ in trait_bounds { - let type_aliases = trait_.items(db).into_iter().filter_map(|item| match item { - hir::AssocItem::TypeAlias(ta) => Some(ta), + let names = trait_.items(db).into_iter().filter_map(|item| match item { + hir::AssocItem::TypeAlias(ta) => Some(ta.name(db)), + hir::AssocItem::Const(cst) => cst.name(db), _ => None, }); - for type_alias in type_aliases { - if assoc_type_name.as_str() == type_alias.name(db).as_text()?.as_str() { + for name in names { + if assoc_item_name.as_str() == name.as_text()?.as_str() { // It is fine to return the first match because in case of // multiple possibilities, the exact trait must be disambiguated // in the definition of trait being implemented, so this search