Disable hole fit suggestions when running Wingman

[isovector]

HLS by default enables hole fit suggestions, and subsumption hole fits. These work nicely if you only ever have one hole, but perform superlinear amounts of work, and get extremely slow when multiple holes are present. This is a particular dealbreaker for Wingman, which introduces lots of holes, all the time.

This PR disables hole fit suggestions whenever Wingman is enabled. As a result, Wingman feels extremely snappy, even when there are 50+ holes present. Users who preferred the default hole-fit suggestions can disable Wingman to maintain the old behavior.

Fixes #1602

[pepeiborra]

This is reasonable, the only thing missing is documentation. Does Wingman have a README or similar?

[isovector]

It does. Are you looking for documentation just about the functionality disabling?

[pepeiborra]

Yes, the fact that hole suggestions will not be available when using Wingman

[isovector]

Not sure what to do about the broken tests here. Just remove them? Don't run Wingman for them? Mark them as failing?

[pepeiborra]

Probably disable Wingman using LSP configs in those tests.
Also, you may want to add a test to check that the hole quick fixes are really disabled when Wingman is on.

[isovector]

The LSP configs don't disable the DynFlags endos fortunately, so that approach doesn't work. I've disabled the tests for now.

[anka-213]

Does wingman replace the basic functionality of hole-fits where it fills in a hole with some variable of the correct type? E.g.

f :: Foo -> Bar
x :: Foo

g :: Bar
g = _ x

would fill in f here.

If not, I will sadly need to disable wingman on my computer if this change lands, since I use that all the time, but I don't want to lose case splitting either. I've never used subsumption hole fits though.

---

Would it be possible to make ghc only compute hole fits when you explicitly ask for them, e.g. when you're at a hole and ask for completions/code actions?

[isovector]

@anka-213 I don't know how feasible that is. My guess is that ghcide implements these by looking at the global diagnostics that come back from GHC, and assigning them to code actions at the given srcspans. One way or another, you're going to need to wait for GHC to compute the holes, and that is always going to be superlinear in the number of holes.

In my eyes, the real bug here is that GHC is hella slow in computing these things. https://gitlab.haskell.org/ghc/ghc/-/issues/16875 (https://gitlab.haskell.org/ghc/ghc/-/issues/16875#note_329814 in particular)

As for the more general problem, @anka-213, you're the first person I've heard from who will actively miss this feature. Would you mind describing what a typical workflow looks like here? Are there hole fits you always ignore? Do you primarily use it for library combinators? Things defined in the same module? Things in local scope? Do you use it primarily for functions or values or both? How polymorphic are the hole fits you end up using?

Although I'd be sad to lose you as a user, the performance implications of this feature are IMO far too burdensome in an interactive application.

[anka-213]

I've never found the subsumption/refinement hole fits like foo _ or id _ useful at all, so I think we should disable those globally, regardless of if Wingman is enabled at all. They can always be simulated manually with _ undefined, _ _ or _ . _ if you expect a hole fit like that.

Most of the time, I think, they are not very polymorphic. I kind of use it as a type based autocomplete, so I don't have to remember what the function is called.
I think I've used all three variants of library combinators, definitions from the same module and values in local scope, but I'm not sure what's most important. I'll have to experiment a bit more to get a better appreciation for what my workflow is.

(I did once figure out that the answer was, as always, traverse by using hole fits.)

There are hole fits that I always ignore, e.g. #1804 (which is useful e.g. for creating a less polymorphic version of an existing function, like traverse). I think I always ignore functions like id, $, const, head _, etc., but those mostly show up in subsumption hole fits. (The subsumption hole fits have made the feature worse, not better)

The best filter I can think of for usefulness is: "Never suggest anything that can provide a value of any type"
So _ undefined shouldn't suggest any of head, last, fst, snd, id.

