feat(comments): support prefix comments on Record's key-value pairs.

[german1608]

This PR tries to add support to record field comments both on formatting and dhall-docs documentation extension. Specifically the following ones:

{
  -- before each record field
  a : Text
  -- after each record field
, ...
}

I updated the Record's Expr constructor from:

Record (Map Text (Expr s a))

to

Record (Map Text (RecordField s a))

where RecordField stores the last of the expression on the right side of the : and the prefix and suffix comments. Although the prefix comment is annotating the label field, the best place to save that comment is there.

However, I'm having a problem with unsafeSubExpressions, because it can't traverse the Maybe s I put on RecordField. A quick solution for that is to change those RecordField's Maybe s' to Nothing, although that means we loose that information when traversing.

I'll keep this PR as a draft to get your opinion in that matter.

[sjakobi]

I'm happy to see that you've started working on this! :)

This PR tries to add support to record field comments both on formatting and dhall-docs documentation extension. Specifically the following ones:

{
  -- before each record field
  a : Text
  -- after each record field
, ...
}

Have you considered including the whitespace between the label and the value, e.g. { x {- A -} : {- B -} T }? These might not be great spots for documentation comments, but in the context of #145, it would probably be useful anyway.

However, I'm having a problem with unsafeSubExpressions, because it can't traverse the Maybe s I put on RecordField. A quick solution for that is to change those RecordField's Maybe s' to Nothing, although that means we loose that information when traversing.

I haven't checked, but I think we should be able to preserve the comments here. We have a similar case with the Binding in Let. Try copying bindingExprs!

If you need more help, please shout! :)

[german1608]

Have you considered including the whitespace between the label and the value, e.g. { x {- A -} : {- B -} T }? These might not be great spots for documentation comments, but in the context of #145, it would probably be useful anyway.

I didn't consider them because as you say is an awkward place to write documentation. But I can add it with no problem :)

I haven't checked, but I think we should be able to preserve the comments here. We have a similar case with the Binding in Let. Try copying bindingExprs!

I haven't check out bindingExprs yet, but on unsafeSubExpressions the Let constructor is handled with the unhandledConstructor.

I'll read bindingExprs to see what can I do to work further.

If you need more help, please shout! :)

Cool! What do you think might suffix for tests to this? A format test?

I think that to make this PR not too big I'll add the record field support for dhall-docs on another PR.

[sjakobi]

I haven't check out bindingExprs yet, but on unsafeSubExpressions the Let constructor is handled with the unhandledConstructor.

Oh, right! I forgot why unsafeSubExpressions is separate! I guess we should move the Record handling from unsafeSubExpressions to subExpressions then. We'll also need to update the other places where we currently use unsafeSubExpressions.

Cool! What do you think might suffix for tests to this? A format test?

Take a look at the tests added in #1273.

Formatting idempotence is a bit tricky to get right, but we can fix it later if necessary. We can extend some property tests to help with that: #1465.

[sjakobi]

BTW, you could try making the same changes for RecordLit in this PR – I believe that the changes would look very similar to those for Record.

Documentation comments on record literals could also prove valuable, for example in package.dhall files, or on schema records.

[german1608]

Documentation comments on record literals could also prove valuable, for example in package.dhall files, or on schema records.

You're right! After getting record types right I'll do it.

Looks like I have a lot of work to do ;)

Thanks for your guidance @sjakobi

[german1608]

I successfully captured the prefix whitespace of a key-value pair, but the suffix is consumed by the annotation expression. Any idea about how to solve this?

For example,

{ -- foo
  a : Text
  -- baz
}

results in (approximate)

...
("a",
RecordField
  { recordFieldSrc0 = " -- foo"
  , recordFieldValue = Note (... "Text \n  -- baz") Text
  , recordFieldSrc1 = ""
  }
)

I believe that almost all expressions consume the suffix whitespace around it

Any ideas? cc: @Gabriel439 @sjakobi

[sjakobi]

I successfully captured the prefix whitespace of a key-value pair, but the suffix is consumed by the annotation expression. Any idea about how to solve this?

Oh yes. I forgot that this is a constraint that we also encountered with let in #1273.

I guess we could try fiddling with the Notes of the type, but even if it would work, it would probably be rather messy.

I hope @Gabriel439 has a better idea – he definitely knows more about parsing!

[Gabriella439]

@german1608: There isn't a good solution to this and the let-related comment parsing has the same limitation. The only way to solve this "for real" is to preserve all whitespace/comments for all constructors in the syntax tree. For now, it's fine to not preserve that whitespace

[german1608]

I'll post an update.

I dropped recordFieldSrc1 (the suffix comment) because of what we last discussed. After that, I updated RecordLit Map to use RecordField as map values.

I started the pretty printing of those comments, this is what I have at this moment:

With this file:

{ -- hi there
  -- testing around
  a: Text,

  {- foo
  -}
  b: Bool
}

dhall format will return:

{ -- hi there
  -- testing around
  a :
    Text
,

  {- foo
  -}
  b :
    Bool
}

for me, it's awkward that the type follows in a newline. Do you know the reason? I'm not too fluent on prettyprint :)

Also, what other test cases you think will be helpful for this? something similar to let-bindings as we had on #1273?

I'll keep these answer open and debug further tomorrow.

[german1608]

These line is what causes the line break there:

dhall-haskell/dhall/src/Dhall/Pretty/Internal.hs

Lines 1317 to 1319 in 71e485b

	Pretty.hardline
	<> " "
	<> prettyValue val

Changing these lines to:

space <> prettyValue val

and supplying a file like this:

{ -- hi there
  -- testing around
  a: Text,
  {- foo
  -}
  b: Bool
}

outputs a file like this:

{ -- hi there
  -- testing around
  a : Text
,
  {- foo
  -}
  b : Bool
}

What do you think of this formatting?

[german1608]

I believe that this is ready for review. You can check out the test-cases I added. What I haven't done yet is:

• Add support for other comments i.e. a {- B -} : {- C -} T because I'm unsure how to format them properly yet. Also, I'd like to finish this PR as soon as possible to work again at dhall-docs relevant features.
• Check idempotent tests setup. I'll try to address Test formatting idempotence with in-code comments too #1465 on this PR

I improved the formatting of key-value pairs to avoid the newline after the : or = as I showed in this comment. The relevant code is here:

dhall-haskell/dhall/src/Dhall/Pretty/Internal.hs

Lines 1333 to 1374 in 79c00ae

	fitsSingleLine e = case e of
	Var{} -> True
	BoolLit{} -> True
	Bool -> True
	Natural -> True
	NaturalLit{} -> True
	NaturalFold -> True
	NaturalBuild -> True
	NaturalIsZero -> True
	NaturalEven -> True
	NaturalOdd -> True
	NaturalToInteger -> True
	NaturalShow -> True
	NaturalSubtract -> True
	Integer -> True
	IntegerLit _ -> True
	IntegerClamp -> True
	IntegerNegate -> True
	IntegerShow -> True
	IntegerToDouble -> True
	Double -> True
	DoubleLit _ -> True
	DoubleShow -> True
	Text -> True
	TextShow -> True
	List -> True
	ListBuild -> True
	ListFold -> True
	ListLength -> True
	ListHead -> True
	ListLast -> True
	ListIndexed -> True
	ListReverse -> True
	Optional -> True
	None -> True
	_ -> False

I'll work to improve the idempotent tests before marking this PR as ready for review, but you can take a look if you want to

EDIT: forgot to mention that an existing test-case was amended due to new formatting.

[sjakobi]

Apologies for the delay @german1608! I've started reviewing this now.

Unfortunately the mass of style changes make the review much harder. I wish I had remembered to suggest that you do a complete stylish-haskell sweep over the code base before doing this work. :(

Could you possibly still do that and then rebase this PR?

[sjakobi]

Oh boy, this task looks like quite the struggle! I don't blame you, @german1608 – I wish I had anticipated these issues a bit better.

I'm wondering whether we could still split this work into more manageable chunks – for you, and for us reviewers.

In particular, I think we could do the pretty-printer update in a separate PR. It's tricky stuff, and frankly I would probably struggle with it too. We can still see who ends up doing this work.

My suggestion is:

• Backup this work on a different branch.
• Remove the work on the pretty-printing logic from this PR, as if the recordFieldSrc didn't exist.
• I will give the remaining code a speedy review.
• Once it's merged you can focus on dhall-docs again, and for example work on using the record field comments in dhall-docs.

Regarding my suggestion to do a complete stylish-haskell sweep over the codebase to simplify the review. I think that's still a good idea. If it causes you trouble, we can maybe do it later though.

Does this sound like a reasonable plan?

[german1608]

Apologies for the delay @german1608! I've started reviewing this now.

No worries!

Unfortunately the mass of style changes make the review much harder. I wish I had remembered to suggest that you do a complete stylish-haskell sweep over the code base before doing this work. :(

Could you possibly still do that and then rebase this PR?

Sure thing! Give me 30minutes do the change.

My suggestion is:

• Backup this work on a different branch.
• Remove the work on the pretty-printing logic from this PR, as if the recordFieldSrc didn't exist.
• I will give the remaining code a speedy review.
• Once it's merged you can focus on dhall-docs again, and for example work on using the record field comments in dhall-docs.

Does this sound like a reasonable plan?

Yeah, let's do it.

I'll ping you when I'm done @sjakobi

[german1608]

@sjakobi the stylish-haskell changes you say are only on imports formatting, right? Aside that, I manually changed in some places:

do e

to

e

when it's valid to do so. Should I do these changes on another PR as well?

[sjakobi]

Aside that, I manually changed in some places:

do e

to

e

when it's valid to do so. Should I do these changes on another PR as well?

I don't feel very strongly about do e vs. e – I mostly don't like the added noise of these changes.

Maybe just do what seems "economical" for your time and the reviewers' time.

[german1608]

Ok, I'll revert those changes too

[german1608]

additions/deletions before stylish-haskell changes: +1,399 −758

additions/deletion after reverting them: +460 −270

I believe that you can review now, @sjakobi

[german1608]

@sjakobi I used bench as you suggested and these are the new reports:

Running on master:

benchmarking dhall-to-yaml --file ../dhall-kubernetes/examples/aws-iam-authenticator-chart.dhall
time                 250.0 ms   (-251.3 ms .. 764.7 ms)
                     0.645 R²   (0.071 R² .. 1.000 R²)
mean                 364.4 ms   (290.4 ms .. 489.3 ms)
std dev              121.4 ms   (15.56 ms .. 153.1 ms)
variance introduced by outliers: 73% (severely inflated)

benchmarking dhall-to-yaml --file ../dhall-kubernetes/examples/deployment.dhall
time                 215.7 ms   (160.6 ms .. 241.7 ms)
                     0.984 R²   (0.940 R² .. 1.000 R²)
mean                 251.1 ms   (234.0 ms .. 275.4 ms)
std dev              26.13 ms   (10.80 ms .. 34.99 ms)
variance introduced by outliers: 19% (moderately inflated)

benchmarking dhall-to-yaml --file ../dhall-kubernetes/examples/deploymentSimple.dhall
time                 256.7 ms   (216.3 ms .. 312.4 ms)
                     0.987 R²   (0.973 R² .. 1.000 R²)
mean                 236.6 ms   (227.7 ms .. 249.4 ms)
std dev              13.70 ms   (6.742 ms .. 17.98 ms)
variance introduced by outliers: 16% (moderately inflated)

benchmarking dhall-to-yaml --file ../dhall-kubernetes/examples/ingress.dhall
time                 240.0 ms   (219.0 ms .. 260.5 ms)
                     0.997 R²   (0.991 R² .. 1.000 R²)
mean                 227.8 ms   (222.7 ms .. 234.2 ms)
std dev              7.514 ms   (3.542 ms .. 8.519 ms)
variance introduced by outliers: 14% (moderately inflated)

benchmarking dhall-to-yaml --file ../dhall-kubernetes/examples/service.dhall
time                 257.0 ms   (228.0 ms .. 280.8 ms)
                     0.993 R²   (0.984 R² .. 1.000 R²)
mean                 229.9 ms   (220.6 ms .. 241.6 ms)
std dev              15.30 ms   (9.510 ms .. 21.69 ms)
variance introduced by outliers: 15% (moderately inflated)

Running on this PR without the "left-refactoring" improvement:

benchmarking dhall-to-yaml --file ../dhall-kubernetes/examples/aws-iam-authenticator-chart.dhall
time                 285.2 ms   (244.9 ms .. 323.2 ms)
                     0.994 R²   (0.991 R² .. 1.000 R²)
mean                 256.6 ms   (245.6 ms .. 269.1 ms)
std dev              14.80 ms   (8.708 ms .. 20.20 ms)
variance introduced by outliers: 16% (moderately inflated)

benchmarking dhall-to-yaml --file ../dhall-kubernetes/examples/deployment.dhall
time                 255.0 ms   (248.6 ms .. 260.5 ms)
                     0.999 R²   (0.997 R² .. 1.000 R²)
mean                 245.1 ms   (232.4 ms .. 250.0 ms)
std dev              9.991 ms   (426.1 μs .. 12.89 ms)
variance introduced by outliers: 16% (moderately inflated)

benchmarking dhall-to-yaml --file ../dhall-kubernetes/examples/deploymentSimple.dhall
time                 236.8 ms   (228.4 ms .. 252.0 ms)
                     0.998 R²   (0.994 R² .. 1.000 R²)
mean                 237.4 ms   (233.8 ms .. 241.3 ms)
std dev              5.500 ms   (3.970 ms .. 6.648 ms)
variance introduced by outliers: 14% (moderately inflated)

benchmarking dhall-to-yaml --file ../dhall-kubernetes/examples/ingress.dhall
time                 280.1 ms   (238.9 ms .. 350.2 ms)
                     0.981 R²   (0.959 R² .. 1.000 R²)
mean                 256.3 ms   (247.0 ms .. 274.1 ms)
std dev              16.45 ms   (5.208 ms .. 21.33 ms)
variance introduced by outliers: 17% (moderately inflated)

benchmarking dhall-to-yaml --file ../dhall-kubernetes/examples/service.dhall
time                 363.2 ms   (262.9 ms .. 500.4 ms)
                     0.952 R²   (0.929 R² .. 1.000 R²)
mean                 274.2 ms   (249.6 ms .. 320.8 ms)
std dev              45.81 ms   (9.337 ms .. 64.70 ms)
variance introduced by outliers: 38% (moderately inflated)

These are the bench results with the "left-refactoring" technique:

benchmarking dhall-to-yaml --file ../dhall-kubernetes/examples/aws-iam-authenticator-chart.dhall
time                 253.1 ms   (229.9 ms .. 277.6 ms)
                     0.997 R²   (0.987 R² .. 1.000 R²)
mean                 244.3 ms   (237.1 ms .. 250.3 ms)
std dev              9.151 ms   (6.878 ms .. 10.27 ms)
variance introduced by outliers: 16% (moderately inflated)

benchmarking dhall-to-yaml --file ../dhall-kubernetes/examples/deployment.dhall
time                 222.0 ms   (208.5 ms .. 232.3 ms)
                     0.997 R²   (0.990 R² .. 1.000 R²)
mean                 237.9 ms   (229.0 ms .. 250.7 ms)
std dev              13.30 ms   (9.459 ms .. 16.75 ms)
variance introduced by outliers: 14% (moderately inflated)

benchmarking dhall-to-yaml --file ../dhall-kubernetes/examples/deploymentSimple.dhall
time                 232.8 ms   (221.5 ms .. 245.4 ms)
                     0.999 R²   (0.997 R² .. 1.000 R²)
mean                 232.3 ms   (227.2 ms .. 235.8 ms)
std dev              5.981 ms   (3.494 ms .. 8.884 ms)
variance introduced by outliers: 14% (moderately inflated)

benchmarking dhall-to-yaml --file ../dhall-kubernetes/examples/ingress.dhall
time                 226.1 ms   (217.7 ms .. 234.4 ms)
                     0.998 R²   (0.993 R² .. 1.000 R²)
mean                 228.0 ms   (223.6 ms .. 233.8 ms)
std dev              7.046 ms   (3.615 ms .. 10.28 ms)
variance introduced by outliers: 14% (moderately inflated)

benchmarking dhall-to-yaml --file ../dhall-kubernetes/examples/service.dhall
time                 224.3 ms   (206.8 ms .. 234.3 ms)
                     0.997 R²   (0.986 R² .. 1.000 R²)
mean                 224.0 ms   (218.0 ms .. 227.1 ms)
std dev              5.803 ms   (2.279 ms .. 8.859 ms)
variance introduced by outliers: 14% (moderately inflated)

the latest results are better! Thanks for that suggestion @sjakobi @Gabriel439

If you want to take a look at Expressions.hs before I merge the PR, go ahead!

[sjakobi]

I think it would be good if @Gabriel439 would take another look at the parser changes.

I wouldn't mind if that happens after the merge though.

Great job, @german1608! 🥇

[german1608]

Thanks @sjakobi and @Gabriel439 for your help and patience!