Pretty-print RecordField comments

[sjakobi]

Context: #145

[Gabriella439]

@sjakobi: Thank you for doing this! 🙂

Would you like me to begin reviewing or do you want to wait until the PR is out of draft status?

[sjakobi]

Would you like me to begin reviewing or do you want to wait until the PR is out of draft status?

The actual code still needs a lot of work and is probably not worth reviewing yet. If you have feedback on the test output though, I can try to incorporate that.

[sjakobi]

Some normalization logic seems to need updating:

    isNormalized should be consistent with normalize:                                                                          FAIL
      *** Failed! Falsified (after 22 tests and 4 shrinks):
      RecordLit (fromList [("",RecordField {recordFieldSrc0 = Nothing, recordFieldValue = Var (V "" 0), recordFieldSrc1 = Nothing, recordFieldSrc2 = Just ()})])
      True /= False
      Use --quickcheck-replay=139332 to reproduce.

[sjakobi]

I've been staring it this part of isNormalized but didn't understand how it would report the wrong thing!

dhall-haskell/dhall/src/Dhall/Normalize.hs

Lines 782 to 785 in f77549a

	RecordLit kvs -> Dhall.Map.isSorted kvs && all decide kvs
	where
	decide (RecordField Nothing exp' Nothing Nothing) = loop exp'
	decide _ = False

The issue is that isNormalized applies denote first:

dhall-haskell/dhall/src/Dhall/Normalize.hs

Line 652 in f77549a

isNormalized e0 = loop (Syntax.denote e0)

How about removing this denote step?!

[sjakobi]

How about removing this denote step?!

That would probably make pattern matching on Exprs like in this part much more complex:

dhall-haskell/dhall/src/Dhall/Normalize.hs

Lines 660 to 685 in f77549a

	App f a -> loop f && loop a && case App f a of
	App (Lam _ _) _ -> False
	App (App (App (App NaturalFold (NaturalLit _)) _) _) _ -> False
	App NaturalBuild _ -> False
	App NaturalIsZero (NaturalLit _) -> False
	App NaturalEven (NaturalLit _) -> False
	App NaturalOdd (NaturalLit _) -> False
	App NaturalShow (NaturalLit _) -> False
	App (App NaturalSubtract (NaturalLit _)) (NaturalLit _) -> False
	App (App NaturalSubtract (NaturalLit 0)) _ -> False
	App (App NaturalSubtract _) (NaturalLit 0) -> False
	App (App NaturalSubtract x) y -> not (Eval.judgmentallyEqual x y)
	App NaturalToInteger (NaturalLit _) -> False
	App IntegerNegate (IntegerLit _) -> False
	App IntegerClamp (IntegerLit _) -> False
	App IntegerShow (IntegerLit _) -> False
	App IntegerToDouble (IntegerLit _) -> False
	App DoubleShow (DoubleLit _) -> False
	App (App ListBuild _) _ -> False
	App (App (App (App (App (App ListFold _) (ListLit _ _)) _) _) _) _ -> False
	App (App ListLength _) (ListLit _ _) -> False
	App (App ListHead _) (ListLit _ _) -> False
	App (App ListLast _) (ListLit _ _) -> False
	App (App ListIndexed _) (ListLit _ _) -> False
	App (App ListReverse _) (ListLit _ _) -> False
	App TextShow (TextLit (Chunks [] _)) ->

[Gabriella439]

@sjakobi: Not necessarily. You could still do the ViewPatterns trick where you do something like:

App (shallowDenote -> App (shallowDenote -> …

… possibly with a short-hand synonym for shallowDenote to save characters.

[sjakobi]

I went with this simple test fix in order to keep the focus on the pretty-printer changes.

[sjakobi]

BTW, note that I'll be mostly AFK during the weekend. I expect to be able to address any review feedback on Monday.

Feel free to take over the PR if you would like to merge it sooner.

[sjakobi]

The output is worth another look at this stage. I'm pretty happy with it.

The code still needs a bit of cleanup though.

[sjakobi]

@Gabriel439 Could you review?

[TristanCacqueray]

I guess preserving and pretty-printing comment for Union type is out of scope, but will those changes also facilitate such addition?

[sjakobi]

I guess preserving and pretty-printing comment for Union type is out of scope, but will those changes also facilitate such addition?

Definitely. The pretty-printer will be well-prepared after this PR. What remains to be done is updating the parser and updating the code in response to the necessary Expr changes.

[german1608]

My only concern with the implementation is that it confuses me a little the difference between render and pretty in some functions (prettyComment and renderComment for example) but I don't have better names for those.

I believe that the output of the formatter is OK as I'm not too opinionated about the result. Making dhall format not delete comment it's the main goal from my POV

[sjakobi]

Thanks for the review @german1608! :)

[Gabriella439]

Thank you for doing this! 🙂