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

Standardize records of mixed kinds #689

Merged
merged 22 commits into from Aug 16, 2019
Merged

Standardize records of mixed kinds #689

merged 22 commits into from Aug 16, 2019

Conversation

ocharles
Copy link
Member

@ocharles ocharles commented Aug 2, 2019

No description provided.

@ocharles
Copy link
Member Author

@ocharles ocharles commented Aug 2, 2019

This currently contains the changes to tests as a result of the changes in dhall-lang/dhall-haskell#1173.

Before I write the prose according to this, could I get a sanity check that these tests changes look good?

@sjakobi
Copy link
Collaborator

@sjakobi sjakobi commented Aug 2, 2019

LGTM so far. I'm missing a few tests though:

@ocharles
Copy link
Member Author

@ocharles ocharles commented Aug 2, 2019

{ a = Kind} should not typecheck

Done


a test for //\

There is tests/typecheck/success/simple/combineMixedRecords, which tests:

{ foo = 1 }  { bar = Text } : { foo : Natural, bar : Type }

Do you have something else in mind?


{ a = 0} // { a = Bool }

Done

@sjakobi
Copy link
Collaborator

@sjakobi sjakobi commented Aug 2, 2019

a test for //\

There is tests/typecheck/success/simple/combineMixedRecords, which tests:

{ foo = 1 }  { bar = Text } : { foo : Natural, bar : Type }

Do you have something else in mind?

That's a test for /\, not //\\.

@ocharles
Copy link
Member Author

@ocharles ocharles commented Aug 2, 2019

🤦‍♂️

Thanks

@ocharles
Copy link
Member Author

@ocharles ocharles commented Aug 2, 2019

How about tests/type-inference/success/unit/RecursiveRecordMergeBoolTypeA.dhall Is that what you have in mind?

@sjakobi
Copy link
Collaborator

@sjakobi sjakobi commented Aug 2, 2019

How about tests/type-inference/success/unit/RecursiveRecordMergeBoolTypeA.dhall Is that what you have in mind?

Cheers! :)


I noticed this though:

⊢ {a : Kind} //\\ {b : Kind}

Error: ❰Sort❱ has no type, kind, or sort

It seems unrelated, but shouldn't it be allowed?

@ocharles
Copy link
Member Author

@ocharles ocharles commented Aug 2, 2019

I think there might be some kind of bug with the repl:

[nix-shell:~/work/dhall-haskell/dhall]$ cabal new-run exe:dhall -- repl
Up to date
⊢ { a : Kind }

{ a : Kind }

⊢ :t { a : Kind }

Error: ❰Sort❱ has no type, kind, or sort


⊢ 
Goodbye.

[nix-shell:~/work/dhall-haskell/dhall]$ cabal new-run exe:dhall -- repl
Up to date
⊢ :t { a : Kind }

Sort
⊢ 
⊢ 
Goodbye.

[nix-shell:~/work/dhall-haskell/dhall]$ cabal new-run exe:dhall -- repl
Up to date
⊢ {a : Kind} //\\ {b : Kind}

{ a : Kind, b : Kind }

I have no idea what's going on there!

I think when it = { a : Kind } makes it into the history, the history has to be type checked, and something goes wrong... somewhere 🤷‍♂️

@sjakobi
Copy link
Collaborator

@sjakobi sjakobi commented Aug 2, 2019

Yeah, it's confusing and should be fixed. I seem to remember that the repl keeps a history, and tries to typecheck previous results, which doesn't always work.

@ocharles
Copy link
Member Author

@ocharles ocharles commented Aug 2, 2019

Yea, that seems to be an existing bug - I've reported dhall-lang/dhall-haskell#1193 as a minimal repro (which happens regardless of my work).

With regards to your comment, that is permitted. If you have a fresh REPL session:

⊢ {a : Kind} //\\ {b : Kind}

{ a : Kind, b : Kind }

As expected.

@ocharles
Copy link
Member Author

@ocharles ocharles commented Aug 7, 2019

How do people feel about 59aa4ee as a first stab? I need to define what the ⋁ judgement is, but that's trivial. Alternatively, rather than mentioning how to cons a Type, Kind or Sort on, I could just say T : S and then S ⋁ t or something

@sjakobi
Copy link
Collaborator

@sjakobi sjakobi commented Aug 7, 2019

Looks pretty good to me so far! I believe the judgments for the operations (selection, projection, merges, possibly merge and toMap) need work too, though.

@ocharles
Copy link
Member Author

@ocharles ocharles commented Aug 7, 2019

Oh sure, plenty more work - just taking it one step at a time.

@Nadrieril
Copy link
Member

@Nadrieril Nadrieril commented Aug 7, 2019

Alternatively, rather than mentioning how to cons a Type, Kind or Sort on, I could just say T : S and then S ⋁ t or something

That would be nice. I think we'd need something like a const(S) predicate, that holds only for Type, Kind and Sort; that'd allow simplifying the various record-merging operations as well.

@Nadrieril
Copy link
Member

@Nadrieril Nadrieril commented Aug 7, 2019

I believe that

    Γ ⊢ T :⇥ Sort
    ────────────────────
    Γ ⊢ { x : T } : Sort

is now redundant, since it's a special case of

    Γ ⊢ T :⇥ Sort   Γ ⊢ { xs… } :⇥ t
    ────────────────────────────────  ; x ∉ { xs… }
    Γ ⊢ { x : T, xs… } : Sort ⋁ t

when xs is empty.
Same thing with the similar rule with Kind.

@ocharles
Copy link
Member Author

@ocharles ocharles commented Aug 7, 2019

I think we'd need something like a const(S)

That, or s \elem {Type, Kind Sort}. const may be a better judgement.

Copy link
Contributor

@Gabriella439 Gabriella439 left a comment

Looks great so far!

standard/type-inference.md Outdated Show resolved Hide resolved
standard/type-inference.md Outdated Show resolved Hide resolved
@ocharles
Copy link
Member Author

@ocharles ocharles commented Aug 8, 2019

Ok, I've now added to function-check.md as a judgement, and I think this really tidies things up. Let me know what you think. Perhaps there is a better way of defining , but I went for what I thought was the clearest definition.

@ocharles
Copy link
Member Author

@ocharles ocharles commented Aug 8, 2019

Ok, I've tried to update the record merge type inference rules too.



Γ ⊢ l :⇥ { ls… }
{ ls… } :⇥ Type
{ ls… } :⇥ t
Copy link
Member Author

@ocharles ocharles Aug 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I need to say t \elem { Type, Kind, Sort }? This comment applies to various other places where I've replaced Type with a metavariable.

Copy link
Member

@Nadrieril Nadrieril Aug 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you don't, because a record type can only have type Type, Kind or Sort

@ocharles
Copy link
Member Author

@ocharles ocharles commented Aug 8, 2019

Ok, I think I've addressed those comments. Let me know if I've gone too far with some of the deletions...

Copy link
Contributor

@Gabriella439 Gabriella439 left a comment

Looks great! Thank you for doing this 🙂

One thing we should do (which can be a follow-up change) is to update the Prelude to take advantage of this

@ocharles ocharles changed the title Standardize records of mixed kinds (wip) Standardize records of mixed kinds Aug 9, 2019
@ocharles
Copy link
Member Author

@ocharles ocharles commented Aug 9, 2019

Yea, I think the Prelude is a separate PR. I like to keep changes in PRs be "required to change" rather than "could now be changed".

@ocharles
Copy link
Member Author

@ocharles ocharles commented Aug 9, 2019

To clarify, do I need to wait 3 days now? Or does mergify take care of waiting for 3 days? If we have to wait, I'll just merge on Monday.

@sjakobi
Copy link
Collaborator

@sjakobi sjakobi commented Aug 9, 2019

To clarify, do I need to wait 3 days now?

In https://github.com/dhall-lang/dhall-lang/blob/master/.github/CONTRIBUTING.md#how-do-changes-get-approved we have

If the change is approved quickly, there is still a minimum 3-day waiting period before merging for changes to the standard.

My interpretation is that a PR should be merged no earlier than 3 days after the last change to the patch.

Or does mergify take care of waiting for 3 days?

Mergify is only enabled for the dhall-haskell repo, not this one. @Gabriel439 is becoming quite the Mergify-expert though, so maybe that will change. :)

@Gabriella439
Copy link
Contributor

@Gabriella439 Gabriella439 commented Aug 9, 2019

@ocharles: The full rules are here:

https://github.com/dhall-lang/dhall-lang/blob/master/.github/CONTRIBUTING.md#how-do-changes-get-approved

This repository is not Mergify-enabled, so you just have to know how long to wait according to those rules.

We don't have clear rules around when the "clock" begins for draft pull requests. My rough rule is to reset the clock after each change, meaning:

  • If I updated the standard/Prelude and I already have all three approvals, wait up to 3 more days before merging

  • If I updated the standard/Prelude and I don't have all three approvals, wait up to 7 more days before merging

    If you get all three approvals in the interim, then only wait up to 3 days

  • If I only updated tests, and I have all three approvals, wait just 1 more day before merging

Given that we only have one approval so far, we should wait up to 7 days to give the other two approvers a chance to comment. If they don't comment then it's an implicit approval and then you can merge. If they both approve then you can merge after just a 3-day wait.

@ocharles
Copy link
Member Author

@ocharles ocharles commented Aug 9, 2019

Super, thanks for the info.

@ocharles
Copy link
Member Author

@ocharles ocharles commented Aug 9, 2019

I've created a new label "Voting Open" just to indicate that the author deems the work finished and is now looking for votes.

Copy link
Collaborator

@sjakobi sjakobi left a comment

I have a few inline comments.

Also: Can you check the rules for field selection? I believe they can be relaxed. If so, please add tests for them.

And does your branch for dhall-haskell already pass all the acceptance tests?

standard/function-check.md Outdated Show resolved Hide resolved


... where `t₀` and `t₁` are either `Type`, `Kind`, or `Sort` (though they may
differ).
Copy link
Collaborator

@sjakobi sjakobi Aug 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

though they may differ

Should that mean that t₀ and t₁ don't have to be the same?

Would

... where t₀ and t₁ each are either Type, Kind, or Sort.

be clearer?!

Copy link
Member Author

@ocharles ocharles Aug 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to emphasize that t₀ = Type and t₁ = Sort is valid. I may have made things more confusing, but I don't have a better suggestion. I'm open to changes here, if anyone wants to suggest anything! Happen to use your wording @sjakobi if it's clearer.

standard/type-inference.md Outdated Show resolved Hide resolved
standard/function-check.md Outdated Show resolved Hide resolved
ocharles and others added 2 commits Aug 9, 2019
Co-Authored-By: Simon Jakobi <simon.jakobi@gmail.com>
Co-Authored-By: Simon Jakobi <simon.jakobi@gmail.com>
f-f
f-f approved these changes Aug 9, 2019
Copy link
Member

@f-f f-f left a comment

Thank you for doing this! 🙂

Copy link
Collaborator

@philandstuff philandstuff left a comment

Looks great from an admittedly superficial skim. I spotted a minor presentation fix but nothing else.

standard/type-inference.md Outdated Show resolved Hide resolved
ocharles added a commit to circuithub/dhall-haskell that referenced this issue Aug 15, 2019
@ocharles
Copy link
Member Author

@ocharles ocharles commented Aug 15, 2019

@sjakobi dhall-lang/dhall-haskell#1173 passes all the tests added here, but fails due to some unrelated changes. Am I good to merge this PR (the standard change)?

Copy link
Collaborator

@sjakobi sjakobi left a comment

Cheers! Sorry for the delay!

@ocharles ocharles merged commit 0c335d3 into master Aug 16, 2019
1 check passed
@ocharles ocharles deleted the mixed-records branch Aug 16, 2019
@ocharles
Copy link
Member Author

@ocharles ocharles commented Aug 16, 2019

And we're done! Thanks for the help, all!

Gabriella439 pushed a commit to dhall-lang/dhall-haskell that referenced this issue Aug 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants