Optimize js_util setProperty calls for non-function values to

_setPropertyUnchecked version that can be inlined.

No change in the generated JavaScript for dart2js.

Change-Id: Ie4a8e5a34826b6c9083d34656aaa27050635cb21
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/200933
Reviewed-by: Srujan Gaddam <srujzs@google.com>
Reviewed-by: Sigmund Cherem <sigmund@google.com>
Commit-Queue: Riley Porter <rileyporter@google.com>

pkg/_js_interop_checks/lib/src/transformations/js_util_optimizer.dart

@@ -11,21 +11,47 @@ import 'package:kernel/kernel.dart';
 class JsUtilOptimizer extends Transformer {
   final Procedure _jsTarget;
   final Procedure _getPropertyTarget;
+  final Procedure _setPropertyTarget;
+  final Procedure _setPropertyUncheckedTarget;
+
+  /// Dynamic members in js_util that interop allowed.
+  static final Iterable<String> _allowedInteropJsUtilMembers = <String>[
+    'callConstructor',
+    'callMethod',
+    'getProperty',
+    'jsify',
+    'newObject',
+    'setProperty'
+  ];
+  final Iterable<Procedure> _allowedInteropJsUtilTargets;
+  final Procedure _allowInteropTarget;
 
   JsUtilOptimizer(CoreTypes coreTypes)
       : _jsTarget =
             coreTypes.index.getTopLevelMember('dart:_foreign_helper', 'JS'),
         _getPropertyTarget =
-            coreTypes.index.getTopLevelMember('dart:js_util', 'getProperty') {}
+            coreTypes.index.getTopLevelMember('dart:js_util', 'getProperty'),
+        _setPropertyTarget =
+            coreTypes.index.getTopLevelMember('dart:js_util', 'setProperty'),
+        _setPropertyUncheckedTarget = coreTypes.index
+            .getTopLevelMember('dart:js_util', '_setPropertyUnchecked'),
+        _allowInteropTarget =
+            coreTypes.index.getTopLevelMember('dart:js', 'allowInterop'),
+        _allowedInteropJsUtilTargets = _allowedInteropJsUtilMembers.map(
+            (member) =>
+                coreTypes.index.getTopLevelMember('dart:js_util', member)) {}
 
-  /// Replaces js_util method calls with lowering straight to JS fragment call.
+  /// Replaces js_util method calls with optimization when possible.
   ///
-  /// Lowers the following types of js_util calls:
-  ///  - `getProperty` for any argument types
+  /// Lowers `getProperty` for any argument type straight to JS fragment call.
+  /// Lowers `setProperty` to `_setPropertyUnchecked` for values that are
+  /// not Function type and guaranteed to be interop allowed.
   @override
   visitStaticInvocation(StaticInvocation node) {
     if (node.target == _getPropertyTarget) {
       node = _lowerGetProperty(node);
+    } else if (node.target == _setPropertyTarget) {
+      node = _lowerSetProperty(node);
     }
     node.transformChildren(this);
     return node;
@@ -49,7 +75,60 @@ class JsUtilOptimizer extends Transformer {
           ],
           // TODO(rileyporter): Copy type from getProperty when it's generic.
           types: [DynamicType()],
-        )..fileOffset = node.arguments.fileOffset)
+        )..fileOffset = arguments.fileOffset)
       ..fileOffset = node.fileOffset;
   }
+
+  /// Lowers the given js_util `setProperty` call to `_setPropertyUnchecked`
+  /// when the additional validation checks in `setProperty` can be elided.
+  /// Removing the checks allows further inlining by the compilers.
+  StaticInvocation _lowerSetProperty(StaticInvocation node) {
+    Arguments arguments = node.arguments;
+    assert(arguments.types.isEmpty);
+    assert(arguments.positional.length == 3);
+    assert(arguments.named.isEmpty);
+
+    if (!_allowedInterop(arguments.positional.last)) {
+      return node;
+    }
+
+    return StaticInvocation(_setPropertyUncheckedTarget, arguments)
+      ..fileOffset = node.fileOffset;
+  }
+
+  /// Returns whether the given TreeNode is guaranteed to be allowed to interop
+  /// with JS.
+  ///
+  /// Returns true when the node is guaranteed to be not a function:
+  ///    - has a DartType that is NullType or an InterfaceType that is not
+  ///      Function or Object
+  /// Also returns true for allowed method calls within the JavaScript domain:
+  ///        - dart:_foreign_helper JS
+  ///        - dart:js `allowInterop`
+  ///        - dart:js_util and any of the `_allowedInteropJsUtilMembers`
+  bool _allowedInterop(TreeNode node) {
+    // TODO(rileyporter): Detect functions that have been wrapped at some point
+    // with `allowInterop`
+    // TODO(rileyporter): Use staticTypeContext to generalize type checking and
+    // allow more non-function types. Currently, we skip all literal types.
+    var checkType;
+    if (node is VariableGet) {
+      checkType = node.variable.type;
+    }
+
+    if (node is StaticInvocation) {
+      if (node.target == _allowInteropTarget) return true;
+      if (node.target == _jsTarget) return true;
+      if (_allowedInteropJsUtilTargets.contains(node.target)) return true;
+      checkType = node.target.function.returnType;
+    }
+
+    if (checkType is InterfaceType) {
+      return checkType.classNode.name != 'Function' &&
+          checkType.classNode.name != 'Object';
+    } else {
+      // Only other DartType guaranteed to not be a function.
+      return checkType is NullType;
+    }
+  }
 }

pkg/compiler/lib/src/kernel/dart2js_target.dart

@@ -112,6 +112,7 @@ class Dart2jsTarget extends Target {
         'dart:_interceptors',
         'dart:_js_helper',
         'dart:_late_helper',
+        'dart:js',
         'dart:js_util'
       ];
 

pkg/dev_compiler/lib/src/kernel/target.dart

@@ -93,6 +93,7 @@ class DevCompilerTarget extends Target {
         'dart:collection',
         'dart:html',
         'dart:indexed_db',
+        'dart:js',
         'dart:js_util',
         'dart:math',
         'dart:svg',

sdk/lib/js_util/js_util.dart

@@ -68,14 +68,24 @@ dynamic newObject() => JS('=Object', '{}');
 
 bool hasProperty(Object o, Object name) => JS('bool', '# in #', name, o);
 
+// All usage optimized away in a CFE transformation. Changes here will not
+// affect the generated JS.
 dynamic getProperty(Object o, Object name) =>
     JS('Object|Null', '#[#]', o, name);
 
+// Some usage optimized away in a CFE transformation. If given value is a
+// function, changes here will not affect the generated JS.
 dynamic setProperty(Object o, Object name, Object? value) {
   assertInterop(value);
   return JS('', '#[#]=#', o, name, value);
 }
 
+/// Unchecked version of setProperty, only used in a CFE transformation.
+@pragma('dart2js:tryInline')
+dynamic _setPropertyUnchecked(Object o, Object name, Object? value) {
+  return JS('', '#[#]=#', o, name, value);
+}
+
 dynamic callMethod(Object o, String method, List<Object?> args) {
   assertInteropArgs(args);
   return JS('Object|Null', '#[#].apply(#, #)', o, method, o, args);

tests/lib/js/js_util/properties_test.dart

@@ -257,6 +257,8 @@ main() {
       js_util.setProperty(f, 'a', 100);
       expect(f.a, equals(100));
       expect(js_util.getProperty(f, 'a'), equals(100));
+      js_util.setProperty(f, 'a', null);
+      expect(f.a, equals(null));
 
       expect(js_util.getProperty(f, 'list') is List, isTrue);
       js_util.setProperty(f, 'list', [8]);
@@ -265,6 +267,23 @@ main() {
 
       js_util.setProperty(f, 'newProperty', 'new');
       expect(js_util.getProperty(f, 'newProperty'), equals('new'));
+
+      // Using a variable for the property value.
+      var num = 4;
+      js_util.setProperty(f, 'a', num);
+      expect(f.a, equals(num));
+      var str = 'bar';
+      js_util.setProperty(f, 'a', str);
+      expect(f.a, equals(str));
+      var b = false;
+      js_util.setProperty(f, 'a', b);
+      expect(f.a, equals(b));
+      var list = [2, 4, 6];
+      js_util.setProperty(f, 'a', list);
+      expect(f.a, equals(list));
+      var fn = allowInterop(dartFunction);
+      js_util.setProperty(f, 'a', fn);
+      expect(f.a, equals(fn));
     });
 
     test('typed literal', () {
@@ -319,6 +338,13 @@ main() {
       String bar = _getBarWithSideEffect();
       js_util.setProperty(f, bar, 'baz');
       expect(js_util.getProperty(f, bar), equals('baz'));
+      js_util.setProperty(f, _getBarWithSideEffect(), 'mumble');
+      expect(js_util.getProperty(f, bar), equals('mumble'));
+
+      // Set property to a function call.
+      js_util.setProperty(f, 'a', dartFunction());
+      String expected = dartFunction();
+      expect(f.a, equals(expected));
 
       // Using a tearoff as the property value
       js_util.setProperty(f, 'tearoff', allowInterop(ExampleTearoff().foo));

tests/lib_2/js/js_util/properties_test.dart

@@ -259,6 +259,8 @@ main() {
       js_util.setProperty(f, 'a', 100);
       expect(f.a, equals(100));
       expect(js_util.getProperty(f, 'a'), equals(100));
+      js_util.setProperty(f, 'a', null);
+      expect(f.a, equals(null));
 
       expect(js_util.getProperty(f, 'list') is List, isTrue);
       js_util.setProperty(f, 'list', [8]);
@@ -267,6 +269,23 @@ main() {
 
       js_util.setProperty(f, 'newProperty', 'new');
       expect(js_util.getProperty(f, 'newProperty'), equals('new'));
+
+      // Using a variable for the property value.
+      var num = 4;
+      js_util.setProperty(f, 'a', num);
+      expect(f.a, equals(num));
+      var str = 'bar';
+      js_util.setProperty(f, 'a', str);
+      expect(f.a, equals(str));
+      var b = false;
+      js_util.setProperty(f, 'a', b);
+      expect(f.a, equals(b));
+      var list = [2, 4, 6];
+      js_util.setProperty(f, 'a', list);
+      expect(f.a, equals(list));
+      var fn = allowInterop(dartFunction);
+      js_util.setProperty(f, 'a', fn);
+      expect(f.a, equals(fn));
     });
 
     test('typed literal', () {
@@ -321,6 +340,13 @@ main() {
       String bar = _getBarWithSideEffect();
       js_util.setProperty(f, bar, 'baz');
       expect(js_util.getProperty(f, bar), equals('baz'));
+      js_util.setProperty(f, _getBarWithSideEffect(), 'mumble');
+      expect(js_util.getProperty(f, bar), equals('mumble'));
+
+      // Set property to a function call.
+      js_util.setProperty(f, 'a', dartFunction());
+      String expected = dartFunction();
+      expect(f.a, equals(expected));
 
       // Using a tearoff as the property value
       js_util.setProperty(f, 'tearoff', allowInterop(ExampleTearoff().foo));