Fix crash on completion with type family

[guibou]

Close #2403.

tcdLName is partial for type/data families, its a record field
accessor which is not defined for all cases of the TyClDecl type.

Instead we use tyClDeclLName which will look for the name through an
indirection.

Note: I've checked that it works with GHC 8.10.7. The symbol is available starting from GHC 8.2: https://hackage.haskell.org/package/ghc-8.2.1/docs/HsDecls.html#v:tyClDeclLName so it should be compatible (minor the change in the module hierarchy, we'll see how the CI behaves).

Side question: I was able to remove the failure observed in #2403. However this bug was a pain to diagnostic. I'm experimenting random failures with HLS and I was used to restart my HLS session because HLS does not crash, it just stops responding.

I suspect (but that's pure guess) that a thread is launched, crashs (because of the exception, which is actually correctly sent to lsp log of my editor), but HLS is never restored in a working state. There is on forkIO in the codebase which may be responsible for it, and on forkFinally, which is supposed to crash the server with Fatal error in server thread which is not sent to my client. Maybe an issue should be opened to discuss that matter, I rather prefer to see a fully crashed HLS than a zombie processus ;)

[guibou]

edit: sorry, I linked the wrong bug, this is fixed now, but if you read the issue from your email, you won't see the change.

[guibou]

I'm not providing a regression test, I'm sorry. Thing is, I don't really see how I can write a regression test which tests that the server does not ends in a weird state when asking for completion for a type family. I'll be super happy to contribute that if you give me a few guidance with the test infrastructure.

However the fix is so simple and obvious that, well, a test may not be necessary ;) (Don't do this at home kids, we are trained engineers here ;)

[guibou]

I'm not sure I understand the CI failure. Guidance will be appreciated.

[pepeiborra]

I'm not sure I understand the CI failure. Guidance will be appreciated.

The failure didn't look related. We have a couple of flakey tests that fail non-deterministically and this was one of them. I have triggered another run

[pepeiborra]

However the fix is so simple and obvious that, well, a test may not be necessary ;) (Don't do this at home kids, we are trained engineers here ;)

The purpose of the test is to ensure that the codebase doesn't regress in the future, which could happen quite easily as the GHC api tends to change a lot across releases.

[pepeiborra]

I'm not providing a regression test, I'm sorry. Thing is, I don't really see how I can write a regression test which tests that the server does not ends in a weird state when asking for completion for a type family. I'll be super happy to contribute that if you give me a few guidance with the test infrastructure.

Just by writing a test that triggers the completion and checks the results. There are a couple of completion tests already at ghcide/test/exe/Main.hs.

[jneira]

@pepeiborra so we could merge it or should wait for the regression test?

[guibou]

Thank you.

I tried to write a regression test, using the following:

diff --git a/test/functional/Completion.hs b/test/functional/Completion.hs
index 5b5052f1..fe8516de 100644
--- a/test/functional/Completion.hs
+++ b/test/functional/Completion.hs
@@ -61,6 +61,24 @@ tests = testGroup "completions" [
              item ^. detail @?= Just "Data.Maybe"
              item ^. kind @?= Just CiModule
 
+     , testCase "completes type family" $ runSession (hlsCommand <> " --test" <> " --logfile /tmp/hls_dev.txt") fullCaps "test/testdata/completion" $ do
+         liftIO $ print "doc open"
+         doc <- openDoc "CompletionTypeFamily.hs" "haskell"
+         liftIO $ print "doc edit"
+
+         -- let te = TextEdit (Range (Position 10 14) (Position 10 20)) "Ba"
+         -- _ <- applyEdit doc te
+         -- liftIO $ print "doc get compl"
+
+         compls <- getCompletions doc (Position 10 16)
+         liftIO $ print "test"
+         let item = head $ filter ((== "Baz") . (^. label)) compls
+         liftIO $ do
+             item ^. label @?= "Bar"
+             item ^. kind @?= Just CiStruct
+             item ^. detail @?= Just ":: String -> IO ()"
+             item ^. insertTextFormat @?= Just Snippet
+             item ^. insertText @?= Just "putStrLn ${1:String}"
      , testCase "completes qualified imports" $ runSession (hlsCommand <> " --test") fullCaps "test/testdata/completion" $ do
          doc <- openDoc "Completion.hs" "haskell"
 
diff --git a/test/testdata/completion/CompletionTypeFamily.hs b/test/testdata/completion/CompletionTypeFamily.hs
new file mode 100644
index 00000000..f888dcd7
--- /dev/null
+++ b/test/testdata/completion/CompletionTypeFamily.hs
@@ -0,0 +1,12 @@
+-- Based on #2403
+{-# LANGUAGE DataKinds #-}
+{-# LANGUAGE TypeFamilies #-}
+
+module MyLib where
+
+type family Bar a --data Bar = Bar
+data Biz
+
+class Foo a where
+  foo :: a -> Ba
+  foo = undefined

Unfortunately, it times out.

However, if I replace Ba in the file by Bi, I get an answer immediately. I'm wondering if there may be another failure in HLS related to type families.

I tried to add logging using " --logfile /tmp/hls_dev.txt" as you can see in the example, and logs correctly show the query and finish with:

2022-01-10 12:52:03.832603091 [ThreadId 316] DEBUG hls:	finish: C:GhcSession:/home/guillaume/srcs/haskell-language-server/test/testdata/completion/CompletionTypeFamily.hs (took 0.00s)

But the test still timeouts.

I also tried:

--- a/ghcide/test/exe/Main.hs
+++ b/ghcide/test/exe/Main.hs
@@ -4527,6 +4527,16 @@ localCompletionTests = [
         ,("abcdefgh", CiFunction, "abcdefgh", True, False, Nothing)
         ,("abcdefghi", CiFunction, "abcdefghi", True, False, Nothing)
         ],
+    completionTest
+        "type family completion"
+        ["{-# LANGUAGE DataKinds, TypeFamilies #-}"
+        ,"type family Bar a"
+        ,"data Biz"
+        ,"a :: Ba"
+        ]
+        (Position 3 7)
+        [("Biz", CiStruct, "Biz", True, False, Nothing)
+        ],
     completionTest
         "class method"
         [

Which also timeout. But if I replace a :: Ba by a :: Bi, it passes the test.

How could I debug / understand the timeout? I tried to run the test with cabal test --enable-profiling ghcide-tests -- +RTS -xc to locate a possible exception, but I was not able to find something interesting.

[wz1000]

Did you try setting the LSP_TEST_LOG_MESSAGES=1 LSP_TEST_LOG_STDERR=1 environment variables?

[guibou]

LSP_TEST_LOG_MESSAGES=1 LSP_TEST_LOG_STDERR=1

Thank you!

And now the error appears, and it is still ghcide: No match in record selector tcdLName. I'm looking for another codepath which uses this call.

[guibou]

Actually, the test were using the haskell-language-server binary available on my system (so not yet patched) instead of the one locally built.

I need to understand how the test are supposed to be run so they use the correct HLS and then I'll push my regression test.

[guibou]

Ok, found, HLS_TEST_EXE... Sorry, I'm really slow.

[guibou]

Thank a lot to all of you for the help and guidance, I've pushed an MR with regression tests.

The purpose of the test is to ensure that the codebase doesn't regress in the future, which could happen quite easily as the GHC api tends to change a lot across releases.

@pepeiborra, Sorry, you are perfectly right. I took that lightly, but indeed there is nothing as a "so simple fix that it does not need regression test". Now that I got how tests works, next PR will have tests. Thank you for the remainder.