I think things like _ . _ is a fairly common part of my workflow, where I know that I want to go from type A to type B, but may convert to some other type C (and possibly D and E) in between. This way I can start either at the beginning or the end and make progress step by step in converting the values until they meet in the middle. This is fairly similar to how I've worked when writing proofs with equational-reasoning in Agda.

---

What I think I would prefer for now is to allow enabling both at the same time with an option, with a warning that it will likely cause serious slowdown. When/if https://gitlab.haskell.org/ghc/ghc/-/issues/16875#note_210045 is implemented, we could switch to asking for them interactively.

If it would not require recompilation of more than the current module, another option could be to have an interactive command for saying "Compute/Enable hole fits once for this module". It does have some diminishing returns though, since if I have to wait a long time, I might as well figure out and the name of the function/value manually, especially for local variables.

But yes, solving the slowness of hole completions would be by far the best solution and I wouldn't mind a slight reduction of functionality for that. One of the easiest (partial) solutions for that would be to completely disable hole fits on unconstrained holes like the one mentioned in https://gitlab.haskell.org/ghc/ghc/-/issues/16875#note_329814.

[isovector]

I plan to merge this as-is later today (modulo some documentation changes), unless anyone objects particularly hard.

[anka-213]

Here's a twitter thread of people praising the feature: https://twitter.com/kerckhove_ts/status/1403031394234818562
I'm not sure if this makes any difference, but at least I'm not the only one using it.

[anka-213]

I'd argue fairly strongly for allowing the user to enable both at the same time, but not doing it by default (and having a big warning about it in the setting). But maybe that could be done in a later PR?

I just want the ability to do both hole fits and case splitting at the same time. I've never used any other feature of Wingman, so I don't know how useful they are. (I've tried "try to fill hole", but it never produced anything useful at all for me)

[isovector]

I'd love some PRs improving the situation!

[pepeiborra]

The main point of contention here is whether the disabling behaviour can be coupled to the Wingman plugin.

Wingman is enabled by default with HLS but we cannot presume that all the HLS users will prefer the Wingman features over the hole fit suggestions, so the least we can do is give them a choice. Until this is possible, I don't think we can merge this change.

[isovector]

Apologies if this comes off as curt --- it's not intended to be --- but I'm feeling extremely frustrated here, for several reasons.

1. There is an existing performance issue in the language server today, completely independent of Wingman (type holes are slow). The TypeCheck rule completely freezes up for several seconds per hole, meaning no further actions can run.
2. Wingman exacerbates this issue by producing holes (eg, when case splitting), but short of emitting undefined instead, there is no alternative.
3. The existence of holes in any modules with 20+ imports make any sort of interactive flow with HLS untenably slow.
4. Case splitting was an extremely well received feature
5. In a universe where case splitting came first, it seems unlikely that we would have ever considered enabling hole fits due to the extreme performance regression.
6. Resolving the tension here seems to have fallen on my lap, but
7. This is outside of my domain expertise, and
8. Nobody else seems to be willing to contribute to help

My thought in getting this merged is that it resolves the tension --- if someone is particularly invested in getting hole-fits working, the burden can fall on them to get it working in a tenable fashion.

[pepeiborra]

But you didn't address my point: can we couple this change to Wingman, in a way that disabling Wingman also disables this change?

I suggested a way to address this here: #1873 (comment)

[pepeiborra]

Cannot merge until the change in behaviour is coupled to Wingman

PR has changes requested

[isovector]

The necessary changes are implemented in #2029

[isovector]

@pepeiborra PTAL

[isovector]

CI is OOMing. I don't know why, and I've spent enough time fixing broken CI this week. Any ideas @wz1000 @berberman @Ailrun @pepeiborra ?

[pepeiborra]

@isovector I sent #2031 yesterday.

CI is very expensive to maintain, we support Cabal and Stack builds, a huge range of GHC versions, on 3 different platforms. It is a team effort and every help is welcome.

[isovector]

@pepeiborra thanks for taking a look, I really appreciate it! Apologies if I came across as grumpy in my last message!