Comments inside ghc-options are not preserved properly

[michaelpj]

Sometimes you want to document why you're setting them. At the moment this:

  ghc-options:
    -threaded
    -- allow user RTS overrides
    -rtsopts
    -- disable idle GC
    -- increase nursery size
    -- Enable collection of heap statistics
    "-with-rtsopts=-I0 -A128M -T"
    -Wno-unticked-promoted-constructors

gets turned into this:

  ghc-options:
    -- allow user RTS overrides
    -- disable idle GC
    -- increase nursery size
    -- Enable collection of heap statistics
    -threaded
    -rtsopts
    "-with-rtsopts=-I0 -A128M -T"
    -Wno-unticked-promoted-constructors

[tfausak]

Similar to #63 (comment), you can work around this problem by defining ghc-options multiple times and putting the comments on the field itself:

ghc-options:
  -threaded
  -Wno-unticked-promoted-constructors

-- allow user RTS overrides
ghc-options: -rtsopts

-- disable idle GC
-- increase nursery size
-- Enable collection of heap statistics
ghc-options: "-with-rtsopts=-I0 -A128M -T"

However unlike #63, it should (in theory) be possible for Gild to leave the comments in the same relative place. It doesn't re-order the options, and a comment can't exist in the middle of an option.

I say "in theory" because the entire value of the ghc-options field is parsed and then rendered. That means Gild would have to remember where each comment is so that it can put them back. As it is currently, comments in fields are always treated the same (by floating them up to the top of the field's value).

cabal-gild/source/library/CabalGild/Unstable/Action/FormatFields.hs

Line 68 in 51425c0

. zip ((,) position <$> concatMap (snd . FieldLine.annotation) fls : repeat [])

[tfausak]

Actually I think this is not a bug. Instead I think that keeping comments as close as possible to their original position is a feature request. Gild tries hard not to lose comments, but in general it's challenging to avoid moving them. Field values get parsed and re-rendered, which is generally what you want. For example, it's what allows Gild to parse GHC options so that both -rtsopts and "-with-rtsopts=..." are handled correctly.

This issue is about ghc-options, but it applies equally well to nearly all of the fields defined in FormatFields (linked above). Indeed this could even apply to build-depends, which is arguably the hardest case due to stuff like this:

build-depends:
  lib-a,
  lib-b
    -- major version 1
    ^>= 1.2.3
    -- major version 4
    || ^>= 4.5.6

I'm not sure exactly what the algorithm would look like, but Gild could collect all the comments up to the nearest "item" when formatting and end up with something like this:

build-depends:
  lib-a,
  -- major version 1
  -- major version 4
  lib-b ^>=1.2.3 || ^>=4.5.6

It would also have to re-indent comments, but it does that already.

It's unclear to me how much this veers into the territory of haskell/cabal#7544.

[michaelpj]

Hmm. So perhaps I was misled by the fact that it seems that cabal-gild does preserve some inner comments. I thought it was inside build-depends, but it seems that it was actually inside import. So in fact cabal-gild probably does move comments more than I would find optimal at the moment, but I appreciate that the lack of an exact-printer makes things difficult.

[tfausak]

Hmm I'm not sure what you mean about import. Do you have an example?

Gild should only move comments that are inside field values that it knows how to parse. Those are listed here:

cabal-gild/source/library/CabalGild/Unstable/Action/FormatFields.hs

Line 96 in 51425c0

[ "asm-options" =: SPP.list @Newtypes.NoCommaFSep @Newtypes.Token',

Anything else should get passed through more or less unchanged, although Gild will re-indent and strip trailing blank spaces.

[michaelpj]

I commented on the HLS diff where it happens: https://github.com/haskell/haskell-language-server/pull/4229/files#r1600078367

Anything else should get passed through more or less unchanged, although Gild will re-indent and strip trailing blank spaces.

I see, that's exactly what happens. To a naive user "re-indent and fix whitespace" looks a lot like "reformat", so I wouldn't have realised that was different!

[tfausak]

Ah right, that is confusing. Thanks for the example! To reproduce it inline:

import:
  foo
  , bar
  -- baz
  , qux

Gild will not reformat that. This is confusing because if you replace import with ghc-options (or build-depends or whatever), Gild will float the -- baz comment up to the top (above foo). This happens because Gild doesn't attempt to parse the import value. Instead it just deals with the raw FieldLines, which have comments attached. But this is an implementation detail, and a user shouldn't have to care about it.

I'm honestly not sure if this is a bug or a feature request 😆 Gild could try to be more consistent by always floating comments to the top. Or it could try to be smarter by keeping comments close(r) to where they came from.

[michaelpj]

Yeah. I'm not really sure what the best thing is here either. If anything, I slightly lean towards making the comment-floating consistent so that if you do add support for a new field like import, then it won't look like a partial regression to users?

[tfausak]

I agree. I think the correct behavior here is to always float comments to the top of field values. That way users don't have to know/care about which fields Gild actually parses.

In the future if Gild wants to try to keep comments close(r) to where they were originally, that can be a separate enhancement.

[tfausak]

There are two places that will need to change. First is the place where there's no parser for the field:

cabal-gild/source/library/CabalGild/Unstable/Action/FormatFields.hs

Line 51 in d32c1fb

Nothing -> f

Currently it passes the field through unchanged. It will need to float all comments up instead.

The second place is when there is a parser for the field, but parsing the field failed:

cabal-gild/source/library/CabalGild/Unstable/Action/FormatFields.hs

Line 96 in d32c1fb

Left _ -> fls

It needs to do the same thing: keep the field contents the same, but float the comments up to the top.

The comment floating logic is here:

cabal-gild/source/library/CabalGild/Unstable/Action/FormatFields.hs

Line 99 in d32c1fb

. zip ((,) position <$> concatMap (snd . FieldLine.annotation) fls : repeat [])