Proposed corrections in Dhall standard (issue #1363)#1365
Proposed corrections in Dhall standard (issue #1363)#1365Gabriella439 merged 21 commits intodhall-lang:masterfrom
Conversation
|
I'll have to fix some errors in the build. |
Gabriella439
left a comment
There was a problem hiding this comment.
Looks great! Thank you so much for doing this. I just have a few comments
standard/beta-normalization.md
Outdated
| Prefer | ||
| (ProjectByLabels (RecordLiteral rs) (filter predicate xs₀)) = | ||
| t₁ | ||
| betaNormalize t₁ |
There was a problem hiding this comment.
This is fix correct, but to more closely match the judgment I'd suggest putting the betaNormalize in the definition of t₁. i.e. let t₁ = betaNormalize (Operator …)
| betaNormalize (Builtin TimeZoneShow) = Builtin TimeZoneShow | ||
| ``` | ||
|
|
||
| ### The precision of seconds in `TimeLiteral` |
There was a problem hiding this comment.
This seems like an idiosyncracy of the Haskell implementation. I'd like to require that unlimited precision is supported even if the Haskell implementation needs to be fixed
There was a problem hiding this comment.
One standard test failed until I implemented the logic that 00:00:00.000 is kept like this with 3 digits of precision even though it's the same as 00:00:00. Then I tried more digits and discovered this unexpected 12-digit cutoff.
I agree that it is more in the spirit of Dhall to support unlimited precision here. But it's out of scope for this PR and also beyond my capacity to fix the Haskell implementation (and presumably other implementations need to be fixed).
Would you like me to change the standard to specify unlimited precision? Something like this:
⊢ let t = 09:00:00.0987654321098765432109876543210000000000 in " let t = " ++ Time/show t
" let t = 09:00:00.0987654321098765432109876543210000000000"There was a problem hiding this comment.
@Gabriella439 I have updated this PR, removing changes in imports.md and making the change you indicated, specifying arbitrary precision for fractional seconds in time literals.
standard/type-inference.md
Outdated
| ──────────────────────── ; import without a headers term | ||
| freeVars(import x) = { } |
There was a problem hiding this comment.
Note that there is no import keyword in Dhall like there is in Nix. Maybe use https://example.com here as a sample import
| Γ ⊢ t₁ : { ts… } | ||
| Γ ⊢ u₁ : < us… > | ||
| x ∉ freeVars(T₀) | ||
| ↑(-1, x, 0, T₀) = T₁ |
There was a problem hiding this comment.
One thing I just realized is that if x is not in the free variables of T₀ then ↑(-1, x, 0, T₀) is a no-op (i.e. it evaluates to T₀) since the shift operator only shifts free variables.
There was a problem hiding this comment.
It's a bit confusing to me. Can T₀ contain x@1 as a free variable? In that case, ↑(-1, x, 0, T₀) is not a no-op. But my understanding of free variables is that they must have a zero de Bruijn index. Is that correct? In the requirement x ∉ freeVars(T₀), is it implied that T₀ also may not contain x@1 as a free variable?
standard/imports.md
Outdated
| Γ(headersPath) = userHeadersExpr | ||
| (Δ, parent, headersPath) × Γ₀ ⊢ userHeadersExpr ⇒ userHeaders ⊢ Γ₁ | ||
| headersPath = env:DHALL_HEADERS ? "${env:XDG_CONFIG_HOME as Text}/dhall/headers.dhall" ? ~/.config/dhall/headers.dhall ? [] | ||
| (Δ, parent, headersPath) × Γ₀ ⊢ headersPath ⇒ userHeaders ⊢ Γ₁ |
There was a problem hiding this comment.
@Gabriella439 Do you agree with these changes? I'm not sure what's the best way to make this clear. The original text doesn't say what userHeadersExpr means. And I couldn't figure out what it means to have Γ(headersPath) = userHeadersExpr and then twice using Γ₀ and Γ₁. It's not clear how you can have a judgment with premises that reuse the same Γ₀ and Γ₁. So, I replaced the final context with Γ₂.
To me, it seems that imports.md needs more work. I'll finish implementing imports and then maybe I will have more material for another PR with corrections.
For now, I'm still trying to understand the standard where it says "Γ₀(import) means to retrieve the expression located at import". It is not clear what "retrieve" means. If it means just to download the contents of the URL, to read the file, etc., then we don't need any Γ₀ while doing that. If it means to fetch the expression from the context Γ₀ then it's not clear from the text of imports.md how that expression got there.
I believe it is necessary to introduce a new symbol, such as READ(import_path) = expr, to denote reading a given external resource that exists at import_path and converting it into a Dhall expression expr.
There was a problem hiding this comment.
I will move the imports.md changes to another PR. I will have several more corrections for imports.md that will need to be taken care of separately.
There was a problem hiding this comment.
I have retained only a couple of minor changes in imports.md that I think should be clearly an improvement. The entire imports.md could benefit from a rewrite, it's too complicated and has a different style from other specifications in the standard.
After implementing the import system, I have some understanding of how it works but I'm not sure I understand why imports.md is the way it is. Maybe I will just add "implementation notes" to imports.md in another PR, showing a step-by-step pseudocode of how the import judgment could be implemented.
Co-authored-by: Gabriella Gonzalez <GenuineGabriella@gmail.com>
Co-authored-by: Gabriella Gonzalez <GenuineGabriella@gmail.com>
Co-authored-by: Gabriella Gonzalez <GenuineGabriella@gmail.com>
Co-authored-by: Gabriella Gonzalez <GenuineGabriella@gmail.com>
Co-authored-by: Gabriella Gonzalez <GenuineGabriella@gmail.com>
|
Thank you for doing this! |
Minor corrections to the Dhall standard documents as discussed in #1363
Most of the corrections are in Haskell code that is missing one of the steps shown in math notation.
There is only one case where the math notation itself appears to lack a step: This is in the type checking rule for
mergeexpressions involvingOptional. The missing step is↑(1, x, 0, t₀) = t₁. Tests fail without this step.I also added a specification for the free variable check. One of the rules for typechecking of
mergeexpressions is thatx is not free in T₀. It appears to be a type error ifxwere indeed free inT₀. But there is no specification of how to check for free variables. I have written it up, please check if this is what is intended. In particular, that only non-bound variables with de Bruijn index equal to zero will be considered free variables in a term.