Merge pull request #82 from greglittlefield-wf/UIP-2410_ddc/dev

UIP-2410: Basic DDC support

Author: aaronlademann-wf

dart_test.yaml

@@ -0,0 +1,9 @@
+tags:
+  # DDC-related tags declared here, since you can't target the DDC using platform selectors:
+  # https://github.com/dart-lang/test/issues/652
+
+  # Tests that should only be run in the dev compiler
+  ddc:
+    skip: "skipping DDC-specific tests until UIP-2410 is implemented"
+  # Tests that should NOT be run in the dev compiler
+  no-ddc:

lib/src/component/dom_components.dart

@@ -35,7 +35,7 @@ DomProps domProps([Map backingMap]) => new DomProps(null, backingMap);
 
 typedef DomProps DomPropsFactory();
 
-class DomProps extends component_base.UiProps with DomPropsMixin, ReactPropsMixin {
+class DomProps extends component_base.UiProps with DomPropsMixin {
   // Wrap Map literal in parens to work around https://github.com/dart-lang/sdk/issues/24410
   DomProps(this.componentFactory, [Map props]) : this.props = props ?? ({});
 
@@ -45,7 +45,7 @@ class DomProps extends component_base.UiProps with DomPropsMixin, ReactPropsMixi
   final Map props;
 }
 
-class SvgProps extends component_base.UiProps with DomPropsMixin, ReactPropsMixin, SvgPropsMixin implements DomProps {
+class SvgProps extends component_base.UiProps with DomPropsMixin, SvgPropsMixin implements DomProps {
   // Wrap Map literal in parens to work around https://github.com/dart-lang/sdk/issues/24410
   SvgProps(this.componentFactory, [Map props]) : this.props = props ?? ({});
 

lib/src/component_declaration/component_base.dart

@@ -28,6 +28,7 @@ import 'package:over_react/over_react.dart' show
     PropError;
 
 import 'package:over_react/src/component_declaration/component_type_checking.dart';
+import 'package:over_react/src/util/ddc_emulated_function_name_bug.dart' as ddc_emulated_function_name_bug;
 import 'package:react/react.dart' as react;
 import 'package:react/react_client.dart';
 
@@ -257,7 +258,31 @@ abstract class UiStatefulComponent<TProps extends UiProps, TState extends UiStat
 ///
 /// Note: Implements MapViewMixin instead of extending it so that the abstract [State] declarations
 /// don't need a constructor. The generated implementations can mix that functionality in.
-abstract class UiState extends Object with MapViewMixin, StateMapViewMixin implements Map {}
+abstract class UiState extends Object implements StateMapViewMixin, MapViewMixin, Map {
+  // Manually implement members from `StateMapViewMixin`,
+  // since mixing that class in doesn't play well with the DDC.
+  // TODO find out root cause and reduced test case.
+  @override Map get _map => this.state;
+  @override String toString() => '$runtimeType: ${prettyPrintMap(_map)}';
+
+  // Manually implement members from `MapViewMixin`,
+  // since mixing that class in doesn't play well with the DDC.
+  // TODO find out root cause and reduced test case.
+  @override operator[](Object key) => _map[key];
+  @override void operator[]=(key, value) { _map[key] = value; }
+  @override void addAll(other) { _map.addAll(other); }
+  @override void clear() { _map.clear(); }
+  @override putIfAbsent(key, ifAbsent()) => _map.putIfAbsent(key, ifAbsent);
+  @override bool containsKey(Object key) => _map.containsKey(key);
+  @override bool containsValue(Object value) => _map.containsValue(value);
+  @override void forEach(void action(key, value)) { _map.forEach(action); }
+  @override bool get isEmpty => _map.isEmpty;
+  @override bool get isNotEmpty => _map.isNotEmpty;
+  @override int get length => _map.length;
+  @override Iterable get keys => _map.keys;
+  @override remove(Object key) => _map.remove(key);
+  @override Iterable get values => _map.values;
+}
 
 /// The string used by default for the key of the attribute added by [UiProps.addTestId].
 const defaultTestIdKey = 'data-test-id';
@@ -275,9 +300,41 @@ typedef PropsModifier(Map props);
 ///
 /// Note: Implements MapViewMixin instead of extending it so that the abstract [Props] declarations
 /// don't need a constructor. The generated implementations can mix that functionality in.
-abstract class UiProps
-    extends Object with MapViewMixin, PropsMapViewMixin, ReactPropsMixin, UbiquitousDomPropsMixin, CssClassPropsMixin
-    implements Map {
+abstract class UiProps extends Object
+    with ReactPropsMixin, UbiquitousDomPropsMixin, CssClassPropsMixin
+    implements PropsMapViewMixin, MapViewMixin, Map {
+
+  UiProps() {
+    // Work around https://github.com/dart-lang/sdk/issues/27647 for all UiProps instances
+    if (ddc_emulated_function_name_bug.isBugPresent) {
+      ddc_emulated_function_name_bug.patchName(this);
+    }
+  }
+
+  // Manually implement members from `MapViewMixin`,
+  // since mixing that class in doesn't play well with the DDC.
+  // TODO find out root cause and reduced test case.
+  @override operator[](Object key) => _map[key];
+  @override void operator[]=(key, value) { _map[key] = value; }
+  @override void addAll(other) { _map.addAll(other); }
+  @override void clear() { _map.clear(); }
+  @override putIfAbsent(key, ifAbsent()) => _map.putIfAbsent(key, ifAbsent);
+  @override bool containsKey(Object key) => _map.containsKey(key);
+  @override bool containsValue(Object value) => _map.containsValue(value);
+  @override void forEach(void action(key, value)) { _map.forEach(action); }
+  @override bool get isEmpty => _map.isEmpty;
+  @override bool get isNotEmpty => _map.isNotEmpty;
+  @override int get length => _map.length;
+  @override Iterable get keys => _map.keys;
+  @override remove(Object key) => _map.remove(key);
+  @override Iterable get values => _map.values;
+
+  // Manually implement members from `StateMapViewMixin`,
+  // since mixing that class in doesn't play well with the DDC.
+  // TODO find out root cause and reduced test case.
+  @override Map get _map => this.props;
+  @override String toString() => '$runtimeType: ${prettyPrintMap(_map)}';
+
   /// Adds an arbitrary prop key-value pair if [shouldAdd] is true, otherwise, does nothing.
   void addProp(propKey, value, [bool shouldAdd = true]) {
     if (!shouldAdd) return;
@@ -362,10 +419,6 @@ abstract class UiProps
   @override
   dynamic noSuchMethod(Invocation invocation) {
     if (invocation.memberName == #call && invocation.isMethod) {
-      var parameters = []
-        ..add(props)
-        ..addAll(invocation.positionalArguments);
-
       assert(() {
         // These checks are within the assert so they are not done in production.
         var children = invocation.positionalArguments;
@@ -377,7 +430,18 @@ abstract class UiProps
         return _validateChildren(children);
       });
 
-      return Function.apply(componentFactory, parameters);
+      final factory = componentFactory;
+      if (factory is ReactComponentFactoryProxy) {
+        // Use `build` instead of using emulated function behavior to work around DDC issue
+        // https://github.com/dart-lang/sdk/issues/29904
+        // Should have the benefit of better performance; TODO optimize type check?
+        return factory.build(props, invocation.positionalArguments);
+      } else {
+        var parameters = []
+          ..add(props)
+          ..addAll(invocation.positionalArguments);
+        return Function.apply(factory, parameters);
+      }
     }
 
     return super.noSuchMethod(invocation);

lib/src/transformer/README.md

@@ -16,7 +16,23 @@ much cleaner and more _grokkable_.
  
  
 
-
+## Transformer options:
+The following configuration options are available for the `over_react` transformer.
+
+All values shown are the defaults 
+
+```yaml
+transformers:
+- over_react:
+    # Whether to apply a workaround in transformed props/state classes for a DDC bug
+    # in which abstract accessors clobber inherited concrete implementations:
+    # https://github.com/dart-lang/sdk/issues/29914.
+    #
+    # Fixes the issue by generating corresponding abstract getters/setters to
+    # complete the pair, limited to problematic accessors within transformed 
+    # props/state classes that have the `@override` annotation.
+    fixDdcAbstractAccessors: false
+```
 
 ## Wiring it all up
 

lib/src/transformer/impl_generation.dart

@@ -53,6 +53,7 @@ class ImplGenerator {
 
   final TransformLogger logger;
   final TransformedSourceFile transformedFile;
+  bool shouldFixDdcAbstractAccessors = false;
 
   SourceFile get sourceFile => transformedFile.sourceFile;
 
@@ -392,6 +393,10 @@ class ImplGenerator {
       AccessorType type,
       NodeWithMeta<ClassDeclaration, annotations.TypedMap> typedMap
   ) {
+    if (shouldFixDdcAbstractAccessors) {
+      fixDdcAbstractAccessors(type, typedMap);
+    }
+
     String keyNamespace = getAccessorKeyNamespace(typedMap);
 
     final bool isProps = type == AccessorType.props;
@@ -588,6 +593,66 @@ class ImplGenerator {
         staticVariablesImpl
     );
   }
+
+  /// Apply a workaround for an issue where, in the DDC, abstract getter or setter overrides declared in a class clobber
+  /// the inherited concrete implementations. <https://github.com/dart-lang/sdk/issues/29914>
+  ///
+  /// Fixes the issue by generating corresponding abstract getters/setters to complete the pair
+  /// for accessors with the `@override` annotation.
+  void fixDdcAbstractAccessors(
+    AccessorType accessorType,
+    NodeWithMeta<ClassDeclaration, annotations.TypedMap> typedMap,
+  ) {
+    var candidateAccessors = new List<MethodDeclaration>.from(
+        typedMap.node.members.where((member) =>
+            member is MethodDeclaration &&
+            (member.isGetter || member.isSetter) &&
+            !member.isSynthetic &&
+            !member.isStatic &&
+            member.metadata.any((meta) => meta.name.name == 'override')
+        )
+    );
+
+    for (var accessor in candidateAccessors) {
+      // Non-abstract accessors don't exhibit this issue.
+      if (!accessor.isAbstract) return;
+
+      var name = accessor.name.name;
+
+      // Don't generate for `Map get props;`/`Map get state;` in mixins
+      if (accessorType == AccessorType.props && name == 'props') continue;
+      if (accessorType == AccessorType.state && name == 'state') continue;
+
+      if (candidateAccessors.any((other) => other != accessor && other.name.name == name)) {
+        // Don't generate when both the getter and the setter are declared.
+        continue;
+      }
+
+      /// Warning: tests rely on this comment as a means of determining whether this fix was applied.
+      ///
+      /// DO NOT modify or remove without updating tests
+      const String generatedComment = ' /* fixDdcAbstractAccessors workaround: */ ';
+
+      if (accessor.isGetter) {
+        var type = accessor.returnType?.toSource();
+        var typeString = type == null ? '' : '$type ';
+
+        transformedFile.insert(sourceFile.location(accessor.end),
+            // use `covariant` here to be extra safe in this typing
+            '${generatedComment}set $name(covariant ${typeString}value);');
+      } else {
+        var parameter = accessor.parameters.parameters.single;
+        var type = parameter is SimpleFormalParameter
+            ? parameter.type?.toSource()
+            // This `null` case is mainly for [FunctionTypedFormalParameter].
+            : null;
+        var typeString = type == null ? '' : '$type ';
+
+        transformedFile.insert(sourceFile.location(accessor.end),
+            '$generatedComment${typeString}get $name;');
+      }
+    }
+  }
 }
 
 enum AccessorType {props, state}

lib/src/util/ddc_emulated_function_name_bug.dart

@@ -0,0 +1,85 @@
+/// Provides detection and patching of the bug described in <https://github.com/dart-lang/sdk/issues/27647>,
+/// in which getters/setters with the identifier `name` don't work for emulated function classes, like [UiProps].
+@JS()
+library over_react.src.util.ddc_emulated_function_name_bug;
+
+import 'package:js/js.dart';
+import 'package:over_react/over_react.dart';
+
+/// Create a reduced test case of the issue, using an emulated function pattern that is similar to [UiProps].
+///
+/// We can't use [UiProps] itself, since it uses [isBugPresent], and that would cause a cyclic initialization error.
+class _NsmEmulatedFunctionWithNameProperty implements Function {
+  void call();
+
+  @override
+  noSuchMethod(i) {}
+
+  String _name;
+
+  // ignore: unnecessary_getters_setters
+  String get name => _name;
+  // ignore: unnecessary_getters_setters
+  set name(String value) => _name = value;
+}
+
+/// Whether this bug, <https://github.com/dart-lang/sdk/issues/27647>, is present in the current runtime.
+///
+/// This performs functional detection of the bug, and  will be `true`
+/// only in the DDC and only in versions of the DDC where this bug is present.
+final bool isBugPresent = (() {
+  const testValue = 'test value';
+
+  var testObject = new _NsmEmulatedFunctionWithNameProperty();
+
+  try {
+    // In the DDC, this throws:
+    //     TypeError: Cannot assign to read only property 'name' of function 'function call(...args) {
+    //       return call.call.apply(call, args);
+    //     }'
+    testObject.name = testValue;
+  } catch(_) {
+    return true;
+  }
+
+  try {
+    // We don't expect accessing this to throw, but just in case...
+    return testObject.name != testValue;
+  } catch(_) {
+    return true;
+  }
+})();
+
+
+@JS()
+@anonymous
+class _PropertyDescriptor {}
+
+@JS('Object.getPrototypeOf')
+external dynamic _getPrototypeOf(dynamic object);
+
+@JS('Object.getOwnPropertyDescriptor')
+external _PropertyDescriptor _getOwnPropertyDescriptor(dynamic object, String propertyName);
+
+@JS('Object.defineProperty')
+external void _defineProperty(dynamic object, String propertyName, _PropertyDescriptor descriptor);
+
+/// Patches the `name` property on the given [object] to have the expected behavior
+/// by copying the property descriptor for `name` from the appropriate prototype.
+///
+/// This is a noop if `name` is not a property on the given object.
+///
+/// __This functionality is unstable, and should not be used when [isBugPresent] is `false`.__
+///
+/// This method also had undefined behavior on non-[UiProps] instances.
+void patchName(dynamic object) {
+  var current = object;
+  while ((current = _getPrototypeOf(current)) != null) {
+    var nameDescriptor = _getOwnPropertyDescriptor(current, 'name');
+
+    if (nameDescriptor != null) {
+      _defineProperty(object, 'name', nameDescriptor);
+      return;
+    }
+  }
+}

lib/src/util/react_wrappers.dart

@@ -101,7 +101,32 @@ Map getJsProps(/* ReactElement|ReactComponent */ instance) {
   return props;
 }
 
-Expando<UnmodifiableMapView> _elementPropsCache = new Expando('_elementPropsCache');
+/// Whether [Expando]s can be used on [ReactElement]s.
+///
+/// At the time this was written, this should return:
+///
+/// - `true` for dart2js and Dart VM
+/// - `false` for DDC
+final bool _canUseExpandoOnReactElement = (() {
+  var expando = new Expando<bool>('_canUseExpandoOnReactElement test');
+  var reactElement = react.div({});
+
+  try {
+    expando[reactElement] = true;
+  } catch(_) {
+    return false;
+  }
+
+  return true;
+})();
+
+/// A cache of props for a given [ReactElement].
+///
+/// If caching isn't possible due to [_canUseExpandoOnReactElement] being false,
+/// then this will be initialized to `null`, and caching will be disabled.
+final Expando<UnmodifiableMapView> _elementPropsCache = _canUseExpandoOnReactElement
+    ? new Expando<UnmodifiableMapView>('_elementPropsCache')
+    : null;
 
 /// Returns an unmodifiable Map view of props for a [ReactElement] or composite [ReactComponent] [instance].
 ///
@@ -140,15 +165,17 @@ Map getProps(/* ReactElement|ReactComponent */ instance, {bool traverseWrappers:
       }
     }
 
-    if (!isCompositeComponent) {
+    if (_elementPropsCache != null && !isCompositeComponent) {
       var cachedView = _elementPropsCache[instance];
       if (cachedView != null) return cachedView;
     }
 
     var propsMap = isDartComponent(instance) ? _getExtendedProps(instance) : getJsProps(instance);
     var view = new UnmodifiableMapView(propsMap);
 
-    if (!isCompositeComponent) _elementPropsCache[instance] = view;
+    if (_elementPropsCache != null && !isCompositeComponent) {
+      _elementPropsCache[instance] = view;
+    }
 
     return view;
   }