Improving correctness

[sjakobi]

I'm quite impressed by (and a bit proud of!) the number of bugs that has been revealed through dhall's usage of prettyprinter!

I wonder how prettyprinter could rely less on the users to discover these bugs though.

In particular I wonder whether property tests could help – IMHO they've been quite effective in dhall!

For example I believe that the recent bugs could have been discovered by tests for the following properties:

• Unbounded layout of grouped Line fails #91: The result of applying layout{Pretty,Smart} opts . group to a Doc that doesn't contain Fail, should never contain SFail.
• For removeTrailingWhitespace:
  • The (rendered) input and output should only differ by trailing spaces.
  • The output should never contain trailing spaces.
  • removeTrailingWhitespace is idempotent.

It's probably more difficult to come up with properties that would reveal unknown bugs, but it might be worth trying!

[sjakobi]

Oh, I see that there is already an Arbitrary instance and a non-trivial property test:

prettyprinter/prettyprinter/test/Testsuite/Main.hs

Lines 79 to 99 in 4d51362

	fusionDoesNotChangeRendering :: FusionDepth -> Property
	fusionDoesNotChangeRendering depth
	= forAll document (\doc ->
	let rendered = render doc
	renderedFused = render (fuse depth doc)
	in counterexample (mkCounterexample rendered renderedFused)
	(render doc == render (fuse depth doc)) )
	where
	render = renderStrict . layoutPretty defaultLayoutOptions
	mkCounterexample rendered renderedFused
	= (T.unpack . render . vsep)
	[ "Unfused and fused documents render differently!"
	, "Unfused:"
	, indent 4 (pretty rendered)
	, "Fused:"
	, indent 4 (pretty renderedFused) ]
	newtype RandomDoc ann = RandomDoc (Doc ann)
	instance Arbitrary (RandomDoc ann) where
	arbitrary = fmap RandomDoc document

[sjakobi]

Here's a property for #83:

When layouting f (align (vsep […, x, …, x, …])), both instances of x should have the same layout.

[quchen]

The main difficulty here is putting the properties into code, but I totally agree! The Arbitrary instance is not used very much right now, but I prefer Quickcheck to handwritten tests whenever possible.

[sjakobi]

Another property had come up in #89:

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

But layoutCompact should be included too, so we probably want

layout (unAnnotate doc) === unAnnotateS (layout doc)

with layout being one of layout{Pretty,Smart,Compact} with arbitrary LayoutOptions.

[sjakobi]

@1Jajen1 mentioned that he has some effective property tests in his kotlin clone.

Maybe we can copy some of those…

[1Jajen1]

@sjakobi Not sure that will work much better, currently I literally just generate random elements of Doc (without Union/Fail to preserve invariants although you could call group on them randomly to produce those as well). Shrinking is also done manually for most of it as I too use a quickcheck derivative for tests. I'll change those tests to more hedgehog style tests later this week to check out how well integrated shrinking works, it seems to be the better choice if you want to preserve invariants on more complex datatypes without writing shrinkers manually.

[sjakobi]

@1Jajen1 Well somehow your tests were able to detect the problems with the modified group, right?

Which properties did you check there?

[1Jajen1]

So what I have right now is basically just a few property tests: (The kotlin source for these is actually readable!)
This one for the group (passes around 100k tests just fine, so it might actually work^^):
https://github.com/1Jajen1/kotlin-pretty/blob/master/kotlin-pretty/src/test/kotlin/pretty/GroupTest.kt
https://github.com/1Jajen1/kotlin-pretty/blob/master/kotlin-pretty/src/test/kotlin/pretty/Generators.kt
https://github.com/1Jajen1/kotlin-pretty/blob/master/kotlin-pretty/src/test/kotlin/pretty/DocTest.kt

They actually find a bug in flatten (did not handle Line correctly, only in the kotlin version).

[sjakobi]

#126 (comment) lists a bunch of tests from dhall's testsuite, at least some of which we ought to adopt into prettyprinter's testsuite…