Skip to content
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.

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

AST changes when formatting type applications #211

Closed
ocharles opened this issue Jun 23, 2022 · 16 comments · Fixed by #243
Closed

AST changes when formatting type applications #211

ocharles opened this issue Jun 23, 2022 · 16 comments · Fixed by #243
Labels
bug Something isn't working

Comments

@ocharles
Copy link

The following

foo = bar @'Bar

Gets reformatted to

foo = bar @ 'Bar

I can't reproduce this on https://ormolu-live.tweag.io/, so think it might be a Fourmolu specific bug.

@georgefst
Copy link
Collaborator

Thanks. That's odd. I can't think of anything relevant that we alter from Ormolu here. And I'm quite surprised this isn't caught be our test suite.

@georgefst
Copy link
Collaborator

georgefst commented Jun 23, 2022

I've tried Ormolu 0.5 and Fourmolu 0.7, both built with the same ghc-lib-parser and can observe a difference.

Enabling/disabling DataKinds or TypeApplications doesn't change anything.

@ocharles
Copy link
Author

ocharles commented Jun 24, 2022

@georgefst Just to clarify, do you mean you can reproduce this bug with Ormolu as well? (Also, hi, isn't it funny that I was adamant I hate autoformatting at ZuriHac, and here I am weeks later, auto-formatting 😝 Great job to all of you with Fourmolu - I'm happy with it!)

@georgefst
Copy link
Collaborator

I mean I can reproduce with Fourmolu but not Ormolu.

@brandonchinn178 Any ideas? A bad merge somewhere?

@brandonchinn178
Copy link
Collaborator

Possibly. I looked at Meat/Value.hs at the AppType section, but it looked the same on both ends. Will probably need good ol' debugging work

@georgefst
Copy link
Collaborator

Also, hi, isn't it funny that I was adamant I hate autoformatting at ZuriHac, and here I am weeks later, auto-formatting

Ha, yeah, glad to have changed your mind.

I don't think I actually caught your name at the time, and only realised who you were a few days later when something about reactive-banana came up and I realised you were the maintainer (I recognise you from a few things e.g. we use your Nix Buildkite plugin at work)!

@ocharles
Copy link
Author

Yes, ZuriHac needs bigger name badges 😆

@ocharles
Copy link
Author

If it helps, this only happens when using quoted constructors - e.g., foo @True is fine, foo @'True is problematic.

@SeungheonOh
Copy link

SeungheonOh commented Aug 27, 2022

I found out putting a ticked type inside of parenthesis makes parser happy again.

-- Unhappy parser
id @'Int

-- Happy parser
id @('Int)

It's quite annoying workaround for -Wunticked-promoted-constructors... I hope it get fixed.

@elland
Copy link

elland commented Oct 26, 2022

Is this still being worked on?

@brandonchinn178
Copy link
Collaborator

No one's working on it yet. Feel free to open a PR

@brandonchinn178
Copy link
Collaborator

Looks like a bad merge 😢

Ormolu fixed this back in 0.1.4.0: tweag/ormolu@286afb7#diff-88777f1040ac8f0f2e2903f2db73e3604774239dff0453bc9fcb68c6ceab1f66

I merged this into Fourmolu as one of my first contributions, and this was before Fourmolu was systematically merging Ormolu changes in, so it was a huge change that I probably overlooked this conflict.
948e32d

I'll make the change (and add a regression test cuz why not)

@SeungheonOh
Copy link

Thank you for the fix! Can I know when this bugfix will be released?

@brandonchinn178
Copy link
Collaborator

I'm hoping to get #245 merged in for the next release, so maybe in the next week or so?

@brandonchinn178
Copy link
Collaborator

Released v0.9.0.0

@SeungheonOh
Copy link

SeungheonOh commented Nov 10, 2022

Awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants