Code actions for filling typed holes #69

jacg · 2019-09-14T18:17:11Z

Add code actions which replace typed holes, using GHC's "Relevant bindings include" hints.

mitchellwrosen · 2019-09-14T18:17:48Z

Just curious, would this involve parsing those things out of GHC's stringy output?

jacg · 2019-09-14T18:18:21Z

src/Development/IDE/LSP/CodeAction.hs

@@ -132,7 +132,8 @@ suggestAction contents Diagnostic{_range=_range@Range{..},..}
    | "Valid hole fits include" `T.isInfixOf` _message = let
      findSuggestedHoleFits :: T.Text -> [T.Text]
      findSuggestedHoleFits = extractFitNames . selectLinesWithFits . dropPreceding . T.lines
-      proposeHoleFit name = ("replace hole with " <> name, [TextEdit _range name])
+      proposeHoleFit name = ("replace hole `" <> holeName <>  "` with " <> name, [TextEdit _range name])
+      holeName = T.strip $ last $ T.splitOn ":" $ head . T.splitOn "::" $ head $ filter ("Found hole" `T.isInfixOf`) $ T.lines _message


I'm not at all proud of the pattern matching in places like holeName and surroundings. Any hints on how to do a better job of pattern matching on Text would be most welcome.

mitchellwrosen · 2019-09-14T18:18:26Z

(oh, oops, this is a PR, not a feature request)

ndmitchell · 2019-09-14T18:53:25Z

@mitchellwrosen yes, it involves parsing crap out of the GHC error message, which is sad, but effective. We turn on Unicode all the time to slightly reduce the amount of surface area we have to cover. We are also working with someone who is writing an error messages as ADT proposal, so as of GHC 8.12 or so we hope to avoid this string parsing.

jacg · 2019-09-14T21:18:17Z

We turn on Unicode all the time to slightly reduce the amount of surface area we have to cover.

Which probably explains the mismatch I had in quotes between the action name that I generate, and the action name that I have to search for in the test.

We are also working with someone who is writing an error messages as ADT proposal, so as of GHC 8.12 or so we hope to avoid this string parsing.

So for 8.6 and 8.4 we're stuck with string parsing ... and I'm guessing that some subtle differences between the crap that 8.4 and 8.6 spit out are responsible for the CI failures that have shown up. So now I have to get ghcide working on the same project with two different GHC versions simultaneously ... I'm so looking forward to that!

jacg · 2019-09-15T06:19:17Z

Given that parsing text is central to writing these actions, should we use some parser combinator library? If so, which one?

ndmitchell · 2019-09-15T07:55:59Z

It's unclear if a parsing library makes it easier or harder. It probably makes it more demanding about the precise layout, which may not be what we want. But someone would need to try it to know.

jacg · 2019-09-15T08:02:23Z

But at least there's no upfront objection to adding a parsing library as a dependency?

ndmitchell · 2019-09-15T08:09:36Z

Not from me. If the code looks better or more robust after it sounds like a good idea. I believe attoparsec would be the "default" parser lib nowadays

jacg · 2019-09-15T10:06:51Z

All the CI checks now pass, but the implementation simply comments out two tests which work on GHC 8.6 but fail on 8.4, because the latter doesn't provide hints about local bindings.

Is there a way of excluding these tests on GHC 8.4 only, without resorting to CPP?

ndmitchell · 2019-09-15T10:12:35Z

Use https://hackage.haskell.org/package/base-4.12.0.0/docs/System-Info.html#v:compilerVersion probably together with https://hackage.haskell.org/package/base-4.12.0.0/docs/Data-Version.html#v:makeVersion

hsenag · 2019-09-15T20:34:56Z

@ndmitchell btw, who's working on error messages as an ADT? I had some thoughts about it after digging through the GHC code a bit so would be interested to know what's planned.

ndmitchell · 2019-09-15T21:07:24Z

@hsenag @bgamari and @alpmestan I think (but I might be wrong)

cocreature

Looks great thank you, the version checks need to be fixed (not your fault, I only added the new macro after you’ve opened this PR) but apart from that it looks great!

cocreature · 2019-09-16T11:38:17Z

src/Development/IDE/LSP/CodeAction.hs


 suggestAction _ _ = []

+topOfHoleFitsMarker = if versionBranch compilerVersion >= [8,6]


versionBranch isn’t going to work when compiling against ghc-lib. I’ve added a MIN_GHC_API_VERSION macro in #73 which should always be used for checking the GHC API version.

OK, but this requires CPP, and CI rejected my first attempt because it used CPP, which hlint didn't like. Do I have to permit CPP for this module somewhere?

We have a whitelist of modules that are allowed to use CPP to make people a bit more cautious when using it. This is definitely a valid usecase, so you can just add your module to the whitelist https://github.com/digital-asset/ghcide/blob/7133f4192f8d11908e5fb4c3f52ca3af54a23459/.hlint.yaml#L78

How do I add the test Main to this whitelist?

I believe that Main should do the trick?

Hm not sure why this doesn’t work, maybe @ndmitchell has an idea?
edit: looks like you figured it out.

I'm struggling to get CPP to work in Main.hs of the tests:

It works in CodeAction.hs, and I've done exactly the same in Main.hs AFAICT, that is:

Add

{-# LANGUAGE CPP #-} #include "ghc-api-version.h"

at the top, then

#if MIN_GHC_API_VERSION(8,6,0)

I did need to add include-dirs: include to test-suite ghcide-tests in ghcide.cabal, for the #include to work at all in Main.hs.

Now I get:

$ cabal v2-test -w ghc-8.6.4 Build profile: -w ghc-8.6.4 -O1 In order, the following will be built (use -v for more details): - ghcide-0.0.2 (test:ghcide-tests) (file test/exe/Main.hs changed) Preprocessing test suite 'ghcide-tests' for ghcide-0.0.2.. Building test suite 'ghcide-tests' for ghcide-0.0.2.. test/exe/Main.hs:507:0: error: error: missing binary operator before token "(" #if MIN_GHC_API_VERSION(8,6,0) | 507 | #if MIN_GHC_API_VERSION(8,6,0) | ^ test/exe/Main.hs:521:0: error: error: missing binary operator before token "(" #if MIN_GHC_API_VERSION(8,6,0) | 521 | #if MIN_GHC_API_VERSION(8,6,0) | ^ `cc' failed in phase `C pre-processor'. (Exit code: 1)

What am I missing?

Oh I see what’s going on here. We rely on the MIN_VERSION pragmas which only exist if you depend on ghc which the test suite does not atm. Not quite sure what a nice solution would be but for now, just adding a dependency on ghc is not that bad.

Thanks, that solves the problem.

Where shall I document the ghc-dependence in the tests? Comment in the cabal file itself?

Yeah I think a comment in the cabal file should be sufficient, thank you!

cocreature · 2019-09-16T11:38:54Z

test/exe/Main.hs

+  , check "replace hole `_c` with globalInt"
+          "_a" "_b"        "_c"
+          "_a" "_b" "globalInt"
+  ] <> if versionBranch compilerVersion < [8,6] then [] else


Same here, this should also use MIN_GHC_API_VERSION.

cocreature · 2019-09-16T11:43:40Z

As for nicer ways of doing the string matching, I’m definitely not opposed to using a parser library (but I would prefer megaparsec over attoparsec). However, ime for these kind of grep-style tasks regex libraries actually work quite well and with regex-applicative(-text) we have a nice library that avoids the usual issue with regex quickly turning into a non-composable and unreadable mess.

Definitely not something for this PR but if someone wants to give it a shot I’d certainly be interested in whether it ends up being cleaner (I hope it does).

Useful if more than one hole appears on the same line. Not so useful if both of these holes are just `_` rather than `_name` (or more than one hole on the same line has the same `_name`): In which case perhaps some numbers could be attached to the action titles, to distinguish the holes. But I suspect that this would not be worth the effort.

These test hints about local bindings, whic GHC 8.4 does not provide.

jacg · 2019-09-16T14:17:47Z

It seems to have passed CI, though I am confused by the hlint output, which includes stuff like:

Warning: Can't find file "ghc-api-version.h" in directories
	./src/Development/IDE/Core
	.
  Asked for by: ./src/Development/IDE/Core/Compile.hs  at line 6 col 1
Warning: Can't find file "ghc-api-version.h" in directories
	./src/Development/IDE/GHC
	.
  Asked for by: ./src/Development/IDE/GHC/CPP.hs  at line 12 col 1
Warning: Can't find file "ghc-api-version.h" in directories
	./src/Development/IDE/GHC
…	.
  Asked for by: ./test/exe/Main.hs  at line 6 col 1

cocreature · 2019-09-16T14:35:00Z

Ah, it looks like we need to pass --cpp-include to HLint. We can do that in a separate PR.

cocreature

Thanks a lot @jacg!

alpmestan · 2019-09-16T17:55:51Z

@hsenag @bgamari and @alpmestan I think (but I might be wrong)

Correct, Ben and I have been discussing various approaches for this and I will in fact soon write a proposal for a design that we settled on with lots of useful inputs from Richard Eisenberg and Neil (Mitchell). One of its goals will be to make the life of IDE-style tooling devs a lot easier, essentially doing away with the need to parse GHC's messages, instead getting good old Haskell values to manipulate, inspect, use and print.

I can perhaps ping the ghcide team and you Ganesh, once I have something in a reviewable state, if some of you would like to give some feedback.

