fix #27258, don't allow dynamic set of a final field

R=vsm@google.com

Review-Url: https://codereview.chromium.org/2847893002 .

pkg/dev_compiler/lib/src/compiler/code_generator.dart

@@ -1303,11 +1303,10 @@ class CodeGenerator extends Object
     // TODO(jacobr): make field readonly when that is supported.
     var tInstanceFields = <JS.Property>[
       new JS.Property(
-          _emitMemberName('index'), _emitAnnotatedType(intClass.type, null))
-    ];
-    var sigFields = <JS.Property>[
-      _buildSignatureField('fields', tInstanceFields)
+          _emitMemberName('index'), _emitFieldSignature(types.intType))
     ];
+    var sigFields = <JS.Property>[];
+    _buildSignatureField(sigFields, 'fields', tInstanceFields);
     var sig = new JS.ObjectInitializer(sigFields);
 
     var result = [
@@ -1846,11 +1845,13 @@ class CodeGenerator extends Object
     emitExtensions(className, _classProperties.extensionMembers);
   }
 
-  JS.Property _buildSignatureField(String name, List<JS.Property> elements) {
+  void _buildSignatureField(
+      List<JS.Property> sigFields, String name, List<JS.Property> elements) {
+    if (elements.isEmpty) return;
     var o = new JS.ObjectInitializer(elements, multiline: elements.length > 1);
     // TODO(vsm): Remove
     var e = js.call('() => #', o);
-    return new JS.Property(_propertyName(name), e);
+    sigFields.add(new JS.Property(_propertyName(name), e));
   }
 
   /// Emit the signature on the class recording the runtime type information
@@ -1884,15 +1885,23 @@ class CodeGenerator extends Object
       if (node.isAbstract) {
         continue;
       }
-      if (node.isStatic &&
-          !options.emitMetadata &&
+      // Static getters/setters cannot be called with dynamic dispatch, nor
+      // can they be torn off.
+      // TODO(jmesserly): can we attach static method type info at the tearoff
+      // point, and avoid saving the information otherwise? Same trick would
+      // work for top-level functions.
+      if (!options.emitMetadata &&
+          node.isStatic &&
           (node.isGetter || node.isSetter)) {
         continue;
       }
       List<JS.Property> tMember;
+      // TODO(jmesserly): these 3 variables should be typed.
       Function getOverride;
       Function lookup;
       Function elementToType;
+      // TODO(jmesserly): we could reduce work by not saving a full function
+      // type for getters/setters. These only need 1 type to be saved.
       if (node.isGetter) {
         elementToType = (ExecutableElement element) => element.type;
         getOverride = classElem.lookUpInheritedConcreteGetter;
@@ -1935,7 +1944,7 @@ class CodeGenerator extends Object
         var memberName = _declareMemberName(element);
         var property = new JS.Property(memberName, type);
         tMember.add(property);
-        // We record the names of static methods seperately so we can
+        // We record the names of static methods separately so we can
         // attach metadata to them individually.
         // TODO(leafp): Revisit this.
         if (node.isStatic && !node.isGetter && !node.isSetter) {
@@ -1947,13 +1956,17 @@ class CodeGenerator extends Object
     var tInstanceFields = <JS.Property>[];
     var tStaticFields = <JS.Property>[];
     for (FieldDeclaration node in fields) {
-      if (!node.isStatic || options.emitMetadata) {
+      // Only instance fields need to be saved for dynamic dispatch.
+      var isStatic = node.isStatic;
+      if (options.emitMetadata || !isStatic) {
         for (VariableDeclaration field in node.fields.variables) {
           var element = field.element as FieldElement;
+          var fieldList = isStatic ? tStaticFields : tInstanceFields;
+
           var memberName = _declareMemberName(element.getter);
-          var type = _emitAnnotatedType(element.type, node.metadata);
-          var property = new JS.Property(memberName, type);
-          (node.isStatic ? tStaticFields : tInstanceFields).add(property);
+          var fieldSig = _emitFieldSignature(element.type,
+              metadata: node.metadata, isFinal: element.isFinal);
+          fieldList.add(new JS.Property(memberName, fieldSig));
         }
       }
     }
@@ -1974,38 +1987,21 @@ class CodeGenerator extends Object
       }
     }
     var sigFields = <JS.Property>[];
-    if (!tCtors.isEmpty) {
-      sigFields.add(_buildSignatureField('constructors', tCtors));
-    }
-    if (!tInstanceFields.isEmpty) {
-      sigFields.add(_buildSignatureField('fields', tInstanceFields));
-    }
-    if (!tInstanceGetters.isEmpty) {
-      sigFields.add(_buildSignatureField('getters', tInstanceGetters));
-    }
-    if (!tInstanceSetters.isEmpty) {
-      sigFields.add(_buildSignatureField('setters', tInstanceSetters));
-    }
-    if (!tInstanceMethods.isEmpty) {
-      sigFields.add(_buildSignatureField('methods', tInstanceMethods));
-    }
-    if (!tStaticFields.isEmpty) {
-      sigFields.add(_buildSignatureField('sfields', tStaticFields));
-    }
-    if (!tStaticGetters.isEmpty) {
-      sigFields.add(_buildSignatureField('sgetters', tStaticGetters));
-    }
-    if (!tStaticSetters.isEmpty) {
-      sigFields.add(_buildSignatureField('ssetters', tStaticSetters));
-    }
+    _buildSignatureField(sigFields, 'constructors', tCtors);
+    _buildSignatureField(sigFields, 'fields', tInstanceFields);
+    _buildSignatureField(sigFields, 'getters', tInstanceGetters);
+    _buildSignatureField(sigFields, 'setters', tInstanceSetters);
+    _buildSignatureField(sigFields, 'methods', tInstanceMethods);
+    _buildSignatureField(sigFields, 'sfields', tStaticFields);
+    _buildSignatureField(sigFields, 'sgetters', tStaticGetters);
+    _buildSignatureField(sigFields, 'ssetters', tStaticSetters);
+    _buildSignatureField(sigFields, 'statics', tStaticMethods);
     if (!tStaticMethods.isEmpty) {
       assert(!sNames.isEmpty);
       // Emit names so that we can lazily attach metadata to statics
       // TODO(leafp): revisit this strategy
-      var aNames = new JS.Property(
-          _propertyName('names'), new JS.ArrayInitializer(sNames));
-      sigFields.add(_buildSignatureField('statics', tStaticMethods));
-      sigFields.add(aNames);
+      sigFields.add(new JS.Property(
+          _propertyName('names'), new JS.ArrayInitializer(sNames)));
     }
     // We set signature here, even if empty, to simplify the work of
     // defineExtensionMembers at runtime. See _defineExtensionMembers.
@@ -2955,6 +2951,16 @@ class CodeGenerator extends Object
     return _emitAnnotatedResult(typeName, metadata);
   }
 
+  JS.Expression _emitFieldSignature(DartType type,
+      {List<Annotation> metadata, bool isFinal: true}) {
+    var args = [_emitType(type)];
+    if (options.emitMetadata && metadata != null && metadata.isNotEmpty) {
+      args.add(new JS.ArrayInitializer(
+          metadata.map(_instantiateAnnotation).toList()));
+    }
+    return _callHelper(isFinal ? 'finalFieldType(#)' : 'fieldType(#)', args);
+  }
+
   JS.ArrayInitializer _emitTypeNames(
       List<DartType> types, List<FormalParameter> parameters,
       {bool nameType: true, bool hoistType: true}) {

pkg/dev_compiler/test/codegen_expected/BenchmarkBase.js

@@ -83,7 +83,7 @@ define(['dart_sdk'], function(dart_sdk) {
   };
   const name$ = Symbol("BenchmarkBase.name");
   dart.setSignature(BenchmarkBase$.BenchmarkBase, {
-    fields: () => ({name: core.String}),
+    fields: () => ({name: dart.finalFieldType(core.String)}),
     methods: () => ({
       run: dart.definiteFunctionType(dart.void, []),
       warmup: dart.definiteFunctionType(dart.void, []),

pkg/dev_compiler/test/codegen_expected/closure.js

@@ -93,10 +93,10 @@ closure.Foo$ = dart.generic(T => {
   const v$ = Symbol("Foo.v");
   dart.setSignature(Foo, {
     fields: () => ({
-      i: core.int,
-      b: core.bool,
-      s: core.String,
-      v: T
+      i: dart.finalFieldType(core.int),
+      b: dart.fieldType(core.bool),
+      s: dart.fieldType(core.String),
+      v: dart.fieldType(T)
     }),
     getters: () => ({prop: dart.definiteFunctionType(core.String, [])}),
     setters: () => ({prop: dart.definiteFunctionType(dart.void, [core.String])}),

pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/classes.dart

@@ -197,11 +197,8 @@ getStaticSetterSig(value) => JS('', '#[#]', value, _staticSetterSig);
 getGenericTypeCtor(value) => JS('', '#[#]', value, _genericTypeCtor);
 
 /// Get the type of a method from an object using the stored signature
-getType(obj) => JS(
-    '',
-    '''(() => {
-  return $obj == null ? $Object : $obj.__proto__.constructor;
-})()''');
+getType(obj) =>
+    JS('', '# == null ? # : #.__proto__.constructor', obj, Object, obj);
 
 bool isJsInterop(obj) {
   if (JS('bool', 'typeof # === "function"', obj)) {
@@ -219,31 +216,37 @@ bool isJsInterop(obj) {
 }
 
 /// Get the type of a method from a type using the stored signature
-getMethodType(type, name) => JS(
-    '',
-    '''(() => {
-  let sigObj = $type[$_methodSig];
-  if (sigObj === void 0) return void 0;
-  return sigObj[$name];
-})()''');
+getMethodType(type, name) {
+  var m = JS('', '#[#]', type, _methodSig);
+  return m != null ? JS('', '#[#]', m, name) : null;
+}
 
-getFieldType(type, name) => JS(
-    '',
-    '''(() => {
-  let sigObj = $type[$_fieldSig];
-  if (sigObj === void 0) return void 0;
-  let fieldType = sigObj[$name];
-  // workaround to handle metadata.
-  return (fieldType instanceof Array) ? fieldType[0] : fieldType;
-})()''');
+/// Gets the type of the corresponding setter (this includes writable fields).
+getSetterType(type, name) {
+  var signature = JS('', '#[#]', type, _setterSig);
+  if (signature != null) {
+    var type = JS('', '#[#]', signature, name);
+    if (type != null) {
+      // TODO(jmesserly): it would be nice not to encode setters with a full
+      // function type.
+      return JS('', '#.args[0]', type);
+    }
+  }
+  signature = JS('', '#[#]', type, _fieldSig);
+  if (signature != null) {
+    var fieldInfo = JS('', '#[#]', signature, name);
+    if (fieldInfo != null && JS('bool', '!#.isFinal', fieldInfo)) {
+      return JS('', '#.type', fieldInfo);
+    }
+  }
+  return null;
+}
 
-getSetterType(type, name) => JS(
-    '',
-    '''(() => {
-  let sigObj = $type[$_setterSig];
-  if (sigObj === void 0) return void 0;
-  return sigObj[$name];
-})()''');
+finalFieldType(type, metadata) =>
+    JS('', '{ type: #, isFinal: true, metadata: # }', type, metadata);
+
+fieldType(type, metadata) =>
+    JS('', '{ type: #, isFinal: false, metadata: # }', type, metadata);
 
 /// Get the type of a constructor from a class using the stored signature
 /// If name is undefined, returns the type of the default constructor
@@ -278,7 +281,7 @@ bind(obj, name, f) => JS(
   // JS interop case: do not bind this for compatibility with the dart2js
   // implementation where we cannot bind this reliably here until we trust
   // types more.
-  if (sig === void 0) return $f;
+  if (sig == null) return $f;
 
   $f = $f.bind($obj);
   $tag($f, sig);
@@ -297,17 +300,22 @@ gbind(f, @rest typeArgs) {
 }
 
 // Set up the method signature field on the constructor
-_setInstanceSignature(f, sigF, kind) => JS(
-    '',
-    '''(() => {
-  $defineMemoizedGetter($f, $kind, () => {
-    let sigObj = $sigF();
-    let proto = $f.__proto__;
-    // We need to set the root proto to null not undefined.
-    sigObj.__proto__ = ($kind in proto) ? proto[$kind] : null;
-    return sigObj;
-  });
-})()''');
+_setInstanceSignature(f, sigF, kind) => defineMemoizedGetter(
+    f,
+    kind,
+    JS(
+        '',
+        '''() => {
+          let sigObj = #();
+          let proto = #.__proto__;
+          // We need to set the root proto to null not undefined.
+          sigObj.__proto__ = (# in proto) ? proto[#] : null;
+          return sigObj;
+        }''',
+        sigF,
+        f,
+        kind,
+        kind));
 
 _setMethodSignature(f, sigF) => _setInstanceSignature(f, sigF, _methodSig);
 _setFieldSignature(f, sigF) => _setInstanceSignature(f, sigF, _fieldSig);

pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart

@@ -80,26 +80,10 @@ dputMirror(obj, field, value) {
   var f = _canonicalMember(obj, field);
   _trackCall(obj);
   if (f != null) {
-    var objType = getType(obj);
-    var setterType = getSetterType(objType, f);
-    if (JS('bool', '# != void 0', setterType)) {
-      return JS(
-          '',
-          '#[#] = #',
-          obj,
-          f,
-          check(
-              value, _stripGenericArguments(JS('', '#.args[0]', setterType))));
-    } else {
-      var fieldType = getFieldType(objType, f);
-      // TODO(jacobr): add metadata tracking which fields are final and throw
-      // if a setter is called on a final field.
-      if (JS('bool', '# != void 0', fieldType)) {
-        return JS('', '#[#] = #', obj, f,
-            check(value, _stripGenericArguments(fieldType)));
-      }
-
-      // Do not support calls on JS interop objects to match Dart2JS behavior.
+    var setterType = getSetterType(getType(obj), f);
+    if (setterType != null) {
+      setterType = _stripGenericArguments(setterType);
+      return JS('', '#[#] = #', obj, f, check(value, setterType));
     }
   }
   return noSuchMethod(
@@ -110,22 +94,13 @@ dput(obj, field, value) {
   var f = _canonicalMember(obj, field);
   _trackCall(obj);
   if (f != null) {
-    var objType = getType(obj);
-    var setterType = getSetterType(objType, f);
-    if (JS('bool', '# != void 0', setterType)) {
-      return JS('', '#[#] = #', obj, f,
-          check(value, JS('', '#.args[0]', setterType)));
-    } else {
-      var fieldType = getFieldType(objType, f);
-      // TODO(jacobr): add metadata tracking which fields are final and throw
-      // if a setter is called on a final field.
-      if (JS('bool', '# != void 0', fieldType)) {
-        return JS('', '#[#] = #', obj, f, check(value, fieldType));
-      }
-      // Always allow for JS interop objects.
-      if (isJsInterop(obj)) {
-        return JS('', '#[#] = #', obj, f, value);
-      }
+    var setterType = getSetterType(getType(obj), f);
+    if (setterType != null) {
+      return JS('', '#[#] = #', obj, f, check(value, setterType));
+    }
+    // Always allow for JS interop objects.
+    if (isJsInterop(obj)) {
+      return JS('', '#[#] = #', obj, f, value);
     }
   }
   return noSuchMethod(
@@ -252,11 +227,11 @@ _checkAndCall(f, ftype, obj, typeArgs, args, name) => JS(
   // If f is a function, but not a method (no method type)
   // then it should have been a function valued field, so
   // get the type from the function.
-  if ($ftype === void 0) {
+  if ($ftype == null) {
     $ftype = $_getRuntimeType($f);
   }
 
-  if (!$ftype) {
+  if ($ftype == null) {
     // TODO(leafp): Allow JS objects to go through?
     if ($typeArgs != null) {
       // TODO(jmesserly): is there a sensible way to handle these?