Consistency of RecordWildCards formatting

[simonchatts]

Thanks for making Brittany!

Currently Brittany formats RecordWildCard pattern matches as {..} (no spaces) but expressions as { .. } (with spaces):

{-# LANGUAGE RecordWildCards #-}

data Record = Record { foo :: Int }

f1 :: IO Record
f1 = do
  Record {..} <- getRecord
  pure Record { .. } -- <=== note extra spaces

f2 :: Record -> ()
f2 (Record {..}) = ()

I have a (two-line) PR to make the expression also {..} but first wanted to check: would this be an acceptable PR? There's an explicit test for the current policy, but consistently no-space makes more sense to me.

(One step further: Personally, I can see the argument for omitting the leading space at all, to more clearly express how, eg, pure Record {..} is parsed as pure $ Record {..}, so is more explicit as pure Record{..}. But I can see how this would be a more controversial change, so don't want to de-rail a potentially simple discussion.)

[tfausak]

Thanks for reporting this! It sounds similar to (but different from) #126 and #223.

[simonchatts]

Thanks for the pointers! Having had a look at those, it's still not obvious to me what the consistent policy would be here:

• Is a RWC invocation more like using a regular record, in which case UseSpaces { .. }?
• Or is it like the other use of (..) in module import/exports, which uses NoSpaces(..)?

I'm happy to submit a PR either way - any views? (Personally I prefer the latter, for the parsing rationale in the original comment, but I suspect the former may be more consistent...?)

[eborden]

There are three areas where space can exist, before the first bracket, after the first bracket and before the second bracket.

The reasonable options are:

Record{..}
Record {..}
Record { .. }
Record{ .. }

I'm partial to Record{..}, but that ship has likely sailed. I would argue in favor of Record { .. } since it is consistently spacious. As well it is consistent with placing spaces around brackets in single line record syntax.

Record { foo = _, bar = _ }

[lspitzner]

We should also consider Record{}. Together, we currently have

match Record{}            = 1
match Record {..}         = 2
match Record { a }        = 3
match Record { a, b }     = 4
match Record { a, ..}     = 5        -- inconsistent before closing }
match Record { a = True } = 6

construct 1 = Record{}
construct 2 = Record { .. }          -- inconsistent with pattern-match
construct 3 = Record { a }
construct 4 = Record { a, b }
construct 5 = Record { a , .. }      -- inconsistent before comma
construct 6 = Record { a = True }

update 1 = record{}
update 3 = record { a }
update 4 = record { a, b }
update 6 = record { a = True }

I think that consistency between pattern-matching and the corresponding expression is most important. I think that in general, consistency between pattern-matching, construction and update is most important, i.e. the three items tagged above. Consistency between different patterns / different expressions would be nice (e.g. whether we have the space before "{"), and consistency with other constructs (lists, import statement parens etc.) comes after that.

(I just opened #257 which also discusses spaces (for lists instead of records). Slightly related.)

I'm partial to Record{..}, but that ship has likely sailed.

I still do currently consider this an option. Although I admit that the only consistent application of this would introduce a rather big change.

Can we get proposals for solutions to these inconsistencies, applied to the full example above?

[lspitzner]

The "rather big change" would be the following:

match Record{}           = 1
match Record{..}         = 2
match Record{ a }        = 3
match Record{ a, b }     = 4
match Record{ a, .. }    = 5
match Record{ a = True } = 6

construct 1 = Record{}
construct 2 = Record{..}
construct 3 = Record{ a }
construct 4 = Record{ a, b }
construct 5 = Record{ a, .. }
construct 6 = Record{ a = True }

update 1 = record{}
update 3 = record{ a }
update 4 = record{ a, b }
update 6 = record{ a = True }

(2 vs 5 is still inconsistent, arguably.)

[eborden]

I'm not against the "big change" as it binds the record identifier tightly with the opening bracket.

For the spacious bracket you'd have:

match Record {}           = 1
match Record { .. }       = 2
match Record { a }        = 3
match Record { a, b }     = 4
match Record { a, .. }    = 5
match Record { a = True } = 6

construct 1 = Record {}
construct 2 = Record { .. }
construct 3 = Record { a }
construct 4 = Record { a, b }
construct 5 = Record { a, .. }
construct 6 = Record { a = True }

update 1 = record {}
update 3 = record { a }
update 4 = record { a, b }
update 6 = record { a = True }

[eborden]

@lspitzner one way to avoid inconsistency on your previous layout is to have spacious wildcards, but tight identifier/bracket binding.

match Record{}           = 1
match Record{ .. }       = 2
match Record{ a }        = 3
match Record{ a, b }     = 4
match Record{ a, .. }    = 5
match Record{ a = True } = 6

construct 1 = Record{}
construct 2 = Record{ .. }
construct 3 = Record{ a }
construct 4 = Record{ a, b }
construct 5 = Record{ a, .. }
construct 6 = Record{ a = True }

update 1 = record{}
update 3 = record{ a }
update 4 = record{ a, b }
update 6 = record{ a = True }

In all examples I assume we are retaining the vertical layout

construct 7 = Record
  { a = True
  , b = False
  }

update 7 = record
  { a = True
  , b = False
  }

[simonchatts]

There's also the

update 8 = record{ a = True }{ b = False }

class of case. Again, on balance, I think I come down on the ("big change") policy just quoted, since, eg:

pure record{ a = True }{ b = False }

does indeed parse as:

pure $ record { a = True } { b = False }

I don't see much code manually formatted with this convention today, but on balance I think it's clearer than the alternatives. And certainly theoretical inconsistency between that and

pure record{..}

is tolerable IMO.

This observation is in favour of the "big change", on the grounds that the whitespace usefully communicates binding precedence. (I'm willing to have a crack at implementing whatever policy the discussion concludes.)

[eborden]

This is the big win for me. The fact that record update/construction binds tighter than application makes removing the spurious space (between the identifier and the opening bracket) a positive visual cue.

pure record{ a = True }{ b = False }

does indeed parse as:

pure $ record { a = True } { b = False }

[eborden]

@lspitzner given your :+1:, it looks like you endorse this layout: https://github.com/lspitzner/brittany/issues/251#issuecomment-527611371

@simonchatts has proposed opening a PR, should he move forward with this layout?

[lspitzner]

Yes, I am fine with merging a PR with the layout from #251 (comment). It seems certainly a bit more consistent than what we have at the moment.

As we are still pre 1.0, I say we accept this breaking change, even without making it configurable. Should people dislike this change, we can always come back and add a second option behind a config flag that toggles Record{ vs Record {.

@simonchatts thanks for offering to work on this! Feel free to contact me if you have any questions on the implementation.

[eborden]

As we are still pre 1.0, I say we accept this breaking change, even without making it configurable. Should people dislike this change, we can always come back and add a second option behind a config flag that toggles Record{ vs Record {.

Agreed on both points.

[simonchatts]

@lspitzner I'm afraid Life has gotten in the way of this, and I won't be able to submit a good PR any time soon. It's probably best if you un-assign this, so that other interested folks aren't dissuaded from taking a shot.