* Add code action for filling type holes * Incorporate hole name into action title Useful if more than one hole appears on the same line. Not so useful if both of these holes are just `_` rather than `_name` (or more than one hole on the same line has the same `_name`): In which case perhaps some numbers could be attached to the action titles, to distinguish the holes. But I suspect that this would not be worth the effort. * Add tests for fill-type-hole actions * Disable two tests on GHC 8.4 These test hints about local bindings, whic GHC 8.4 does not provide. * Replace compilerVersion with new MIN_GHC_API_VERSION macro

jacg commented Sep 14, 2019

View reviewed changes

jacg force-pushed the typed-hole-actions branch 2 times, most recently from d1d3dbb to 0eccd2e Compare September 14, 2019 22:45

jacg force-pushed the typed-hole-actions branch from 0eccd2e to 91eb2a2 Compare September 15, 2019 09:58

jacg force-pushed the typed-hole-actions branch from 91eb2a2 to 4908400 Compare September 15, 2019 11:10

cocreature reviewed Sep 16, 2019

View reviewed changes

jacg added 4 commits September 16, 2019 13:47

Add code action for filling type holes

263e39d

Add tests for fill-type-hole actions

895912e

Disable two tests on GHC 8.4

4303974

These test hints about local bindings, whic GHC 8.4 does not provide.

jacg force-pushed the typed-hole-actions branch 2 times, most recently from 510f478 to cec1323 Compare September 16, 2019 13:18

Replace compilerVersion with new MIN_GHC_API_VERSION macro

cebaadb

jacg force-pushed the typed-hole-actions branch from cec1323 to cebaadb Compare September 16, 2019 13:36

cocreature approved these changes Sep 16, 2019

View reviewed changes

cocreature merged commit 4fc09fa into haskell:master Sep 16, 2019

jacg deleted the typed-hole-actions branch October 4, 2019 08:05

maralorn mentioned this pull request Jan 1, 2021

Code action: Add missing signatures haskell/haskell-language-server#1099

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code actions for filling typed holes #69

Code actions for filling typed holes #69

jacg commented Sep 14, 2019

mitchellwrosen commented Sep 14, 2019

jacg Sep 14, 2019

mitchellwrosen commented Sep 14, 2019

ndmitchell commented Sep 14, 2019

jacg commented Sep 14, 2019

jacg commented Sep 15, 2019

ndmitchell commented Sep 15, 2019

jacg commented Sep 15, 2019

ndmitchell commented Sep 15, 2019

jacg commented Sep 15, 2019

ndmitchell commented Sep 15, 2019

hsenag commented Sep 15, 2019

ndmitchell commented Sep 15, 2019

cocreature left a comment

cocreature Sep 16, 2019

jacg Sep 16, 2019

cocreature Sep 16, 2019 •

edited

Loading

jacg Sep 16, 2019

cocreature Sep 16, 2019

cocreature Sep 16, 2019 •

edited

Loading

jacg Sep 16, 2019 •

edited

Loading

cocreature Sep 16, 2019

jacg Sep 16, 2019

cocreature Sep 16, 2019

cocreature Sep 16, 2019

cocreature commented Sep 16, 2019

jacg commented Sep 16, 2019

cocreature commented Sep 16, 2019

cocreature left a comment

alpmestan commented Sep 16, 2019 •

edited

Loading


		suggestAction _ _ = []

		topOfHoleFitsMarker = if versionBranch compilerVersion >= [8,6]

Code actions for filling typed holes #69

Code actions for filling typed holes #69

Conversation

jacg commented Sep 14, 2019

mitchellwrosen commented Sep 14, 2019

Choose a reason for hiding this comment

mitchellwrosen commented Sep 14, 2019

ndmitchell commented Sep 14, 2019

jacg commented Sep 14, 2019

jacg commented Sep 15, 2019

ndmitchell commented Sep 15, 2019

jacg commented Sep 15, 2019

ndmitchell commented Sep 15, 2019

jacg commented Sep 15, 2019

ndmitchell commented Sep 15, 2019

hsenag commented Sep 15, 2019

ndmitchell commented Sep 15, 2019

cocreature left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cocreature Sep 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cocreature Sep 16, 2019 • edited Loading

Choose a reason for hiding this comment

jacg Sep 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cocreature commented Sep 16, 2019

jacg commented Sep 16, 2019

cocreature commented Sep 16, 2019

cocreature left a comment

Choose a reason for hiding this comment

alpmestan commented Sep 16, 2019 • edited Loading

cocreature Sep 16, 2019 •

edited

Loading

cocreature Sep 16, 2019 •

edited

Loading

jacg Sep 16, 2019 •

edited

Loading

alpmestan commented Sep 16, 2019 •

edited

Loading