Import Regression: Wrong Import Placement on first Import

[emilypi]

Here's a weird one: first imports are inserted in wrong place. Unsure why.

Your environment

Output of haskell-language-server --probe-tools or haskell-language-server-wrapper --probe-tools:

askell-language-server version: 1.3.0.0 (GHC: 8.10.4) (PATH: /Users/emilypi/.ghcup/bin/haskell-language-server-wrapper-1.3.0) (GIT hash: e7c5e90b6df5dff2760d76169eddaea3bdd6a831)
Tool versions found on the $PATH
cabal:		3.4.0.0
stack:		2.5.1
ghc:		8.10.4

Which OS do you use:
MacOS Big Sure

Which lsp-client do you use:

Emacs

Describe your project (alternative: link to the project):
*.cabal files, cabal.project

Contents of hie.yaml:

cradle:
  cabal:
    - path: "src"
      component: "lib:order"

    - path: "test"
      component: "order:test:doctests"

Steps to reproduce

1. Make a first import with functions defined (along with comments)

2. Import goes in wrong place.

3. If importing another thing, imports are added below first import

4. However, when physically moved, to the right spot, imports work as expected.

Expected behaviour

Imports should be inserted in correct area of the module.

Actual behaviour

The first import is inserted above the first signicant decl, regardless of comments.

[jneira]

OMG this is the neverending history. A regression test is absolutely needed as first step.
@nini-faroux could you have the chance to take a look? thanks!

[nini-faroux]

sure! I can have a look tomorrow

[nini-faroux]

hi, so I played around a little with this just now and it seems the issue is whenever the first (non-empty) line after the 'module ... where' is a comment, if you put anything above the comment the 'import' seems to be added above that correctly, as an example, with a data declaration above the comment the import gets added above that: 

module Test 
( SomeData(..)
) where

import Data.Monoid -- newly added
data Something = Something

-- | some comment
class Semigroup a => SomeData a
instance SomeData All

The problem seems to be with this function:

haskell-language-server/ghcide/src/Development/IDE/Plugin/CodeAction.hs

Line 1291 in a5d70ee

newImportInsertRange :: ParsedSource -> Maybe (Range, Int)

Where there are two cases, when the import list is empty, it finds the position from 'hsmodDecls', and if it matches this case and the first line is a comment, we have the problem with the import being added under the comment.

The second case is when there are imports, so the import list isn't empty, in which case it finds the position from 'hsmodImports', and will add the suggested import under the current last import in the file, which works fine once the existing imports are in the correct position, as mentioned already.

So I'd be happy to try to make a fix for this some evening later in the week if that suits, but it would be good to add a number of tests as well, so I was wondering where's the best place to add them? There's a 'test' folder in '/ghcide/...', but I didn't see any tests for this functionality there, and there's two import plugins (refine and explicit) but I don't think either are particularly related to this issue. Just to make sure I add them in a sensible place.

Thanks!

[jneira]

@nini-faroux many thanks for the head up, i think the place for such tests could be

haskell-language-server/ghcide/test/exe/Main.hs

Line 1579 in a5d70ee

suggestImportTests = testGroup "suggest import actions"

[nini-faroux]

that's great, thanks @jneira