tweag / ormolu

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

Against space in record updates and record patterns #565

Open cjay opened 4 years ago

cjay commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

{}, () 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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

cjay commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

Your right I miss read it.

cjay commented 4 years ago

@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 commented 4 years ago

@cjay Yes, I see you're right.