Operator precedence sensitive layout

[sniperrifle2004]

Consider the following piece of code as formatted by britanny:

dP =
  string "ne"
    $>  NE
    <|> string "nw"
    $>  NW
    <|> string "n"
    $>  N
    <|> string "se"
    $>  SE
    <|> string "sw"
    $>  SW
    <|> string "s"
    $>  S

This doesn't reflect the precedence of the operators involved very well and also makes the meaning less clear.

I would expect it to be formatted as such:

dP =
  string "ne" $>  NE
  <|> string "nw" $>  NW
  <|> string "n" $>  N
  <|> string "se" $>  SE
  <|> string "sw" $>  SW
  <|> string "s" $>  S

And I am sure there are other cases where long expressions with different operators could be layed out better if precedence was taken into account.

Is this something brittany can do?

[eschnett]

I don't know where the Haskell compiler learns about operator precedence and fixity. In case this information has to be read from *.hi files, I would want to suggest a language extension:

Allow structured comments at the beginning of a Haskell source file that re-declare operator precedence and fixity. Brittany would look at these comments to decide on the layout. In the future, hlint would verify these declarations, and intero would semi-automatically add such comments where missing.

[tfausak]

I know these aren't "good" solutions, especially with respect to pretty printers, but I would work around this by either using parentheses or using a function rather than an operator.

dP =
  (string "ne" $> NE)
  <|> (string "nw" $> NW)
  -- ...

