upgrade lsp to 1.5 #3072

kokobd · 2022-08-01T06:34:06Z

This is a prerequisite of #3046

kokobd · 2022-08-03T10:02:20Z

I'm receiving a lot of logs like this if --verbose is enabled in ghcide-tests. And finally got OOM even with 64GB memory.

So disabling it for now

kokobd · 2022-08-04T04:30:32Z

I see. In lsp, there is a debug log, printing response body:
https://github.com/haskell/lsp/blob/8b63438313828a31ba1065ea865e504d532d093e/lsp/src/Language/LSP/Server/Control.hs#L224-L234
And in HLS, our logger sends a copy of log to LSP client:

haskell-language-server/exe/Main.hs

Lines 72 to 82 in 347a718

    
           withDefaultRecorder logFilePath Nothing minPriority $ \textWithPriorityRecorder -> do 
        
             let 
        
               recorder = cmapWithPrio pretty $ mconcat 
        
                   [textWithPriorityRecorder 
        
                       & cfilter (\WithPriority{ priority } -> priority >= minPriority) 
        
                   , lspMessageRecorder 
        
                       & cfilter (\WithPriority{ priority } -> priority >= Error) 
        
                       & cmapWithPrio renderDoc 
        
                   , lspLogRecorder 
        
                       & cfilter (\WithPriority{ priority } -> priority >= minPriority) 
        
                       & cmapWithPrio (renderStrict . layoutPretty defaultLayoutOptions)

Then it goes into infinite recursion... So we should not enable debug logs in lsp package itself.

This reverts commit 1fd6658.

michaelpj · 2022-08-08T11:02:16Z

Hmm, I thought I had convinced myself that wasn't a problem :| I'm not 100% sure what to do about this. It's nice to be able to log the responses from LSP, but we do indeed have a loop problem if we then forward them on like this.

kokobd · 2022-08-08T14:19:38Z

It's nice to be able to log the responses from LSP

And some other tests depend on the logs, I had to fix them one by one. This makes the PR harder than I initially thought.

Currently I'm working on hlint tests

This reverts commit 2ecd76d.

kokobd · 2022-08-09T12:50:44Z

Finally, the CI is almost green.

fendor

LGTM, outstanding work!

plugins/hls-floskell-plugin/src/Ide/Plugin/Floskell.hs

fendor · 2022-08-10T08:21:27Z

plugins/hls-rename-plugin/src/Ide/Plugin/Rename.hs

-instance Hashable Location
-instance Hashable Range
-instance Hashable Position
-instance Hashable UInt


Instances have been added to lsp-types-1.5. This is not a question or complaint, just a note for the next reviewer

hls-test-utils/src/Test/Hls/Util.hs

plugins/hls-floskell-plugin/src/Ide/Plugin/Floskell.hs

hls-test-utils/src/Test/Hls/Util.hs

michaelpj · 2022-08-10T09:56:47Z

ghcide/src/Development/IDE/Core/Shake.hs

@@ -706,6 +706,7 @@ shakeRestart recorder IdeState{..} vfs reason acts =
              backlog <- readTVarIO $ dirtyKeys shakeExtras
              queue <- atomicallyNamed "actionQueue - peek" $ peekInProgress $ actionQueue shakeExtras

+              -- this log is required by tests


michaelpj · 2022-08-10T09:58:50Z

ghcide/src/Development/IDE/LSP/LanguageServer.hs

@@ -106,8 +109,15 @@ runLanguageServer options inH outH defaultConfig onConfigurationChange setup = d
            , LSP.options = modifyOptions options
            }

+    let lspCologAction :: MonadIO m2 => Colog.LogAction m2 (Colog.WithSeverity LspServerLog)
+        lspCologAction = toCologActionWithPrio $ cfilter
+            (\msg -> priority msg >= Info)


This is to filter out the bad debug logs from lsp right? I'd like to come up with a better solution there. Can you open a ticket in lsp and link to it here, so we remember why we're doing this? At the moment this means that the low-level message logs won't even go to the file logging, which is bad since they're useful.

An alternative solution could be to change the log action in HLS that sends logs to the client to never send debug logs. But I guess that's also surprising :|

michaelpj · 2022-08-10T09:59:33Z

ghcide/src/Development/IDE/Types/Logger.hs

@@ -381,3 +380,14 @@ priorityToLsp =
    Info    -> MtInfo
    Warning -> MtWarning
    Error   -> MtError
+
+toCologActionWithPrio :: (MonadIO m, HasCallStack) => Recorder (WithPriority msg) -> LogAction m (WithSeverity msg)


👍 Worth checking if there's anywhere else doing this already? I can't remember if there is...

This is the first time co-log-core was introduced in HLS, so I think there isn't a existing function for this.

michaelpj · 2022-08-10T10:03:20Z

ghcide/test/exe/Main.hs

+                        pure (rope, range, replacement)
+                monadicIO $ forAllM gen
+                    $ \(rope, range, replacement) -> do
+                        newRope <- liftIO $ applyChange (toCologActionWithPrio $ cmapWithPrio LogVfs recorder) rope (TextDocumentContentChangeEvent (Just range) Nothing replacement)


Why not pass mempty :: LogAction Identity here? then you don't need IO, and you'll just not get hte log messages which you don't care about anyway. Then I think you can keep the old code.

Good advice!

* upgrade lsp to 1.5 * fix stack.yaml * try fix tests * disable verbose logging in ghcide * fix more tests in ghcide * fix floskell test * disable debug log in func-test * disable debug log in lsp itself * Revert "disable debug log in func-test" This reverts commit 1fd6658. * remove unused import * fix hls test utils * upgrade lsp in nix * fix func-tests * Revert "fix func-tests" This reverts commit 2ecd76d. * fix waitForDiagnosticsFromSourceWithTimeout * use Null as dummy message in waitForDiagnosticsFromSourceWithTimeout * simplify a test case * add comment about lsp bad logs

kokobd requested review from jneira, OliverMadine, berberman and pepeiborra as code owners August 1, 2022 06:34

kokobd force-pushed the kokobd/lsp-1.5 branch from 1713969 to 6840121 Compare August 1, 2022 06:36

upgrade lsp to 1.5

2f50ba7

kokobd force-pushed the kokobd/lsp-1.5 branch from 6840121 to 2f50ba7 Compare August 1, 2022 06:57

fix stack.yaml

ba518a2

kokobd marked this pull request as draft August 1, 2022 12:08

kokobd added 2 commits August 3, 2022 00:02

try fix tests

278d029

disable verbose logging in ghcide

c763e79

kokobd added 3 commits August 3, 2022 21:32

fix more tests in ghcide

ba9a761

fix floskell test

d8600d1

disable debug log in func-test

1fd6658

kokobd added 4 commits August 4, 2022 12:34

disable debug log in lsp itself

62815a1

Revert "disable debug log in func-test"

bb7c09e

This reverts commit 1fd6658.

Merge remote-tracking branch 'origin/master' into kokobd/lsp-1.5

4e2ca61

remove unused import

56a00c9

kokobd added 6 commits August 9, 2022 17:55

fix hls test utils

15a2320

Merge branch 'master' into kokobd/lsp-1.5

cf134bd

upgrade lsp in nix

51d13f2

fix func-tests

2ecd76d

Revert "fix func-tests"

ef84255

This reverts commit 2ecd76d.

fix waitForDiagnosticsFromSourceWithTimeout

c89ccfd

kokobd marked this pull request as ready for review August 9, 2022 12:50

kokobd requested a review from michaelpj as a code owner August 9, 2022 12:50

kokobd requested review from guibou and Ailrun as code owners August 9, 2022 12:50

fendor approved these changes Aug 10, 2022

View reviewed changes

kokobd commented Aug 10, 2022

View reviewed changes

plugins/hls-floskell-plugin/src/Ide/Plugin/Floskell.hs Show resolved Hide resolved

hls-test-utils/src/Test/Hls/Util.hs Show resolved Hide resolved

use Null as dummy message in waitForDiagnosticsFromSourceWithTimeout

bf25f01

kokobd force-pushed the kokobd/lsp-1.5 branch from c501042 to bf25f01 Compare August 10, 2022 09:27

michaelpj approved these changes Aug 10, 2022

View reviewed changes

kokobd added 2 commits August 12, 2022 06:19

Merge remote-tracking branch 'origin/master' into kokobd/lsp-1.5

94eba10

simplify a test case

7f97557

kokobd mentioned this pull request Aug 12, 2022

Inifinite recursion in logger haskell/lsp#447

Open

kokobd added 2 commits August 12, 2022 08:08

add comment about lsp bad logs

f565fb2

Merge branch 'master' into kokobd/lsp-1.5

81179cc

kokobd added the merge me Label to trigger pull request merge label Aug 12, 2022

mergify bot merged commit 2554981 into master Aug 12, 2022

kokobd deleted the kokobd/lsp-1.5 branch September 13, 2022 06:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

upgrade lsp to 1.5 #3072

upgrade lsp to 1.5 #3072

kokobd commented Aug 1, 2022 •

edited by gitpod-io bot

Loading

kokobd commented Aug 3, 2022

kokobd commented Aug 4, 2022

michaelpj commented Aug 8, 2022

kokobd commented Aug 8, 2022 •

edited

Loading

kokobd commented Aug 9, 2022

fendor left a comment

fendor Aug 10, 2022

michaelpj Aug 10, 2022

michaelpj Aug 10, 2022

michaelpj Aug 10, 2022

kokobd Aug 11, 2022

michaelpj Aug 10, 2022

kokobd Aug 11, 2022

upgrade lsp to 1.5 #3072

upgrade lsp to 1.5 #3072

Conversation

kokobd commented Aug 1, 2022 • edited by gitpod-io bot Loading

kokobd commented Aug 3, 2022

kokobd commented Aug 4, 2022

michaelpj commented Aug 8, 2022

kokobd commented Aug 8, 2022 • edited Loading

kokobd commented Aug 9, 2022

fendor left a comment

Choose a reason for hiding this comment

fendor Aug 10, 2022

Choose a reason for hiding this comment

michaelpj Aug 10, 2022

Choose a reason for hiding this comment

michaelpj Aug 10, 2022

Choose a reason for hiding this comment

michaelpj Aug 10, 2022

Choose a reason for hiding this comment

kokobd Aug 11, 2022

Choose a reason for hiding this comment

michaelpj Aug 10, 2022

Choose a reason for hiding this comment

kokobd Aug 11, 2022

Choose a reason for hiding this comment

kokobd commented Aug 1, 2022 •

edited by gitpod-io bot

Loading

kokobd commented Aug 8, 2022 •

edited

Loading