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

Fix toMap type inference (and some merge tweaks) #671

Merged
merged 7 commits into from Aug 3, 2019

Conversation

philandstuff
Copy link
Collaborator

@philandstuff philandstuff commented Jul 28, 2019

Currently, there is only one type inference rule for toMap e : T, and
it requires that e is of type {}. This adds a second rule which
allows a toMap expression on a nonempty record type to be annotated
with a compatible type.

There is prior art for this in merge t u : T.


When I looked at said merge t u : T prior art, I noticed that the requirements
could be relaxed.

Currently, if you want to annotate a merge t u expression, you have to
annotate it with a type that exactly matches the type generated by the
type inference algorithm. This relaxes that requirement so that it only
has to be judgmentally equal to the inferred type.

I also removed the Γ ⊢ t :⇥ { ts… } precondition; the other
precondition, Γ ⊢ merge t u : T, implies that Γ ⊢ t :⇥ { ts… } so we
do not need to state this explicitly. I think it makes the rule
clearer: it's more obvious that "a merge expression can always be
annotated in a compatible way".


After opening this PR, I noticed that unannotated toMap e also cannot typecheck.

Currently, there is no base case for type inference of toMap e: the
only rule which can infer a type for an expression of the form toMap e
requires, as precondition, another expression of the form toMap e to
typecheck. This cyclic dependency effectively means that no expression
of the form toMap e (without annotation) can typecheck.

I added a commit which fixes that, by using toMap e : T as the base case instead
of toMap e. This pattern has prior art in the rules for merge t u
and merge t u : T expressions.

Currently, there is only one type inference rule for `toMap e : T`, and
it requires that `e` is of type `{}`.  This adds a second rule which
allows a `toMap` expression on a nonempty record type to be annotated
with a compatible type.

There is prior art for this in `merge t u : T`.
Currently, if you want to annotate a `merge t u` expression, you have to
annotate it with a type that exactly matches the type generated by the
type inference algorithm.  This relaxes that requirement so that it only
has to be judgmentally equal to the inferred type.

I also removed the `Γ ⊢ t :⇥ { ts… }` precondition; the other
precondition, `Γ ⊢ merge t u : T`, implies that `Γ ⊢ t :⇥ { ts… }` so we
do not need to state this explicitly.  I think it makes the rule
clearer: it's more obvious that "a merge expression can always be
annotated in a compatible way".
@philandstuff philandstuff force-pushed the allow-tomap-annotation-nonempty-record branch from cc77d7a to e187c1a Compare July 28, 2019 12:07
Currently, there is no base case for type inference of `toMap e`: the
only rule which can infer a type for an expression of the form `toMap e`
requires, as precondition, another expression of the form `toMap e` to
typecheck.  This cyclic dependency effectively means that no expression
of the form `toMap e` (without annotation) can typecheck.

This commit fixes that, by using `toMap e : T` as the base case instead
of `toMap e`.  This pattern has prior art in the rules for `merge t u`
and `merge t u : T` expressions.
@philandstuff philandstuff changed the title Allow toMap e : T where e is nonempty (and some merge tweaks) Fix toMap type inference (and some merge tweaks) Jul 28, 2019
@philandstuff philandstuff merged commit bb60fb5 into master Aug 3, 2019
@philandstuff philandstuff deleted the allow-tomap-annotation-nonempty-record branch August 3, 2019 19:07
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 this pull request may close these issues.

None yet

4 participants