completion relevance consider if types can be unified

[JoshMcguigan]

This PR improves completion relevance scoring for generic types, in cases where the types could unify.

Before

After

changelog feature improve completions

[JoshMcguigan]

I do have a few questions since I am not very familiar with the type inference code. @flodiebold ?

1. Am I using type inference correctly? It seems to work in this case, but I'm not sure if there are any rough edges to be aware of.
2. Does it make sense to add these inference functions to the public interface of hir_ty/hir as I do here?
3. Is there an easy fix to make this work for functions which a generic parameter in their return type fn foo<T>() -> Option<T> as shown in the FIXME in the unit test?

[JoshMcguigan]

Hmm, CI is failing for a lint I don't see locally. And applying the fix breaks compilation.

  --> crates/hir_ty/src/infer.rs:48:1
   |
48 | pub use unify::could_unify;
   | ---^^^^^^^^^^^^^^^^^^^^^^^^
   | |
   | help: consider restricting its visibility: `pub(crate)`
   |
   = note: `-D unreachable-pub` implied by `-D warnings`

[lnicola]

We use stable on CI, FYI.

[JoshMcguigan]

$ rustup --version
rustup 1.23.1 (3df2264a9 2020-11-30)
info: This is the version for the rustup toolchain manager, not the rustc compiler.
info: The currently active `rustc` version is `rustc 1.50.0 (cb75ad5db 2021-02-10)`

I believe I'm running with the latest stable compiler locally. I can re-create the lint by running RUSTFLAGS="-D warnings -W unreachable-pub" cargo check locally, as described in the ci config.

But I'm still not sure why I'm seeing this lint, given the suggested fix doesn't work.

[JoshMcguigan]

Looks like this is the same as a bug reported a couple years ago, although it seems that was fixed 🤷

rust-lang/rust#52664

rust-lang/rust#47816

[JoshMcguigan]

Actually it looks like there is a newer bug report for this, which is still open. I suppose I should add an #[allow(unreachable_pub)] on this item for now?

rust-lang/rust#57411

[flodiebold]

Does it make sense to add these inference functions to the public interface of hir_ty/hir as I do here?

I think adding it for now is fine. We might revamp the HIR type API, but only after the Chalk move.

Am I using type inference correctly? It seems to work in this case, but I'm not sure if there are any rough edges to be aware of.
Is there an easy fix to make this work for functions which a generic parameter in their return type fn foo() -> Option as shown in the FIXME in the unit test?

So, the problem here is that the Function::ret_type gives us the declared return type, which is ok for displaying, but that's not quite the type we would get when we call it. We would want to fill in the type parameters, using type variables where we don't know them yet (imagine e.g. if we have an Option<Foo> and call unwrap() on it, then can already tell the returned value will be Foo). That's not trivial though. Of course for non-method calls, we could probably mostly get by with just replacing placeholders by Unknown before the unify check.

(Also worth noting that what we'd really want is to check coercion, if the completion is happening at a coercion site...)

[matklad]

Let’s distinguish between “can unify” and “exact match” in the relevance, so that things like Default::default are less relevant than specific types.

[JoshMcguigan]

These weren't used, because CompletionRelevance should only be compared by score. I could re-implement them in terms of score, but that would require updating the Eq impl which we use in a different way, so it would broaden the scope of this PR a bit.

The reason I removed these at all in this PR is because it didn't make sense to me to derive them on the new enum CompletionRelevanceTypeMatch.

[JoshMcguigan]

This change is unrelated to this PR, but combining these two examples has been discussed as something we'd like to do.

[JoshMcguigan]

Let’s distinguish between “can unify” and “exact match” in the relevance, so that things like Default::default are less relevant than specific types.

I've updated this to distinguish between these cases, and score an exact match as more relevant than types which can unify.

[JoshMcguigan]

bors r+

[bors]

Build succeeded:

• Rust (macos-latest)
• Rust (ubuntu-latest)
• Rust (windows-latest)
• TypeScript (ubuntu-latest)
• TypeScript (windows-latest)

[lnicola]

changelog feature for completion, take into account whether types unify: