Skip to content

Fix comments in if blocks #1092

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jan 27, 2024
Merged

Fix comments in if blocks #1092

merged 1 commit into from
Jan 27, 2024

Conversation

brandonchinn178
Copy link
Collaborator

@brandonchinn178 brandonchinn178 commented Jan 21, 2024 •
edited
Loading

Resolves #998

The issue has two bugs associated with it, which are both solved in this PR.

Comments between then/else tokens and body

This bug happens if there's a comment between the then/else tokens and the body of the branch:

-- without comments
if cond
  then case a of
    Just b -> ...
  else
    foo
      True
      ""

-- expected
if cond
  then
    -- comment
    -- comment
    case a of
      Just b -> ...
  else
    -- comment
    -- comment
    foo
      True
      ""

-- previous behavior
if cond
  then -- comment
  -- comment
  case a of
    Just b -> ...
  else -- comment
  -- comment

    foo
      True
      ""

There are a few bugs here:

  1. The comment is moved up next to the keyword instead of staying on the next line
  2. Subsequent comments are not indented
  3. Expressions that normally "hang" like case are not indented
  4. Expressions that don't hang like the second branch here get an extra line added

The issue is that the code does the following steps:

  1. Print then/else token
  2. located A: Spit out comments
  3. located B: Set breakpoint layout
  4. placeHanging Apply hanging format
  5. Print body

But we need to spit out the comments AFTER applying the hanging layout. Additionally, we need to never hang when there are comments. So I fixed this by moving "Apply hanging format" before "Spit out comments", and manually setting the breakpoint layout before:

  1. Print then/else token
  2. switchLayout: Set breakpoint layout
  3. placeHanging Apply hanging format
  4. located A: Spit out comments
  5. located B: Set breakpoint layout
  6. placeHanging Apply hanging format
  7. Print body

Comments before then/else keywords

In this bug, comments before the then/else keywords get moved to after:

-- input + expected
if cond
  -- asdf
  then foo
  -- asdf
  then bar

-- previous behavior
if cond
  then -- asdf
    foo
  else -- asdf
    bar

This happens because we only spit out comments when we call located, but we dont call located for keywords; we just do a plain txt "then".

The fix for this was relatively easy: I just wrap txt "then" in a located containing the SrcSpan for the then keyword from the annotations. I think there's future work here to always do this for all keywords, now that GHC has prettyprinting information with SrcSpans for all keywords

amesgen
amesgen approved these changes Jan 22, 2024
View reviewed changes
Copy link
Member

@amesgen amesgen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks good to me. It introduces some complexity, but it is localized, and I think the direction of using something like locatedToken could be promising to fix these kinds of subtle comment placement issues; there is also a movement in GHC to store the token spans in a systematic way: https://gitlab.haskell.org/ghc/ghc/-/issues/23447

I diffed our hackageTests output before and after this PR, the results look good to me: https://gist.github.com/amesgen/88b2b92cc9e0d7bc13e7fc8357db057a (pipe through eg delta for a prettier/easier-to-read diff).

@brandonchinn178
Copy link
Collaborator Author

hm Thanks for the hackage diff. It's showing two diffs that I didn't expect (or want):

  1. There are some function calls that are now always being put below then/else. I dont want to change behavior there
  2. I should also add a test for comments on the same line as then/else. Those should not float above the then/else

I'll take a look tonight

@amesgen
Copy link
Member

amesgen commented Jan 22, 2024

hm Thanks for the hackage diff. It's showing two diffs that I didn't expect (or want):

1. There are some function calls that are now always being put below then/else. I dont want to change behavior there

2. I should also add a test for comments on the same line as then/else. Those should not float above the then/else

I'll take a look tonight

To avoid any confusion: The diff is between the Ormolu output before and after this PR, not between the input and the Ormolu output; the original is not visible (but you can find it on Hackage ofc). Ideally, this would be a three-way diff, but I think I would have to set up a git repo for this?

@brandonchinn178 brandonchinn178 force-pushed the bchinn/comments branch 2 times, most recently from d6348a5 to c3264ae Compare January 25, 2024 05:19
mrkkrp
mrkkrp reviewed Jan 26, 2024
View reviewed changes
@mrkkrp
Copy link
Member

mrkkrp commented Jan 26, 2024

@brandonchinn178 Do you consider this ready or is there anything else you'd like to add here?

@mrkkrp
Copy link
Member

mrkkrp commented Jan 26, 2024

It is nice that keyword spans are now available, it wasn't the case when I was originally implementing this.

@brandonchinn178
Copy link
Collaborator Author

This is ready on my end!

@brandonchinn178
Copy link
Collaborator Author

oh you added commits. I would appreciate it if you could avoid adding commits to my PRs without consulting me. It's a bit disorienting; if you just left a comment, I would have been perfectly happy to make the changes.

Let me take another look

@mrkkrp
Copy link
Member

mrkkrp commented Jan 26, 2024

Sorry, I usually do that when I'm about to merge, but this time around I wasn't completely sure that you were done with it.

@brandonchinn178
Copy link
Collaborator Author

Sigh I wanted to see if I could improve the comment-next-to-keyword case, but it doesn't look trivial. It seems like the comments are attached to "after the else keyword", and there's no way to increase the indentation for comments.

I doubt this will happen often, and this PR is already a huge improvement, so let's merge. Thanks @mrkkrp!

@mrkkrp mrkkrp merged commit 9b3a78f into tweag:master Jan 27, 2024
@mrkkrp
Copy link
Member

mrkkrp commented Jan 27, 2024

Thanks a lot!

@brandonchinn178 brandonchinn178 deleted the bchinn/comments branch January 27, 2024 19:13
@amesgen amesgen mentioned this pull request May 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amesgen amesgen amesgen approved these changes

@mrkkrp mrkkrp mrkkrp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Comment placement on then/else branches
3 participants
@brandonchinn178 @amesgen @mrkkrp