Regression in code action "add xxx in context type signature" with ghc 9.10 when haddock comment appears in type signature

[guibou]

The code action which adds a constraint in type signatures produces invalid code when the type signature contains comment.

However it seems that it depends on ghc version.

Your environment

Which OS do you use?

Linux Nixos

Which version of GHC do you use and how did you install it?

I observed first the problem using ghc 9.12.2 on work codebase with a recent dev version of HLS.

How is your project built (alternative: link to the project)?

Which LSP client (editor/plugin) do you use?

Neovim+neovim lsp

Which version of HLS do you use and how did you install it?

Details about GHC versions and HLS later in this description

Have you configured HLS in any way (especially: a hie.yaml file)?

Standalone file (but can be reproduced on projects with hie files)

Steps to reproduce

For example, the following piece of code:

foo :: 
  (Applicative m) =>
  -- | This is a comment
  x -> 
  m x
foo x = pure (x + 1)

Produced the following error:

 Could not deduce ‘Num x’ arising from a use of ‘+’
  from the context: Applicative m
    bound by the type signature for:
               foo :: forall (m :: * -> *) x. Applicative m => x -> m x
    at /home/guillaume/HLSAddConstraint.hs:(1,1)-(5,5)
  Possible fix:
    add (Num x) to the context of
      the type signature for:
        foo :: forall (m :: * -> *) x. Applicative m => x -> m x

Associated with code action "Add Num x to the context of the type signature for foo".

Using the same HLS version, 2.10.0.0, I observe different behavior with different GHC version:

Expected behaviour

• With ghc 9.8.4 the code action returns the correct expected result:

foo :: 
  (Applicative m, Num x) =>
  -- | This is a comment
  x ->
  m x
foo x = pure (x + 1)

Actual behavior

• With ghc 9.10, I obtain an invalid source file:

foo :: 
  (Applicative m,
  -- | This is a comment Num x) =>

  x ->
  m x
foo x = pure (x + 1)

See how Num x) => was moved at the end of the haddock comment

Debug information

I've checked with HLS 29b2ecb (current master at the time of authoring this issue) and GHC 9.12.2 and I can confirm that the code produced is invalid with this configuration.

To save a bit on build time, I had not tested master branch with more recent GHC version. HLS behaves differently with the same HLS (2.10.0.0) but different GHC (9.8 vs 9.10) and the issue is still there with the most recent GHC and HLS version availables. I may test on different GHC later to refine / understand if I find time to build.

I'll have a look to see if I understand the bug and if I can provide a quick fix. First I'll add a test.

[guibou]

I think the issue comes from the appendConstraint function in plugins/hls-refactor-plugin/src/Development/IDE/Plugin/CodeAction/ExactPrint.hs

I'm a bit puzzled because I initially thought that the code edit would be done "blindly" on text, which would perfectly justify why it fails. However it seems that appendConstraint is editing GHC AST, and I don't see how this can be wrong, and I don't see how the pretty printer can be wrong.

That's super impressive, if the comment is an "inline" comment, such as:

foo :: 
  (Applicative m) =>
  {- | This is a comment -}
  x -> 
  m x
foo x = pure (x + 1)

It works fine, but with some dubious aesthetic results ;):

foo :: 
  (Applicative m,
   {- | This is a comment -} Num x) =>

  x ->
  m x
foo x = pure (x + 1)

I'll investigate the appendConstraint function, I think the problem is that the annotation which contains the comment is not placed on the correct position. I'm really impressed because it seems that the comment is INSIDE the constraint node of the ast, instead of being outside, that's really puzzling.

A simpler version of the reproducer:

bar :: 
  () =>
  -- This is m
  m ()
bar = pure ()

[guibou]

Note for discoverability: I found GHCIDE_DEBUG_AST environment variable, which display the some of the generated / read ast in those phases, convenient.

[guibou]

My current understanding (which needs to be refined) is that we produce incorrect annotations.

On the following block:

bar :: 
  () =>
  -- This is m
  m ()
bar = pure ()

There is an annotation for the comment which SrcSpan is where the * lies.

bar :: 
  () =>*
**********
**m ()
bar = pure ()

We then edit the ast to insert the constraint, which will lies where $$$$$$ appears:

```haskell
bar :: 
  ($$$$$$$$$$$$$) =>*
**********
**m ()
bar = pure ()

However (that's unclear for me), either we produce incorrect span for $$$$$ or we do not correctly edit the span for *****, but it looks like the "exact print" logic decides to first print ******* instead of $$$$$, leading to the inversed blocks.

At that time I'm unsure if that's a bug in the exact print library or in hls. What bugs me is that it appears with GHC 9.10.

The migration guide shows that some stuff changed regarding the AST, see https://gitlab.haskell.org/ghc/ghc/-/wikis/migration/9.10#ghc, and there is a guide to this migration guide https://gist.github.com/alanz/e127e7561ddf1cfeb07fbdee9a966794

[guibou]

Another point I just observed.

If there is NO prior constraint, e.g.:

bar :: 
  -- | This is m
  m ()
bar = pure ()

In all cases, the generated code is correct (with all version of GHC I've tried, 9.8 and 9.12). However the comment is moved on top of the additionnal constraint:

bar :: 
  -- | This is m
  Applicative m => m ()
bar = pure ()

The choice is indeed arbitrary and whatever the position of the comment is, it is correct (one can argue that if the comment starts by -- |, it is an haddock comment and is associated with the argument, not the constraint, and should be put under, but it shows that the logic does not take care of moving/editing comments associated with the type signature.

[guibou]

Working on the following example (sorry, copy paste from the testsuite)

    [ "module Testing where"
    , "foo "
    , "    :: (" <> constraint <> ") =>"
    , "    -- This is a comment"
    , "    m ()"
    , "foo = pure ()"
    ]

 (EpaComments
                                                                                     [(L
                                                                                       (EpaSpan
                                                                                        { /tmp/nix-shell.f9y1Cx/extra-dir-34868239627555/Testing.hs:4:5-24 })
                                                                                       (EpaComment
                                                                                        (EpaLineComment
                                                                                         "-- This is a comment")
                                                                                        { /tmp/nix-shell.f9y1Cx/extra-dir-34868239627555/Testing.hs:3:11-12 }))]))
                                                                                   [])

The span 4:5-24 represents the position of -- | This is a comment and should not change. HOWEVER the span 3:11-12 represents the position of the position after the previous token (e.g. =>) and indeed this should be changed.

I don't know what changed before GHC 9.8 and 9.10, maybe the printing semantic changed in case something is incorrect like that. I also don't really know what is the correct way to solve that. Walking all the comments from this node and if there are line comment and if there "previous token" is on the same line, move the previous token. This feels really adhoc, but perhaps that's the way.

It may also be possible to remove all the annotation from this block and let the natural rules decide about how to position things. However how does it behaves for line comment, because if there are multiples of them, they may stack them on the same line.

[guibou]

I've found a solution.

moveCommentsToTheEnd (EpAnn entry anns (EpaComments priors)) =
   EpAnn entry anns (EpaCommentsBalanced { priorComments = [], followingComments = priors})

The followingComments block of the EpaCommentsBalanced is able to move the comment to "after" the block, which seems to be designed for this purpose.

I will investigate a bit, maybe this should be applied with a bit of filtering (e.g. if the "previous token" of the comment is after the current context block), but seems to be a working solution.