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

WIP: Implement 'shrink' for Docs by hand #113

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

sjakobi
Copy link
Collaborator

@sjakobi sjakobi commented Jan 20, 2020

No description provided.

@sjakobi
Copy link
Collaborator Author

sjakobi commented Jan 20, 2020

If I cause the fusion tests to fail, for example with

--- a/prettyprinter/src/Data/Text/Prettyprint/Doc/Internal.hs
+++ b/prettyprinter/src/Data/Text/Prettyprint/Doc/Internal.hs
@@ -551,7 +551,7 @@ changesUponFlattening :: Doc ann -> Maybe (Doc ann)
 changesUponFlattening = \doc -> case doc of
     FlatAlt _ y     -> Just (flatten y)
     Line            -> Just Fail
-    Union x _       -> changesUponFlattening x <|> Just x
+    Union x _       -> Nothing
     Nest i x        -> fmap (Nest i) (changesUponFlattening x)
     Annotated ann x -> fmap (Annotated ann) (changesUponFlattening x)

shrink still produces nonsensical Docs like Cat Empty Fail:

Tests
  Fusion
    Deep fusion does not change rendering:                          FAIL (0.03s)
      *** Failed! (after 61 tests and 44 shrinks):
      Exception:
        »SFail« must not appear in a rendered »SimpleDocStream«. This is a bug in the layout algorithm! Please report this as a bug
        CallStack (from HasCallStack):
          error, called at src/Data/Text/Prettyprint/Doc/Render/Util/Panic.hs:13:21 in prettyprinter-1.5.1-inplace:Data.Text.Prettyprint.Doc.Render.Util.Panic
      Cat Empty Fail
      LayoutPretty (LayoutOptions {layoutPageWidth = AvailablePerLine 55 56.4237144509971})
      Exception thrown while showing test case:
        »SFail« must not appear in a rendered »SimpleDocStream«. This is a bug in the layout algorithm! Please report this as a bug
        CallStack (from HasCallStack):
          error, called at src/Data/Text/Prettyprint/Doc/Render/Util/Panic.hs:13:21 in prettyprinter-1.5.1-inplace:Data.Text.Prettyprint.Doc.Render.Util.Panic
      
      Use --quickcheck-replay=824164 to reproduce.

I think what could help is to filter the raw shrink results with a valid function that checks certain invariants such as the absence of Fail (except in a first Union alternative?!).

Such a valid function could be good documentation too…

Nest _ x -> go mayFail x
Union x y -> go True x && go mayFail y
Column f -> all (go mayFail) (map f [0..80])
WithPageWidth f -> all (go mayFail) (map f (Unbounded : [AvailablePerLine c r | c <- [1..80], r <- [0, 0.1 .. 1]]))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is 0 a valid ribbon width at all? Not sure how to interpret the docs there:

= AvailablePerLine Int Double
-- ^ Layouters should not exceed the specified space per line.
--
-- - The 'Int' is the number of characters, including whitespace, that
-- fit in a line. A typical value is 80.
--
-- - The 'Double' is the ribbon with, i.e. the fraction of the total
-- page width that can be printed on. This allows limiting the length
-- of printable text per line. Values must be between 0 and 1, and
-- 0.4 to 1 is typical.

Suggested change
WithPageWidth f -> all (go mayFail) (map f (Unbounded : [AvailablePerLine c r | c <- [1..80], r <- [0, 0.1 .. 1]]))
WithPageWidth f -> all (go mayFail) (map f (Unbounded : [AvailablePerLine c r | c <- [1..80], r <- [0.1, 0.2 .. 1]]))

Cat x y -> go mayFail x && go mayFail y
Nest _ x -> go mayFail x
Union x y -> go True x && go mayFail y
Column f -> all (go mayFail) (map f [0..80])
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure how to best handle these functions. Checking them exhaustively would take too long. Even the current check with "typical" values 0..80 seems excessive.

shrink doc = filter valid $ case doc of
Fail -> [Empty]
Empty -> []
Char c -> Empty : map Char (filter (/= '\n') (shrink c))
Copy link
Owner

Choose a reason for hiding this comment

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

Might be one of the few places where a list comprehension is nicer to read! :-)

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