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

Against space in record updates and record patterns #565

Open
cjay opened this issue Apr 25, 2020 · 12 comments
Open

Against space in record updates and record patterns #565

cjay opened this issue Apr 25, 2020 · 12 comments
Labels
style Nitpicking and things related to purely visual aspect for formatting.

Comments

@cjay
Copy link

cjay commented Apr 25, 2020

Currently, Ormolu always seems to format record updates and patterns with a space between the record data constructor or variable and the { like this:
foo {bar} instead of alternatives like foo{ bar } or foo{bar}.

This has the disadvantage that the {} part looks like an additional function parameter when used in function calls or pattern matches in function definitions, which is especially confusing to newbies:
foo a {b} c -- function call: looks like 3 parameters but is actually 2
foo Bar {baz} blerg = ... -- same with pattern match in function definition

Personally, I prefer foo{ bar }. It makes visually clear that the {} part belongs to the preceding identifier, while having superior readability compared to foo{bar}. Side note, it is only one character longer than the current foo {bar}.

If there are good reasons for this design decision, it would be nice to have them collected in the design document, or at least in this issue.

@neongreen
Copy link
Collaborator

I always assumed that foo{ bar } was making it slightly easier for people to grasp that {bar} is not a separate parameter, but honestly I never actually asked :/

Does anybody have relevant experience / feedback from people?

(Personally I prefer foo{ bar } as well.)

@mrkkrp mrkkrp added the style Nitpicking and things related to purely visual aspect for formatting. label Apr 25, 2020
@mrkkrp
Copy link
Member

mrkkrp commented Apr 25, 2020

Right now spacing of {} is the same as what we do for () and []. I personally do not find the motivation in the issue good enough to special case {}.

@mboes
Copy link
Member

mboes commented May 2, 2020

{}, () and [] are all symbols that come in pairs, but in Haskell that's where the similarity stops. Unlike (...) and [...], {...} is never an expression. In fact, it's always just part of a syntactic construct for expressions, like case x of {...}, do {...}, foo {...}. Record updates bind more tightly than applications. It's an unfortunate design decision that SPJ is on record saying he regrets, but here we are. So it makes sense to avoid any formatting that would suggest that {...} is an argument passed to some function. Or worse, that it's an extra argument when appearing in a pattern. On the other hand, do we want to special-case record-updates relative to the styling for every other use of braces? Not sure.

@mrkkrp
Copy link
Member

mrkkrp commented May 2, 2020

The fact that {...} can never be an expression makes it clear and unambiguous that when we see braces, they only can be part of the preceding record. Which to me speaks in favor of keeping the formatting as it is now.

@cdsmith
Copy link

cdsmith commented Jun 9, 2020

Just to share this experience, I spent a long time fixing this manually every time I ran Ormolu. Of course, it got exhausting. My solution was finally to add parentheses around record syntax in parameters. That clarifies the precedence just as well. It's a little more noisy, but I think it's worth it given that the precedence is obviously wrong in the first place. I definitely recommend just adding the parentheses, after which the extra space doesn't really matter.

@neongreen
Copy link
Collaborator

My solution was finally to add parentheses around record syntax in parameters.

The problem with this is that some people can't stand unnecessary tokens in code, and scoff at extraneous parentheses. "Did you know that record update has higher precedence than function application?", yes, yes, I know that.

Otherwise I like this solution, but I'd like the space to go away even more. "[...] makes it clear and unambiguous" — only for people who know subtleties of the Haskell grammar.

@Avi-D-coder
Copy link
Contributor

Avi-D-coder commented Jun 9, 2020

I believe this issue would break ormolu's support for record-dot-preprocessor syntax which uses the space to differentiate desugaring.

@cjay
Copy link
Author

cjay commented Jun 9, 2020

I believe this issue would break ormolu's support for record-dot-preprocessor syntax which uses the space to differentiate desugaring.

I suppose that preprocessor should be deprecated as soon as this accepted proposal is implemented in ghc though.

@cdsmith
Copy link

cdsmith commented Jun 9, 2020

@Avi-D-coder I don't think so. This issue is about the space between a variable or constructor name and a { that follows it. The whitespace-sensitivity in record dot syntax doesn't really affect this case, does it?

@Avi-D-coder
Copy link
Contributor

Your right I miss read it.

@cjay
Copy link
Author

cjay commented Jun 9, 2020

@cdsmith the space is used to determine if the record update is done via a HasField instance or not, see the last paragraph here.

I'm pretty sure the accepted GHC proposal always uses HasField and there is no whitespace-sensitivity there, right? Interestingly, when supporting the preprocessor, Ormolu already has to preserve the non-existence of whitespace before the {. And any code that uses HasField via the preprocessor can already have no space before { in record updates.

@cdsmith
Copy link

cdsmith commented Jun 9, 2020

@cjay Yes, I see you're right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
style Nitpicking and things related to purely visual aspect for formatting.
Projects
None yet
Development

No branches or pull requests

6 participants