Skip to content

Fix #91, take 3#98

Merged
sjakobi merged 1 commit intohaskell-prettyprinter:masterfrom
sjakobi:91-properly-3
Jan 22, 2020
Merged

Fix #91, take 3#98
sjakobi merged 1 commit intohaskell-prettyprinter:masterfrom
sjakobi:91-properly-3

Conversation

@sjakobi
Copy link
Copy Markdown
Collaborator

@sjakobi sjakobi commented Nov 18, 2019

This is essentially an improved version of #92, except this version also passes the fusion tests using Unbounded.

Since layoutPretty's and layoutSmart's FittingPredicates were unused in the case of Unbounded, I was able to simplify some code.

I also ended up avoiding the breaking changes I had considered in #97.

The commit message:

Fix Unbounded layouts of hard linebreaks within group

Previously, using layoutPretty or layoutSmart with an Unbounded page
width would fail when the document contained a hard line break
(hardline).

Unbounded caused a shortcutting behaviour in the FittingPredicates
of these layouters that didn't check whether the layout might fail.

This patch changes layoutWadlerLeijen.selectNicer to handle
Unbounded page widths separately, allowing a simplification of the
FittingPredicate type.

Fixes #91.

Comment thread prettyprinter/src/Data/Text/Prettyprint/Doc/Internal.hs Outdated
Comment thread prettyprinter/test/Testsuite/Main.hs
(max 0 . min lineLength . round)
(fromIntegral lineLength * ribbonFraction)
Unbounded
-- TODO: Figure out why it's sufficient to check the first line.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thats a good observation!

Because of the algorithms recursive nature on Union all unions below this point have been evaluated to simpledoc and found to be non-fail already and. The only way fail is introduced is through group and thus through Union on the left side. And you cannot have a flattened document where it first has a Line and then a Fail without at least one Union above that Fail!

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.

Many thanks for the explanation! :)

Could you check the Note [Detecting failure with Unbounded page width] I just pushed and suggest any possible improvements?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Makes sense 👍 The fact that you cannot have a Fail without having a Union prior is quite the hard thing to wrap my head around ^^ I am still thinking about ways this could not be true, but I don't think there are any, because as soon as the algorithm hits fail it returns it, and the only branching code is from Union (which made implementing this in a non-lazy language really hard because it's not a tail-call!).

Previously, using layoutPretty or layoutSmart with an `Unbounded` page
width would fail when the document contained a hard line break
(`hardline`).

`Unbounded` caused a shortcutting behaviour in the `FittingPredicate`s
of these layouters that didn't check whether the layout might fail.

This patch changes `layoutWadlerLeijen.selectNicer` to handle
`Unbounded` page widths separately, allowing a simplification of the
`FittingPredicate` type.

Fixes haskell-prettyprinter#91.
@sjakobi
Copy link
Copy Markdown
Collaborator Author

sjakobi commented Jan 22, 2020

I'm pretty happy with this PR, modulo the documentation I'm discussing with @1Jajen1.

I'm not sure yet when to best merge this, since I believe this should probably result in a major version bump:

  1. This changes the SimpleDocStreams layout{Pretty,Smart} produce, and the SimpleDocStream internals are public. (Although my impression is that @quchen didn't always make a major version bump when a SimpleDocStream result changed, see e.g. the 1.5 to 1.5.1 bump).

  2. As Gabriel argued in Unbounded layout of grouped Line fails #91, people may actually have relied on the old failure-producing behaviour.

@quchen Could you weigh in on the minor-vs-major version bump question and possibly give this a final review?

@quchen
Copy link
Copy Markdown
Collaborator

quchen commented Jan 22, 2020

My rationale here was usually that people might use prettyprinting to test output against a golden file, for example to test whether some syntax tree is rendered correctly. With this in mind, all changes (even bugfixes) that change the output rendering are a breaking major version bump.

I think that on a more technical side, these are easily released as patch version bumps, since actual code is not likely to horribly break if e.g. redundant whitespace is trimmed correctly all of a sudden. But then again I hope people write tests, and use prettyprinter should they need prettyprinting in their tests.

I thought about both of these, and went with the low barrier to make »breaking« version bumps, even though this might make the lib look more unstable than it actually is.

@sjakobi
Copy link
Copy Markdown
Collaborator Author

sjakobi commented Jan 22, 2020

Thanks for the feedback @quchen! :) I'll go with a major bump here then.

@sjakobi sjakobi merged commit ca70266 into haskell-prettyprinter:master Jan 22, 2020
@sjakobi sjakobi deleted the 91-properly-3 branch January 22, 2020 16:28
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.

Unbounded layout of grouped Line fails

3 participants