Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Complete assoc. types on paths without a trait #4149

Closed
wants to merge 1 commit into from
Closed

Complete assoc. types on paths without a trait #4149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion crates/ra_hir_ty/src/method_resolution.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -532,7 +532,10 @@ fn is_valid_candidate(
let data = db.const_data(c);
name.map_or(true, |name| data.name.as_ref() == Some(name)) && receiver_ty.is_none()
}
_ => false,
AssocItemId::TypeAliasId(t) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r? @flodiebold

From the look of it, the old code looks correct, as it only uses the values namespace. I would expect two similar fuctions, one for consttants & methods, and one for types. The caller can then select the appropriate namespace

Copy link
Member

@flodiebold flodiebold Apr 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, method_resolution is for values only, and associated types resolution has different rules. (You can't just write S::Item if S is a struct implementing Iterator, the only situation where you can write T::AssociatedType where T is a type is when T is a type parameter. I thought you could do this as well before I actually implemented it!)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might still be a good idea to replace _ with explicit match & comment

let data = db.type_alias_data(t);
name.map_or(true, |name| data.name == *name) && receiver_ty.is_none()
}
}
}

Expand Down
57 changes: 39 additions & 18 deletions crates/ra_ide/src/completion/complete_qualified_path.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,12 @@ pub(super) fn complete_qualified_path(acc: &mut Completions, ctx: &CompletionCon
Some(path) => path.clone(),
_ => return,
};
let def = match ctx.scope().resolve_hir_path(&path) {
let scope = ctx.scope();
let def = match scope.resolve_hir_path(&path) {
Some(PathResolution::Def(def)) => def,
_ => return,
};
let context_module = ctx.scope().module();
let context_module = scope.module();
match def {
hir::ModuleDef::Module(module) => {
let module_scope = module.scope(ctx.db, context_module);
Expand Down Expand Up @@ -46,11 +47,8 @@ pub(super) fn complete_qualified_path(acc: &mut Completions, ctx: &CompletionCon
hir::ModuleDef::TypeAlias(a) => a.ty(ctx.db),
_ => unreachable!(),
};
// Iterate assoc types separately
// FIXME: complete T::AssocType
let krate = ctx.krate;
if let Some(krate) = krate {
let traits_in_scope = ctx.scope().traits_in_scope();
if let Some(krate) = ctx.krate {
let traits_in_scope = scope.traits_in_scope();
ty.iterate_path_candidates(ctx.db, krate, &traits_in_scope, None, |_ty, item| {
if context_module.map_or(false, |m| !item.is_visible_from(ctx.db, m)) {
return None;
Expand All @@ -64,20 +62,10 @@ pub(super) fn complete_qualified_path(acc: &mut Completions, ctx: &CompletionCon
}
None::<()>
});

ty.iterate_impl_items(ctx.db, krate, |item| {
if context_module.map_or(false, |m| !item.is_visible_from(ctx.db, m)) {
return None;
}
match item {
hir::AssocItem::Function(_) | hir::AssocItem::Const(_) => {}
hir::AssocItem::TypeAlias(ty) => acc.add_type_alias(ctx, ty),
}
None::<()>
});
}
}
hir::ModuleDef::Trait(t) => {
// Handles `Trait::assoc` as well as `<Ty as Trait>::assoc`.
for item in t.items(ctx.db) {
if context_module.map_or(false, |m| !item.is_visible_from(ctx.db, m)) {
continue;
Expand Down Expand Up @@ -584,6 +572,39 @@ mod tests {
);
}

#[test]
fn completes_trait_associated_type() {
assert_debug_snapshot!(
do_reference_completion(
r"
mod foo {
pub struct S;
}
trait Tr {
type Assoc;
}
impl Tr for foo::S {}

fn f() {
foo::S::<|>
}
",
),
@r###"
[
CompletionItem {
label: "Assoc",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... so this completion is actually wrong, rustc will complain about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh great, not sure how I missed that :/

I think this is actually a bug in rustc, namely rust-lang/rust#22519. But yeah, rust-analyzer shouldn't suggest something that doesn't work.

Copy link
Member

@flodiebold flodiebold Apr 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can see, that issue doesn't propose allowing foo::S::AssocTy. It's rather about the fact that even if T is a type parameter, <T>::AssocTy doesn't work.

(Edit: Ok, eddyb's last comment does refer to allowing this, but still it doesn't seem like this will change anytime soon.)

source_range: 248..248,
delete: 248..248,
insert: "Assoc",
kind: TypeAlias,
detail: "type Assoc;",
},
]
"###
);
}

#[test]
fn associated_item_visibility() {
assert_debug_snapshot!(
Expand Down