dhall-haskell icon indicating copy to clipboard operation
dhall-haskell copied to clipboard

`assert` field in record triggers an `Invalid Input` error

Open divarvel opened this issue 3 years ago • 5 comments

Starting between dhall-1.32.0 and dhall-1.34.0, having a record field named assert causes the following issue:

❯ echo "{ assert = 1 }" | dhall # dhall 1.40.1
dhall: 
Error: Invalid input

(input):1:3:
  |
1 | { assert = 1 }
  |   ^
unexpected 'a'
expecting ',', =, whitespace, or }

For reference, the expected behaviour is

❯ echo "{ assert = 1 }" | dhall # dhall 1.32.0
{ assert = 1 }

I've checked on the changelog, asserts were introduced before 1.32, so assert being a reserved keyword might not be the issue directly. I'm not familiar with grammar changes that occurred between 1.32 and 1.34

divarvel avatar Mar 16 '22 00:03 divarvel

I think the parser simply became a bit stricter about checking for disallowed keywords in record labels.

To use a keyword as a record label, enclose it in backticks:

{ `assert` = 1 }

sjakobi avatar Mar 16 '22 00:03 sjakobi

Oh I get it now. So the original file where the field is declared indeed uses backticks (it's a purescript package set, which i've not authored), but our CI uses an old dhall version to combine files, and this one does not keep backticks around assert. So updating dhall in the CI job should fix my issue.

No code fix needed, yay!

However, this looks like a breaking change that was not properly advertised in the changelog. Is it too late to add it? (I'm not sure of the exact version where the change was introduced)

divarvel avatar Mar 16 '22 00:03 divarvel

However, this looks like a breaking change that was not properly advertised in the changelog.

Do bugfixes count as breaking changes? I'm not sure.

Is it too late to add it? (I'm not sure of the exact version where the change was introduced)

Feel free to send a PR.

sjakobi avatar Mar 16 '22 00:03 sjakobi

I'm fine amending the CHANGELOG to mention this change as a breaking change, but for future reference, I don't treat bug fixes as breaking changes from a versioning standpoint.

Gabriella439 avatar Mar 16 '22 14:03 Gabriella439

Sure, it's purely for documentation purposes: when I hit this failure and narrowed it down to a couple releases, i went to the changelog to see if something was mentioned and found nothing, hence my surprise; the introduction of assert was mentioned as "technically a breaking change" in a couple releases earlier, so i did not expect any breakage, just from reading the changelog.

If "breaking change" is too strong for bug fixes, no worries, but mentioning that programs could break would have helped me. Anyway i'm fine, but it might help people in the same situation

divarvel avatar Mar 16 '22 15:03 divarvel

Also discussed in #1896.

kukimik avatar Nov 24 '23 00:11 kukimik