ormolu icon indicating copy to clipboard operation
ormolu copied to clipboard

Against space in record updates and record patterns

Open cjay opened this issue 4 years ago • 12 comments

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.

cjay avatar Apr 25 '20 00:04 cjay

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

neongreen avatar Apr 25 '20 06:04 neongreen

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 {}.

mrkkrp avatar Apr 25 '20 08:04 mrkkrp

{}, () 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.

mboes avatar May 02 '20 17:05 mboes

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.

mrkkrp avatar May 02 '20 18:05 mrkkrp

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.

cdsmith avatar Jun 09 '20 14:06 cdsmith

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.

neongreen avatar Jun 09 '20 18:06 neongreen

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

Avi-D-coder avatar Jun 09 '20 20:06 Avi-D-coder

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.

cjay avatar Jun 09 '20 20:06 cjay

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

cdsmith avatar Jun 09 '20 20:06 cdsmith

Your right I miss read it.

Avi-D-coder avatar Jun 09 '20 20:06 Avi-D-coder

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

cjay avatar Jun 09 '20 23:06 cjay

@cjay Yes, I see you're right.

cdsmith avatar Jun 09 '20 23:06 cdsmith