Skip to content

Add back sitcc to ExplicitTuple.p_arg#868

Merged
mrkkrp merged 1 commit intotweag:masterfrom
brandonchinn178:tuple-indent
Apr 8, 2022
Merged

Add back sitcc to ExplicitTuple.p_arg#868
mrkkrp merged 1 commit intotweag:masterfrom
brandonchinn178:tuple-indent

Conversation

@brandonchinn178
Copy link
Copy Markdown
Collaborator

sitcc was removed as part of the ghc-lib-parser-9.2 upgrade (#779), but it looks like that was erroneous; the only change that needed to happen in p_larg was the removal of located. This causes problems in fourmolu with leading commas (see fourmolu/fourmolu#148 (comment)), and this fixes it. I'm not quite sure how to come up with a failing test case for this in ormolu, but I'm fairly certain that the removal of sitcc is incorrect, and may cause other unexpected behaviors.

@brandonchinn178
Copy link
Copy Markdown
Collaborator Author

cc @amesgen, just because you did the ghc-lib-parser-9.2 upgrade

@mrkkrp
Copy link
Copy Markdown
Member

mrkkrp commented Apr 4, 2022

Can you come up with an example that would demonstrate the difference between the current behavior and the behavior with sitcc added?

@brandonchinn178
Copy link
Copy Markdown
Collaborator Author

so I found some differences with do-blocks in tuple sections:

-- without sitcc
x =
  [ (do
      x <- asdf
      return x,)
  ]
x =
  [ (do
      x <- asdf
      return x,do
      x <- asdf2
      return x,)
  ]

-- with sitcc
x =
  [ (do
       x <- asdf
       return x,)
  ]
x =
  [ (do
       x <- asdf
       return x,do
                  x <- asdf2
                  return x,)
  ]

without sitcc, do blocks in a tuple section don't know that they're in a tuple; they're indented only once (in the list context) as opposed to twice (in the list + tuple context). In the second case, I'm actually surprised that the first isn't a parse error, since I thought x <- asdf2 needs to be to the right of the do keyword, but GHC accepts it. Regardless, I think the case with sitcc is better than the case without (although both should be fixed to add the appropriate space/newline before the do).

This only comes up with tuple sections because commaDel adds a breakpoint before, which seems to handle the missing sitcc logic. So it might not be that big of a deal (who does tuple sections with multiline blocks?), but I'd argue that:

  1. adding sitcc does the least-unexpected thing in the uncommon case
  2. adding sitcc does nothing in the common case
  3. adding sitcc fixes an issue in the downstream fourmolu project

I know (3) probably doesn't matter to you, but in combination with (1), i don't see any reason not to do it, and it has a nice side benefit of helping downstream. plus, i suspect that if ormolu changes something around styling tuples here, this sitcc will be needed, so this is also future-proofing

Copy link
Copy Markdown
Contributor

@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.

I can confirm that removing sitcc in #779 was just an oversight, so I agree that there is no reason not to add it back. Thanks for the detailed example!

@mrkkrp mrkkrp merged commit 5f0a597 into tweag:master Apr 8, 2022
@mrkkrp
Copy link
Copy Markdown
Member

mrkkrp commented Apr 8, 2022

Thanks!

@brandonchinn178 brandonchinn178 deleted the tuple-indent branch April 9, 2022 06:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants