Skip to content

Starting to sketch out IDE-level plugin modularity #45

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 25 commits into from
Mar 16, 2020

Conversation

alanz
Copy link
Collaborator

@alanz alanz commented Feb 17, 2020

Address #25

Currently WIP

  • formatters
  • hover providers
  • code action providers
  • diagnostic providers
  • symbols
  • code lens provider
  • completions

@alanz
Copy link
Collaborator Author

alanz commented Feb 19, 2020

It seems diagnostics work by generating a rule, and custom RuleResult, so get generated via the graph update process.

@ndmitchell
Copy link
Collaborator

I'm afraid I don't see how this works. I thought the PartialHandlers couldn't be combined? But you seem to be using them as is? Where does the merging for code lens happen, for example?

@alanz
Copy link
Collaborator Author

alanz commented Mar 3, 2020

@ndmitchell this is in a very incomplete state at the moment, and I had to stop for a week. Working on it at the moment, will let you know when it makes sense to look at it again.

Comment on lines 51 to 77
-- | Map a set of plugins to the underlying ghcide engine. Main point is
-- IdePlugins are arranged by kind of operation, 'Plugin' is arranged by message
-- category ('Notifaction', 'Request' etc).
asGhcIdePlugin :: IdePlugins -> Plugin Config
asGhcIdePlugin mp =
mkPlugin executeCommandPlugins (Just . pluginCommands) <>
mkPlugin codeActionPlugins pluginCodeActionProvider <>
-- diagnostics from pluginDiagnosticProvider
mkPlugin hoverPlugins pluginHoverProvider <>
-- symbols via pluginSymbolProvider
mkPlugin formatterPlugins pluginFormattingProvider
-- completions
where
justs (p, Just x) = [(p, x)]
justs (_, Nothing) = []

ls = Map.toList (ipMap mp)

mkPlugin :: ([(PluginId, b)] -> t) -> (PluginDescriptor -> Maybe b) -> t
mkPlugin maker selector
= maker $ concatMap (\(pid, p) -> justs (pid, selector p)) ls


Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the key part of this approach.

@alanz
Copy link
Collaborator Author

alanz commented Mar 12, 2020

ping @bubba re e473612

@alanz alanz marked this pull request as ready for review March 15, 2020 11:34
@alanz
Copy link
Collaborator Author

alanz commented Mar 15, 2020

This is by no means polished, but provides a skeleton for how we should proceed. My intention is to merge it soon, so it provides a context for others to be able to build.

alanz added 18 commits March 15, 2020 11:39
Tests do not pass, but want to rebase, so marking a checkpoint.
Proof of concept, seems to work in big pieces, needs huge amount of cleanup.
Currently (for me) neither ghcide nor haskell-language-server can resolve a
simple project. Assume some sort of underlying hie-bios issue, will wait.
Demonstrated by adding missing pragmas derived from GHC error messages.
It piggy-backs existing args from ghcide, probably a bad idea
Starting to look pretty solid.
Currently suspect the range we get to format the whole file is wrong, second
time around.
Moving PluginCommand into hls.
Conclusion is that getDocumentContents is returning junk, doing the idempotent
test manually on vscode works as expected, but ends up with junk at the end of
the file in the test.

2020-03-12 21:11:05.79062259 [ThreadId 38] - Formatter.doFormatting: contents=
"{-# LANGUAGE NoImplicitPrelude #-}
module    Format where
foo   :: Int ->  Int
foo  3 = 2
foo    x  = x
bar   :: String ->   IO String
bar s =  do
      x <- return \"hello\"
      return \"asdf\"
"

2020-03-12 21:11:07.896114974 [ThreadId 7] - <--2--{"result":[{"range":{"start":{"line":0,"character":0},"end":{"line":9,"character":0}},"newText":
"{-# LANGUAGE NoImplicitPrelude #-}

module Format where

foo :: Int -> Int
foo 3 = 2
foo x = x

bar :: String -> IO String
bar s = do
  x <- return \"hello\"
  return \"asdf\"
"}],"jsonrpc":"2.0","id":1}

2020-03-12 21:11:07.897123428 [ThreadId 5] - ---> {"jsonrpc":"2.0","method":"textDocument/didChange","params":{"textDocument":{"version":0,"uri":"file:///home/alanz/mysrc/github/alanz/haskell-language-server/test/testdata/Format.hs"},"contentChanges":[{"text":
"{-# LANGUAGE NoImplicitPrelude #-}

module Format where

foo :: Int -> Int
foo 3 = 2
foo x = x

bar :: String -> IO String
bar s = do
  x <- return \"hello\"
  return \"asdf\"
","range":{"start":{"line":0,"character":0},"end":{"line":9,"character":0}}}]}}

-------------------------------------------------------

2020-03-12 21:11:07.899375044 [ThreadId 213] - Formatter.doFormatting: contents="{-# LANGUAGE NoImplicitPrelude #-}
module    Format where
foo   :: Int ->  Int
foo  3 = 2
foo    x  = x
bar   :: String ->   IO String
bar s =  do
      x <- return \"hello\"
      return \"asdf\"
"

2020-03-12 21:11:07.902320214 [ThreadId 7] - <--2--{"result":[{"range":{"start":{"line":0,"character":0},"end":{"line":9,"character":0}},"newText":"{-# LANGUAGE NoImplicitPrelude #-}

module Format where

foo :: Int -> Int
foo 3 = 2
foo x = x

bar :: String -> IO String
bar s = do
  x <- return \"hello\"
  return \"asdf\"
"}],"jsonrpc":"2.0","id":2}

2020-03-12 21:11:07.902812215 [ThreadId 5] - ---> {"jsonrpc":"2.0","method":"textDocument/didChange","params":{"textDocument":{"version":0,"uri":"file:///home/alanz/mysrc/github/alanz/haskell-language-server/test/testdata/Format.hs"},"contentChanges":[{"text":
"{-# LANGUAGE NoImplicitPrelude #-}

module Format where

foo :: Int -> Int
foo 3 = 2
foo x = x

bar :: String -> IO String
bar s = do
  x <- return \"hello\"
  return \"asdf\"
","range":{"start":{"line":0,"character":0},"end":{"line":9,"character":0}}}]}}

--------------------------------

hieCommand: haskell-language-server --lsp -d -l test-logs/hie-stack-8.6.5.yaml.log
HIE cache is warmed up

Format
  formatting provider
    formatting is idempotent FAILED [1]

Failures:

  test/functional/FormatSpec.hs:64:42:
  1) Format, formatting provider, formatting is idempotent
       expected: "{-# LANGUAGE NoImplicitPrelude #-}\n\nmodule Format where\n\nfoo :: Int -> Int\nfoo 3 = 2\nfoo x = x\n\nbar :: String -> IO String\nbar s = do\n  x <- return \"hello\"\n  return \"asdf\"\n"
        but got: "{-# LANGUAGE NoImplicitPrelude #-}\n\nmodule Format where\n\nfoo :: Int -> Int\nfoo 3 = 2\nfoo x = x\n\nbar :: String -> IO String\nbar s = do\n  x <- return \"hello\"\n  return \"asdf\"\nbar s = do\n  x <- return \"hello\"\n  return \"asdf\"\n"

  To rerun use: --match "/Format/formatting provider/formatting is idempotent/"

Randomized with seed 1814425400
The CompletionProvider handler type should probably be extended to include a
WithSnippets parameter, and the prefix.
It seems lsp-test is not applying edits properly, doing the same sequence in
vscode results in the correct result.
Otherwise the tests crash.
@ndmitchell
Copy link
Collaborator

The basic idea looks reasonable. I think a defaultDescriptor would be a good idea so people don't have to list all the types of plugins they don't provide. Why does the plugin id get passed to every method in the descriptor? That seemed unnecessary as if you really needed it you could just put it in the closure. Other than that, seems pretty sensible. I didn't review all the changes in detail, more just tried to get a high-level sense of the design.

@alanz
Copy link
Collaborator Author

alanz commented Mar 15, 2020

Why does the plugin id get passed to every method in the descriptor?

We use the plugin id when constructing a command name to be sent to the client. These end up being the concatenation of the server process id, the plugin id, and the exposed command.

For me the critical thing is that the plugin id can be configured from the Main module when assembling a list of plugins for the particular instantiation, so we do not have to worry about command name clashes. If there is some simpler way to do this, we can move in that direction.

This mirrors the approach we are currently taking in hie, so also makes it easier to port existing plugins.

@alanz alanz merged commit 17b150b into haskell:master Mar 16, 2020
pepeiborra added a commit that referenced this pull request Dec 27, 2020
* Test for #45

* Remove redundant symbols from imports

Fixes #45

* Update src/Development/IDE/LSP/CodeAction.hs

Co-Authored-By: Andreas Herrmann <[email protected]>

* Apply suggestions from code review

Co-Authored-By: Andreas Herrmann <[email protected]>

* Add regex-tdfa extra deps to ghc-lib build

* Fix for GHC 8.4 (error message prints qualified binding)

GHC ticket #14881 changed this to print identifiers unqualified

* dropBindingsFromImportLine: make total

Co-authored-by: Andreas Herrmann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants