Generalise objects by rossberg · Pull Request #182 · caffeinelabs/motoko

rossberg · 2019-02-21T10:17:53Z

Allow arbitrary declarations in object literals.
Unify blocks and objects in typing & interpretation.
Remove analysis mode for object literals, since it is bogus due to bindings.
Remove special case for typing blocks in IR (now handled by desugaring).
Reconcile type annotations (make sure to use the inferred type).
Drop extra type annotation from BlockE.
Remove DecE from AST.
Drop object sort annotation from DotE.
Drop double id from ClassD.
Move name type from AST to IR.
Introduce a separate module Dom for defining extended sets.
Some minor bug fixes on the way.
Some renamings and clean-ups.

Follow-ups:

TODO(andreas): Invert public/private defaults; separate short-hand field forms.

TODO(joachim): Currently, decs are desugared into fields in the IR. The backend still needs proper support for decs, e.g. to handle let with patterns in an object.

TODO(claudio): Handle type fields.

* Unify blocks and objects in typing & interpretation. * Remove analysis mode for object literals, since it is bogus due to bindings. * Reconcile type annotations (make sure to use the inferred type). * Drop extra type annotation from BlockE. * Remove DecE from AST. * Make Type.con a fully abstract type. * Some minor bug fixes on the way. * Some renamings and clean-ups. Follow-ups: TODO(andreas): Invert public/private defaults; separate short-hand field forms. TODO(joachim): Currently, decs are desugared into fields in the IR. The backend still needs proper support for decs, e.g. to handle `let` with patterns in an object. TODO(claudio): Handle type fields.

nomeata

Phew. Too big to review, I think. At least to do code review; I’ll leave it to Claudio to do a design review ;-)

nomeata · 2019-02-21T15:06:26Z

src/type.ml

-(* The con field is a reference to break the recursion in open_binds,
-   and to allow the multiple passes in typing *)
-let kind con = !(Con.kind con)
+module Con =


This conflicts with the cleanup in #181, which allows us to write Con.fresh instead of going through Type. Maybe merge #181 into this PR?

Yes, I'm fine with resolving the conflict here once you landed. Already spent multiple hours resolving the conflicts with the previous con change, so I'm used to it. :)

Landed. Enjoy the merge :-)

Merged. I moved the ConSet impl to a new module Dom that is to Set what Env is to Map.

nomeata · 2019-02-21T15:09:06Z

src/check_ir.ml

-    check_typ env t0;
-    check (T.eq t T.unit || T.eq t1 t0) "unexpected expected block type";
-    t0 <: t;
+    if t <> T.unit then t1 <: t;


This is an odd non-continuity (and it might bite in the backend, or at least cause some extra checks there). Is that explained somewhere? Maybe add a comment here why we need this.

Note that this already existed before (see check on old l.392), I just changed the way it's checked.

This matches the type system. The reason this case exists is so that a block whose last thing is a valueful dec is not a type error. Stupid example:

func g() { func f() {} };

So the type of the last expression is either a subtype of the expected type, unless the expected type is ()?

So I can do { true } : ()? (On the phone, can't check)

During typing, this rule is only applicable to proper declarations, not expressions. Check_ir is a bit more permissive. If that's a problem I suppose we could tighten it.

It might. The backend compiles a block returning a unit differently than one returning true (nothing on the stack vs. something), so it would have to check which case is taken here in the type checker and adjust the return value accordingly, I believe.

Ah, I get what you are saying: This can’t be exploited by source, and in practice the backend can rely on a block having the same type (a supertype of) its last expression, but it is tedious to check this in the IR properly. Ok, fine with me for now, we can revisit that in later improvements to Check_ir.

this always bugged me a bit too, but I can see the reasoning behind it. Check_ir may need fixin or can we just desugar to insertion of a final expD tupE[]?

or can we just desugar to insertion of a final expD tupE[]?

that sounds like a fine way out, good idea (and the current backend will already not produce code for that, because a unit tuple is represented as a zero stack).

Good idea, done.

src/diag.mli

nomeata · 2019-02-21T15:25:28Z

Currently, decs are desugared into fields in the IR. The backend still needs proper support for decs, e.g. to handle let with patterns in an object.

Can we avoid this? Couldn’t this be desugared to an outer BlockE with all the declarations, and a final NewObjE? (It seems that the decl-as-object-fields is convenient for user syntax, but not really the most elegant design for an IR.)

I guess it would work for NewObjE, but we still have not found out how to do a similar thing with actors, where we cannot just move the declarations out of the actor. And all designs that I can think of are ugly in one way or another. Here is one of those ugly proposals, that more closely models how actors are actually implemented:

  | ActorE  of (var list * (label * func) list * exp)

The first argument declares (but not defines) all fields, the comes a list of publicly exposed functions, and then an initialization expression that defines all fields (using DefineE).

I am not happy with this, of course, maybe someone has a better idea?

rossberg

Yeah, sorry for the size. As usual this grew top-down.

rossberg · 2019-02-21T15:44:32Z

src/check_ir.ml

-    check_typ env t0;
-    check (T.eq t T.unit || T.eq t1 t0) "unexpected expected block type";
-    t0 <: t;
+    if t <> T.unit then t1 <: t;


Note that this already existed before (see check on old l.392), I just changed the way it's checked.

This matches the type system. The reason this case exists is so that a block whose last thing is a valueful dec is not a type error. Stupid example:

func g() { func f() {} };

src/diag.mli

rossberg · 2019-02-21T15:53:08Z

src/type.ml

-(* The con field is a reference to break the recursion in open_binds,
-   and to allow the multiple passes in typing *)
-let kind con = !(Con.kind con)
+module Con =


Yes, I'm fine with resolving the conflict here once you landed. Already spent multiple hours resolving the conflicts with the previous con change, so I'm used to it. :)

