search

tweag / ormolu

A formatter for Haskell source code
https://ormolu-live.tweag.io
Other
956 stars 83 forks source link

Fix comment in empty export list #933

Closed brandonchinn178 closed 1 year ago

brandonchinn178 commented 1 year ago

Resolves https://github.com/tweag/ormolu/issues/906

brandonchinn178 commented 1 year ago

@mrkkrp is it possible to get this PR reviewed any time soon?

amesgen commented 1 year ago

@mrkkrp and I actually talked about this some time ago, but I forgot to comment here; sorry for the delay!

The changes are well-separated, but due to the heuristic nature of comment placing, the changes in espc. the last commit are rather ad-hoc, and introduce (albeit relatively isolated) complexity that seems somewhat disproprionate compared to the low severity of the issue they are resolving, as you note in #906.

Therefore, I played a bit around to see if there maybe is an easier way to fix the issue, and I think the core issue for this (and related bugs) is that when a specific region with a list of items (like a module exports list, but also a record field declaration, see the following example) is empty, there is no chance for a comment to be printed inside because there is no "last" element before which it could be printed. Concretely, this means that the comments in this example "escape" their enclosing brackets:

module Foo
  ( -- a
  )
where

data Test = Test {
  -- b
  }

Hence, an idea would be to use located with bogus spans corresponding to the opening and closing bracket. I implemented this here, and it indeed seems to work in principle (and might even be adapted to work e.g. with the record example above): it passes all tests, but there are some idempotency issues in the hackageTests.

I might have some time this week to see if the idempotency issue of this approach can be resolved relatively easily, but also feel free to have a look, as you might have considered something similar when working on this PR, but discarded it (or other approaches).

brandonchinn178 commented 1 year ago

That does look much better 😃 I do like some of the commits in this branch that clean up some of the comments code (e.g. the popComment commit is much more readable IMO), so maybe I can put those up in a separate cleanup PR. But :+1: to your approach

brandonchinn178 commented 1 year ago

@amesgen any update on your separate approach?

amesgen commented 1 year ago

@amesgen any update on your separate approach?

Sorry, I finally opened #1013. I agree that some commits in this PR would be nice to incorporate just as generic code cleanup, feel free to open a PR for those!