isovector / haskell-language-server

Integration point for ghcide and haskell-ide-engine. One IDE to rule them all.
Apache License 2.0
0 stars 2 forks source link

Feature Request: Expose split tactic #21

Open ProofOfKeags opened 3 years ago

ProofOfKeags commented 3 years ago
data A = ...
data B = ...
data C = ...

thing :: (A, B) -> C
thing = ...

expCurrentlyUnderImplementation :: C
expCurrentlyUnderImplementation = 
    let baz = case foo of
        Bar a -> _
    in thing baz

Consider the above scenario. I'd like to have a code action that fills the hole with (, ). Conceptually this is working backwards from the result rather than case splitting which is working forward from the arguments. Perhaps this should only kick in if the desired result has a single top level constructor.

If baz' type infer's to HLS' Any type, then no code action would be suggested.

isovector commented 3 years ago

PR welcome. Implementation sketch:

  1. Add a dcon for the code action https://github.com/isovector/haskell-language-server/blob/14e71db4a34cda595cba9ba5f9ab4a1422dd2744/plugins/tactics/src/Ide/Plugin/Tactic/TestTypes.hs#L11
  2. Hook it up https://github.com/isovector/haskell-language-server/blob/14e71db4a34cda595cba9ba5f9ab4a1422dd2744/plugins/tactics/src/Ide/Plugin/Tactic.hs#L113 to the const split tactic https://github.com/isovector/haskell-language-server/blob/14e71db4a34cda595cba9ba5f9ab4a1422dd2744/plugins/tactics/src/Ide/Plugin/Tactic/Tactics.hs#L154
  3. Customize the filter to only show it when there is a single dcon for the type of the hole https://github.com/isovector/haskell-language-server/blob/14e71db4a34cda595cba9ba5f9ab4a1422dd2744/plugins/tactics/src/Ide/Plugin/Tactic.hs#L90
  4. Write a test showing your code action comes up when there is a single dcon, and not when there are many: https://github.com/isovector/haskell-language-server/blob/14e71db4a34cda595cba9ba5f9ab4a1422dd2744/test/functional/Tactic.hs#L78-L87

Shouldn't take more than half an hour to get everything up and running.

WorldSEnder commented 3 years ago

I've worked a bit to make the TacticCommand more strongly typed as preparation for this, see https://github.com/WorldSEnder/haskell-language-server/commit/0bb120e7f85825a5310835569ceb3e85b4e65e6f. The idea behind all this work is to expose an option to split on each constructor (and maybe pattern synonyms) in case a type has multiple of those ~and merge it with the existing Intros for function types~.

Let me know what you think. @isovector I suppose part of the implementation could be replaced by using singletons but after reading

The singletons library requires GHC 8.8.1 or greater

I'm not so sure about compatibility.

isovector commented 3 years ago

Great to see that you're diving in, @WorldSEnder. Do you have further plans for these dependently typed things? As it stands I'm not convinced the machinery is worth the cost. Type-level stuff works best to prevent dangerous and silent issues, but right now the worst that can happen is we use an OccName of "" and accidentally do no work. It's the sort of thing a test would catch immediately, and even if it did make it to production, hell would not break loose.

Furthermore, as I understand LSP, our interface is extremely limited and it's unlikely we'll ever push more information into TacticsParam; there simply isn't any good way to fill that information in. As such, I doubt there will ever be anything other than the var_name (which admittedly should be a Maybe OccName instead of a String).

WorldSEnder commented 3 years ago

Not further than that, I'm not fully convinced either. Having to add a TacticCommand in 11 places instead of 5 is overhead, definitely. I will ponder the question the next few days while I'm on vacation.

ProofOfKeags commented 3 years ago

I think I'd like to understand the purpose of the type machinery added. Looking at the commit referenced, I'm not quite sure exactly what it is doing. That said, one of the things I've used it for is basically hacking GHC's constraint solver to solve arbitrary constraints in the programmer's code.

WorldSEnder commented 3 years ago

@ProofOfKeags currently some TacticCommands are using a OccName as an argument, for example Destruct takes as argument the name of the variable to destruct. But there are also tactics that don't have any arguments, i.e. they currently just throw away the argument they are given. What the commit does is