Fix #83 by changing the choice of minNestingLevel by sjakobi · Pull Request #89 · haskell-prettyprinter/prettyprinter

sjakobi · 2019-10-24T19:41:17Z

No description provided.

sjakobi · 2019-10-24T19:41:56Z

I'll try to explain this later… ;)

sjakobi · 2019-10-24T23:19:42Z

-        minNestingLevel = min lineIndent currentColumn
+        minNestingLevel = case y of
+            SLine _ _ -> lineIndent
+            _         -> currentColumn


We check whether the second alternative (y) starts with a line break in order to detect hanging layouts as (partially) explained in #83 (comment).

The loss of laziness in y is a bit unfortunate. Since best seems to be quite lazy, I hope it's not too bad though.

The loss of laziness in y is a bit unfortunate.

I think this is probably outweighed by the efficiencies gained from the greater minNestingLevel when we select currentColumn:

We don't have to check x as far as with lineIndent.

We can more often stick with x instead of switching to y.

Also note that layoutPretty is completely unaffected since it ignores the minNestingLevel:

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

If y starts with one of the annotation constructors, we should probably look further for the first "proper" constructor…

I went with the »annotations might mean something important even if they contain something unimportant« elsewhere (strip trailing whitespace), so I think ignoring annotations here is alright. What do you think?

I'm actually not really familiar with how annotations work or how they are used.

But it seems to me that the result of layoutWadlerLeijen should not depend on the presence of annotations. Wouldn't the following property be desirable?

layoutWadlerLeijen fp opts (unAnnotate doc) === unAnnotateS (layoutWadlerLeijen fp opts doc)

So I think it would be better to check what's behind the annotations.

quchen · 2019-10-25T14:02:35Z

Ha nice! At first I was concerned about your pattern matching on y, but then realized it’s a SimpleDoc, not a Doc, so matching is well-behaved and not just a hack that works in case the rest of the document is already normalized.

I agree the loss of laziness in y is unfortunate, but on the other hand selecting the nicer of two alternatives kind of has comparison between two things built in. I was actually surprised quite a few times that the library could away with looking only at the first alternative.

I’ll add another test to make sure layoutPretty isn’t affected after having another look at this at home this weekend, and make a release.

Thanks for all your work! I’m not sure how long it would have taken me to spot this one…

Fixes haskell-prettyprinter#83.

sjakobi · 2019-10-25T17:36:45Z

+            -- See https://github.com/quchen/prettyprinter/issues/83.
+            if startsWithLine y
+                -- y might be a (more compact) hanging layout. Let's check x
+                -- thoroughly with the smaller lineIndent.


If we wanted to detect hanging layouts more precisely, we could also check that the indentation of the SLine is < currentColumn. But what we have right now is the slightly more conservative change.

sjakobi · 2019-10-25T18:54:46Z

      | fits pWidth minNestingLevel availableWidth x = x
      | otherwise = y
      where
-        minNestingLevel = min lineIndent currentColumn


Any idea why this was computed with min and didn't just return lineIndent?

Doesn't the invariant from above hold here?

-- * current column >= current nesting level

sjakobi · 2019-10-26T21:52:40Z

+                -- y definitely isn't a hanging layout. Let's allow the first
+                -- line of x to be checked on its own and format it consistently
+                -- with subsequent lines with the same indentation.
+                else currentColumn


These are the general conditions under which layoutSmart, given a Union x y, will change its behaviour to return x instead of y with this change:

y doesn't start with a line break

lineIndent must be < currentColumn

a line of x (but not the first) with an indentation of lineIndent < i <= currentColumn is too wide.

a line of x (but not the first) with an indentation of lineIndent < i <= currentColumn is too wide.

Actually that line must be "reachable" too. I.e. there's no other line with an indentation <= lineIndent before it.

sjakobi · 2019-10-30T12:06:08Z

@quchen Did you get a chance to look at this further?

Ideally I'd like to smoke-test this patch in projects that are likely to be impacted. Do you know any suitable projects?

The next best thing would IMHO be to just release this and wait for feedback. :)

quchen · 2019-11-05T08:13:52Z

Released 1.5. I don’t have that much time on my hands right now, sorry for the delays!

sjakobi · 2019-11-05T12:16:02Z

No worries. Thanks for the releases! :)

quchen and others added 2 commits October 24, 2019 21:30

Implement (failing) regresion test for haskell-prettyprinter#83

506aaaf

Fix the testcase

a53b60b

sjakobi commented Oct 24, 2019

View reviewed changes

quchen mentioned this pull request Oct 25, 2019

Inconsistent formatting within align #83

Closed

quchen reviewed Oct 25, 2019

View reviewed changes

Comment thread prettyprinter/src/Data/Text/Prettyprint/Doc/Internal.hs Outdated

sjakobi force-pushed the min-nesting-line branch from e84417c to a4d633b Compare October 25, 2019 15:08

sjakobi changed the title ~~Draft: Fix #83 by changing the choice of minNestingLevel~~ Fix #83 by changing the choice of minNestingLevel Oct 25, 2019

Layout aligned elements more consistently

4a8b609

Fixes haskell-prettyprinter#83.

sjakobi force-pushed the min-nesting-line branch from a4d633b to 4a8b609 Compare October 25, 2019 15:51

sjakobi commented Oct 25, 2019

View reviewed changes

sjakobi commented Oct 26, 2019

View reviewed changes

sjakobi mentioned this pull request Oct 31, 2019

Rewrite 'dhall format' to be a Vonderhaar style pretty-printer dhall-lang/dhall-haskell#1496

Open

quchen merged commit a006aff into haskell-prettyprinter:master Nov 5, 2019

sjakobi deleted the min-nesting-line branch November 5, 2019 12:15

sjakobi mentioned this pull request Nov 6, 2019

Improving correctness #95

Open

Uh oh!

Conversation

sjakobi commented Oct 24, 2019

Uh oh!

sjakobi commented Oct 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjakobi Oct 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quchen commented Oct 25, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjakobi Oct 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjakobi commented Oct 30, 2019

Uh oh!

quchen commented Nov 5, 2019

Uh oh!

sjakobi commented Nov 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sjakobi Oct 24, 2019 •

edited

Loading

sjakobi Oct 26, 2019 •

edited

Loading