Skip to content

Don't fail when rendering an Unbounded layout of a grouped Line#92

Closed
sjakobi wants to merge 1 commit intohaskell-prettyprinter:masterfrom
sjakobi:91-fail
Closed

Don't fail when rendering an Unbounded layout of a grouped Line#92
sjakobi wants to merge 1 commit intohaskell-prettyprinter:masterfrom
sjakobi:91-fail

Conversation

@sjakobi
Copy link
Copy Markdown
Collaborator

@sjakobi sjakobi commented Nov 4, 2019

Fixes Doesn't fix #91.


TODO:

  • Add a more complex test case
  • Check whether any documentation needs updating or clarification.

-> SimpleDocStream ann -- ^ Choice B.
-> SimpleDocStream ann -- ^ Choice A if it fits, otherwise B.
selectNicer (FittingPredicate fits) lineIndent currentColumn x y
| isFail x = y
Copy link
Copy Markdown
Collaborator Author

@sjakobi sjakobi Nov 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering whether we need to worry about SFail appearing elsewhere than the beginning of a left Union branch. At least fuse doesn't seem to "hide" the Fail in a (Union Fail x) though:

https://github.com/quchen/prettyprinter/blob/4d51362d7ea8bca0fe9e7ffec2ccd8fcd0065c0d/prettyprinter/src/Data/Text/Prettyprint/Doc/Internal.hs#L1407

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isFail is False if the document fails, but that’s the same thing the two fitting predicates (for layoutPretty and layoutSmart) yield. What does the explicit isFail clause change?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've explained this in #91 (comment).

The short version is that both fitting predicates shortcut to True when the maxWidth is Nothing – which is the case with Unbounded layout options.

@sjakobi
Copy link
Copy Markdown
Collaborator Author

sjakobi commented Nov 5, 2019

This doesn't seem to fix the issue completely:

If I change dhall's layoutOpts to use Unbounded and run its test suite with this branch, I still see a bunch of these errors:

Exception: »SFail« must not appear in a rendered »SimpleDocStream«. This is a bug in the layout algorithm! Please report this as a bug

@sjakobi
Copy link
Copy Markdown
Collaborator Author

sjakobi commented Nov 5, 2019

Maybe we should instead remove the shortcuts from the fitting predicates…

@sjakobi
Copy link
Copy Markdown
Collaborator Author

sjakobi commented Nov 5, 2019

For reference, these are some of of the test failures:

      ./tests/format/letNewlineComments:                                                                                       FAIL
      ./tests/format/escapeSingleQuotedOpenInterpolation:                                                                      FAIL
      ./tests/format/issue1413:                                                                                                FAIL
      ./tests/format/importAccess:                                                                          
      ./tests/format/letComments:                                                                                              FAIL
      ./tests/format/innerMultiline:                                                                                           FAIL
    Issue #126:                                                                                                                FAIL
  tutorial
    Interpolation
      Example #1:                                                                                                              FAIL

@sjakobi
Copy link
Copy Markdown
Collaborator Author

sjakobi commented Nov 5, 2019

Actually I wonder whether the current design with Unbounded as alternative to AvailablePageWidth makes sense at all. The entire difference between layoutSmart and layoutPretty lies in their fitting predicates – and with Unbounded you completely turn them off! layoutSmart (LayoutOptions Unbounded) and layoutPretty (LayoutOptions Unbounded) will always produce the same layout!

IMHO it would be better to have a separate function layoutUnbounded that handles Unions simply by checking whether the first alternative results in SFail.

@sjakobi
Copy link
Copy Markdown
Collaborator Author

sjakobi commented Nov 8, 2019

Superseded by #97.

@sjakobi sjakobi closed this Nov 8, 2019
@sjakobi sjakobi mentioned this pull request Nov 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants