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

Record projection by expression #499

Merged
merged 21 commits into from May 28, 2019

Conversation

Projects
None yet
6 participants
@dingxiangfei2009
Copy link
Contributor

commented Apr 21, 2019

Close #183, replacing #494.

This PR extends record project by permitting expression normalising to records as the projection selector.

The highlight is that to project fields prescribed by an expression from a record, a round bracket is necessary, eg., r.(e).

@dingxiangfei2009 dingxiangfei2009 changed the title Record projection by xpression Record projection by expression Apr 21, 2019

@dingxiangfei2009 dingxiangfei2009 force-pushed the dingxiangfei2009:selector-extension branch from 56e6862 to cd1e4f2 Apr 21, 2019

@dingxiangfei2009

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

@Gabriel439 I noticed that #493 is quite relevant to this PR. I am thinking of expanding the scope of this work to address the requirement in that issue, though I am not sure if this is okay.

@Gabriel439
Copy link
Contributor

left a comment

There are two other things that need to be added:

  • How to α-normalize t.(s)
  • How to encode/decode t.(s) (i.e. in binary.md)
Show resolved Hide resolved standard/semantics.md Outdated
@@ -3894,6 +3906,16 @@ You can only select field(s) from the record if they are present:
Γ ⊢ e.{ x } : { x : T }


Γ ⊢ e :⇥ { ts… } : Kind Γ ⊢ s :⇥ { ss… } : Kind
──────────────────────────────────────────────────── ; ss ⊆ ts
Γ ⊢ e.(s) : { ss… }

This comment has been minimized.

Copy link
@Gabriel439

Gabriel439 Apr 23, 2019

Contributor

I believe that the type of e.(s) should be the normal form of s rather than the type of s. In other words:

Γ ⊢ e :⇥ { ts… }   Γ ⊢ s : T   s ⇥ { ss… }
───────────────────────────────────────────  ; ss ⊆ ts
Γ ⊢ e.(s) : { ss… }

I also don't think you need the Kind/Sort type annotations. The type of the type of each record would be checked along the way as part of type-checking e and s

This comment has been minimized.

Copy link
@dingxiangfei2009

dingxiangfei2009 Apr 26, 2019

Author Contributor

My idea is to make it explicit that, in order to type check e.(s), s should lie in a universe immediately above the one where e lives. But I am not sure how to express this constraint.

By the way, what is T in Γ ⊢ s : T ?

This comment has been minimized.

Copy link
@Gabriel439

Gabriel439 May 8, 2019

Contributor

Think of Γ ⊢ s : T as pseudo-code for:

T <- inferTypeWithContext Γ s

In other words, Γ and s are the two inputs to the inferTypeWithContext function and T is the output of the function. In this case T is the inferred type of s

Show resolved Hide resolved standard/semantics.md Outdated
Show resolved Hide resolved standard/semantics.md Outdated
@Gabriel439

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

@dingxiangfei2009: I think #493 should probably be handled as part of a separate pull request so that each change can be understood separately in the history

@singpolyma

This comment has been minimized.

Copy link
Collaborator

commented Apr 24, 2019

I would also request that unit test cases be added for each new rule.

@f-f f-f referenced this pull request Apr 25, 2019

Closed

Row Type/Kind #494

@dingxiangfei2009

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

@Gabriel439 Speaking of adding a α-normalisation rule, though it is not immediately related, I find the deduction rules for ↑(d, x, m, ε) a bit under-specified. The deduction rules in standard/README.md never mention the situation for different values of d and m. The definition of the data in these tuples is lacking, too. Is there any literature I can refer to regarding this notation?

@Nadrieril

This comment has been minimized.

Copy link
Collaborator

commented Apr 26, 2019

@dingxiangfei2009 I may be wrong, but it sounds to me like you haven't seen this: https://github.com/dhall-lang/dhall-lang/blob/master/standard/shift.md . If you have, would you mind being more precise about what you find lacking ?

@dingxiangfei2009

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

@Nadrieril Ah, I see! It has what I want to read about.

@Nadrieril

This comment has been minimized.

Copy link
Collaborator

commented Apr 26, 2019

#509 should make it easier to spot which judgments are defined and where in the future.

Show resolved Hide resolved standard/beta-normalization.md Outdated

encode(t₀) = t₁ encode(T₀) = T₁
─────────────────────────────────
encode(t₀.(T₀)) = [ 27, t₁, T₁ ]

This comment has been minimized.

Copy link
@Gabriel439

Gabriel439 May 6, 2019

Contributor

I suggest using 19 instead of 27, since record projection by type can survive β-normalization. For example, this is a well-typed expression that is in normal form:

λ(e : { x : Bool })  e.({ x : Bool })

This comment has been minimized.

Copy link
@dingxiangfei2009

dingxiangfei2009 May 7, 2019

Author Contributor

I see.

Is there any rule on how these numbers are assigned, for backward compatibility?

This comment has been minimized.

Copy link
@Nadrieril

Nadrieril May 7, 2019

Collaborator

As mentioned here, we try to assign labels below 24 for syntactic features that survive normalization. Currently we use 0 to 18, except 13 and 17. 13 was used before so for backward-compatibility we want to reuse it as late as possible (see here). I don't know why 17 was skipped, but I'm assuming it is similar to 13. That means that the next free label is 19.

Show resolved Hide resolved tests/parser/success/recordProjectByType.dhall Outdated
@singpolyma

This comment has been minimized.

Copy link
Collaborator

commented May 7, 2019

@Gabriel439

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

@singpolyma: Oh yeah, good point. We should probably use the same label as normal record projection

@@ -0,0 +1 @@
{ a = 10, b = Some 10 }

This comment has been minimized.

Copy link
@singpolyma

singpolyma May 7, 2019

Collaborator

missing newline

This comment has been minimized.

Copy link
@dingxiangfei2009

dingxiangfei2009 May 8, 2019

Author Contributor

dhall format automatically removes the new line when I add it.

@@ -0,0 +1 @@
{=}

This comment has been minimized.

Copy link
@singpolyma

singpolyma May 7, 2019

Collaborator

missing newline

This comment has been minimized.

Copy link
@dingxiangfei2009

dingxiangfei2009 May 8, 2019

Author Contributor

Same here.

@@ -0,0 +1 @@
{ a = 10, b = Some 10, c = [ "text" ]}.({ a : Natural, b : Optional Natural }) : { a : Natural, b : Optional Natural }

This comment has been minimized.

Copy link
@singpolyma

singpolyma May 7, 2019

Collaborator

type annotation on the end here seems unneeded?

@@ -0,0 +1 @@
let e = { a = 10, b = "Text" } let s = { a : Natural } in e.(s) : { a : Natural }

This comment has been minimized.

Copy link
@singpolyma

singpolyma May 7, 2019

Collaborator

this should be named A and have a B part with it -- the let seems like a lot of extra noise for a unit test.

This comment has been minimized.

Copy link
@Gabriel439

Gabriel439 May 8, 2019

Contributor

Note that we should still have a test like this one, even if not necessarily for a unit test. It's important to test that type inference works when the s is an abstract type since that's a likely case that an implementation might implement incorrectly.

@Gabriel439
Copy link
Contributor

left a comment

This looks great! I think the only thing left is to see if we can have the binary encoding for e.(s) reuse the same label as for e.x like @singpolyma. Other than that I don't see anything missing

@@ -0,0 +1 @@
let e = { a = 10, b = "Text" } let s = { a : Natural } in e.(s) : { a : Natural }

This comment has been minimized.

Copy link
@Gabriel439

Gabriel439 May 8, 2019

Contributor

Note that we should still have a test like this one, even if not necessarily for a unit test. It's important to test that type inference works when the s is an abstract type since that's a likely case that an implementation might implement incorrectly.

@dingxiangfei2009 dingxiangfei2009 force-pushed the dingxiangfei2009:selector-extension branch from 48597d3 to 6185c96 May 8, 2019

dingxiangfei2009 added some commits May 15, 2019

Show resolved Hide resolved standard/binary.md Outdated
@singpolyma

This comment has been minimized.

Copy link
Collaborator

commented May 15, 2019

@Gabriel439

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

@singpolyma: Oh yeah, that's right. Ignore my concern, then.

@Gabriel439

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

I think the only thing missing is to add the expected inferred type for tests/type-inference/success/unit/RecordProjectionByType.dhall (i.e. rename it to tests/type-inference/success/unit/RecordProjectionByTypeA.dhall and add a tests/type-inference/success/unit/RecordProjectionByTypeB.dhall)

dingxiangfei2009 added some commits May 21, 2019

fix type inference unit test
Signed-off-by: Ding Xiang Fei <dingxiangfei2009@gmail.com>
@dingxiangfei2009

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

Sorry for late response. Caught with some other business earlier this week. 😅

Gabriel439 added a commit to dhall-lang/dhall-haskell that referenced this pull request May 22, 2019

@Gabriel439
Copy link
Contributor

left a comment

Almost there! 🙂

I just implemented this in dhall-lang/dhall-haskell#958 and noticed a few small things along the way

Show resolved Hide resolved standard/binary.md
@@ -501,6 +501,13 @@ You can only select field(s) from the record if they are present:
Γ ⊢ e :⇥ { x : T, ts… } Γ ⊢ { x : T, ts… } :⇥ Sort
────────────────────────────────────────────────────
Γ ⊢ e.{ x } : { x : T }


This comment has been minimized.

Copy link
@Gabriel439

Gabriel439 May 22, 2019

Contributor

Perhaps add some prose here explaining that the e.(t) syntax lets the user select record fields by type. Currently there isn't any text in the standard explaining this feature

@@ -501,6 +501,13 @@ You can only select field(s) from the record if they are present:
Γ ⊢ e :⇥ { x : T, ts… } Γ ⊢ { x : T, ts… } :⇥ Sort
────────────────────────────────────────────────────
Γ ⊢ e.{ x } : { x : T }


Γ ⊢ e :⇥ { ts… }

This comment has been minimized.

Copy link
@Gabriel439

Gabriel439 May 22, 2019

Contributor

This judgment needs to somehow enforce that { ts… } and { ss… } are syntactically record types. One way to do that is define the judgment via induction (i.e. define a judgment to type-check the case where the two record types are empty and then define a judgment to type-check the case where the two record types are non-empty), like this:

Γ  e :⇥ {}
Γ  s : T
s  {}
────────────────
Γ  e.(s) : {}

Γ  e :⇥ { x : T₀,  }
Γ  s : T
s  { x : T₁, xs₁ }
Γ  e.({ xs₁ }) : T
────────────────────────
Γ  e.(s) : { x : T₁ ,xs₁ }

This comment has been minimized.

Copy link
@dingxiangfei2009

dingxiangfei2009 May 22, 2019

Author Contributor

@Gabriel439 For the base case, we can relax the requirement on e, since it is still sound, in the same universe, to project empty record from any record.

Also, do you think that the following induction step make sense?

Γ ⊢ e :⇥ { x : T₀, ts… }
Γ ⊢ s : T₀
s ⇥ { x : T₀, ss… }
Γ ⊢ e.({ ss… }) : T₁
───────────────────────────
Γ ⊢ e.(s) : { x : T₀, ss… }

This comment has been minimized.

Copy link
@dingxiangfei2009

dingxiangfei2009 May 22, 2019

Author Contributor

The concern is with the induction step that you suggested earlier, in which Γ ⊢ s : T and Γ ⊢ e.({ xs₁… }) : T cannot reconcile.

This comment has been minimized.

Copy link
@Gabriel439

Gabriel439 May 22, 2019

Contributor

@Nadrieril: Oh yeah, the base case doesn't need to require that e is the empty record. That's a mistake on my part. Good catch!

Also, you're right that the two occurrences of T in the induction judgement should be distinct. That's another mistake on my part.

@Gabriel439

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

@singpolyma: Actually, I did run across an ambiguous encoding caught by the binary serialization round-trip property test. See:

https://hydra.dhall-lang.org/build/13281/nixlog/5

The issue is that a projection of an expression which is a built-in:

x.(Bool)

... and an expression projecting a quoted field of the same name:

x.{ `Bool` }

... share the same encoding. They are both encoded as:

[ 10, [ "x", 0 ], "Bool" ]

Technically, it's an ill-typed expression, but it still seems wrong to permit two different expressions to be encoded the same way.

It might be worth still using a different label just to avoid this potential ambiguity (and also to make it easier to write decoders).

@Gabriel439

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

Alternatively, we could add a "sub-label" to indicate that it's a record projection by type, perhaps by adding a null element, like this:

encode(e.(t)) = [ 10, encode(e), null, encode(t) ]
@singpolyma

This comment has been minimized.

Copy link
Collaborator

commented May 24, 2019

Hmm, ok. My initial reaction is that projection on a builtin is a crazy thing to do. But I suppose it's not impossible that a builtin that maps to a record type could ever be added to the language.

If they can't overlap, then I guess I don't feel strongly about label vs sub-label

@Gabriel439

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

@dingxiangfei2009: How about we do this, then: store the projection by type inside of another list so that it visually resembles the syntax:

encode(e.(t)) = [ 10, encode(e), [ encode(t) ] ]
@dingxiangfei2009

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2019

@Gabriel439 sounds like a better plan!

@f-f

f-f approved these changes May 26, 2019

Copy link
Member

left a comment

Looks great, thanks @dingxiangfei2009!

@Gabriel439
Copy link
Contributor

left a comment

Thank you for doing this! 🙂

@Gabriel439

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

I'll go ahead and merge this since you have all the approvals. Thanks again! 🙂

@Gabriel439 Gabriel439 merged commit 034d48a into dhall-lang:master May 28, 2019

1 check passed

hydra Hydra build #13617 of dhall-lang:499:dhall-lang
Details

Gabriel439 added a commit to dhall-lang/dhall-haskell that referenced this pull request May 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.