Implement case slitting through ghc-mod #530

txsmith · 2018-04-20T16:36:00Z

This implements a command that calls ghc-mod's splits and parses the output into a WorkspaceEdit.

There's quite some code involved in parsing the output from ghc-mod. An example of the ouput:
11 0 11 13 "f (Just x) = \"Hello\"\NULf Nothing = \"Bye\""
This indicates the range of text (start line, start column, end line and end column) to replace and the new text to replace it with. Newlines are indicated by \NUL as per the default ghc-mod settings.
If there's a better way to parse this and turn it into a WorkspaceEdit with less code I'd love to know!
Now uses the new ghc-mod api!

I've added tests similar to those for "insert type". I'll also open a PR on vscode-hie-server so you can try it there for real.

Let me know what you think! I'm happy to follow up with improvements if needed.

Fixes #259

wz1000 · 2018-04-21T02:14:39Z

Thanks¸ this looks good.

However, the way ghc-mods splits function works, it forces reparsing and retypechecking of the module. But HIE has a module cache with the TypecheckedModule already on hand.

If getSrcSpanTypeForFnSplit is modified to accept a typechecked module, we could hand it the one we already have in HIE, making the whole process much, much faster.

Also, it would eliminate the need for parsing as you would get the SplitInfo data structure directly.

txsmith · 2018-04-21T07:14:02Z

Thanks, good point. Seems doable so I'll see if I can get something going there.

alanz · 2018-05-07T18:16:22Z

I am about to merge this, there is a minor fixup required for master, the name of one of the tests changed.

alanz · 2018-05-07T18:46:13Z

@txsmith I am getting the following when testing with GHC 8.4.2 (the default stack.yaml), in my branch https://github.com/alanz/haskell-ide-engine/tree/case-split-259

      runs the lint command
      runs the info command
      runs the type command
      runs the type command with an absolute path from another folder, correct params
EXCEPTION: splits:     Prelude.last: empty list
EXCEPTION: splits:     Prelude.last: empty list
      runs the casesplit command FAILED [1]
EXCEPTION: splits:     Prelude.last: empty list
EXCEPTION: splits:     Prelude.last: empty list
      runs the casesplit command with an absolute path from another folder, correct params FAILED [2]
HaRePlugin

So I am going to hand this back, please fix it. Sorry.

txsmith · 2018-05-07T18:54:28Z

I'll take a look. I was also still updating this to use the new split' in your ghc-mod fork so this is not really ready to be merged just jet ;)

alanz · 2018-05-07T18:56:55Z

Ok, thanks. I felt bad about forcing you to rebase the whole time, thought I would try a quick fixup and land.

txsmith · 2018-05-07T21:00:04Z

I probably made a mistake somewhere. Will investigate more tomorrow.

txsmith · 2018-05-09T07:44:18Z

See alanz/ghc-mod#10

wolverian · 2018-06-03T14:34:18Z

Did alanz/ghc-mod#11 fix the issue?

alanz · 2018-06-03T14:37:54Z

I have not really paid attention to what this PR does, so I do not know.

I did see a trivial error, which was fixed in ghc-mod, but it seems there may be other issues too?

wolverian · 2018-06-03T17:08:19Z

Right, I meant @txsmith, sorry.

txsmith · 2018-06-04T07:35:59Z

I believe I fixed the issue in alanz/ghc-mod#10. The problem was that case splitting never worked in ghc-mod for newer GHC versions in the first place.
Then there was also the trivial mistake that was fixed in alanz/ghc-mod#11

alanz · 2018-06-04T07:38:28Z

Ok, So I will merge alanz/ghc-mod#10 and we can see about getting this in.

Sorry to mess you around so much.

alanz · 2018-06-20T09:58:30Z

@txsmith Current hie uses ghc-mod with your patch, so you can get this one finished,

txsmith · 2018-06-22T13:52:15Z

I think this is ready for another review now!

alanz · 2018-06-22T14:13:55Z

src/Haskell/Ide/Engine/Plugin/GhcMod.hs

@@ -290,6 +293,66 @@ cmp a b
 isSubRangeOf :: Range -> Range -> Bool
 isSubRangeOf (Range sa ea) (Range sb eb) = sb <= sa && eb >= ea

+
+
+data DocumentPosition =


Shouldn't this be an instance of TextDocumentPositionParams

interface TextDocumentPositionParams { /** * The text document. */ textDocument: TextDocumentIdentifier; /** * The position inside the text document. */ position: Position; }

as per https://microsoft.github.io/language-server-protocol/specification

This is made available via haskell-lsp-types, see https://github.com/alanz/haskell-lsp/blob/406b9e0e795b5e9c2ae4a2ab29b2339c29fa0354/haskell-lsp-types/src/Language/Haskell/LSP/TH/DataTypesJSON.hs#L1008

wz1000 · 2018-06-22T20:15:52Z

src/Haskell/Ide/Engine/Plugin/GhcMod.hs

+splitCaseCmd' :: Uri -> Position -> IdeGhcM (IdeResult WorkspaceEdit)
+splitCaseCmd' uri position =
+  pluginGetFile "splitCaseCmd: " uri $ \path -> do
+    origText <- GM.withMappedFile path $ liftIO . T.readFile


This version of the file may differ from the version we have a TypecheckedModule for. Use newPosToOld and oldPosToNew to correlate positions across versions of the file.

wz1000 · 2018-06-22T20:16:43Z

src/Haskell/Ide/Engine/Plugin/GhcMod.hs

+    cachedMod <- getCachedModule path
+    case cachedMod of
+      ModuleCached checkedModule _ -> runGhcModCommand $ do
+        Just splitResult <- GM.splits' path (tcMod checkedModule) line column


A pattern match failure may cause HIE to crash

wz1000 · 2018-06-22T20:17:45Z

src/Haskell/Ide/Engine/Plugin/GhcMod.hs

+        Just splitResult <- GM.splits' path (tcMod checkedModule) line column
+        return $ splitResultToWorkspaceEdit origText splitResult
+      ModuleFailed errText -> return $ IdeResultFail $ IdeError PluginError (T.append "hie-ghc-mod: " errText) Null
+      ModuleLoading -> return $ IdeResultOk mempty


You should use withCachedModule, as that will automatically defer the action if the module is still loading.

Alternatively, you can defer it manually over here.

I've looked at this but could not find a nice way to defer the action while keeping it in IdeGhcM. To defer an IdeGhcM action we'd need to use IdeResponse', both of which do not work with withCachedModule. Furthermore, in the end all plugins seem to use CommandFunc which expects to return an IdeResult. I can't really find a sensible way to make these work together without creating a mess. Also note that other plugins like typeCmd don't defer either. Am I missing something?

txsmith · 2018-07-11T11:30:02Z

Thanks for your feedback! Finally had some time to get it fixed. Please take a look :)

wz1000 · 2018-07-11T12:18:33Z

Looks good. Thanks a lot.

txsmith mentioned this pull request Apr 20, 2018

Add command for case splitting haskell/vscode-haskell#70

Merged

txsmith mentioned this pull request Apr 22, 2018

Extend CaseSplit to provide a more convenient API to HIE alanz/ghc-mod#8

Merged

txsmith changed the title ~~Implement case slitting through ghc-mod~~ [WIP] - Implement case slitting through ghc-mod May 7, 2018

txsmith added 2 commits May 7, 2018 21:59

Implement case splitting through ghc-mod

97c72e5

Add tests for case splitting

77071f9

txsmith closed this May 7, 2018

txsmith reopened this May 7, 2018

Modify to use split'

920b2e3

Merge branch 'master' into case-split-259

8fa1b81

txsmith changed the title ~~[WIP] - Implement case slitting through ghc-mod~~ Implement case slitting through ghc-mod Jun 22, 2018

alanz reviewed Jun 22, 2018

View reviewed changes

wz1000 reviewed Jun 22, 2018

View reviewed changes

Use TextDocumentPositionParams instead of DocumentPosition

6fa0ba1

txsmith added 2 commits July 11, 2018 10:41

Return an empty WorkspaceEdit when there is no result from CaseSplit

9e659b4

Fix mapping of old and new document positions

023d9dc

alanz merged commit 883e552 into haskell:master Jul 11, 2018

txsmith deleted the case-split-259 branch July 11, 2018 17:56

alanz added this to the prehistory milestone Feb 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement case slitting through ghc-mod #530

Implement case slitting through ghc-mod #530

txsmith commented Apr 20, 2018 •

edited

Loading

wz1000 commented Apr 21, 2018

txsmith commented Apr 21, 2018

alanz commented May 7, 2018

alanz commented May 7, 2018

txsmith commented May 7, 2018

alanz commented May 7, 2018

txsmith commented May 7, 2018

txsmith commented May 9, 2018

wolverian commented Jun 3, 2018

alanz commented Jun 3, 2018

wolverian commented Jun 3, 2018

txsmith commented Jun 4, 2018

alanz commented Jun 4, 2018

alanz commented Jun 20, 2018

txsmith commented Jun 22, 2018

alanz Jun 22, 2018

wz1000 Jun 22, 2018

wz1000 Jun 22, 2018

wz1000 Jun 22, 2018

txsmith Jul 11, 2018

txsmith commented Jul 11, 2018

wz1000 commented Jul 11, 2018

Implement case slitting through ghc-mod #530

Implement case slitting through ghc-mod #530

Conversation

txsmith commented Apr 20, 2018 • edited Loading

wz1000 commented Apr 21, 2018

txsmith commented Apr 21, 2018

alanz commented May 7, 2018

alanz commented May 7, 2018

txsmith commented May 7, 2018

alanz commented May 7, 2018

txsmith commented May 7, 2018

txsmith commented May 9, 2018

wolverian commented Jun 3, 2018

alanz commented Jun 3, 2018

wolverian commented Jun 3, 2018

txsmith commented Jun 4, 2018

alanz commented Jun 4, 2018

alanz commented Jun 20, 2018

txsmith commented Jun 22, 2018

alanz Jun 22, 2018

Choose a reason for hiding this comment

wz1000 Jun 22, 2018

Choose a reason for hiding this comment

wz1000 Jun 22, 2018

Choose a reason for hiding this comment

wz1000 Jun 22, 2018

Choose a reason for hiding this comment

txsmith Jul 11, 2018

Choose a reason for hiding this comment

txsmith commented Jul 11, 2018

wz1000 commented Jul 11, 2018

txsmith commented Apr 20, 2018 •

edited

Loading