Add window/progress reporting for typechecking #1190

lukel97 · 2019-04-22T11:56:38Z

Todo

Add test
haskell-lsp[-types] upload
Update vscode-hie-server

Depends on a couple of tweaks with the VS code extension

lukel97 · 2019-04-22T11:58:15Z

hie-plugin-api/Haskell/Ide/Engine/PluginsIdeMonads.hs

+    lf <- lift $ asks ideEnvLspFuncs
+    withIndefiniteProgress' lf t f
+
+withIndefiniteProgress' :: MonadIO m => Maybe (Core.LspFuncs Config) -> T.Text -> m a -> m a


I couldn't get this to type check with withIndefiniteProgress = lift . withIndefiniteProgress, this is a workaround. Open to suggestions to make this nicer

fendor

Besides tests, I think it would be nice to have examples on how to integrate them into a plugin. E.g. some plugins might want to be able to report progress as well. Will that be possible?
Otherwise, great job, the diff is clean and easy to follow!

alanz · 2019-04-22T12:32:32Z

hie-plugin-api/Haskell/Ide/Engine/PluginsIdeMonads.hs

@@ -344,14 +344,15 @@ data IdeEnv = IdeEnv
  }

 -- | The class of monads that support common IDE functions, namely IdeM/IdeGhcM/IdeDeferM
-class Monad m => MonadIde m where
+class MonadIO m => MonadIde m where


I guess IO is needed to actually send the message back?

alanz · 2019-04-22T12:33:06Z

What changes are needed for the vscode extension? How portable is this to other clients?

lukel97 · 2019-04-22T13:05:11Z

What changes are needed for the vscode extension? How portable is this to other clients?

Mainly updating the client library and enabling the proposed features, haskell/vscode-haskell#150
Since there is a ClientCapabilities for the window/progress notifications, haskell-lsp won't send them unless the client declares it so it should be backwards compatible with other clients

lukel97 · 2019-04-22T13:06:57Z

Besides tests, I think it would be nice to have examples on how to integrate them into a plugin. E.g. some plugins might want to be able to report progress as well. Will that be possible?
Otherwise, great job, the diff is clean and easy to follow!

Yup! Since withIndefiniteProgress is inside IdeM, any plugin can just wrap their computations inside it to start a progress session. Are there any long running plugins at the moment we can add this to?

alanz · 2019-04-22T13:10:18Z

Are there any long running plugins at the moment we can add this to?

Possibly the liquid haskell one

lukel97 · 2019-04-22T13:26:51Z

@alanz @fendor an example:

haskell-ide-engine/src/Haskell/Ide/Engine/Plugin/Liquid.hs

Lines 123 to 125 in 0bdb86a

    
           let progTitle = "Running Liquid Haskell on " <> T.pack (takeFileName file) 
        
           tid <- withIndefiniteProgress progTitle $ 
        
             liftIO $ async $ generateDiagnosics cb uri file

lorenzo · 2019-04-22T18:47:37Z

hie-plugin-api/Haskell/Ide/Engine/PluginsIdeMonads.hs

+
+withProgress' :: MonadIO m => Maybe (Core.LspFuncs Config) -> T.Text -> ((Core.Progress -> m ()) -> m a) -> m a
+withProgress' lspFuncs t f =
+  let mWp = Core.withProgress <$> lspFuncs


wouldn't it be better to pass the actual Core.withProgress instead of getting it here?

And the passed in function could be of the form a -> IO (), so obviating the need for MonadIO too.

@lorenzo currently there is the possibility that lspFuncs returns Nothing, so it would have to be a Maybe, I think this is due to the existence of the JSON transport. I think this also asks the question, should we just give plugins full access to lsp funcs?

Either give full access, or make the progress reporting function available to them by default, as in my earlier comment. I think we already do something similar with accessibility to the VFS stuff. I would have to take a look though.

I am very much in favour of constraining what plugins/monads can do, so we do not get weird stuff happening

I've reworked the class in 8d1925d, so only the functions that require MonadIO have the constraint. @lorenzo is it ok if once JSONStdio is gone and there is the guarantee of LspFuncs (#1105) I can revisit this to directly pass it instead?

Sure, that makes sense

lorenzo · 2019-04-22T18:48:16Z

hie-plugin-api/Haskell/Ide/Engine/PluginsIdeMonads.hs

+  -- 'withProgress' @title f@ wraps a progress reporting session for long running tasks.
+  -- f is passed a reporting function that can be used to give updates on the progress
+  -- of the task.
+  withProgress :: T.Text -> ((Core.Progress -> m ()) -> m a) -> m a


It would be ore clear to name this withProgressReport

mpickering · 2019-04-22T22:15:21Z

One thing we noticed on IRC is that the type of withProgress needs to be modified so the continuation is of type Progress -> IO (). Otherwise you can't use these functions to pass the callback into GHC for example.

lukel97 · 2019-04-22T22:37:55Z

@mpickering see haskell/lsp#154

lorenzo

LGTM

mpickering · 2019-04-24T09:19:20Z

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

-    tid <- liftIO $ async $ generateDiagnosics cb uri file
+    let progTitle = "Running Liquid Haskell on " <> T.pack (takeFileName file)
+    tid <- withIndefiniteProgress progTitle $
+      liftIO $ async $ generateDiagnosics cb uri file


This isn't right. The progress report needs to go inside the call to async and if you do that, you'll discover that the types for withIndefiniteProgress are not quite right still..

You’re right, this completely slipped by me! I was testing it by adding a threadDelay 10000000…

mpickering · 2019-04-24T09:20:42Z

I don't think this is quite right yet. It's weird that the haskell-lsp functions are all parameterised over MonadIO but the other ones in haskell-lsp aren't. Once you fix those types to IO then you can provide a polymorphic interface in hie-plugin-api by using UnliftIO.

mpickering · 2019-04-24T10:25:06Z

See mpickering@3b41a98#diff-5c94e6cdb53262cd6976feddb9d4cedfR126

and mpickering@ed9fefa

and mpickering/haskell-lsp@f5e47d9

For how I think this should be fixed.

lukel97 · 2019-04-25T21:58:36Z

I don't think this is quite right yet. It's weird that the haskell-lsp functions are all parameterised over MonadIO but the other ones in haskell-lsp aren't. Once you fix those types to IO then you can provide a polymorphic interface in hie-plugin-api by using UnliftIO.

You're right: I got ahead of myself and was thinking of withProgress being used in the context of IdeM etc., I agree that haskell-lsp functions should remain in IO.
I'll update this PR and haskell-lsp to cherry-pick in your changes, and I imagine the MonadUnliftIO instance can be used in lots of other places in HIE

Co-authored-by: Matthew Pickering <[email protected]> Co-authored-by: Luke Lau <[email protected]>

…into window-progress

mpickering

Still looks like a few things need cleaning up.

hie-plugin-api/Haskell/Ide/Engine/PluginsIdeMonads.hs

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

mpickering · 2019-04-29T08:27:46Z

You should use the lifted-async package to fix the liquid haskell progress report rather than define the *IO variant.

lukel97 · 2019-04-29T18:30:23Z

@mpickering lifted-async worked a charm!

alanz · 2019-05-01T15:37:54Z

@bubba there are now conflicts here

…into window-progress

lukel97 · 2019-05-01T19:36:33Z

Will this be merged in time for #1227 ?

alanz · 2019-05-01T19:44:22Z

Probably not. I need to land the release tonight, as I am traveling the next two days, so can't predict time to pay attention.

…

On Wed, 01 May 2019, 21:36 Luke Lau, ***@***.***> wrote: Will this be merged in time for #1227 <#1227> ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1190 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADEAB6JTDGFBEWX6Y6E3B3PTHWMFANCNFSM4HHPINVQ> .

alanz · 2019-05-04T08:23:48Z

@bubba @mpickering Can you guys refresh this so we can merge?

mpickering

Nice job.

Add window/progress reporting for typechecking

b1cb9ac

lukel97 requested review from lorenzo, alanz and fendor April 22, 2019 11:56

lukel97 commented Apr 22, 2019

View reviewed changes

fendor reviewed Apr 22, 2019

View reviewed changes

alanz reviewed Apr 22, 2019

View reviewed changes

lukel97 added 2 commits April 22, 2019 14:24

Add withProgress

2b365a7

Report progress during Liquid Haskell

0bdb86a

lukel97 added 2 commits April 22, 2019 15:34

Update haskell-lsp to 0.10.0.0

cb32ff9

Add test for window/progress

1bf711f

lorenzo reviewed Apr 22, 2019

View reviewed changes

Rework MonadIde class

8d1925d

lukel97 requested review from lorenzo, alanz and fendor and removed request for lorenzo April 23, 2019 19:01

lukel97 marked this pull request as ready for review April 23, 2019 19:09

lorenzo approved these changes Apr 23, 2019

View reviewed changes

mpickering reviewed Apr 24, 2019

View reviewed changes

mpickering and others added 3 commits April 28, 2019 20:42

Update withProgress to match new haskell-lsp

7929ed9

Co-authored-by: Matthew Pickering <[email protected]> Co-authored-by: Luke Lau <[email protected]>

Merge branch 'master' of https://github.com/haskell/haskell-ide-engine …

2b3cde5

…into window-progress

Remove unused extensions

8db04f9

mpickering suggested changes Apr 28, 2019

View reviewed changes

hie-plugin-api/Haskell/Ide/Engine/PluginsIdeMonads.hs Outdated Show resolved Hide resolved

hie-plugin-api/Haskell/Ide/Engine/PluginsIdeMonads.hs Outdated Show resolved Hide resolved

src/Haskell/Ide/Engine/Plugin/Liquid.hs Outdated Show resolved Hide resolved

lukel97 added 2 commits April 28, 2019 22:22

Keep withProgress updater callback in IO

0b09502

Do up liquid haskell progress reporting

a0db9b5

Use lifted-async

1a3f467

Merge branch 'master' of https://github.com/haskell/haskell-ide-engine …

97a18d0

…into window-progress

lukel97 requested a review from mpickering May 2, 2019 22:31

mpickering approved these changes May 4, 2019

View reviewed changes

Merge branch 'master' into window-progress

c121eb2

alanz merged commit c121eb2 into haskell:master May 4, 2019

alanz added this to the 2019-05 milestone Jun 1, 2019

Add window/progress reporting for typechecking #1190

Add window/progress reporting for typechecking #1190

Uh oh!

Conversation

lukel97 commented Apr 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Todo

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fendor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alanz commented Apr 22, 2019

Uh oh!

lukel97 commented Apr 22, 2019

Uh oh!

lukel97 commented Apr 22, 2019

Uh oh!

alanz commented Apr 22, 2019

Uh oh!

lukel97 commented Apr 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukel97 Apr 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpickering commented Apr 22, 2019

Uh oh!

lukel97 commented Apr 22, 2019

Uh oh!

lorenzo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpickering commented Apr 24, 2019

Uh oh!

mpickering commented Apr 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukel97 commented Apr 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mpickering left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mpickering commented Apr 29, 2019

Uh oh!

lukel97 commented Apr 29, 2019

Uh oh!

alanz commented May 1, 2019

Uh oh!

lukel97 commented May 1, 2019

Uh oh!

alanz commented May 1, 2019 via email

Uh oh!

alanz commented May 4, 2019

Uh oh!

lukel97 commented Apr 22, 2019 •

edited

Loading

lukel97 Apr 22, 2019 •

edited

Loading

mpickering commented Apr 24, 2019 •

edited

Loading

lukel97 commented Apr 25, 2019 •

edited

Loading