Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework group and remove fail #126

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 92 additions & 2 deletions prettyprinter/src/Data/Text/Prettyprint/Doc/Internal.hs
Original file line number Diff line number Diff line change
Expand Up @@ -514,16 +514,106 @@ softline' = group line'
hardline :: Doc ann
hardline = Line

group :: Doc ann -> Doc ann
group = \doc -> case doc of
FlatAlt x y -> case changesOnFlattening y of
HasLine -> x
NoChange -> Union y x
Flat y' -> Union y' x

x@(Cat a b) -> case (changesOnFlattening a, changesOnFlattening b) of
(HasLine , _ ) -> x
(_ , HasLine ) -> x
(NoChange , NoChange) -> x
(NoChange , Flat b' ) -> Cat a (Union b' b)
(Flat a' , NoChange) -> Cat (Union a' a) b
(Flat a' , Flat b' ) -> Union (Cat a' b') (Cat a b)

Annotated ann x -> Annotated ann (group x)
Nest i x -> Nest i (group x)

Column f -> Column (group . f)
Nesting f -> Nesting (group . f)
WithPageWidth f -> WithPageWidth (group . f)

x@Union{} -> x
x@Char{} -> x
x@Text{} -> x
x@Line -> x
x@Empty -> x
-- Should never happen on a valid document
x@Fail -> x

changesOnFlattening :: Doc ann -> FlatteningResult (Doc ann)
changesOnFlattening = \doc -> case doc of
FlatAlt _ y -> case changesOnFlattening y of
HasLine -> HasLine
NoChange -> Flat y
Flat y' -> Flat y'

Union x _ -> Flat x

Cat a b -> case (changesOnFlattening a, changesOnFlattening b) of
(HasLine , _ ) -> HasLine
(_ , HasLine ) -> HasLine
(NoChange , NoChange) -> NoChange
(Flat a' , NoChange) -> Flat (Cat a' b)
(NoChange , Flat b' ) -> Flat (Cat a b')
(Flat a' , Flat b' ) -> Flat (Cat a' b')

Annotated ann x -> Annotated ann <$> (changesOnFlattening x)
Nest i x -> Nest i <$> (changesOnFlattening x)

Column f -> Flat (Column (flatten . f))
Nesting f -> Flat (Nesting (flatten . f))
WithPageWidth f -> Flat (WithPageWidth (flatten . f))
Copy link
Collaborator

Choose a reason for hiding this comment

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

So why do we need to use flatten instead of group here? I think you tried to explain that before but I haven't grokked it yet. ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because changesOnFlattening is expected to return a value that tells the caller if the doc changed (either by flattening, by encountering a Line or not changing). And you cannot get that information out of a function :/

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, I don't understand what you're saying here. Both flatten and group return Docs. And group can actually add information by wrapping things in Union.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats what you are thinking about! Yes definitly that is a good idea, I'll test that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fails quickcheck with a huge counterexample again and my own tests with a small one. Now to figure out whats wrong...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Counterexample: column (\_ -> hardline). This flattens to column (\_ -> Fail) but when using group to column (\_ -> Line).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, that's very interesting! Would that explain the test failures I got for the first version of this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, or at least the problem was very similar iirc.


Line -> HasLine

-- Should actually be impossible here. HasLine has the same effect tho
Fail -> HasLine

Text{} -> NoChange
Char{} -> NoChange
Empty -> NoChange
where
flatten :: Doc ann -> Doc ann
flatten = \doc -> case doc of
FlatAlt _ y -> flatten y
Cat x y -> Cat (flatten x) (flatten y)
Nest i x -> Nest i (flatten x)
Line -> Fail
Union x _ -> flatten x
Column f -> Column (flatten . f)
WithPageWidth f -> WithPageWidth (flatten . f)
Nesting f -> Nesting (flatten . f)
Annotated ann x -> Annotated ann (flatten x)

x@Fail -> x
x@Empty -> x
x@Char{} -> x
x@Text{} -> x

data FlatteningResult a
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this type have different semantics than FlattenResult, or what's the reason for adding it?

If you don't like the constructor names on FlattenResult, feel free to change them. (I might bikeshed your suggestions a bit though. ;))

Copy link
Contributor Author

@1Jajen1 1Jajen1 Jan 23, 2020

Choose a reason for hiding this comment

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

Oh I just added names this way to not conflict with the others. No technically you could the other type, just wanted to make a clear distinction, if this ends up performing much better than master and you want to merge it, I can change that 👍

= HasLine
| NoChange
| Flat a

instance Functor FlatteningResult where
fmap _ HasLine = HasLine
fmap _ NoChange = NoChange
fmap f (Flat a) = Flat (f a)

-- | @('group' x)@ tries laying out @x@ into a single line by removing the
-- contained line breaks; if this does not fit the page, @x@ is laid out without
-- any changes. The 'group' function is key to layouts that adapt to available
-- space nicely.
--
-- See 'vcat', 'line', or 'flatAlt' for examples that are related, or make good
-- use of it.
group :: Doc ann -> Doc ann
simpleGroup :: Doc ann -> Doc ann
-- See note [Group: special flattening]
group x = case changesUponFlattening x of
simpleGroup x = case changesUponFlattening x of
Flattened x' -> Union x' x
AlreadyFlat -> x
NeverFlat -> x
Expand Down
14 changes: 14 additions & 0 deletions prettyprinter/test/Testsuite/Main.hs
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,9 @@ tests = testGroup "Tests"
, testCase "Line within align" regressionUnboundedGroupedLineWithinAlign
]
]
, testGroup "Group" [
testProperty "simpleGroup == group" groupLayoutEqualsSimpleGroupLayout
]
]

fusionDoesNotChangeRendering :: FusionDepth -> Property
Expand All @@ -111,6 +114,17 @@ fusionDoesNotChangeRendering depth
, "Fused:"
, indent 4 (pretty renderedFused) ]

groupLayoutEqualsSimpleGroupLayout :: Property
groupLayoutEqualsSimpleGroupLayout = forAllShow (arbitrary :: Gen (Doc Int)) (show . diag) (\doc ->
forAll arbitrary (\layouter ->
let grouped = group $ doc
groupedSimple = simpleGroup doc
groupedLayedOut = layout layouter grouped
groupedSimpleLayedOut = layout layouter groupedSimple
in counterexample ("Grouped: " ++ (show . diag $ grouped))
(counterexample ("Grouped (Simple) " ++ (show . diag $ groupedSimple))
(groupedLayedOut === groupedSimpleLayedOut))))

instance Arbitrary ann => Arbitrary (Doc ann) where
arbitrary = document
shrink = genericShrink -- Possibly not a good idea, may break invariants
Expand Down