Use haskell-lsp for lsp types #696

DavidM-D · 2019-04-25T12:59:28Z

First step to pulling out our custom LSP implementation

Pull Request Checklist

Read and understand the contribution guidelines
Include appropriate tests
Set a descriptive title and thorough description
Add a reference to the issue this PR will solve, if appropriate
Add a line to the release notes, if appropriate

NOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with /AzurePipelines run to
trigger the build.

WORKSPACE

neil-da · 2019-04-25T13:02:24Z

WORKSPACE

@@ -413,13 +417,18 @@ hazel_repositories(
            hazel_hackage("shake", "0.17.8", "ade4162f7540f044f0446981120800076712d1f98d30c5b5344c0f7828ec49a2") +
            hazel_hackage("filepattern", "0.1.1", "f7fc5bdcfef0d43a793a3c64e7c0fd3b1d35eea97a37f0e69d6612ab255c9b4b") +
            hazel_hackage("terminal-progress-bar", "0.4.0.1", "c5a9720fcbcd9d83f9551e431ee3975c61d7da6432aa687aef0c0e04e59ae277") +
+            hazel_hackage("rope-utf16-splay" , "0.2.0.0", "83d1961bf55355da49a6b55d6f58d02483eff1f8e6df53f4dccdab1ac49e101d") +


Is there a ticket to add this to stackage?

Yup, on that now

neil-da · 2019-04-25T13:03:09Z

compiler/haskell-ide-core/src/Development/IDE/State/RuleTypes.hs

@@ -17,6 +17,7 @@ import           Development.IDE.Functions.Compile             (TcModuleResult,
 import qualified Development.IDE.Functions.Compile             as Compile
 import           Development.IDE.Functions.FindImports         (Import(..))
 import           Development.IDE.Functions.DependencyInformation
+import           Development.IDE.State.Shake


Why move this import?

I don't think I have moved it

neil-da · 2019-04-25T13:03:23Z

compiler/haskell-ide-core/src/Development/IDE/State/RuleTypes.hs

@@ -36,44 +36,44 @@ import           Development.IDE.Types.SpanInfo
 --   Foo* means Foo for me and Foo+

 -- | Kick off things
-type instance RuleResult OfInterest = IdeResult ()


Why rename IdeResult?

I haven't, IdeResult is what you produce from the rules and IdeReturn is what you get back when you return a rule. Better name suggestions would be welcome

cocreature

Nice, thank you!

cocreature · 2019-04-25T13:09:52Z

WORKSPACE

-        pkgs = packages,
+            ) +
+            # This is a special version of Haskell LSP without GPL dependencies
+            [ ("haskell-lsp", {"url": "https://github.com/DavidM-D/haskell-lsp/archive/{}.zip".format(HASKELL_LSP_COMMIT), "sha256": "7d706fbc1beb49a9345083d3eadb5e311936a2a8449e7765a65aa7d298dff9d6", "stripPrefix": "haskell-lsp-{}".format(HASKELL_LSP_COMMIT)})


What needs to happen for us to switch to upstream apart from haskell/lsp#70 being merged?

This PR needs to be resolved haskell/lsp#70, there's some LSP discussion going on, perhaps it could be resolved by the release of ropey, but our patches are pretty tiny at the moment. I think I'm also going to try and upstream our instances to make the diff even smaller

Can we fork this to the digital-asset namespace please, and use it from there.

cocreature · 2019-04-25T13:11:38Z

compiler/haskell-ide-core/src/Development/IDE/Functions/GHCError.hs

@@ -74,18 +76,19 @@ srcSpanToFilename (UnhelpfulSpan fs) = FS.unpackFS fs
 srcSpanToFilename (RealSrcSpan real) = FS.unpackFS $ srcSpanFile real

 srcSpanToLocation :: SrcSpan -> Location
-srcSpanToLocation src = Location (srcSpanToFilename src) (srcSpanToRange src)
+srcSpanToLocation src =
+  Location (LSP.filePathToUri $ srcSpanToFilename src) (srcSpanToRange src)


This should hopefully fix URIs on Windows 👍

cocreature · 2019-04-25T13:13:09Z

compiler/haskell-ide-core/src/Development/IDE/State/Shake.hs

@@ -134,8 +133,10 @@ instance Hashable Key where
 --   not propagate diagnostic errors through multiple phases.
 type IdeResult v = ([Diagnostic], Maybe v)

+type IdeReturn v = Maybe v


This seems like a pretty confusing type synonym, especially given the fact that we already have IdeResult. I would just inline this.

I think this is something that you'd reasonably want to change at a later date and it's all over the place

Tat would be true if we hadn't just introduced IdeReturn but reused IdeResult. Since we are changing it everywhere, might as well change it to the end result.

cocreature · 2019-04-25T13:28:52Z

compiler/haskell-ide-core/src/Development/IDE/Types/Diagnostics.hs

+  LSP.Diagnostic
+addLocation loc d =
+  d {
+    LSP._relatedInformation =


Suggested change

LSP._relatedInformation =

LSP._relatedInformation = Just $ LSP.List (rel loc : maybe [] toList (_relatedInformation d))

Implemented

neil-da

There seems to be a moving of invariants and types around IdeResult vs IdeReturn, so what was once encoded as ([Diagnostic], Maybe a) now gets moved to Maybe a through the type alias. Is that a necessary change? Or an unrelated change?

neil-da · 2019-04-25T13:34:02Z

compiler/haskell-ide-core/src/Development/IDE/State/Shake.hs

@@ -134,8 +133,10 @@ instance Hashable Key where
 --   not propagate diagnostic errors through multiple phases.
 type IdeResult v = ([Diagnostic], Maybe v)

+type IdeReturn v = Maybe v


Tat would be true if we hadn't just introduced IdeReturn but reused IdeResult. Since we are changing it everywhere, might as well change it to the end result.

neil-da · 2019-04-25T13:35:34Z

daml-foundations/daml-ghc/src/Development/IDE/State/API/Testing.hs

+        Map.lookup (filePathToUri fp)
+
+-- | These functions live in the test framework rather that further down the stack because
+--   you really should be deconstructing LSP diagnostic stores in this manner bar when testing


neil-da · 2019-04-25T13:35:43Z

daml-foundations/daml-ghc/src/Development/IDE/State/API/Testing.hs

+-- | These functions live in the test framework rather that further down the stack because
+--   you really should be deconstructing LSP diagnostic stores in this manner bar when testing
+fromStore :: D.StoreItem -> [D.Diagnostic]
+fromStore (D.StoreItem _ ds) = join $ map toList $ Map.elems ds


neil-da · 2019-04-25T13:35:51Z

daml-foundations/daml-ghc/src/Development/IDE/State/API/Testing.hs

+fromStore (D.StoreItem _ ds) = join $ map toList $ Map.elems ds
+
+allDiagnostics :: D.DiagnosticStore -> [D.Diagnostic]
+allDiagnostics = join . map fromStore . Map.elems


... require and what they return

DavidM-D · 2019-04-25T13:52:01Z

There seems to be a moving of invariants and types around IdeResult vs IdeReturn, so what was once encoded as ([Diagnostic], Maybe a) now gets moved to Maybe a through the type alias. Is that a necessary change? Or an unrelated change?

It's (somewhat) necessary. We no longer store errors in the shake graph, we now store them the haskell-lsp data structure. I was going to write some code to handle this, but I realised that we weren't actually using the error return values, so I got rid of them.

neil-da · 2019-04-25T13:56:45Z

That makes sense, but it feels like we're in an odd middle ground. Before IdeResult captured and abstracted over two levels of type constructor - now it abstracts over one, and is more complex than what it abstracts. I'd suggest:

Ditch IdeResult in all the type functions, e.g. type instance RuleResult GetModificationTime = UTCTime and then shove the Maybe in where you need it. I think this is probably a simplification over what you have now. It's possible we should have done this as a preparatory patch anyway.

neil-da · 2019-04-25T14:48:43Z

compiler/haskell-ide-core/src/Development/IDE/Types/Diagnostics.hs

+        hasSeriousErrors (List a) = any ((/=) uri . _uri . _location) a
+        uri = LSP.filePathToUri fp
+
+addDiagnostics ::


What are the semantics of this function? How does it remove old diagnostics that are no longer required? What if the Parser had a warning, and then that warning goes away - what's the flow?

neil-da · 2019-04-25T19:46:40Z

I sliced off a chunk of this commit and simplified it in #708

DavidM-D requested a review from neil-da April 25, 2019 13:02

neil-da reviewed Apr 25, 2019

View reviewed changes

cocreature approved these changes Apr 25, 2019

View reviewed changes

neil-da reviewed Apr 25, 2019

View reviewed changes

DavidM-D added 25 commits April 25, 2019 15:41

Added GPL-less LSP

b25c2d7

Upgraded to haskell-lsp version with NFData

9edeb5d

Replaced some of our data types with haskell-lsp versions

8618aa1

Replace severity with the LSP implementation

9f34198

Removed diagnostics

6145302

Add lenses handle serious errors

d9402e7

Seperate out diagnostics

49a469f

Making the shake graph only store results

1c41137

PR feedback

10015ac

Migrating to Maybe instead of returning extra values

871f810

Added the IdeReturn type to distinguish between the data rules...

e0f2763

... require and what they return

Fixed DAML rules

14e175d

WIP Fixing tests

0871e43

Fixed tests

41141d6

Fixed handle

83b2757

Converted daml-ghc to the LSP data types

d75a8bd

Changed file diagnostics to be more in line with haskell-lsp

c8fc50c

Converted LSP methods

32871ab

Added missing dep

e8f519f

Finish moving to return values being Maybe

98cfec7

Remove Either from rules apart from in legacy rules

9d8c855

Removed useless comment

4956d0b

Hlint fix

836303e

Cleaned up build rules

1ec59db

Simplified code + removed unneeded dependency

64f436b

DavidM-D force-pushed the new-lsp branch from 9aafd8a to 64f436b Compare April 25, 2019 13:42

Fixed typo

37d1b21

DavidM-D added 3 commits April 25, 2019 16:04

Cleanup

b3991da

Added extra args to internal github deps

dc5dde2

Used internal haskell-lsp fork

c191718

neil-da reviewed Apr 25, 2019

View reviewed changes

neil-da mentioned this pull request Apr 25, 2019

Simplify IDE rules #708

Merged

DavidM-D closed this Apr 27, 2019

dasormeter deleted the new-lsp branch September 4, 2019 17:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use haskell-lsp for lsp types #696

Use haskell-lsp for lsp types #696

DavidM-D commented Apr 25, 2019

neil-da Apr 25, 2019

DavidM-D Apr 25, 2019

neil-da Apr 25, 2019

DavidM-D Apr 25, 2019

neil-da Apr 25, 2019

DavidM-D Apr 25, 2019

cocreature left a comment

cocreature Apr 25, 2019

DavidM-D Apr 25, 2019

neil-da Apr 25, 2019

cocreature Apr 25, 2019

cocreature Apr 25, 2019

DavidM-D Apr 25, 2019

neil-da Apr 25, 2019

cocreature Apr 25, 2019

DavidM-D Apr 25, 2019

neil-da left a comment

neil-da Apr 25, 2019

neil-da Apr 25, 2019

neil-da Apr 25, 2019

neil-da Apr 25, 2019

DavidM-D commented Apr 25, 2019

neil-da commented Apr 25, 2019

neil-da Apr 25, 2019

neil-da commented Apr 25, 2019

	LSP._relatedInformation =
	LSP._relatedInformation = Just $ LSP.List (rel loc : maybe [] toList (_relatedInformation d))

Use haskell-lsp for lsp types #696

Use haskell-lsp for lsp types #696

Conversation

DavidM-D commented Apr 25, 2019

Pull Request Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cocreature left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-da left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidM-D commented Apr 25, 2019

neil-da commented Apr 25, 2019

Choose a reason for hiding this comment

neil-da commented Apr 25, 2019