rossberg · 2019-02-21T16:00:11Z

Sorry, I don’t know what you want to say here :-)

That the IR somehow needs to be able to represent objects with decs in them (or some simplification thereof).

Can we avoid this?

Maybe. This is essentially requires pattern binding compilation, so if we do that earlier than we wouldn't need decs in objects later. Other than that, I don't have a good idea how to avoid it without implementing the same logic twice.

nomeata · 2019-02-21T16:48:28Z

This is essentially requires pattern binding compilation,

Really? At least for objects, we can just move the object creation inside the decls (this is what the current desugarer and IR does already), but leaves the pattern matching in decls as it is.

rossberg · 2019-02-21T17:14:42Z

Aye, you are right. I'll look into it. Public type definitions might become a problem, though, unless we can erase them.

nomeata · 2019-02-21T17:20:09Z

The current branch accepts

> actor { let (foo,bar) = (shared func () = (), shared func () = ()) };
{bar = func; foo = func} : actor {bar : shared () -> (); foo : shared () -> ()}

or even

> actor { let foo = if true  (shared func () = ()) else (shared func () = ()) };
{foo = func} : actor {foo : shared () -> ()}

In other words: Actor methods are no longer just always static functions, but rather can be calculated values.

This is new. I was about to say “we can’t do that”, but in fact, we can: The backend would generate an exported function that does nothing but call a closure stored at a well-known location in the heap. (We use such static locations for private actor variables already). The init function of the actor would then fill these memory locations, just like it does with private calculated data. This way, we can expose closures.

The compiler could optionally optimize cases where the function is statically known to export it directly.

This points to maybe

 | ActorE of (decs * (name, call_conv, var) list)`

for the IR: An actor consists of a bunch of declarations (run as part of the init function), and a list of exported functions together with their exposed name, their calling convention (or maybe their full type) and the variable that defines them.

So not just decs? Because this change allows us to remove the public/private field from decls from the IR, and it separates between names (cannot be renamed) and variables (can be renamed), which I believe some of @crusso’s passes need.

crusso · 2019-02-21T21:33:13Z

src/typing.ml

  t'

-and infer_exp_mut env exp : T.typ =
+and infer_exp' f env exp : T.typ =


Nit: why not call this infer_exp_core and leave infer_exp' for infer_exp'' (to match the other patterns).

crusso · 2019-02-21T21:34:54Z

src/typing.ml

-    infer_obj env' sort.it id None fields
-  | DotE (exp1, sr, {it = Name n; _}) ->
+    infer_obj env' sort.it id T.Pre fields exp.at
+  | DotE (exp1, sr, {it = Name l; _}) ->


Nit: rename Name to Lab for consistency - or is that already used for jump labels?

Yes, confusion with jump labels was the problem. But I moved Name from AST to IR, since we don't rename on the AST anymore.

crusso · 2019-02-21T21:38:41Z

src/typing.ml

  exp.note <- {note_typ = t'; note_eff = e}

-and check_exp' env t exp =
+and check_exp' env t exp : T.typ =


And you guys gave me such hell for having check in Check_ir return a typ... (or maybe that was just Joachim)

It was me. Good point. Why does check_exp' need to return a type?

This is just the helper, not the "main" check function. ;) It returns the inferred type in cases where it switches to synthesis mode. That's only to allow putting the more specific type annotation on the node and getting rid of e.g. the extra annotation on blocks (and another small bug in desugaring).

crusso · 2019-02-21T21:49:48Z

src/typing.ml

-    T.ConSet.singleton c
+    infer_id_typdecs env id c k
+
+and infer_id_typdecs env id c k : con_env =


env is unused in infer_id_typdecs

crusso · 2019-02-21T21:54:42Z

test/run/nested-lexpr.as

 assert (a[0][0] == 2);

-let b : [{var x : Int}] = [new {var x = 1}];
+let b : [{var x : Int}] = [new {var x : Int = 1}];


So we can't propagate expected types into binders anymore (here and below)? Ok by me, I'm just checking this is an example....

Yes. At least not for covariabnt binders, contravariant ones (like func args) would be fine.

crusso · 2019-02-21T22:06:07Z

src/typing.ml

+  let env' = add_val env id.it t in
+  (* Prepass to infer type for id *)
+  let _, scope = infer_block {env' with pre = true} decs at in
+  let pub_typ, pub_val = pub_fields fields in


Nicely done! Is this why you were complaining about the separate namespaces?

Yes, among other things (before I removed analysis mode for binders it came up in more places).

crusso · 2019-02-21T22:22:18Z

test/run/objects1.as

 assert(p.x == 3);
 assert(p.get_y() == 2);
+
+let o : {a : {}; b : Nat} = new {a = new {x = 0}; b = a.x};


I wonder if we could keep public by default (convenient for records) but make private/public sticky so the last modifier applies to all decs following it. new (for records) might be public by default, objects/actors private by default.

object { private y = ...; // private x = ...; // private public move(dx,dy) {}; // public swap() {}; // public private unsafe(); // private again }

Yes, (later) I want to make new a purely record-like syntax that is all public.

Sticky visibility modifiers OTOH would be like in C++, which is terrible IME -- it sometimes makes it really difficult to find the corresponding visibility of a dec. If we wanted accumulative visibility then we should rather introduce a shorthand like public { ... }, which at least enables you to find the surrounding braces. But so far I'm reluctant, given that most languages get by without it.

src/interpret.ml

nomeata · 2019-02-21T22:43:35Z

After some more thinking about it, I think

 | ActorE of (decs * (name, call_conv, var) list)`