dP = foldr (<|>) empty
  [ string "ne" $> NE
  , string "nw" $> NW
  -- ...

[lspitzner]

Is this something brittany can do?

as usual: not yet :p

For general information (some of you might be aware of this, but i'll make it explicit): The ghc parser does not consider precedences/fixities. The AST for

dP =
  string "ne" $>  NE
  <|> string "nw" $>  NW
  <|> string "n" $>  N

looks like

OpApp
  OpApp
    OpApp
      OpApp
        OpApp
          HsApp
            HsVar (Unqual {OccName: string})
            HsLit (HsString "\"ne\"")
          HsVar (Unqual {OccName: $>})
          HsVar (Unqual {OccName: NE})
        HsVar (Unqual {OccName: <|>})
        HsApp
          HsVar (Unqual {OccName: string})
          HsLit (HsString "\"nw\"")
      HsVar (Unqual {OccName: $>})
      HsVar (Unqual {OccName: NW})
    HsVar (Unqual {OccName: <|>})
    HsApp
      HsVar (Unqual {OccName: string})
      HsLit (HsString (SourceText "\"n\"")
  HsVar (Unqual {OccName: $>})
  HsVar (Unqual {OccName: N})

And ghc re-balances this tree at a later stage. (Indeed it probably needs *.hi files for that.)

So brittany currently does the only thing it can do given the limited information present in the AST.

If we had some/any information about precedences and fixities then brittany could re-balance the tree and layout appropriately (by changing the layouter for nested OpApps - not a trivial task, but manageable).

I am tempted to discuss options of where this information comes from (tooling like @eschnett proposes, config file, hardcode etc.) but really we don't need to decide on this yet: I think the first step regardless is to add a corresponding field to the config that contains precedence/fixity information for some operators, and writing implementation that consumes this information. We can fill it initially with some hardcoded-info (e.g. just ($) for starters) but it should be no problem improving on the providing side later.

[mezuzza]

I took a quick look, and it seems that fixity/precedence is indeed found in the interface for the file. You can get one of these interfaces using the LoadIface module in ghc.

[lukaszlew]

We are using Lenses a lot and have long 1, 2, 3-line-long 'lens paths'. Many of them in certain functions. You can imagine that they get very vertical (x3 to x7 in line numbers).

This seems to be a single biggest obstacle to acceptance.

[mightybyte]

I'd like to add another vote for this feature. I was just about to create an issue saying exactly the same thing. At the moment this seems like the most likely blocker to getting my team to use brittany.

[emilypi]

You have my vote.

[eborden]

I'm also heavily in favor of this feature. This would be a boon for many operator heavy use cases, like lens and DSLs.

[jwiegley]

Very much want this feature also.

[23Skidoo]

We (@scrive) use Brittany on a fairly large codebase, and this issue is our main pain point with it.

[23Skidoo]

I am also told that Ormolu does a better job at this.

[eborden]

It is worth looking at Ormolu's handling. It seems Ormolu mostly side steps the fixity issue by retaining new lines. If we take the example above and format it we will see this in action.

Previous examples

Before:

dP =
  string "ne"
    $>  NE
    <|> string "nw"
    $>  NW
    <|> string "n"
    $>  N
    <|> string "se"
    $>  SE
    <|> string "sw"
    $>  SW
    <|> string "s"
    $>  S

After:

dP =
  string "ne"
    $>  NE
    <|> string "nw"
    $>  NW
    <|> string "n"
    $>  N
    <|> string "se"
    $>  SE
    <|> string "sw"
    $>  SW
    <|> string "s"
    $>  S

Before

dP =
  string "ne" $>  NE
  <|> string "nw" $>  NW
  <|> string "n" $>  N
  <|> string "se" $>  SE
  <|> string "sw" $>  SW
  <|> string "s" $>  S

after

dP =
  string "ne" $> NE
    <|> string "nw" $> NW
    <|> string "n" $> N
    <|> string "se" $> SE
    <|> string "sw" $> SW
    <|> string "s" $> S

New line preservation

Ormolu seems to really be concerned with consistent use of indentation, not newline normalization. We can see that here.

Before:

dP =
  string "ne"
     $>  NE
  <|> string "nw"
          $>  NW
  <|> string "n"
        $>  N
  <|> string "se" $>  SE
  <|> string "sw" $>  SW
  <|> string "s" $>  S

After:

dP =
  string "ne"
    $> NE
    <|> string "nw"
      $> NW
    <|> string "n"
      $> N
    <|> string "se" $> SE
    <|> string "sw" $> SW
    <|> string "s" $> S

Long lines

Interestingly these longs lines don't seem to trigger anything.

Before:

dP = string "ne" $> NE <|> string "nw" $> NW <|> string "n" $> N <|> string "se" $> SE <|> string "sw" $>  SW <|> string "s" $> S

After:

dP = string "ne" $> NE <|> string "nw" $> NW <|> string "n" $> N <|> string "se" $> SE <|> string "sw" $> SW <|> string "s" $> S

Before:

foo = aaa . bbb. ccc . ddd . eee . fff . ggg . hhh . iii . jjj . lll . mmm . nnn . ooo . ppp . qqq . rrr . sss . ttt $ uuu

After:

foo = aaa . bbb . ccc . ddd . eee . fff . ggg . hhh . iii . jjj . lll . mmm . nnn . ooo . ppp . qqq . rrr . sss . ttt $ uuu

Wacky lens usage

This wacky lens formatting does trigger some formatting.

Before:

foo & persistFieldLens LicenseSubject .~ SubjectMath . persistFieldLens
  LicenseStartsAt
      .~ (asOf `subtractDays` 1) . persistFieldLens LicenseExpiresAt .~ (asOf `addDays` 1)

After:

foo & persistFieldLens LicenseSubject
  .~ SubjectMath
    . persistFieldLens
      LicenseStartsAt
  .~ (asOf `subtractDays` 1) . persistFieldLens LicenseExpiresAt
  .~ (asOf `addDays` 1)

Though as Ormolu seems to focus heavily on new line preservation we get different types of formats depending on how we input our lens wackyness.

Before:

foo & persistFieldLens LicenseSubject .~ SubjectMath
 . persistFieldLens
  LicenseStartsAt
      .~ (asOf `subtractDays` 1) . persistFieldLens LicenseExpiresAt .~ (asOf `addDays` 1)

After:

foo & persistFieldLens LicenseSubject .~ SubjectMath
  . persistFieldLens
    LicenseStartsAt
    .~ (asOf `subtractDays` 1)
  . persistFieldLens LicenseExpiresAt .~ (asOf `addDays` 1)

[codygman]

The parens workaround wasn't too bad, but brittany supporting this would be very nice :smile: