Fix completion for qualified import

[xsebek]

• add the missing test case for Completions broken for pre-qualified import #2824
• fix how we get the module name considering it can be preceded by qualified
• use parsed context for import completions
  • add regression test for fixed multiline import
• refactor getCompletions function

[michaelpj]

Can we mark this as failing somehow so we can merge it without the fix?

[xsebek]

@michaelpj there are other tests marked as such, so it can surely be done.

The second one should be passing though, so I need to check what I am testing. 😅

[xsebek]

@michaelpj I marked it as expected to fail, but then I found the fix anyway 😀

[xsebek]

Btw. I have no idea what is CI trying to tell me:

EDIT: OK, now that I have my glasses, I see there is one grey job not skipped but cancelled, which caused this condition to fail.

[michaelpj]

I reran the failed jobs, let's see what happens.

[xsebek]

@michaelpj Good news, using the parsed context seems to have fixed the problem with multiline import completions! 👍

[michaelpj]

Good news, using the parsed context seems to have fixed the problem with multiline import completions!

Could we add a regression test for this?

[xsebek]

@michaelpj sure, I added a regression test. 🙂

[xsebek]

The CI smiles on me this day! ☀️

@pepeiborra could you please check the comments above? 🙂

[xsebek]

After inspecting the CI (see comment above), I decided to keep the manual parsing.

@pepeiborra @michaelpj If the CI passes, I think it is ready to merge.

EDIT: Let me backtrack on backtracking and try the CI again... 👀

[xsebek]

Weird, the test fails on GHC 8.8.4 because there is an unexpected command with import Control.Monad qualified as M (j)

-- explicit qualified post:    FAIL (1.80s)
-- test/exe/Main.hs:4826:
-- Expected no command but got:
Just (Command {
  _title = "extend import",
  _command = "143210:ghcide-completions:extendImport",
  _arguments = Just [Object (fromList 
    [("doc",String "file:///tmp/extra-dir-35255587795340/A.hs")
    ,("importName",String "Control.Monad")
    ,("importQual",Null)
    ,("newThing",String "join")
    ,("thingParent",Null)
  ])]
})

This seems very strange to me because it does not happen on 8.10 and to normal pre-qualified import test. 😕

Also from what I gather "extend import" should show up only if the join was written outside the import list.

[michaelpj]

I restarted the tests, and now the 8.6.5 one fails as well in the same way. It also doesn't fail except on ubuntu. Perhaps it's just flaky, and we're missing a call to wait for something somewhere?

[pepeiborra]

Consistent failure:

Building test suite 'func-test' for hls-1.7.0.0..
Running 1 test suites...
Test suite func-test: RUNNING...
haskell-language-server
  completions
    import function completions: FAIL (2.28s)
      test/functional/Completion.hs:144:
      expected: Just CiFunction
       but got: Just CiStruct
1 out of 1 tests failed (2.28s)
Test suite func-test: FAIL

https://github.com/haskell/haskell-language-server/runs/7592685137?check_suite_focus=true#step:10:290

The test needs adjusting, the new result is more correct thanks to the changes in this PR.

@xsebek are you able to adjust the test and get this PR finally merged?

[xsebek]

Sorry for leaving this open for so long!

@pepeiborra I fixed the test failing on CiStruct but after rebase on upstream/master I could not find the test again. 😕

I will wait to see what the CI says this time and hopefully finally get this merged.

[fendor]

I fixed the test failing on CiStruct but after rebase on upstream/master I could not find the test again.

@xsebek That's my fault, we cleaned up the func-test suite, and apparently I misjudged that the completion tests are redundant.

[xsebek]

@fendor thanks for the quick response. 👍

The CI did not finish well:

• on macOS all tests were skipped, but are shown as green
• ubuntu with GHC 9.2 failed on one flaky test sends indefinite progress notifications
• Nix failed on ubuntu because "No space left on device"
• Nix failed on macOS because of fourmolu dependency

Any tips to make the CI green?

[fendor]

We don't run the tests on macos (honestly, I don't know why, this will surely bite us at some point) and nix CI is optional. It will actually be reduced #3804 quite soon, so don't worry about that.

I restarted the test suite for the flaky test, sounds like this PR is green and doesn't require your input at the moment.

[fendor]

The changes look good to me. Am I correct in the assumption, the way this actually fixes the issue is by using maybeContext parsing the module and reliably giving a module name?

This PR has the trade-off that we sometimes can't suggest a module correctly if there is a parse error in the file, for example,

module Test where 

import
import 
--      ^ Put your cursor here

will not suggest imports. That's a change in behaviour, but I don't think it is a blocking one.

I think we should document, perhaps in the function documentation of getCompletions, which classes of completions are dependent on an existing parsed module and which are not?

[xsebek]

@fendor Yes, I switched to using the (hopefully) parsed context, which makes it work in more situations including multiline.

Maintaining the ad-hoc parsing does not seem worthwhile to me. I assume GHC does not stop on first parse error, so anything that it can manage to parse we will be able to use for suggestions. Your example seems to me like something GHC could (and should) handle better than HLS.

The rest of the completions seem to use a mix of HIE AST (recordDotSyntaxCompls) and ad-hoc text parsing (enteredQual). I don't know the detail, so my guess might be more misleading than useful. 😕

[fendor]

I assume GHC does not stop on first parse error

GHC does stop after the first parse error.

I think that's fine by me. A Haskell file shouldn't have parse errors in it anyway.