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.

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

Type-aware autocompletion #2081

Open
viluon opened this issue Aug 5, 2021 · 8 comments
Open

Type-aware autocompletion #2081

viluon opened this issue Aug 5, 2021 · 8 comments
Labels
component: ghcide status: in discussion Not actionable, because discussion is still ongoing or there's no decision yet type: enhancement New feature or request

Comments

@viluon
Copy link

viluon commented Aug 5, 2021

Hi, I'm surprised that I couldn't find a similar issue, except for #817. Please excuse me if it is misplaced.

I wish HLS provided type-aware autocompletion. The autocompletion it does have often isn't very useful:
image

In this case, the obvious type-correct autocompletion would be ssDynTrace, of type [DynTraceEntry]. Instead, the autocompletion is full of relevant but quite useless definitions. In fact, ssDynTrace isn't even listed until I type a d.
image

Another issue is that with record wildcards, there are duplicate completions available. Here, ssDynTrace is listed once as the value from the wildcard and once more as the getter function.
image

In theory, I could use a hole instead, but invoking Wingman with many bindings in scope isn't a good idea:
image

Even if VS Code made the quick action list searchable, the "attempt to fill hole" and "refine hole" commands time out. Plus, Wingman doesn't seem like the right tool for the job – I know pretty well what I want to write, I just don't want to have to spell every identifier out, and the types involved don't uniquely identify the correct solution.

It'd be nice if HLS could trim the autocompletion suggestions by type and offer relevant suggestions even with no prior identifier input.

@Ailrun Ailrun added status: in discussion Not actionable, because discussion is still ongoing or there's no decision yet type: enhancement New feature or request labels Aug 5, 2021
@jneira
Copy link
Member

jneira commented Aug 5, 2021

Really a good point, thanks for suggesting it. There is a similar issue which asks that import symbols would take in account types too: #754

@isovector
Copy link
Collaborator

isovector commented Aug 6, 2021

Sweet mother of mary that's a lot of code actions from Wingman. Mind sharing the file? I'm curious how you got so many bindings in scope! Maybe we should limit it to like 8 suggestions?

@pepeiborra
Copy link
Collaborator

We could implement this feature by:

  1. Replacing the partial text (in your example ssDy) by a hole _
  2. Asking GHC to fill in the hole
  3. Filtering out all the suggestions that do not start with the partial text

The problem is that currently typed holes are hopelessly slow (#2037) and not suitable for completions. Maybe when VSCode and other editors implement the LSP spec support for partial responses, and/or when typed holes become faster.

On the other hand, someone could reimplement the typed holes machinery out of GHC with a better performance, then it would be more realistic to offer this feature.

@viluon
Copy link
Author

viluon commented Aug 6, 2021

@isovector the file I was editing is on GitHub, although ssDynTrace specifically is only in my local fork. Just use modify and a record wildcard for the StgState constructor, all the ss... bindings come from there. Using record wildcards clutters the local environment but leads to a lot more concise code.

@Tarmean
Copy link

Tarmean commented Aug 9, 2021

I've been playing with wingman to add a 'guess' command which looks at the types of local identifiers in scope and then tries to guess functions that could be relevant. (The really halfbaked implementation is here but it needs some significant changes)

Some things I noticed:

  • Demanding perfect hole fits gives clear results but is probably too fragile for autocompletion. Usually you write the function before the arguments.
  • Adding a secondary filter of 'has a type constructor in common with a local variable' works reasonably well and is index friendly, but it still requires some sort heuristic to get good ordering
  • Hoogle doesn't quite fit because the heuristic assume the query is hand-written
  • Type defaults and Any are super annoying
  • Standard type inference is insufficient for nice usage on local bindings
foo :: Int
foo = x
    where
       x = _

Unfortunately the hole is fully polymorphic.

Also, having some sort of TyCon -> UniqueSet index on ExternalPackageState to quickly filter down to a set of plausible types seems useful to keep things fast. I'm not quite sure what the most efficient way to get all identifiers+types for package-local things is, though.

Edit: Does GhcSessionDeps have an up-to-date InteractiveContext?

@Tarmean
Copy link

Tarmean commented Aug 12, 2021

I've pushed a commit that wires my quick-and-dirty-attempt into an autocompletion handler. I hacked the standard autocompletion rules to work with typechecked modules and stash the types in the completion items to avoid duplicating them, I'm not sure how significant the slowdown of this is and how much it breaks.

This still loops through all completion items and first checks whether they contain relevant type constructors, and then does a really inefficient search that tries to unify all local variables with all argument positions. The naive approach is definitely too slow, but if the types are concrete enough it sometimes give useful results:

out.2.mp4

For discoverability completion for empty prefixes would be useful, but it can't give completions for empty prefixes because the ast doesn't work out. [wingman|guess|] sometimes kind of works but the correct answer often isn't the first result.

@isovector
Copy link
Collaborator

@Tarmean this is fantastic work! RE:

does a really inefficient search that tries to unify all local variables with all argument positions

Did you throw GHC's unification at the problem? It's quite good, though a little tricky to get right in the presence of skolems. Wingman wraps the problem via:

------------------------------------------------------------------------------
-- | Like 'tcUnifyTy', but takes a list of skolems to prevent unification of.
tryUnifyUnivarsButNotSkolems :: Set TyVar -> CType -> CType -> Maybe TCvSubst
tryUnifyUnivarsButNotSkolems skolems goal inst =
case tcUnifyTysFG
(bool BindMe Skolem . flip S.member skolems)
[unCType inst]
[unCType goal] of
Unifiable subst -> pure subst
_ -> Nothing

[wingman|guess|] sometimes kind of works but the correct answer often isn't the first result.

Yeah, this is a general problem for Wingman, caused by the fact that LSP sucks and really gives us nothing we can be interactive with. Though now that I think about it --- we could experiment with showing the top 5 results from the tactic search as code actions.

@Tarmean
Copy link

Tarmean commented Aug 16, 2021

I did use your tryUnifyUnivarsButNotSkolems and it was super useful!

I struggle more with the whole fuzzy matching part. If the candidate is Map.intersect and there are two maps with String keys involved it probably should be scored high. But zipWith7 is probably still not what you want even if you have a lot of lists in scope. There probably is something a lot smarter that I could do, but hoogles heuristics seem to break down if there is unrelated noise.

Also, I know GHC distinguishes rigid with polymorphic type variables. Is there something like this for constraints?

-- only have to search for types with this constraint
foo :: Foldable f => f a -> Int
-- also have to search for types which implement this constraint
foo :: Bool
foo = null _

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: ghcide status: in discussion Not actionable, because discussion is still ongoing or there's no decision yet type: enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants