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

assert field in record triggers an Invalid Input error #2402

Closed
divarvel opened this issue Mar 16, 2022 · 6 comments · Fixed by #2551
Closed

assert field in record triggers an Invalid Input error #2402

divarvel opened this issue Mar 16, 2022 · 6 comments · Fixed by #2551

Comments

@divarvel
Copy link
Contributor

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

@sjakobi
Copy link
Collaborator

sjakobi commented Mar 16, 2022

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 }

@divarvel
Copy link
Contributor Author

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)

@sjakobi
Copy link
Collaborator

sjakobi commented Mar 16, 2022

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.

@Gabriella439
Copy link
Collaborator

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.

@divarvel
Copy link
Contributor Author

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

@kukimik
Copy link
Collaborator

kukimik commented Nov 24, 2023

Also discussed in #1896.

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 a pull request may close this issue.

4 participants