Mirrors/rust-analyzer
2
0
Fork 0
mirror of https://github.com/rust-lang/rust-analyzer synced 2024-11-16 17:58:16 +00:00
Code Issues Projects Releases Packages Wiki Activity

Merge #8036 #8046

Browse source 
8036: completions: provide relevance bonus for enum types, and suggest ref matches for fn and enum r=matklad a=JoshMcguigan

This PR makes several improvements to completions and completion sorting:

1. Provide exact match type relevance score bonus for enum variants
2. Suggest `&Foo` (ref_match) for enums if that is an exact type match
3. Suggest `&foo()` (ref_match) if `foo` returns a type which would be an exact match either with the reference or due to a `Deref` impl

### Before

![pre-ref-relevance-centralized](https://user-images.githubusercontent.com/22216761/111189377-3f05a580-8573-11eb-89be-58a45cb7f829.png)

### After

![post-ref-relevance-centralized](https://user-images.githubusercontent.com/22216761/111189395-45941d00-8573-11eb-871b-09186b35cbb9.png)

### Caveats

I think generic types will require some kind of fancier logic when testing for `exact_type_match`, so for now `Option`/`Result`/etc unfortunately still don't have great completions.

### Implementation

I implemented this in a way that I think makes it most likely for each completion type to be handled consistently. Just replace `CompletionItem::new` with `CompletionItem::new_with_type_info` and `exact_type_match`/`exact_name_match`/`ref_match` are all handled for you, in a way which is sure to be consistent across completion types. 

This approach does introduce some coupling/plumbing that didn't exist before. Notice for example `set_is_local` on the builder, because `set_relevance` was removed from the builder to enforce that the relevance was built "properly" with `CompletionItem::new_with_type_info`. But I think there are benefits to this approach, like `CompletionRelevance` should probably consider deprecation status, and we already tell the builder about that, so in the (likely near term) future we can just pass that information along to `CompletionRelevance` when the user calls `set_deprecated` rather than the user having to manually set it in two places. This basically just hides `CompletionRelevance` from the individual completions, so they only worry about the `CompletionItem` interface. At the moment this seems like a cleaner approach to me, but I'm open to feedback here. 

edit - I've reimplemented this in a simpler way, per feedback below.

8046: Prefer match to if let else r=matklad a=matklad

bors r+
🤖

Co-authored-by: Josh Mcguigan <joshmcg88@gmail.com>
Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>
This commit is contained in:
bors[bot] 2021-03-16 07:57:33 +00:00 committed by GitHub
commit 62ec04bbd5
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
4 changed files with 195 additions and 52 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

190
crates/ide_completion/src/render.rs
View file

@ -10,10 +10,8 @@ pub(crate) mod type_alias;
mod builder_ext;
use base_db::Upcast;
use hir::{
db::HirDatabase, AsAssocItem, Documentation, HasAttrs, HirDisplay, ModuleDef, Mutability,
ScopeDef, Type,
AsAssocItem, Documentation, HasAttrs, HirDisplay, ModuleDef, Mutability, ScopeDef, Type,
};
use ide_db::{
helpers::{item_name, SnippetCap},
@ -22,8 +20,8 @@ use ide_db::{
use syntax::TextRange;
use crate::{
item::{CompletionRelevance, ImportEdit},
CompletionContext, CompletionItem, CompletionItemKind, CompletionKind,
item::ImportEdit, CompletionContext, CompletionItem, CompletionItemKind, CompletionKind,
CompletionRelevance,
};
use crate::render::{enum_variant::render_variant, function::render_fn, macro_::render_macro};
@ -144,7 +142,15 @@ impl<'a> Render<'a> {
.set_documentation(field.docs(self.ctx.db()))
.set_deprecated(is_deprecated);
item.set_relevance(compute_relevance(&self.ctx, &ty, &name.to_string()));
item.set_relevance(CompletionRelevance {
exact_type_match: compute_exact_type_match(self.ctx.completion, ty),
exact_name_match: compute_exact_name_match(self.ctx.completion, name.to_string()),
..CompletionRelevance::default()
});
if let Some(ref_match) = compute_ref_match(self.ctx.completion, ty) {
item.ref_match(ref_match);
}
item.build()
}
@ -234,32 +240,19 @@ impl<'a> Render<'a> {
if !ty.is_unknown() {
item.detail(ty.display(self.ctx.db()).to_string());
}
item.set_relevance(CompletionRelevance {
exact_type_match: compute_exact_type_match(self.ctx.completion, &ty),
exact_name_match: compute_exact_name_match(self.ctx.completion, local_name.clone()),
is_local: true,
..CompletionRelevance::default()
});
if let Some(ref_match) = compute_ref_match(self.ctx.completion, &ty) {
item.ref_match(ref_match);
}
};
if let ScopeDef::Local(local) = resolution {
let ty = local.ty(self.ctx.db());
let mut relevance = compute_relevance(&self.ctx, &ty, &local_name);
relevance.is_local = true;
item.set_relevance(relevance);
if let Some(expected_type) = self.ctx.completion.expected_type.as_ref() {
if &ty != expected_type {
if let Some(ty_without_ref) = expected_type.remove_ref() {
if relevance_type_match(self.ctx.db().upcast(), &ty, &ty_without_ref) {
cov_mark::hit!(suggest_ref);
let mutability = if expected_type.is_mutable_reference() {
Mutability::Mut
} else {
Mutability::Shared
};
item.ref_match(mutability);
}
}
}
}
}
// Add `<>` for generic types
if self.ctx.completion.is_path_type
&& !self.ctx.completion.has_type_args
@ -313,17 +306,44 @@ impl<'a> Render<'a> {
}
}
fn compute_relevance(ctx: &RenderContext, ty: &Type, name: &str) -> CompletionRelevance {
let mut res = CompletionRelevance::default();
res.exact_type_match = Some(ty) == ctx.completion.expected_type.as_ref();
res.exact_name_match = Some(name) == ctx.completion.expected_name.as_deref();
res
fn compute_exact_type_match(ctx: &CompletionContext, completion_ty: &hir::Type) -> bool {
if let Some(expected_type) = ctx.expected_type.as_ref() {
// We don't ever consider unit type to be an exact type match, since
// nearly always this is not meaningful to the user.
completion_ty == expected_type && !expected_type.is_unit()
} else {
false
}
}
fn relevance_type_match(db: &dyn HirDatabase, ty: &Type, expected_type: &Type) -> bool {
ty == expected_type || ty.autoderef(db).any(|deref_ty| &deref_ty == expected_type)
fn compute_exact_name_match(ctx: &CompletionContext, completion_name: impl Into<String>) -> bool {
let completion_name = completion_name.into();
Some(&completion_name) == ctx.expected_name.as_ref()
}
fn compute_ref_match(ctx: &CompletionContext, completion_ty: &hir::Type) -> Option<Mutability> {
let mut ref_match = None;
if let Some(expected_type) = &ctx.expected_type {
if completion_ty != expected_type {
if let Some(expected_type_without_ref) = expected_type.remove_ref() {
if completion_ty == &expected_type_without_ref
|| completion_ty
.autoderef(ctx.db)
.any(|deref_ty| deref_ty == expected_type_without_ref)
{
cov_mark::hit!(suggest_ref);
let mutability = if expected_type.is_mutable_reference() {
Mutability::Mut
} else {
Mutability::Shared
};
ref_match = Some(mutability);
}
}
}
};
ref_match
}
#[cfg(test)]
@ -477,6 +497,11 @@ fn main() { let _: m::Spam = S$0 }
),
lookup: "Spam::Bar",
detail: "(i32)",
relevance: CompletionRelevance {
exact_name_match: false,
exact_type_match: true,
is_local: false,
},
trigger_call_info: true,
},
CompletionItem {
@ -498,6 +523,11 @@ fn main() { let _: m::Spam = S$0 }
),
lookup: "Spam::Foo",
detail: "()",
relevance: CompletionRelevance {
exact_name_match: false,
exact_type_match: true,
is_local: false,
},
},
CompletionItem {
label: "main()",
@ -1169,4 +1199,86 @@ fn foo(bar: u32) {
"#]],
);
}
#[test]
fn enum_owned() {
check_relevance(
r#"
enum Foo { A, B }
fn foo() {
bar($0);
}
fn bar(t: Foo) {}
"#,
expect![[r#"
ev Foo::A [type]
ev Foo::B [type]
en Foo []
fn bar(…) []
fn foo() []
"#]],
);
}
#[test]
fn enum_ref() {
check_relevance(
r#"
enum Foo { A, B }
fn foo() {
bar($0);
}
fn bar(t: &Foo) {}
"#,
expect![[r#"
ev Foo::A []
ev &Foo::A [type]
ev Foo::B []
ev &Foo::B [type]
en Foo []
fn bar(…) []
fn foo() []
"#]],
);
}
#[test]
fn suggest_deref_fn_ret() {
check_relevance(
r#"
#[lang = "deref"]
trait Deref {
type Target;
fn deref(&self) -> &Self::Target;
}
struct S;
struct T(S);
impl Deref for T {
type Target = S;
fn deref(&self) -> &Self::Target {
&self.0
}
}
fn foo(s: &S) {}
fn bar() -> T {}
fn main() {
foo($0);
}
"#,
expect![[r#"
tt Deref []
fn bar() []
fn &bar() [type]
fn foo(…) []
st T []
st S []
fn main() []
"#]],
)
}
}

13
crates/ide_completion/src/render/enum_variant.rs
View file

@ -6,7 +6,8 @@ use itertools::Itertools;
use crate::{
item::{CompletionItem, CompletionKind, ImportEdit},
render::{builder_ext::Params, RenderContext},
render::{builder_ext::Params, compute_exact_type_match, compute_ref_match, RenderContext},
CompletionRelevance,
};
pub(crate) fn render_variant<'a>(
@ -74,6 +75,16 @@ impl<'a> EnumRender<'a> {
item.lookup_by(self.short_qualified_name);
}
let ty = self.variant.parent_enum(self.ctx.completion.db).ty(self.ctx.completion.db);
item.set_relevance(CompletionRelevance {
exact_type_match: compute_exact_type_match(self.ctx.completion, &ty),
..CompletionRelevance::default()
});
if let Some(ref_match) = compute_ref_match(self.ctx.completion, &ty) {
item.ref_match(ref_match);
}
item.build()
}

27
crates/ide_completion/src/render/function.rs
View file

@ -6,7 +6,10 @@ use syntax::ast::Fn;
use crate::{
item::{CompletionItem, CompletionItemKind, CompletionKind, CompletionRelevance, ImportEdit},
render::{builder_ext::Params, RenderContext},
render::{
builder_ext::Params, compute_exact_name_match, compute_exact_type_match, compute_ref_match,
RenderContext,
},
};
pub(crate) fn render_fn<'a>(
@ -52,23 +55,19 @@ impl<'a> FunctionRender<'a> {
self.ctx.is_deprecated(self.func) || self.ctx.is_deprecated_assoc_item(self.func),
)
.detail(self.detail())
.add_call_parens(self.ctx.completion, self.name, params)
.add_call_parens(self.ctx.completion, self.name.clone(), params)
.add_import(import_to_add);
let mut relevance = CompletionRelevance::default();
if let Some(expected_type) = &self.ctx.completion.expected_type {
let ret_ty = self.func.ret_type(self.ctx.db());
let ret_type = self.func.ret_type(self.ctx.db());
item.set_relevance(CompletionRelevance {
exact_type_match: compute_exact_type_match(self.ctx.completion, &ret_type),
exact_name_match: compute_exact_name_match(self.ctx.completion, self.name.clone()),
..CompletionRelevance::default()
});
// We don't ever consider a function which returns unit type to be an
// exact type match, since nearly always this is not meaningful to the
// user.
relevance.exact_type_match = &ret_ty == expected_type && !ret_ty.is_unit();
if let Some(ref_match) = compute_ref_match(self.ctx.completion, &ret_type) {
item.ref_match(ref_match);
}
if let Some(expected_name) = &self.ctx.completion.expected_name {
relevance.exact_name_match =
expected_name == &self.func.name(self.ctx.db()).to_string();
}
item.set_relevance(relevance);
item.build()
}

21
docs/dev/style.md
View file

@ -787,6 +787,27 @@ assert!(0 > x);
**Rationale:** Less-then comparisons are more intuitive, they correspond spatially to [real line](https://en.wikipedia.org/wiki/Real_line).
## If-let
Avoid `if let ... { } else { }` construct, use `match` instead.
```rust
// GOOD
match ctx.expected_type.as_ref() {
Some(expected_type) => completion_ty == expected_type && !expected_type.is_unit(),
None => false,
}
// BAD
if let Some(expected_type) = ctx.expected_type.as_ref() {
completion_ty == expected_type && !expected_type.is_unit()
} else {
false
}
```
**Rational:** `match` is almost always more compact.
The `else` branch can get a more precise pattern: `None` or `Err(_)` instead of `_`.
## Token names