Operator precedence sensitive layout #79

New Issue

2017-12-13T10:56:16+01:00

sniperrifle2004 commented

2017-12-13 10:56:16 +01:00

(Migrated from github.com)

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?

Consider the following piece of code as formatted by britanny: ```haskell 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: ```haskell 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?

👍 9

eschnett commented

2017-12-13 18:26:45 +01:00

(Migrated from github.com)

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.

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 commented

2017-12-13 18:43:34 +01:00

(Migrated from github.com)

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

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. ``` hs dP = (string "ne" $> NE) <|> (string "nw" $> NW) -- ... ``` ``` hs dP = foldr (<|>) empty [ string "ne" $> NE , string "nw" $> NW -- ... ```

👍 1

lspitzner commented

2017-12-17 15:28:19 +01:00

(Migrated from github.com)

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.

> 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 ~~~~.hs 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 commented

2018-05-29 10:22:41 +02:00

(Migrated from github.com)

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.

I took a quick look, and it seems that fixity/precedence is indeed found in [the interface for the file](https://hackage.haskell.org/package/ghc-8.4.1/docs/HscTypes.html#t:ModIface). You can get one of these interfaces using the [`LoadIface module`](https://hackage.haskell.org/package/ghc-8.4.1/docs/LoadIface.html) in ghc.

👍 1

lukaszlew commented

2018-06-09 00:11:58 +02:00

(Migrated from github.com)

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.

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 commented

2018-09-11 22:24:31 +02:00

(Migrated from github.com)

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.

👍 2

emilypi commented

2018-09-11 22:26:23 +02:00

(Migrated from github.com)

You have my vote.

eborden commented

2018-09-11 23:39:48 +02:00

(Migrated from github.com)

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 commented

2018-12-15 04:28:18 +01:00

(Migrated from github.com)

Very much want this feature also.

👍 1

23Skidoo commented

2019-11-19 21:03:50 +01:00

(Migrated from github.com)

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

23Skidoo commented

2019-11-19 21:37:30 +01:00

(Migrated from github.com)

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

eborden commented

2019-11-19 22:14:45 +01:00

(Migrated from github.com)

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)

It is worth looking at [Ormolu's handling](https://github.com/tweag/ormolu/blob/0a8cfd62eff05b092a2b99754ed6f6fa3788132b/src/Ormolu/Printer/Operators.hs). 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: ```hs dP = string "ne" $> NE <|> string "nw" $> NW <|> string "n" $> N <|> string "se" $> SE <|> string "sw" $> SW <|> string "s" $> S ``` After: ```hs dP = string "ne" $> NE <|> string "nw" $> NW <|> string "n" $> N <|> string "se" $> SE <|> string "sw" $> SW <|> string "s" $> S ``` Before ```hs dP = string "ne" $> NE <|> string "nw" $> NW <|> string "n" $> N <|> string "se" $> SE <|> string "sw" $> SW <|> string "s" $> S ``` after ```hs 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: ```hs dP = string "ne" $> NE <|> string "nw" $> NW <|> string "n" $> N <|> string "se" $> SE <|> string "sw" $> SW <|> string "s" $> S ``` After: ```hs 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: ```hs dP = string "ne" $> NE <|> string "nw" $> NW <|> string "n" $> N <|> string "se" $> SE <|> string "sw" $> SW <|> string "s" $> S ``` After: ```hs dP = string "ne" $> NE <|> string "nw" $> NW <|> string "n" $> N <|> string "se" $> SE <|> string "sw" $> SW <|> string "s" $> S ``` Before: ```hs foo = aaa . bbb. ccc . ddd . eee . fff . ggg . hhh . iii . jjj . lll . mmm . nnn . ooo . ppp . qqq . rrr . sss . ttt $ uuu ``` After: ```hs 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: ```hs foo & persistFieldLens LicenseSubject .~ SubjectMath . persistFieldLens LicenseStartsAt .~ (asOf `subtractDays` 1) . persistFieldLens LicenseExpiresAt .~ (asOf `addDays` 1) ``` After: ```hs 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: ```hs foo & persistFieldLens LicenseSubject .~ SubjectMath . persistFieldLens LicenseStartsAt .~ (asOf `subtractDays` 1) . persistFieldLens LicenseExpiresAt .~ (asOf `addDays` 1) ``` After: ```hs foo & persistFieldLens LicenseSubject .~ SubjectMath . persistFieldLens LicenseStartsAt .~ (asOf `subtractDays` 1) . persistFieldLens LicenseExpiresAt .~ (asOf `addDays` 1) ```

codygman commented

2020-06-03 17:08:49 +02:00

(Migrated from github.com)

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

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

Sign in to join this conversation.

No Milestone

No project

No Assignees

1 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: hexagoxel/brittany#79