Use InitializeParams.rootUri for initial session setup #713

sureyeaah · 2020-07-28T03:12:11Z

Pass the config param to runLanguageServer so that we can extract the directory from InitializeRequest.

Fixes https://github.com/digital-asset/ghcide/issues/646

sureyeaah · 2020-07-28T03:13:24Z

Is there a better way to extract something from the config besides the InitCBConfig type class?
How do I test this?

lukel97 · 2020-07-28T10:20:11Z

src/Development/IDE/LSP/LanguageServer.hs

            ide <- getIdeState getNextReqId sendFunc (makeLSPVFSHandle lspFuncs) clientCapabilities
-                               withProgress withIndefiniteProgress
+                               withProgress withIndefiniteProgress cfg


I think the rootUri is available through LspFuncs as rootPath, which is a bit confusingly admittedly. Can we just pass that here instead? Instead of passing through the config.

lukel97 · 2020-07-28T10:20:14Z

exe/Main.hs

+        -- we use `Maybe InitializeParams` as the config type.
+        -- Change this if you want to parse InitializeRequest/DidChangeConfigurationNotification differently.
+    let onInitialConfiguration RequestMessage{_params=initParams} = Right $ Just initParams
+        onConfigurationChange = const $ Right Nothing


The config type is the settings that users can customise from the language clients, not something internal to the language server. haskell-lsp handles this a bit weirdly in that it lets us parse it ourselves from the initialisation message here but it's probably best to not use it as a mechanism for passing about information from the initialization request

lukel97 · 2020-07-28T10:28:40Z

To test this I would take a look at runInDir https://github.com/digital-asset/ghcide/blob/826e886adac32378cb8a9f1ad735ca9234806eab/test/exe/Main.hs#L3043-L3064 and probably make another version that doesn't set the --cwd flag and calls runSession in a specific directory, and seeing if it can load documents relative to the specified root directory

sureyeaah · 2020-07-29T06:44:11Z

In the tests, I open ghcide in dirA and run the session with root as dirB. Both these directories have different hie.yaml files. But regardless of whether I use InitializeParams.rootUri, the dirB/hie.yaml file is always used.

pepeiborra

Thank you for fixing this!

We already have code in place to parse and the InitializeParams, and in the interest of consistency it would be better to reuse it.

I've quickly put some code together to show how this works. Can you take a look at pepeiborra@9d94d82 and adapt your PR accordingly? Please check that it works as expected as I have not actually run it.

sureyeaah · 2020-07-29T09:47:41Z

@pepeiborra thanks I'll have a look. I followed @bubba 's advice and retrieved the path from LSPFuncs instead. Should I go back to the parsing approach?

Also, do you have any idea why my testing approach isn't working?

pepeiborra · 2020-07-29T10:07:58Z

@pepeiborra thanks I'll have a look. I followed @bubba 's advice and retrieved the path from LSPFuncs instead. Should I go back to the parsing approach?

If you can follow @bubba 's advice with minimal changes, then I believe that should be fine.

Also, do you have any idea why my testing approach isn't working?

I believe it doesn't work because ghcide calls findCradle see Main.hs:264

sureyeaah · 2020-07-29T10:19:16Z

Aaha that makes sense. So then what is the change in behaviour that using rootUri should give us (and that i can test for)?

lukel97 · 2020-07-29T11:57:28Z

@pepeiborra @sureyeaah just to make sure I understand this, the findCradle logic is independent of the root directory and only depends on the file being loaded. But findCradle I think is really misnamed, since it only finds the hie.yaml file. The root directory is used for the implicit cradle instead, as dir here:

https://github.com/digital-asset/ghcide/blob/765967d19be4ca64abc999173029a6bfe6aec113/session-loader/Development/IDE/Session.hs#L199-L205

So perhaps a file structure like:

- a/
  - Foo.hs
  - a.cabal
- b/
  - Bar.hs
  - b.cabal

Launch ghcide in a/, but set the rootDir to ../b. Then try to load Bar.hs and see what happens?

sureyeaah · 2020-07-29T14:12:02Z

Thanks @pepeiborra @bubba! Ready for review.

pepeiborra

Looks good to me, other than the pending simplification and the confirmation about the test

pepeiborra · 2020-07-29T14:23:20Z

src/Development/IDE/LSP/LanguageServer.hs

@@ -133,7 +133,7 @@ runLanguageServer options userHandlers onInitialConfig onConfigChange getIdeStat
        handleInit exitClientMsg clearReqId waitForCancel clientMsgChan [email protected]{..} = do

            ide <- getIdeState getNextReqId sendFunc (makeLSPVFSHandle lspFuncs) clientCapabilities
-                               withProgress withIndefiniteProgress
+                               withProgress withIndefiniteProgress lspFuncs


A bunch of the arguments to getIdeState are contained inside lspFuncs. Please simplify

Didn't realize. Fixed.

pepeiborra · 2020-07-29T14:24:06Z

test/exe/Main.hs

@@ -2781,9 +2782,22 @@ benchmarkTests =
        , Bench.name e /= "edit" -- the edit experiment does not ever fail
    ]

+-- | checks if we use InitializeParams.rootUri for loading session
+rootUriTests :: TestTree
+rootUriTests = testCase "use rootUri" . withoutStackEnv . runTest "dirA" "dirB" $ \dir -> do


Does this test fail without the new code?

Yeah it does.

lukel97

This LGTM! Thanks for getting this squared off

pepeiborra · 2020-09-02T17:43:03Z

rebase needed

sureyeaah · 2020-09-02T18:42:16Z

@pepeiborra rebased

…e#713) * add rootUri tests * use rootUri in session loader

lukel97 reviewed Jul 28, 2020

View reviewed changes

sureyeaah force-pushed the rooturi branch from 936827d to 6876b39 Compare July 29, 2020 07:19

pepeiborra suggested changes Jul 29, 2020

View reviewed changes

sureyeaah force-pushed the rooturi branch from 80947c3 to 29fa0d8 Compare July 29, 2020 14:17

pepeiborra approved these changes Jul 29, 2020

View reviewed changes

lukel97 approved these changes Jul 29, 2020

View reviewed changes

sureyeaah force-pushed the rooturi branch from eaa662f to 63b6a7d Compare September 2, 2020 18:21

sureyeaah added 2 commits September 3, 2020 00:09

add rootUri tests

38da486

use rootUri in session loader

b8d7418

sureyeaah force-pushed the rooturi branch from 63b6a7d to b8d7418 Compare September 2, 2020 18:39

lukel97 merged commit cb2fd66 into haskell:master Sep 3, 2020

pepeiborra pushed a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020

Use InitializeParams.rootUri for initial session setup (haskell/ghcid…

5d3f476

…e#713) * add rootUri tests * use rootUri in session loader

pepeiborra pushed a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020

Use InitializeParams.rootUri for initial session setup (haskell/ghcid…

f7bf4c2

…e#713) * add rootUri tests * use rootUri in session loader

pepeiborra pushed a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020

Use InitializeParams.rootUri for initial session setup (haskell/ghcid…

f201cda

…e#713) * add rootUri tests * use rootUri in session loader

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use InitializeParams.rootUri for initial session setup #713

Use InitializeParams.rootUri for initial session setup #713

sureyeaah commented Jul 28, 2020 •

edited

Loading

sureyeaah commented Jul 28, 2020 •

edited

Loading

lukel97 Jul 28, 2020

lukel97 Jul 28, 2020

lukel97 commented Jul 28, 2020

sureyeaah commented Jul 29, 2020 •

edited

Loading

pepeiborra left a comment

sureyeaah commented Jul 29, 2020 •

edited

Loading

pepeiborra commented Jul 29, 2020

sureyeaah commented Jul 29, 2020

lukel97 commented Jul 29, 2020

sureyeaah commented Jul 29, 2020

pepeiborra left a comment

pepeiborra Jul 29, 2020

sureyeaah Jul 29, 2020

pepeiborra Jul 29, 2020

sureyeaah Jul 29, 2020

lukel97 left a comment

pepeiborra commented Sep 2, 2020

sureyeaah commented Sep 2, 2020

Use InitializeParams.rootUri for initial session setup #713

Use InitializeParams.rootUri for initial session setup #713

Conversation

sureyeaah commented Jul 28, 2020 • edited Loading

sureyeaah commented Jul 28, 2020 • edited Loading

lukel97 Jul 28, 2020

Choose a reason for hiding this comment

lukel97 Jul 28, 2020

Choose a reason for hiding this comment

lukel97 commented Jul 28, 2020

sureyeaah commented Jul 29, 2020 • edited Loading

pepeiborra left a comment

Choose a reason for hiding this comment

sureyeaah commented Jul 29, 2020 • edited Loading

pepeiborra commented Jul 29, 2020

sureyeaah commented Jul 29, 2020

lukel97 commented Jul 29, 2020

sureyeaah commented Jul 29, 2020

pepeiborra left a comment

Choose a reason for hiding this comment

pepeiborra Jul 29, 2020

Choose a reason for hiding this comment

sureyeaah Jul 29, 2020

Choose a reason for hiding this comment

pepeiborra Jul 29, 2020

Choose a reason for hiding this comment

sureyeaah Jul 29, 2020

Choose a reason for hiding this comment

lukel97 left a comment

Choose a reason for hiding this comment

pepeiborra commented Sep 2, 2020

sureyeaah commented Sep 2, 2020

sureyeaah commented Jul 28, 2020 •

edited

Loading

sureyeaah commented Jul 28, 2020 •

edited

Loading

sureyeaah commented Jul 29, 2020 •

edited

Loading

sureyeaah commented Jul 29, 2020 •

edited

Loading