Incorrect break caused by and cause of column-violation #159

New Issue

2018-07-05T19:47:25+02:00

pbrisbin commented

2018-07-05 19:47:25 +02:00

(Migrated from github.com)

I'm encountering a case where Brittany gets the line-breaks of an it "" . f $ do expression wrong, which seems to be both caused by, and the cause of, a trailing comment going past our column limit many expressions away.

Here's what it gets wrong:

spec = do
  it "creates a snapshot at the given level"
    . withGraph runDB
    $ do
        lift $ do
          studentDiagnosticReadingLevel updatedStudent `shouldBe` Just 10 -- still
          elaSnapshotReadingLevel snapshot `shouldBe` 12

It should look like this:

  it "creates a snapshot at the given level" . withGraph runDB $ do
    lift $ do
      studentDiagnosticReadingLevel updatedStudent `shouldBe` Just 10 -- still
      elaSnapshotReadingLevel snapshot `shouldBe` 12

(This has been extracted from a larger test case, so forgive the redundant dos and lack of context on what any of this means.)

It seems to be caused by the trailing -- still which is a column violoation but only when the it formatting is incorrect.

If we remove it, Brittany gets things right:

  it "creates a snapshot at the given level" . withGraph runDB $ do
    lift $ do
      studentDiagnosticReadingLevel updatedStudent `shouldBe` Just 10
      elaSnapshotReadingLevel snapshot `shouldBe` 12

We can also force Brittany to get things right by removing one of the lines (either one will do):

  it "creates a snapshot at the given level" . withGraph runDB $ do
    lift $ do
      studentDiagnosticReadingLevel updatedStudent `shouldBe` Just 10 -- still

  it "creates a snapshot at the given level" . withGraph runDB $ do
    lift $ do
      elaSnapshotReadingLevel snapshot `shouldBe` 12

So somehow the number of expressions in that last do matters as well.

I'm a bit baffled :)

And here's our config for reference

---
conf_debug:
  dconf_roundtrip_exactprint_only: false
  dconf_dump_bridoc_simpl_par: false
  dconf_dump_ast_unknown: false
  dconf_dump_bridoc_simpl_floating: false
  dconf_dump_config: false
  dconf_dump_bridoc_raw: false
  dconf_dump_bridoc_final: false
  dconf_dump_bridoc_simpl_alt: false
  dconf_dump_bridoc_simpl_indent: false
  dconf_dump_annotations: false
  dconf_dump_bridoc_simpl_columns: false
  dconf_dump_ast_full: false
conf_forward:
  options_ghc:
    - -XBangPatterns
    - -XConstraintKinds
    - -XDataKinds
    - -XDeriveDataTypeable
    - -XDeriveGeneric
    - -XDoAndIfThenElse
    - -XEmptyDataDecls
    - -XFlexibleContexts
    - -XFlexibleInstances
    - -XFunctionalDependencies
    - -XGADTs
    - -XKindSignatures
    - -XLambdaCase
    - -XMultiParamTypeClasses
    - -XMultiWayIf
    - -XNamedFieldPuns
    - -XNoImplicitPrelude
    - -XNoMonomorphismRestriction
    - -XOverloadedStrings
    - -XPolyKinds
    - -XQuasiQuotes
    - -XRank2Types
    - -XRecordWildCards
    - -XScopedTypeVariables
    - -XStandaloneDeriving
    - -XTemplateHaskell
    - -XTupleSections
    - -XTypeApplications
    - -XTypeFamilies
    - -XTypeOperators
    - -XViewPatterns
conf_errorHandling:
  econf_ExactPrintFallback: ExactPrintFallbackModeInline
  econf_Werror: false
  econf_omit_output_valid_check: false
  econf_produceOutputOnErrors: false
conf_preprocessor:
  ppconf_CPPMode: CPPModeAbort
  ppconf_hackAroundIncludes: false
conf_version: 1
conf_layout:
  lconfig_altChooser:
    tag: AltChooserBoundedSearch
    contents: 3
  lconfig_importColumn: 60
  lconfig_alignmentLimit: 1
  lconfig_indentListSpecial: true
  lconfig_indentAmount: 2
  lconfig_alignmentBreakOnMultiline: true
  lconfig_cols: 80
  lconfig_indentPolicy: IndentPolicyLeft
  lconfig_indentWhereSpecial: true
  lconfig_columnAlignMode:
    tag: ColumnAlignModeDisabled
    contents: 0.7

I'm encountering a case where Brittany gets the line-breaks of an `it "" . f $ do` expression wrong, which seems to be both caused by, and the cause of, a trailing comment going past our column limit many expressions away. Here's what it gets wrong: ```hs spec = do it "creates a snapshot at the given level" . withGraph runDB $ do lift $ do studentDiagnosticReadingLevel updatedStudent `shouldBe` Just 10 -- still elaSnapshotReadingLevel snapshot `shouldBe` 12 ``` It should look like this: ```hs it "creates a snapshot at the given level" . withGraph runDB $ do lift $ do studentDiagnosticReadingLevel updatedStudent `shouldBe` Just 10 -- still elaSnapshotReadingLevel snapshot `shouldBe` 12 ``` (This has been extracted from a larger test case, so forgive the redundant `do`s and lack of context on what any of this means.) It seems to be caused by the trailing `-- still` which is a column violoation _but only when the `it` formatting is incorrect_. If we remove it, Brittany gets things right: ```hs it "creates a snapshot at the given level" . withGraph runDB $ do lift $ do studentDiagnosticReadingLevel updatedStudent `shouldBe` Just 10 elaSnapshotReadingLevel snapshot `shouldBe` 12 ``` We can also force Brittany to get things right by removing one of the lines (either one will do): ```hs it "creates a snapshot at the given level" . withGraph runDB $ do lift $ do studentDiagnosticReadingLevel updatedStudent `shouldBe` Just 10 -- still it "creates a snapshot at the given level" . withGraph runDB $ do lift $ do elaSnapshotReadingLevel snapshot `shouldBe` 12 ``` So somehow the number of expressions in that last `do` matters as well. I'm a bit baffled :) <details> <summary>And here's our config for reference</summary> ```yaml --- conf_debug: dconf_roundtrip_exactprint_only: false dconf_dump_bridoc_simpl_par: false dconf_dump_ast_unknown: false dconf_dump_bridoc_simpl_floating: false dconf_dump_config: false dconf_dump_bridoc_raw: false dconf_dump_bridoc_final: false dconf_dump_bridoc_simpl_alt: false dconf_dump_bridoc_simpl_indent: false dconf_dump_annotations: false dconf_dump_bridoc_simpl_columns: false dconf_dump_ast_full: false conf_forward: options_ghc: - -XBangPatterns - -XConstraintKinds - -XDataKinds - -XDeriveDataTypeable - -XDeriveGeneric - -XDoAndIfThenElse - -XEmptyDataDecls - -XFlexibleContexts - -XFlexibleInstances - -XFunctionalDependencies - -XGADTs - -XKindSignatures - -XLambdaCase - -XMultiParamTypeClasses - -XMultiWayIf - -XNamedFieldPuns - -XNoImplicitPrelude - -XNoMonomorphismRestriction - -XOverloadedStrings - -XPolyKinds - -XQuasiQuotes - -XRank2Types - -XRecordWildCards - -XScopedTypeVariables - -XStandaloneDeriving - -XTemplateHaskell - -XTupleSections - -XTypeApplications - -XTypeFamilies - -XTypeOperators - -XViewPatterns conf_errorHandling: econf_ExactPrintFallback: ExactPrintFallbackModeInline econf_Werror: false econf_omit_output_valid_check: false econf_produceOutputOnErrors: false conf_preprocessor: ppconf_CPPMode: CPPModeAbort ppconf_hackAroundIncludes: false conf_version: 1 conf_layout: lconfig_altChooser: tag: AltChooserBoundedSearch contents: 3 lconfig_importColumn: 60 lconfig_alignmentLimit: 1 lconfig_indentListSpecial: true lconfig_indentAmount: 2 lconfig_alignmentBreakOnMultiline: true lconfig_cols: 80 lconfig_indentPolicy: IndentPolicyLeft lconfig_indentWhereSpecial: true lconfig_columnAlignMode: tag: ColumnAlignModeDisabled contents: 0.7 ``` </details>

lspitzner commented

2018-07-05 21:39:18 +02:00

(Migrated from github.com)

Interesting, thanks for reporting.

Fixed in 3c5670d5cd.

This was connected to forcing a multi-line layout for sequences of operator applications, e.g.

test =
  (  a -- abc
  ++ b -- test
  ++ c
  )

where there is a single-line layout but we don't want it due to the comments. This "layouting rule" also prevented the put-things-in-same-line in it .. . withGraph .. $ do .. because the do part contained, somewhere below, a comment.

Fix was to be less eager when searching for comments, i.e. we now ignore comments on the last element of such a operator sequence.

Interesting, thanks for reporting. Fixed in 3c5670d5cdb9373e18ef9588729a38a7a9f81534. This was connected to forcing a multi-line layout for sequences of operator applications, e.g. ~~~~.hs test = ( a -- abc ++ b -- test ++ c ) ~~~~ where there is a single-line layout but we don't want it due to the comments. This "layouting rule" also prevented the put-things-in-same-line in `it .. . withGraph .. $ do ..` because the `do` part contained, somewhere below, a comment. Fix was to be less eager when searching for comments, i.e. we now ignore comments on the last element of such a operator sequence.

🎉 3

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#159