Use NormalizedFilePath and adapt types of haskell-lsp-0.21 #479

jneira · 2020-03-11T21:08:55Z

This should wait to a haskell-lsp-types release with Add NormalizedFilePath from ghcide lsp#224 included but i wanted to implement the ghcide adaption in case we have to change the actual haskell-lsp pr
To keep ghcide special case for empty paths i had to duplicate fromNormalizedFilePath and toNormalizedFilePath and make a newtype for NormalizedPath to override the IsString instance. It needs hiding those definitions from Haskell.LSP.Types in several modules. Let me know if you would prefer another solution (like remove the IsString instance? use functions versions with ' to avoid hidings?)
I am little bit worried with performance and i am not sure how to measure it for the pr. Assuming that test suite duration can be an aproximation, i've ran it with master and the pr in azure several times:

#20200311.20 master c74e9b5 All 233 tests passed (322.79s)
#20200311.24 master c74e9b5 All 233 tests passed (340.53s)
#20200311.27 master c74e9b5 All 233 tests passed (321.96s)
#20200311.32 master c74e9b5 All 233 tests passed (280.83s)

#20200311.23 norm-filepath d718658 All 233 tests passed (276.39s)
#20200311.25 norm-filepath d718658 All 233 tests passed (312.28s)
#20200311.28 norm-filepath d718658 All 233 tests passed (327.65s)
#20200311.30 norm-filepath d718658 All 233 tests passed (328.59s)

#20200311.22 norm-fp-nowrap 04d4679 All 233 tests passed (295.12s)
#20200311.26 norm-fp-nowrap 04d4679 All 233 tests passed (298.30s)
#20200311.29 norm-fp-nowrap 04d4679 All 233 tests passed (319.41s)
#20200311.31 norm-fp-nowrap 04d4679 All 233 tests passed (330.24s)

At least the pr times are similar to master.

jneira · 2020-03-13T10:44:37Z

I am getting an error statring ghcide with this branch:

[Error - 11:43:14] Request textDocument/documentSymbol failed.
  Message: Error when running Shake build system:
  at action, called at exe\\Main.hs:101:80 in main:Main
  at apply, called at src\Development\IDE\Core\Shake.hs:532:63 in ghcide-0.1.0-KJEoEjwtfuKt9utY5Hxte:Development.IDE.Core.Shake
* Depends on: TypeCheck; D:\ws\haskell\ghcide\src\Development\IDE\Core\IdeConfiguration.hs
  at apply, called at src\Development\IDE\Core\Shake.hs:532:63 in ghcide-0.1.0-KJEoEjwtfuKt9utY5Hxte:Development.IDE.Core.Shake
* Depends on: GetParsedModule; D:\ws\haskell\ghcide\src\Development\IDE\Core\IdeConfiguration.hs
  at apply, called at src\Development\IDE\Core\Shake.hs:532:63 in ghcide-0.1.0-KJEoEjwtfuKt9utY5Hxte:Development.IDE.Core.Shake
* Depends on: GhcSession; D:\ws\haskell\ghcide\src\Development\IDE\Core\IdeConfiguration.hs
  at apply, called at src\Development\IDE\Core\Shake.hs:532:63 in ghcide-0.1.0-KJEoEjwtfuKt9utY5Hxte:Development.IDE.Core.Shake
* Depends on: LoadCradle; D:\ws\haskell\ghcide\hie.yaml
* Raised the exception:
thread killed

  Code: -32603

After the error vscode seems to work fine, though 🤔

cocreature

Looks great, thanks a lot! I’d like to go for the '-suffixed version instead of hiding the import.

As for the error you are seeing, are you sure this was introduced by this branch? It looks like somehow the exception we send to kill a thread if something changed is interpreted as an actual error. I’m not sure I’ve seen this before but it also doesn’t look related to your PR. Maybe this is something windows specific?

As for performance tests, I expect that there might be too much noise in the test suite for it to be a reliable test and all the tests have a small number of files whereas the performance here matters more for a large number of files. I would recommend to run a shake profile on hover, like in #254 (comment) and compare that with and without this PR.

src/Development/IDE/Types/Location.hs

jneira · 2020-03-17T08:22:12Z

Thanks for reviewing this

As for the error you are seeing, are you sure this was introduced by this branch? It looks like somehow the exception we send to kill a thread if something changed is interpreted as an actual error. I’m not sure I’ve seen this before but it also doesn’t look related to your PR. Maybe this is something windows specific?

I've tested again in my windows 10 and the error is not reproduced. 👍

As for performance tests, I expect that there might be too much noise in the test suite for it to be a reliable test and all the tests have a small number of files whereas the performance here matters more for a large number of files. I would recommend to run a shake profile on hover, like in #254 (comment) and compare that with and without this PR.

Again the shake profiling files are generated in windows 10 (it did not work in my other windows env, to investigate). I am just generating them for both versions.

jneira · 2020-03-17T23:31:37Z

I've tested to open all the files under src\Development\Ide\Core,making hovers, change an identifier and applying a quick fix with results quite similar between the pr and master:

This pr:

Master (8b328bb)

jneira · 2020-03-19T00:47:38Z

@cocreature i've renamed the ghcide specific newtype and functions adding '.

src/Development/IDE/Types/Location.hs

jneira · 2020-03-19T22:17:28Z

@cocreature It looks cleaner without the IsString instance and the newtype.
We have to wait for the lsp release to remove the git dependencies from stack.yaml's though

cocreature

Great, looks good to me know, thank you very much! I’ll hold off on merging this until there is a haskell-lsp release.

jneira · 2020-03-20T15:35:37Z

Mmm i cant reproduce in windows the error shown in ci with stack test --ghc-options=-Werror --stack-yaml ghc88.yaml for linux:

[4 of 5] Compiling Rules

/home/vsts/work/1/s/exe/Rules.hs:22:1: error: [-Wunused-imports, -Werror=unused-imports]
    The import of ‘Development.IDE.Types.Location’ is redundant
      except perhaps to import instances from ‘Development.IDE.Types.Location’
    To import instances alone, use: import Development.IDE.Types.Location()
   |
22 | import           Development.IDE.Types.Location (fromNormalizedFilePath)
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

If i remove the import the build fails in local. 🤔

cocreature · 2020-03-20T15:39:34Z

Note that azure tests merge commits rather than your branch. So if you get an error that you cannot reproduce locally it might just be that you need to rebase on master.

jneira · 2020-03-20T15:45:40Z

Just i had rebased master in the commit force pushed before the first ci error, i'll try again

jneira · 2020-03-22T00:21:14Z

@cocreature i've used the recent released version of haskell-lsp but i have to make some changes to adapt ghcide code to other type changes. I hope they will be sensible

jneira · 2020-03-22T15:40:19Z

I've corrected the stack.yaml used in the key of azure cache ci.
~~I think the build errors were related with that.~~
EDIT: The build for ghc-8.8 in linux has failed again after the correction 🤔

I want to note the drawback of not override the IsString instance of NormalizedFilePath:

unitTests :: TestTree
unitTests = do
  testGroup "Unit"
     [ testCase "empty file path does NOT work with the empty String literal" $
         uriToFilePath' (fromNormalizedUri $ filePathToUri' "") @?= Just "."
     , testCase "empty file path works using toNormalizedFilePath'" $
         uriToFilePath' (fromNormalizedUri $ filePathToUri' (toNormalizedFilePath' "")) @?= Just ""
     ]

jneira · 2020-03-22T20:51:05Z

I've added stack test --ghc-options="-Wall" --no-run-tests to try to reproduce the error
The import of ‘Development.IDE.Types.Location’ is redundant...

It is reproduced in windows and linux but only for ghc88.yaml. I cant reproduce it in my windows using the exactly same stack invocation and stack88.yaml files. I've just double checked masteris rebased.

jneira · 2020-03-22T23:08:10Z

ed0e4b8 has fixed the build for ghc-8.8.2, but it worked for other ghc versions in ci and in other envs for that ghc-version before so 🤷‍♂

cocreature · 2020-03-23T08:05:33Z

@jneira Ah yeah, GHC 8.8 has a different/improved unused imports checker. I noticed that when I was trying to upgrade the DAML codebase as well.

cocreature

Awesome, thanks!

wip/multi-rebase-az-on-fork-point --- Use tasty-rerun to allow rerunning failed tests only (haskell#484) Use by stack --stack-yaml=stack84.yaml test --test-arguments "--rerun" --- Working on Plugin support for haskell-language-server (haskell#477) * Working on Plugin support for hls Fix PluginCommand reply type for executeCommand needs * Remove PluginCommand It will move to haskell-language-server instead * Make azure CI hlint happy By removing explicit OverloadedStrings pragma, in favour of the one already enabled in the cabal file. * Remove unneeded 'do' * Fix more nits from review --- Expose underlying hover and gotoDefinition handlers (haskell#490) For use in haskell-language-server --- Add azure job for windows and restructure azure config files (haskell#475) * Add azure job for windows and ghc-8.6 * Trigger build in branches starting with azure * Add other valid stack.yaml to windows using matrix * Using azure task Cache@2 instead CacheBeta@0 * Ignore tests in windows for now * Install happy standalone to avoid spurious build error * Add comment about installing happy * Use matrix names more consistent with existing ones * Enable build using ghc-8.8.2 for windows * Ignore .vscode dir * Use templates and matrix in linux job --- Pass correct SafeHaskell information to mkIfaceTc (haskell#489) Seems like this was never implemented the first time, woops! Fixes #424 --- Make keywords customizable (haskell#493) This is necessary for DAML where we have additional keywords (e.g. `with`) and other keywords don’t exist (e.g. `foreign`). I considered using just a modify function `[T.Text] -> [T.Text]` but decided against it in the end. The list is small enough and I think it’s much easier to understand with an explicit enumeration (and you can just show the field in the options which is often convenient for debugging). --- Allow disabling the simplifier in compileModule (haskell#496) It causes problems for our conversion to DAML-LF atm and isn’t necessary (since we don’t have template Haskell) so let’s make it configurable. I originally thought we could just copy paste all of compileModule to DAML but it turns out that this pull in too much stuff that I don’t want to see diverge from `ghcide` so I abandoned that idea. --- Improve completion contexts (haskell#495) The completion context determines whether we show completions for types or completions for values. This is done by looking at the parsed module. This PR fixes two things: 1. While we only use the parsed module for getting the context previously we got the parsed module out of the typechecked module. This means that if you have a module that parses but doesn’t typecheck, we will use the parsed module at the point where it last typechecked which is out of date and produces incorrect (or just no) contexts. 2. When we could not find a context, we defaulted to assuming we are in a value context. Especially in combination with 1 but also just in general, this is rather annoying. If we aren’t sure we should show the user everything we have and not filter out some completions. Filtering out completions interacts particularly badly with VSCode’s default behavior of accepting the first completion when you press return. --- Treat alex the same way as happy to avoid CI issues (haskell#497) --- Use NormalizedFilePath and adapt types of haskell-lsp-0.21 (haskell#479) * Use custom version of h-l-t * Use normalized path functions from h-l-t * Restore empty path corner case * Create a wrapper over NFP to override IsString * Use maybe instead fromMaybe * Use patched version of lsp-types in all yaml files * Remove unused import * Rename specific NormalizeFilePath to NormalizeFilePath' * Remove specific newtype and IsString instance * Use released haskell-lsp-0.21 * Adapt to type changes of haskell-lsp-0.21 * Add tags field to CompletionItem * Fix test case about empty file path * Correct stack.yaml used in azure ci cache * Build ghcide including tests in windows azure ci * Qualify haskell-lsp modules to avoid name clashes --- Fix emptyPathUri (haskell#502) * Fix emptyPathUri * Remove platform dependency --- Support for interface files (haskell#457) * Rules for loading interface files * Typechecking with interface files * Add a note in the README about the optimal project setup * Improve support for hs-boot files The branch was failing to load GHC because the module graph was missing edges between a .hs file and its .hs-boot file. This means the .hs-boot file was getting added into the HPT after the .hs file which led to confusing errors about variables being out of scope. The fix is to maintain a map from hs-boot to hs files and then add an edge for this case when calling `transitiveDependencies`. Also tidy up some code in setupEnv which I assume was attempting to fix this but in an incorrect manner. Add the -boot suffix when looking for hi-boot files. For some reason, the `hi` path is not set to the right thing for `hs-boot` files. I don't know why not perhaps it is ok to use an existing `.hi` file in place of an `hs-boot` file. More investigation needed. My experience is that GHC is quite bad a recompilation avoidance for hs-boot files anyway. For example: https://gitlab.haskell.org/ghc/ghc/issues/17434 Add the -boot suffix when writing interface files * Generate .hi and .hie files during type checking * Refactor GetModIface to not retain TypeChecked module in memory This improves memory performance on a cold cache. * Trailing whitespace * Turn debug log messages into diagnostics * Implement "hie" files for ghc-8.6.5 This means that the .hi files patch can also be used with 8.6.5 * Add tests for hover/definition on imported symbols * hlints * Generate .hie files when missing * Fix subtle bug in setDefaultHieDir * Simplify optimal project setup in README * Move interface loading diagnostics behind --test flag Reusing the --test flag for this seems harmless, I cannot justify introducing a new flag * Avoid expensive interface file generation for files of interest * avoid redundant arguments (thanks Moritz K) * qualify a DAML only comment * Skip module source when generating hie file thanks Moritz Kiefer for noting that we don't care for the generated .hie files to embed module sources * runGhcEnv <-> evalGhcEnv * Apply suggestions from code review Thanks Moritz Kiefer Co-Authored-By: Moritz Kiefer <[email protected]> * Add suggested Show instance Co-Authored-By: Matthew Pickering <[email protected]> * Use Control.Exception.Safe This is to avoid accidentally capturing asynchronous exceptions * Rename atomicFileUpdate * Fix a flaky test We have to be careful with module naming in tests to avoid interference of .hi files across tests * Undo formatting of D.IDE.GHC.Util * follow changes in master Co-authored-by: Matthew Pickering <[email protected]> Co-authored-by: Moritz Kiefer <[email protected]> --- Detect ghc mismatch (haskell#462) * Detect ghc version mismatches * Add ghc-check to stack extra deps * ghc-check: explicit libdir and delay version error --- Expose codeAction and codeLens providers for haskell-language-server (haskell#499) * Expose codeAction and codeLens providers for haskell-language-server Also tweak the code action reply type to generate well-formed JSON * Expose moduleOutline for symbolProvider in hls too * Revert to using [CAResult] rather than List CAResult

…hcide#479) * Use custom version of h-l-t * Use normalized path functions from h-l-t * Restore empty path corner case * Create a wrapper over NFP to override IsString * Use maybe instead fromMaybe * Use patched version of lsp-types in all yaml files * Remove unused import * Rename specific NormalizeFilePath to NormalizeFilePath' * Remove specific newtype and IsString instance * Use released haskell-lsp-0.21 * Adapt to type changes of haskell-lsp-0.21 * Add tags field to CompletionItem * Fix test case about empty file path * Correct stack.yaml used in azure ci cache * Build ghcide including tests in windows azure ci * Qualify haskell-lsp modules to avoid name clashes

jneira changed the title ~~Use NormalizedFilePath from haskell-lsp~~ [WIP] Use NormalizedFilePath from haskell-lsp Mar 13, 2020

cocreature reviewed Mar 16, 2020

View reviewed changes

src/Development/IDE/Types/Location.hs Outdated Show resolved Hide resolved

cocreature self-assigned this Mar 16, 2020

jneira mentioned this pull request Mar 17, 2020

Add NormalizedFilePath from ghcide haskell/lsp#224

Merged

cocreature reviewed Mar 19, 2020

View reviewed changes

src/Development/IDE/Types/Location.hs Outdated Show resolved Hide resolved

jneira changed the title ~~[WIP] Use NormalizedFilePath from haskell-lsp~~ Use NormalizedFilePath from haskell-lsp Mar 19, 2020

cocreature reviewed Mar 20, 2020

View reviewed changes

jneira added 11 commits March 20, 2020 16:48

Use custom version of h-l-t

c3a373c

Use normalized path functions from h-l-t

09922c5

Restore empty path corner case

dd28826

Create a wrapper over NFP to override IsString

cfa86c5

Use maybe instead fromMaybe

bb58001

Use patched version of lsp-types in all yaml files

19f97f5

Remove unused import

58d02ad

Rename specific NormalizeFilePath to NormalizeFilePath'

d4b497c

Remove specific newtype and IsString instance

14950b4

Use released haskell-lsp-0.21

39c0f74

Adapt to type changes of haskell-lsp-0.21

db206b1

jneira changed the title ~~Use NormalizedFilePath from haskell-lsp~~ Use NormalizedFilePath and adapt types of haskell-lsp-0.21 Mar 22, 2020

jneira added 2 commits March 22, 2020 02:00

Add tags field to CompletionItem

74f1fc6

Fix test case about empty file path

5a15f9a

Correct stack.yaml used in azure ci cache

78bea8a

Build ghcide including tests in windows azure ci

69519a3

Qualify haskell-lsp modules to avoid name clashes

ed0e4b8

cocreature approved these changes Mar 23, 2020

View reviewed changes

cocreature merged commit 9951f35 into haskell:master Mar 23, 2020

cocreature mentioned this pull request Mar 23, 2020

Support for interface files #457

Merged

jneira deleted the norm-filepath branch March 26, 2020 06:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use NormalizedFilePath and adapt types of haskell-lsp-0.21 #479

Use NormalizedFilePath and adapt types of haskell-lsp-0.21 #479

jneira commented Mar 11, 2020 •

edited

Loading

jneira commented Mar 13, 2020

cocreature left a comment

jneira commented Mar 17, 2020 •

edited

Loading

jneira commented Mar 17, 2020 •

edited

Loading

jneira commented Mar 19, 2020

jneira commented Mar 19, 2020 •

edited

Loading

cocreature left a comment

jneira commented Mar 20, 2020 •

edited

Loading

cocreature commented Mar 20, 2020

jneira commented Mar 20, 2020

jneira commented Mar 22, 2020

jneira commented Mar 22, 2020 •

edited

Loading

jneira commented Mar 22, 2020 •

edited

Loading

jneira commented Mar 22, 2020 •

edited

Loading

cocreature commented Mar 23, 2020

cocreature left a comment

Use NormalizedFilePath and adapt types of haskell-lsp-0.21 #479

Use NormalizedFilePath and adapt types of haskell-lsp-0.21 #479

Conversation

jneira commented Mar 11, 2020 • edited Loading

jneira commented Mar 13, 2020

cocreature left a comment

Choose a reason for hiding this comment

jneira commented Mar 17, 2020 • edited Loading

jneira commented Mar 17, 2020 • edited Loading

jneira commented Mar 19, 2020

jneira commented Mar 19, 2020 • edited Loading

cocreature left a comment

Choose a reason for hiding this comment

jneira commented Mar 20, 2020 • edited Loading

cocreature commented Mar 20, 2020

jneira commented Mar 20, 2020

jneira commented Mar 22, 2020

jneira commented Mar 22, 2020 • edited Loading

jneira commented Mar 22, 2020 • edited Loading

jneira commented Mar 22, 2020 • edited Loading

cocreature commented Mar 23, 2020

cocreature left a comment

Choose a reason for hiding this comment

jneira commented Mar 11, 2020 •

edited

Loading

jneira commented Mar 17, 2020 •

edited

Loading

jneira commented Mar 17, 2020 •

edited

Loading

jneira commented Mar 19, 2020 •

edited

Loading

jneira commented Mar 20, 2020 •

edited

Loading

jneira commented Mar 22, 2020 •

edited

Loading

jneira commented Mar 22, 2020 •

edited

Loading

jneira commented Mar 22, 2020 •

edited

Loading