Reland "[dartdevc] fix for const / overridden fields"

This reverts commit 43cacaf.

Patchset 1 is the original CL.  Compare PS 1 to 4 to see additional fix.

It undoes an optimization that assumes private fields
are not overridden in the SDK.  This patterns happens in dart:ui
and would be difficult to enforce now that flutter web also adds to
the SDK.  As a result, all private SDK fields are virtualized, adding 0.7% to the size of
dart_sdk.js.

Fixes #38455

Change-Id: If969dddcb7143316ac8c771df1ed83def21412b2
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/118362
Commit-Queue: Vijay Menon <[email protected]>
Reviewed-by: Nate Bosch <[email protected]>

Author: vsmenon

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

@@ -252,6 +252,7 @@ abstract class SharedCompiler<Library, Class, InterfaceType, FunctionNode> {
     /// runtime call.
     js_ast.TemporaryId initPrivateNameSymbol() {
       var idName = name.endsWith('=') ? name.replaceAll('=', '_') : name;
+      idName = idName.replaceAll('.', '_');
       var id = js_ast.TemporaryId(idName);
       moduleItems.add(js.statement('const # = #.privateName(#, #)',
           [id, runtimeModule, emitLibraryName(library), js.string(name)]));
@@ -262,6 +263,17 @@ abstract class SharedCompiler<Library, Class, InterfaceType, FunctionNode> {
     return privateNames.putIfAbsent(name, initPrivateNameSymbol);
   }
 
+  /// Emits a private name JS Symbol for [memberName] unique to a Dart
+  /// class [className].
+  ///
+  /// This is now required for fields of constant objects that may be
+  /// overridden within the same library.
+  @protected
+  js_ast.TemporaryId emitClassPrivateNameSymbol(
+      Library library, String className, String memberName) {
+    return emitPrivateNameSymbol(library, '$className.$memberName');
+  }
+
   /// Emits an expression to set the property [nameExpr] on the class [className],
   /// with [value].
   ///

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

@@ -2796,10 +2796,9 @@ class ProgramCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
 
   void _emitVirtualFieldSymbols(Class c, List<js_ast.Statement> body) {
     _classProperties.virtualFields.forEach((field, virtualField) {
-      body.add(js.statement('const # = Symbol(#);', [
-        virtualField,
-        js.string('${getLocalClassName(c)}.${field.name.name}')
-      ]));
+      var symbol = emitClassPrivateNameSymbol(
+          c.enclosingLibrary, getLocalClassName(c), field.name.name);
+      body.add(js.statement('const # = #;', [virtualField, symbol]));
     });
   }
 
@@ -5413,8 +5412,15 @@ class ProgramCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
     entryToProperty(MapEntry<Reference, Constant> entry) {
       var constant = visitConstant(entry.value);
       var member = entry.key.asField;
-      return js_ast.Property(
-          _emitMemberName(member.name.name, member: member), constant);
+      var cls = member.enclosingClass;
+      // Enums cannot be overridden, so we can safely use the field name
+      // directly.  Otherwise, use a private symbol in case the field
+      // was overridden.
+      var symbol = cls.isEnum
+          ? _emitMemberName(member.name.name, member: member)
+          : emitClassPrivateNameSymbol(
+              cls.enclosingLibrary, getLocalClassName(cls), member.name.name);
+      return js_ast.Property(symbol, constant);
     }
 
     var type = visitInterfaceType(node.getType(_types) as InterfaceType);

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

@@ -147,11 +147,6 @@ class _LibraryVirtualFieldModel {
       // Enums are not extensible.
       return false;
     }
-    var libraryUri = class_.enclosingLibrary.importUri;
-    if (libraryUri.scheme == 'dart' && libraryUri.path.startsWith('_')) {
-      // There should be no extensible fields in private SDK libraries.
-      return false;
-    }
 
     if (!field.name.isPrivate) {
       // Public fields in public classes (or extensible private classes)
@@ -161,6 +156,13 @@ class _LibraryVirtualFieldModel {
       if (_extensiblePrivateClasses.contains(class_)) return true;
     }
 
+    if (class_.constructors.any((c) => c.isConst)) {
+      // Always virtualize fields of a (might be) non-enum (see above) const
+      // class.  The way these are lowered by the CFE, they need to be
+      // writable from different modules even if overridden.
+      return true;
+    }
+
     // Otherwise, the field is effectively private and we only need to make it
     // virtual if it's overridden.
     return _overriddenPrivateFields.contains(field);

tests/language_2/override_const_field_test.dart

@@ -0,0 +1,23 @@
+// Copyright (c) 2019, 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.
+// Dart test checking that static/instance field shadowing do not conflict.
+
+import 'package:expect/expect.dart';
+
+class A {
+  final field;
+  const A(this.field);
+}
+
+class B extends A {
+  final field;
+  const B(this.field, fieldA) : super(fieldA);
+  get fieldA => super.field;
+}
+
+main() {
+  const b = B(1, 2);
+  Expect.equals(1, b.field);
+  Expect.equals(2, b.fieldA);
+}