Exactprint-based import list modification introduces extra newline when there is at least one newline between the module header and the topmost import

[konn]

As found in PR #1264, it seems that exactprint-based import list modification seems to introduce an extra newline in certain situation.
Bisecting it in the master branch, this behaviour is first introduced in eb557b3.

Steps to reproduce

1. Checkout eb557b3 and build ghcide the binary and make sure your LSP client will use it as a language server executable.

2. Save the following as Lib.hs:

   module Lib where
   
   import Data.Vector ()
   
   theFun = fromList

   Make sure there is at least one newline between the module header and the import declaration; otherwise this bug won't occur.

3. Open Lib.hs and wait for the module to be loaded.

4. Apply Add fromList to Data.Vector code action.

Expected behaviour

fromList is added to the import list, without any modification to the lines except for import list itself, resulting in:

module Lib where

import Data.Vector (fromList)

theFun = fromList

Actual behaviour

An extra newline added between the module header and the import declaration:

module Lib where


import Data.Vector (fromList)

theFun = fromList

Note that there are two blank lines in-between instead of just one.

[berberman]

Thanks for your report. I extracted extendImport into a standalone project to check it without the help of ghcide, using exactPrint function to print the result.

I had a module A to be imported in module Test:

module A where

class C a where
  m1 :: a
  m2 :: a

I tried running extendImportViaParent df "C" "m1" in several scenarios:

1. no empty lines between module declaration and the target import declaration

module Test where
import A ()

resulted in "\nimport A (C (m1))".

2. one empty line

module Test where

import A ()

resulted in "\n\nimport A (C (m1))".

3. two empty lines

module Test where


import A ()

resulted in "\n\n\nimport A (C (m1))".

4. no empty lines, but there is another adjoining import declaration above

module Test where

import Data.Text
import A ()

resulted in "\nimport A (C (m1))".

It's possible to conclude that the number of unexpected line feeds in the result of exact print depends on how many empty lines between the previous declaration and this import declaration. I think tests didn't expose this error was because in ghcide's procedure, the first line feed was "eaten" somehow; for example, in this test case:

 testSession "extend single line import with value" $ template
            [("ModuleA.hs", T.unlines
                    [ "module ModuleA where"
                    , "stuffA :: Double"
                    , "stuffA = 0.00750"
                    , "stuffB :: Integer"
                    , "stuffB = 123"
                    ])]
            ("ModuleB.hs", T.unlines
                    [ "module ModuleB where"
                    , "import ModuleA as A (stuffB)"
                    , "main = print (stuffA, stuffB)"
                    ])
            (Range (Position 3 17) (Position 3 18))
            ["Add stuffA to the import list of ModuleA"]
            (T.unlines
                    [ "module ModuleB where"
                    , "import ModuleA as A (stuffB, stuffA)"
                    , "main = print (stuffA, stuffB)"
                    ])

there are no empty lines between module ModuleB where and import ModuleA as A (stuffB), so the result is exactly what we expect.

If you change your example to:

module Lib where
import Data.Vector ()

theFun = fromList

I think the result would be:

module Lib where
import Data.Vector (fromList)

theFun = fromList

Actually, I have encountered this issue when I was implementing haddock comments plugin, but unfortunately, I didn't figure out the real reason, and didn't know what was happening under the hood. So my workaround is to use T.strip :(

haskell-language-server/plugins/hls-haddock-comments-plugin/src/Ide/Plugin/HaddockComments.hs

Line 71 in 46d2a3d

result <- T.strip . T.pack $ exactPrint locDecl anns' =

[konn]

Thank you for your detailed explanation!
So, for a tentative workaround, it seems that your T.strip usage seems reasonable to me. Though, in some cases in import lists, it might also eat proper indentation as well. So, perhaps T.dropWhile (`elem` "\r\n") would be more appropriate.

[konn]

I want to keep this issue open until the real reason is discovered. Any objections?

[pepeiborra]

I haven't checked, but I think the real reason is that the annotated source contains annotations that record the pre and post white space, whereas the SrcSpan in the non annotated AST does not include the surrounding white space.

Therefore the real solution will be to remove the outermost white space annotations before asking exactprint to prettyprint the rewrite.

[berberman]

I just tried setEntryDPT importDecl dp00, where importDecl is the top-level AST we want to rewrite. It solved the issue.

[konn]

I confirmed @berberman's approach applying setEntryDPT to the original import declaration (and drops tail in rewriteToEdit) works also in import disambiguation and even indented case! Thank you @pepeiborra and @berberman for pointing it out!

[konn]

So, what should be done next?
Should I also address this issue also in PR #1264?
Or, another PR must be filed first (possibly by @berberman?), merge them into master, and then fix #1264 based on it?

[pepeiborra]

Imho the call to setEntryDPT needs to be made by rewrite.