or something similar for the IR is a good next step, and somewhat in line with the NewObjE idiom. I can tackle that refactoring.

Meta question: Shall I do that on this branch, and land it together? Or can we land this in master as is, and then the refactoring separately?

crusso · 2019-02-21T22:45:39Z

So not just decs? Because this change allows us to remove the public/private field from decls from the IR, and it separates between names (cannot be renamed) and variables (can be renamed), which I believe some of @crusso’s passes need.

Indeed. I think I needed to be able to rename to cps convert an object literal with awaited field value. I expect the same issue may arise again. It wasn't necessary for the naive await translation, but became an issue when avoiding admistrative redexed because I wound up inlining continuations at occurences with intervening bindings, allowing capture if I didn't rename apart.

nomeata · 2019-02-21T22:48:00Z

I think I needed to be able to rename to cps convert an object literal with awaited field value. I expect the same issue may arise again.

Luckily we don't have await’ed actor field values (I hope). But if only for consistency this might be a good idea.

rossberg · 2019-02-22T08:57:49Z

@nomeata:

Really? At least for objects, we can just move the object creation inside the decls (this is what the current desugarer and IR does already), but leaves the pattern matching in decls as it is.

Sorry, I was confused yesterday. This of course is the case for objects (as it was already), but doesn't work for actors. The function in question is only used for the latter. As you observed in a later reply, we will need to generalise the IR's actor construct somehow. I hope you're fine with me leaving that to you.

crusso · 2019-02-22T10:33:37Z

After some more thinking about it, I think
| ActorE of (decs * (name, call_conv, var) list)`

or something similar for the IR is a good next step, and somewhat in line with the NewObjE idiom. I can tackle that refactoring.

That looks sensible and would support internal renaming. But why include the call_conv?

Meta question: Shall I do that on this branch, and land it together? Or can we land this in master as is, and then the refactoring separately?

If the entire pipeline isn't working yet (is it?) then I'd definitely keep it on a branch...

rossberg · 2019-02-22T10:39:10Z

Okay, all comments addressed, I believe.

@nomeata, I suggest doing follow-up work on master, to avoid more rebasing hell.

crusso · 2019-02-22T15:23:54Z

Looks good to me.

crusso

Good stuff.

nomeata · 2019-02-23T01:11:05Z

Created https://dfinity.atlassian.net/browse/AST-58 for the follow-up work.

## Changelog for ic-hs: Branch: master Commits: [dfinity/ic-hs@406decfa...9152a0ff](dfinity/ic-hs@406decf...9152a0f) * [`d4db8d07`](dfinity/ic-hs@d4db8d0) bump nixpkgs to 7c786944f801745310578d1cfc019923396f830c ([dfinity/ic-hs⁠#163](https://github.com/dfinity/ic-hs/issues/163)) * [`31d535d2`](dfinity/ic-hs@31d535d) increase the number of allowed delegations in a request from 4 to 20 ([dfinity/ic-hs⁠#166](https://github.com/dfinity/ic-hs/issues/166)) * [`4a310c0d`](dfinity/ic-hs@4a310c0) support ic0.is_controller ([dfinity/ic-hs⁠#169](https://github.com/dfinity/ic-hs/issues/169)) * [`5fc27bdc`](dfinity/ic-hs@5fc27bd) do not include keep-alive header in httpbin response ([dfinity/ic-hs⁠#170](https://github.com/dfinity/ic-hs/issues/170)) * [`96448083`](dfinity/ic-hs@9644808) drop nix-build-uncached ([dfinity/ic-hs⁠#175](https://github.com/dfinity/ic-hs/issues/175)) * [`c6fbe1f7`](dfinity/ic-hs@c6fbe1f) increase ingress_expiry in reference test suite ([dfinity/ic-hs⁠#176](https://github.com/dfinity/ic-hs/issues/176)) * [`a1b3f670`](dfinity/ic-hs@a1b3f67) add Connection: close header to httpbin ([dfinity/ic-hs⁠#178](https://github.com/dfinity/ic-hs/issues/178)) * [`d3812ffc`](dfinity/ic-hs@d3812ff) increase delegation expiry in tests ([dfinity/ic-hs⁠#182](https://github.com/dfinity/ic-hs/issues/182)) * [`40a46e2f`](dfinity/ic-hs@40a46e2) sync universal-canister with IC repo ([dfinity/ic-hs⁠#177](https://github.com/dfinity/ic-hs/issues/177)) * [`7a6259c2`](dfinity/ic-hs@7a6259c) decrease number of threads and request submission latency ([dfinity/ic-hs⁠#179](https://github.com/dfinity/ic-hs/issues/179)) * [`a9f73dba`](dfinity/ic-hs@a9f73db) fix decoding compressed WASM modules during snapshotting ([dfinity/ic-hs⁠#184](https://github.com/dfinity/ic-hs/issues/184)) * [`64c19a95`](dfinity/ic-hs@64c19a9) bump nixpkgs to eaf03591711b46d21abc7082a8ebee4681f9dbeb ([dfinity/ic-hs⁠#189](https://github.com/dfinity/ic-hs/issues/189)) * [`9152a0ff`](dfinity/ic-hs@9152a0f) add date header to httpbin responses and make http header names lower-case ([dfinity/ic-hs⁠#188](https://github.com/dfinity/ic-hs/issues/188)) Includes and closes #3915. Reason: `ic-hs` and `nixpkgs` must be in sync, so that the artefact caching can work.

rossberg added 2 commits February 20, 2019 22:20

First step

c66c13f

rossberg requested a review from crusso February 21, 2019 10:17

nomeata reviewed Feb 21, 2019

View reviewed changes

rossberg commented Feb 21, 2019

View reviewed changes

crusso reviewed Feb 21, 2019

View reviewed changes

nomeata reviewed Feb 21, 2019

View reviewed changes

src/interpret.ml Show resolved Hide resolved

crusso closed this Feb 21, 2019

crusso reopened this Feb 21, 2019

rossberg added 2 commits February 22, 2019 08:56

Remove special case for blocks from IR

f537c8b

Merge branch 'master' into obj.syntax

d77aece

rossberg added 3 commits February 22, 2019 10:35

Remove name from AST

9fc7d16

Remove duplicate ids from ClassD

d2ac449

Nits

a44371f

rossberg added 3 commits February 22, 2019 15:00

Remove object sort from DotE

86e4117

Update syntax readme

db47aff

Update comment

326fedb

crusso approved these changes Feb 22, 2019

View reviewed changes

rossberg merged commit af17e8d into master Feb 22, 2019

rossberg deleted the obj.syntax branch February 22, 2019 16:21

Conversation

rossberg commented Feb 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nomeata left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nomeata Feb 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crusso Feb 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nomeata commented Feb 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rossberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rossberg commented Feb 21, 2019

Uh oh!

nomeata commented Feb 21, 2019

Uh oh!

rossberg commented Feb 21, 2019

Uh oh!

nomeata commented Feb 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crusso Feb 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rossberg commented Feb 21, 2019 •

edited

Loading

nomeata left a comment •

edited

Loading

nomeata Feb 21, 2019 •

edited

Loading

crusso Feb 21, 2019 •

edited

Loading

nomeata commented Feb 21, 2019 •

edited

Loading

crusso Feb 21, 2019 •

edited

Loading

crusso Feb 21, 2019 •

edited

Loading

crusso Feb 21, 2019 •

edited

Loading

crusso commented Feb 22, 2019 •

edited

Loading