Some Con-related cleanups. by nomeata · Pull Request #181 · caffeinelabs/motoko

nomeata · 2019-02-21T02:53:59Z

Three commits here:

Get rid of modify_con
Move the ref-handling into Con
Change how async deals with stamps and kinds

please also consider their commit messages (especially the last one).

The first two should be uncontroversial, the last one maybe not. Claudio, if
you disagree with the direction of the last one I can separte that into a
separate PR, so that we can merge the other two first.

this function is dubious, as we want to treat the con annotations like futures, and use their mutability only in `typing`, but not later on. So all mutation should happen via `clone_con`.

which gives both a more convenient interface and better static guarantees.

to allow for the (I think) desirable invariant: Same stamp ⟺ same ref. Previously, the pass would clone all type constructors, even those not defined in the current `prog`. It worked, but I find it fishy: Why cone `Int` when `Int` is not defined here? So this patch proposes to do something different, namely leaving already defined cons alone (rationale: if they are not defined here, we don't get to change them). Eventually, if we want to do an `async` translation on two separate program fragments, then we will have to pass the `con_renaming` down; everything else is a hack that does not generalize to more complicated type-changing passes (e.g. those where the type change depends on the actual code, such as inlining or unboxing). Oh, and it would be cleaner to separate `t_con` into `t_con_def` for binding sites (where we always clone) and `t_con_def` for occurrence sites (where we only lookup and fail if it is no there), and the invariant that the domain of `con_renaming` is always the in-scope cons. I did not do that because it is somewhat tedious with recursive declarations, and the current code works. But we might have to do that eventually. (It is a pattern that is very familiar to me from GHC.)

crusso

Nice changes!

Looks good to me, especially the addition of the scope to async.transform. I think it would be better if all transforms had the same interface (perhaps we should add a signature), so perhaps best to add the scope argument to async and tco too.

nomeata · 2019-02-21T15:04:02Z

I think it would be better if all transforms had the same interface (perhaps we should add a signature), so perhaps best to add the scope argument to async and tco too.

I find it helpful to see from the interface of a transformation what kind of information they use (Just like I prefer polymorphic type signatures over concrete ones, if possible). And it is really simple to adjust their interface in pipeline.ml as needed, using a simple fun _ => _.

crusso · 2019-02-21T16:18:27Z

Well, you could still make the transform implementation parametric to stress that point. But go ahead an merge if you like.

rossberg · 2019-02-22T08:07:44Z

src/con.ml

+let fresh name k =
+  {name; stamp = fresh_stamp name; kind = ref k}
+let clone c k =
+  { c with stamp = fresh_stamp c.name; kind = ref k } (* Does not change the stamp! *)


While merging this with my change, I noticed this comment. Doesn't it contradict the actual code? What is the intention?

This does look broken to me. The original intention of clone was to preserve the stamp but change the interpretation so that you can still link fragments that have gone through the same translation.
So you can do async on the prelude and async on the program and the stamps will be the same, the refs will be different, but have the same contents.

Actually, it's ok since Joachim has changed the invariant (and documented it) and requires you to pass down the con_renaming for passes that need it. Probably the better solution in the long run but will require more code changes once the Prelude mentions types that need tranforming.

Okay, I'll remove the comment in my change.

Thanks, and sorry for the confusion.

nomeata added 4 commits February 20, 2019 22:18

Actually run Check_ir on prog', not prog

ad8b9e0

Get rid of modify_con

84645f4

this function is dubious, as we want to treat the con annotations like futures, and use their mutability only in `typing`, but not later on. So all mutation should happen via `clone_con`.

Move the ref-handling into Con

50b4902

which gives both a more convenient interface and better static guarantees.

crusso requested changes Feb 21, 2019

View reviewed changes

nomeata mentioned this pull request Feb 21, 2019

Generalise objects #182

Merged

crusso approved these changes Feb 21, 2019

View reviewed changes

nomeata merged commit 99f2a8d into master Feb 21, 2019

nomeata deleted the joachim/con-cleanup branch February 21, 2019 17:04

rossberg reviewed Feb 22, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some Con-related cleanups.#181

Some Con-related cleanups.#181
nomeata merged 4 commits intomasterfrom
joachim/con-cleanup

nomeata commented Feb 21, 2019

Uh oh!

crusso left a comment •

edited

Loading

Uh oh!

nomeata commented Feb 21, 2019

Uh oh!

crusso commented Feb 21, 2019

Uh oh!

rossberg Feb 22, 2019

Uh oh!

crusso Feb 22, 2019

Uh oh!

crusso Feb 22, 2019 •

edited

Loading

Uh oh!

rossberg Feb 22, 2019

Uh oh!

nomeata Feb 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

nomeata commented Feb 21, 2019

Uh oh!

crusso 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!

nomeata commented Feb 21, 2019

Uh oh!

crusso commented Feb 21, 2019

Uh oh!

rossberg Feb 22, 2019

Choose a reason for hiding this comment

Uh oh!

crusso Feb 22, 2019

Choose a reason for hiding this comment

Uh oh!

crusso Feb 22, 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!

rossberg Feb 22, 2019

Choose a reason for hiding this comment

Uh oh!

nomeata Feb 23, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

crusso left a comment •

edited

Loading

crusso Feb 22, 2019 •

edited

Loading