[vm/compiler] Make closures have no context if no variables are closed over

As a side-effect of making closures without captured variables have an
empty context, we not only avoid a memory leak, but also allow such
closures to be sent across [SendPort]s.

Issue #36983
Issue #45603

TEST=vm/dart{_2,}/isolates/closures_without_captured_variables_test

Change-Id: I5e8845203059ff625f7cff3f072d845b5e48ba36
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/212297
Commit-Queue: Martin Kustermann <[email protected]>
Reviewed-by: Slava Egorov <[email protected]>

Author: mkustermann

runtime/tests/vm/dart/isolates/closures_without_captured_variables_test.dart

@@ -0,0 +1,95 @@
+// Copyright (c) 2021, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+// VMOptions=--enable-isolate-groups
+
+// The tests in this file will only succeed when isolate groups are enabled
+// (hence the VMOptions above).
+
+import 'dart:async';
+import 'dart:isolate';
+
+import 'package:expect/expect.dart';
+
+import 'fast_object_copy_test.dart' show ClassWithNativeFields;
+
+main() async {
+  final rp = ReceivePort();
+
+  testNormalEnclosingFunction(rp);
+  testNormalNestedEnclosingFunction(rp);
+  testNormalNestedEnclosingFunction2(rp);
+
+  final si = StreamIterator(rp);
+  for (int i = 0; i < 3; ++i) {
+    Expect.isTrue(await si.moveNext());
+    Expect.equals(42, (si.current)());
+  }
+  si.cancel(); // closes the port
+}
+
+testNormalEnclosingFunction(ReceivePort rp) {
+  final invalidObject = ClassWithNativeFields();
+  final normalObject = Object();
+
+  captureInvalidObject() => invalidObject;
+  captureNormalObject() => normalObject;
+  captureNothing() => 42;
+
+  Expect.throwsArgumentError(() => rp.sendPort.send(captureInvalidObject));
+
+  // TODO(http://dartbug.com/36983): Avoid capturing more than needed.
+  Expect.throwsArgumentError(() => rp.sendPort.send(captureNormalObject));
+
+  // Should not throw, since the [captureNothing] closure should not have a
+  // parent context and therefore not transitively refer [rp].
+  rp.sendPort.send(captureNothing);
+}
+
+testNormalNestedEnclosingFunction(ReceivePort rp) {
+  final invalidObject = ClassWithNativeFields();
+  final normalObject = Object();
+  nested() {
+    captureInvalidObject() => invalidObject;
+    captureNormalObject() => normalObject;
+    captureNothing() => 42;
+
+    Expect.throwsArgumentError(() => rp.sendPort.send(captureInvalidObject));
+
+    // TODO(http://dartbug.com/36983): Avoid capturing more than needed.
+    Expect.throwsArgumentError(() => rp.sendPort.send(captureNormalObject));
+
+    // Should not throw, since the [captureNothing] closure should not have a
+    // parent context and therefore not transitively refer [rp].
+    rp.sendPort.send(captureNothing);
+  }
+
+  nested();
+}
+
+testNormalNestedEnclosingFunction2(ReceivePort rp) {
+  final invalidObject = ClassWithNativeFields();
+  final normalObject = Object();
+
+  captureInvalidObject() {
+    local() => invalidObject;
+    return local;
+  }
+
+  captureNormalObject() {
+    local() => normalObject;
+    return local;
+  }
+
+  captureNothing() => 42;
+
+  Expect.throwsArgumentError(() => rp.sendPort.send(captureInvalidObject));
+
+  // TODO(http://dartbug.com/36983): Avoid capturing more than needed.
+  Expect.throwsArgumentError(() => rp.sendPort.send(captureNormalObject));
+
+  // Should not throw, since the [captureNothing] closure should not have a
+  // parent context and therefore not transitively refer [rp].
+  rp.sendPort.send(captureNothing);
+}

runtime/tests/vm/dart_2/isolates/closures_without_captured_variables_test.dart

@@ -0,0 +1,97 @@
+// Copyright (c) 2021, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+// VMOptions=--enable-isolate-groups
+
+// The tests in this file will only succeed when isolate groups are enabled
+// (hence the VMOptions above).
+
+// @dart=2.9
+
+import 'dart:async';
+import 'dart:isolate';
+
+import 'package:expect/expect.dart';
+
+import 'fast_object_copy_test.dart' show ClassWithNativeFields;
+
+main() async {
+  final rp = ReceivePort();
+
+  testNormalEnclosingFunction(rp);
+  testNormalNestedEnclosingFunction(rp);
+  testNormalNestedEnclosingFunction2(rp);
+
+  final si = StreamIterator(rp);
+  for (int i = 0; i < 3; ++i) {
+    Expect.isTrue(await si.moveNext());
+    Expect.equals(42, (si.current)());
+  }
+  si.cancel(); // closes the port
+}
+
+testNormalEnclosingFunction(ReceivePort rp) {
+  final invalidObject = ClassWithNativeFields();
+  final normalObject = Object();
+
+  captureInvalidObject() => invalidObject;
+  captureNormalObject() => normalObject;
+  captureNothing() => 42;
+
+  Expect.throwsArgumentError(() => rp.sendPort.send(captureInvalidObject));
+
+  // TODO(http://dartbug.com/36983): Avoid capturing more than needed.
+  Expect.throwsArgumentError(() => rp.sendPort.send(captureNormalObject));
+
+  // Should not throw, since the [captureNothing] closure should not have a
+  // parent context and therefore not transitively refer [rp].
+  rp.sendPort.send(captureNothing);
+}
+
+testNormalNestedEnclosingFunction(ReceivePort rp) {
+  final invalidObject = ClassWithNativeFields();
+  final normalObject = Object();
+  nested() {
+    captureInvalidObject() => invalidObject;
+    captureNormalObject() => normalObject;
+    captureNothing() => 42;
+
+    Expect.throwsArgumentError(() => rp.sendPort.send(captureInvalidObject));
+
+    // TODO(http://dartbug.com/36983): Avoid capturing more than needed.
+    Expect.throwsArgumentError(() => rp.sendPort.send(captureNormalObject));
+
+    // Should not throw, since the [captureNothing] closure should not have a
+    // parent context and therefore not transitively refer [rp].
+    rp.sendPort.send(captureNothing);
+  }
+
+  nested();
+}
+
+testNormalNestedEnclosingFunction2(ReceivePort rp) {
+  final invalidObject = ClassWithNativeFields();
+  final normalObject = Object();
+
+  captureInvalidObject() {
+    local() => invalidObject;
+    return local;
+  }
+
+  captureNormalObject() {
+    local() => normalObject;
+    return local;
+  }
+
+  captureNothing() => 42;
+
+  Expect.throwsArgumentError(() => rp.sendPort.send(captureInvalidObject));
+
+  // TODO(http://dartbug.com/36983): Avoid capturing more than needed.
+  Expect.throwsArgumentError(() => rp.sendPort.send(captureNormalObject));
+
+  // Should not throw, since the [captureNothing] closure should not have a
+  // parent context and therefore not transitively refer [rp].
+  rp.sendPort.send(captureNothing);
+}

runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc

@@ -5252,7 +5252,7 @@ Fragment StreamingFlowGraphBuilder::BuildFunctionNode(
     bool has_valid_annotation,
     bool has_pragma,
     intptr_t func_decl_offset) {
-  intptr_t offset = ReaderOffset();
+  const intptr_t offset = ReaderOffset();
 
   FunctionNodeHelper function_node_helper(this);
   function_node_helper.ReadUntilExcluding(FunctionNodeHelper::kTypeParameters);
@@ -5385,7 +5385,11 @@ Fragment StreamingFlowGraphBuilder::BuildFunctionNode(
 
   Fragment instructions;
   instructions += Constant(function);
-  instructions += LoadLocal(parsed_function()->current_context_var());
+  if (scopes()->IsClosureWithEmptyContext(offset)) {
+    instructions += NullConstant();
+  } else {
+    instructions += LoadLocal(parsed_function()->current_context_var());
+  }
   instructions += flow_graph_builder_->AllocateClosure();
   LocalVariable* closure = MakeTemporary();
 

runtime/vm/compiler/frontend/scope_builder.cc

@@ -1492,7 +1492,7 @@ void ScopeBuilder::VisitTypeParameterType() {
 
 void ScopeBuilder::HandleLocalFunction(intptr_t parent_kernel_offset) {
   // "Peek" ahead into the function node
-  intptr_t offset = helper_.ReaderOffset();
+  const intptr_t offset = helper_.ReaderOffset();
 
   FunctionNodeHelper function_node_helper(&helper_);
   function_node_helper.ReadUntilExcluding(FunctionNodeHelper::kTypeParameters);
@@ -1534,6 +1534,14 @@ void ScopeBuilder::HandleLocalFunction(intptr_t parent_kernel_offset) {
 
   VisitFunctionNode();  // read function node.
 
+  // Remember if this closure and all closures nested within it don't
+  // capture any variables from outer scopes.
+  if (scope_->function_level() == 1) {
+    if (scope_->NumCapturedVariables() == 0) {
+      result_->closure_offsets_without_captures.Add(offset);
+    }
+  }
+
   ExitScope(function_node_helper.position_, function_node_helper.end_position_);
   depth_ = saved_depth_state;
   current_function_scope_ = saved_function_scope;

runtime/vm/compiler/frontend/scope_builder.h

@@ -189,6 +189,15 @@ class ScopeBuildingResult : public ZoneAllocated {
         yield_context_variable(NULL),
         raw_variable_counter_(0) {}
 
+  bool IsClosureWithEmptyContext(intptr_t function_node_offset) {
+    for (intptr_t i = 0; i < closure_offsets_without_captures.length(); ++i) {
+      if (closure_offsets_without_captures[i] == function_node_offset) {
+        return true;
+      }
+    }
+    return false;
+  }
+
   IntMap<LocalVariable*> locals;
   IntMap<LocalScope*> scopes;
   GrowableArray<FunctionScope> function_scopes;
@@ -230,6 +239,10 @@ class ScopeBuildingResult : public ZoneAllocated {
   // For-in iterators, one per for-in nesting level.
   GrowableArray<LocalVariable*> iterator_variables;
 
+  // Remembers closure function kernel offsets that do not capture any
+  // variables.
+  GrowableArray<intptr_t> closure_offsets_without_captures;
+
  private:
   DISALLOW_COPY_AND_ASSIGN(ScopeBuildingResult);
 };

tests/lib/isolate/illegal_msg_function_test.dart

@@ -2,7 +2,6 @@
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
-// VMOptions=--enable-isolate-groups
 // VMOptions=--no-enable-isolate-groups
 
 library illegal_msg_function_test;

tests/lib_2/isolate/illegal_msg_function_test.dart

@@ -4,7 +4,6 @@
 
 // @dart = 2.9
 
-// VMOptions=--enable-isolate-groups
 // VMOptions=--no-enable-isolate-groups
 
 library illegal_msg_function_test;