Close over free variables in lambdas and object literals #1648

sylvanc · 2017-03-09T21:21:44Z

Previously, using a variable from the surrounding lexical scope in an object literal or a lambda produced a compiler error. This was surprising to the programmer.

This change allows both lambdas and object literals to use variables in the surrounding lexical scope, as the programmer would expect. In both cases, the closure is expressed under the hood as adding a field that is initialised by passing an alias of the variable to the constructor. For example:

let x = "hi"
let f = {(): String => x}

Here, when f rewrites to an object literal, it becomes:

let f =
  object
    var x: String = x
    fun apply(): String => x
  end

…paramrefs

sylvanc · 2017-03-09T21:22:20Z

I didn't create an RFC for this, as I think it is a "principle of least surprise" bug.

SeanTAllen · 2017-03-09T21:27:21Z

THIS MAKES ME SO GOD DAMN HAPPY!

SeanTAllen · 2017-03-09T21:28:56Z

src/libponyc/expr/lambda.c

+  }
+
+  // TODO: add as a field, add as a constructor parameter, init field from
+  // parameter in constructor, add at the call site


does this TODO need to be handled before merging? Should we create an issue for it?

Oops, stray TODO, already handled.

SeanTAllen · 2017-03-09T21:30:14Z

src/libponyc/pass/expr.c

@@ -292,6 +292,11 @@ ast_result_t pass_expr(ast_t** astp, pass_opt_t* options)
    case TK_ADDRESS:    r = expr_addressof(options, ast); break;
    case TK_DIGESTOF:   r = expr_digestof(options, ast); break;

+    case TK_OBJECT:


for my own edificaton, what's this?

That's moving object literals from the sugar pass (where they used to be, as pure sugar for an anonymous type and a constructor) to the expression pass, where they live now, because they need to know about the existence and type of variables within lexical scope.

SeanTAllen · 2017-03-09T21:31:04Z

@sylvanc should the 6 commits be squashed to one logical commit before merging?

sylvanc · 2017-03-09T22:44:01Z

Yes, squash before merge, definitely. I'm afraid those commits are a bit "stream of work".

SeanTAllen · 2017-03-09T23:05:36Z

Looks good to me if CI passes and the squashing happens. But @jemc is far more qualified than I in all matters compiler.

jemc

Awesome!

jemc · 2017-03-10T01:12:32Z

src/libponyc/ast/astbuild.h

@@ -69,7 +69,7 @@
 #define TREE_CLEAR_PASS(tree) \
  { \
    if(ast_parent(tree) != NULL) tree = ast_dup(tree); \
-    ast_resetpass(tree); \
+    ast_resetpass(tree. 0); \


Looks like a typo here - should this period be a comma?

Oops! Yup, typo. I guess that macro is never used :)

jemc · 2017-03-10T01:28:47Z

test/libponyc/sugar.cc

-    "    object ref be foo() => 4 end";
-
-  TEST_ERROR(short_form);
-}


There's a lot of tests removed here that were covering some important aspects of object literals.

I understand that the object processing was moved out of the sugar pass and into the expr pass, so that these tests don't really belong in this file anymore, and that TEST_EQUIV no longer makes sense. However, adapting these tests for the expr pass and figuring out how to cover the same aspects would be ideal, so we can prevent regressions as we move forward.

Also, any other tests that can be made to reflect possible pitfalls of the capture process would be great. I saw that this PR adds a test to builtin_test, but it doesn't seem like enough to really cover this major feature, especially with all these tests having been removed.

If you don't have the time to devote to adding more tests to this PR, I'd be happy to help chip in and make sure we have some more tests.

One aspect coming to mind - it would be great to confirm the right behaviour and/or error message in some of the problem cases, like closing over a non-sendable local in an object iso, for example.

jemc · 2017-03-10T01:57:36Z

@sylvanc Does anything in the tutorial need to be updated to reflect this new feature?

sylvanc · 2017-03-10T09:01:43Z

I agree that additional tests would be good, probably in parse_expr.cc. If you're up for chipping in, that would be awesome! Feel free to push changes to this branch, or make a PR to do after this, or whatever suits you.

For the tutorial:

https://tutorial.ponylang.org/expressions/object-literals.html

That page could use a bit of editing to show closing over free variables. I don't think it needs a big explanation or anything, just showing how we can capture from the lexical scope anywhere, not just in field initialisers.

sylvanc · 2017-03-10T22:36:35Z

I added a few test cases.

sylvanc · 2017-03-10T22:50:13Z

Added a tutorial PR as well:

ponylang/pony-tutorial#172

jemc

Thanks for adding some more test cases!

If I think of any more to add, I can do it asynchronously in another PR.

Looks good to me - feel free to merge when CI passes.

sylvanc and others added 6 commits March 7, 2017 08:58

move object literal handling from sugar to expr pass

b055373

comment out capture code

0f2b7c7

parameterised type aliases aren't generating correct refcaps for type…

879b332

…paramrefs

reset the pass when resolving type aliases

a407728

capture free variables

84ca9eb

free variable capture test

8bc0cf4

sylvanc added the changelog - added Automatically add "Added" CHANGELOG entry on merge label Mar 9, 2017

sylvanc requested review from SeanTAllen and jemc March 9, 2017 21:21

SeanTAllen reviewed Mar 9, 2017

View reviewed changes

remove stray TODO comment

37f0a8c

remove unnecessary explicit lambda captures in the stdlib

eb0138d

jemc reviewed Mar 10, 2017

View reviewed changes

sylvanc added 3 commits March 10, 2017 21:13

merge

d64cd33

fix macro typo

71b848e

lambda and object literal tests through the expr pass

8359f0a

jemc approved these changes Mar 10, 2017

View reviewed changes

SeanTAllen merged commit eaf1dcd into master Mar 11, 2017

ponylang-main added a commit that referenced this pull request Mar 11, 2017

Update CHANGELOG for PR #1648

a4f3fab

SeanTAllen deleted the close-over-free branch March 11, 2017 00:48

jemc mentioned this pull request Mar 17, 2017

Nested lambdas can't auto-capture free references from the outer scope. #1708

Open

jemc mentioned this pull request Apr 21, 2017

Refactor: Add the refer pass to the compiler. #1715

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Close over free variables in lambdas and object literals #1648

Close over free variables in lambdas and object literals #1648

sylvanc commented Mar 9, 2017

sylvanc commented Mar 9, 2017

SeanTAllen commented Mar 9, 2017

SeanTAllen Mar 9, 2017

sylvanc Mar 9, 2017

SeanTAllen Mar 9, 2017

sylvanc Mar 9, 2017

SeanTAllen commented Mar 9, 2017

sylvanc commented Mar 9, 2017

SeanTAllen commented Mar 9, 2017

jemc left a comment

jemc Mar 10, 2017

sylvanc Mar 10, 2017

jemc Mar 10, 2017

jemc Mar 10, 2017 •

edited

Loading

jemc commented Mar 10, 2017

sylvanc commented Mar 10, 2017 •

edited

Loading

sylvanc commented Mar 10, 2017

sylvanc commented Mar 10, 2017 •

edited

Loading

jemc left a comment

Close over free variables in lambdas and object literals #1648

Close over free variables in lambdas and object literals #1648

Conversation

sylvanc commented Mar 9, 2017

sylvanc commented Mar 9, 2017

SeanTAllen commented Mar 9, 2017

SeanTAllen Mar 9, 2017

Choose a reason for hiding this comment

sylvanc Mar 9, 2017

Choose a reason for hiding this comment

SeanTAllen Mar 9, 2017

Choose a reason for hiding this comment

sylvanc Mar 9, 2017

Choose a reason for hiding this comment

SeanTAllen commented Mar 9, 2017

sylvanc commented Mar 9, 2017

SeanTAllen commented Mar 9, 2017

jemc left a comment

Choose a reason for hiding this comment

jemc Mar 10, 2017

Choose a reason for hiding this comment

sylvanc Mar 10, 2017

Choose a reason for hiding this comment

jemc Mar 10, 2017

Choose a reason for hiding this comment

jemc Mar 10, 2017 • edited Loading

Choose a reason for hiding this comment

jemc commented Mar 10, 2017

sylvanc commented Mar 10, 2017 • edited Loading

sylvanc commented Mar 10, 2017

sylvanc commented Mar 10, 2017 • edited Loading

jemc left a comment

Choose a reason for hiding this comment

jemc Mar 10, 2017 •

edited

Loading

sylvanc commented Mar 10, 2017 •

edited

Loading

sylvanc commented Mar 10, 2017 •

edited

Loading