Revert "[cfe] Encode field references as @getterS and @setters"

This reverts commit 3892e95.

Reason for revert: Breaks Flutter web

Original change's description:
> [cfe] Encode field references as @getterS and @setters
>
> This removes the @fields and @=fields canonical name
> encodings that allowed for encoding of conflicting members
> and didn't support field<->getter/setter conversion
> between dills or between outline and full dill.
>
> TEST=existing tests
>
> Change-Id: Id15e58ad4d1847d2c98a688705e5945196146c6d
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/184783
> Commit-Queue: Johnni Winther <[email protected]>
> Reviewed-by: Jens Johansen <[email protected]>
> Reviewed-by: Alexander Markov <[email protected]>

Change-Id: I744e284b16e097fa0833c5bdf1bc7653f13bdf63
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/186147
Reviewed-by: Jens Johansen <[email protected]>
Reviewed-by: Johnni Winther <[email protected]>
Commit-Queue: Johnni Winther <[email protected]>

Author: johnniwinther

pkg/front_end/testcases/incremental/changing_modules_2.yaml.world.1.expect

@@ -6,8 +6,8 @@ library from "package:example/a.dart" as a {
 }
 library from "package:foo/foo.dart" as foo {
 additionalExports = (a::example,
-  a::a,
-  a::example)
+  a::example,
+  a::a)
 
   export "package:example/a.dart";
 

pkg/front_end/testcases/incremental/changing_modules_2.yaml.world.2.expect

@@ -6,8 +6,8 @@ library from "package:example/a.dart" as a {
 }
 library from "package:foo/foo.dart" as foo {
 additionalExports = (a::example,
-  a::a,
-  a::example)
+  a::example,
+  a::a)
 
   export "package:example/a.dart";
 

pkg/front_end/testcases/incremental/no_outline_change_43.yaml.world.1.expect

@@ -10,8 +10,8 @@ library from "org-dartlang-test:///lib.dart" as lib {
 }
 library from "org-dartlang-test:///libExporter.dart" as lib2 {
 additionalExports = (lib::libField,
-  lib::requireStuffFromLibExporter,
-  lib::libField)
+  lib::libField,
+  lib::requireStuffFromLibExporter)
 
   export "org-dartlang-test:///lib.dart";
 

pkg/front_end/testcases/incremental/no_outline_change_43.yaml.world.2.expect

@@ -10,8 +10,8 @@ library from "org-dartlang-test:///lib.dart" as lib {
 }
 library from "org-dartlang-test:///libExporter.dart" as lib2 {
 additionalExports = (lib::libField,
-  lib::requireStuffFromLibExporter,
-  lib::libField)
+  lib::libField,
+  lib::requireStuffFromLibExporter)
 
   export "org-dartlang-test:///lib.dart";
 

pkg/front_end/testcases/incremental/no_outline_change_43.yaml.world.3.expect

@@ -10,8 +10,8 @@ library from "org-dartlang-test:///lib.dart" as lib {
 }
 library from "org-dartlang-test:///libExporter.dart" as lib2 {
 additionalExports = (lib::libField,
-  lib::requireStuffFromLibExporter,
-  lib::libField)
+  lib::libField,
+  lib::requireStuffFromLibExporter)
 
   export "org-dartlang-test:///lib.dart";
 

pkg/front_end/testcases/incremental/no_outline_change_43.yaml.world.4.expect

@@ -10,8 +10,8 @@ library from "org-dartlang-test:///lib.dart" as lib {
 }
 library from "org-dartlang-test:///libExporter.dart" as lib2 {
 additionalExports = (lib::libField,
-  lib::requireStuffFromLibExporter,
-  lib::libField)
+  lib::libField,
+  lib::requireStuffFromLibExporter)
 
   export "org-dartlang-test:///lib.dart";
 

pkg/kernel/binary.md

@@ -147,7 +147,7 @@ type CanonicalName {
 
 type ComponentFile {
   UInt32 magic = 0x90ABCDEF;
-  UInt32 formatVersion = 56;
+  UInt32 formatVersion = 55;
   Byte[10] shortSdkHash;
   List<String> problemsAsJson; // Described in problems.md.
   Library[] libraries;

pkg/kernel/lib/binary/ast_from_binary.dart

@@ -678,6 +678,8 @@ class BinaryBuilder {
       for (CanonicalName child in parentChildren) {
         if (child.name != '@methods' &&
             child.name != '@typedefs' &&
+            child.name != '@fields' &&
+            child.name != '@=fields' &&
             child.name != '@getters' &&
             child.name != '@setters' &&
             child.name != '@factories' &&

pkg/kernel/lib/binary/tag.dart

@@ -173,7 +173,7 @@ class Tag {
   /// Internal version of kernel binary format.
   /// Bump it when making incompatible changes in kernel binaries.
   /// Keep in sync with runtime/vm/kernel_binary.h, pkg/kernel/binary.md.
-  static const int BinaryFormatVersion = 56;
+  static const int BinaryFormatVersion = 55;
 }
 
 abstract class ConstantTag {

pkg/kernel/lib/canonical_name.dart

@@ -30,14 +30,9 @@ import 'ast.dart';
 ///         "@constructors"
 ///         Qualified name
 ///
-///      Field or the implicit getter of a field:
+///      Field:
 ///         Canonical name of enclosing class or library
-///         "@getters"
-///         Qualified name
-///
-///      Implicit setter of a field:
-///         Canonical name of enclosing class or library
-///         "@setters"
+///         "@fields"
 ///         Qualified name
 ///
 ///      Typedef:
@@ -137,11 +132,11 @@ class CanonicalName {
   }
 
   CanonicalName getChildFromField(Field field) {
-    return getChild('@getters').getChildFromQualifiedName(field.name!);
+    return getChild('@fields').getChildFromQualifiedName(field.name!);
   }
 
   CanonicalName getChildFromFieldSetter(Field field) {
-    return getChild('@setters').getChildFromQualifiedName(field.name!);
+    return getChild('@=fields').getChildFromQualifiedName(field.name!);
   }
 
   CanonicalName getChildFromConstructor(Constructor constructor) {
@@ -156,11 +151,11 @@ class CanonicalName {
   }
 
   CanonicalName getChildFromFieldWithName(Name name) {
-    return getChild('@getters').getChildFromQualifiedName(name);
+    return getChild('@fields').getChildFromQualifiedName(name);
   }
 
   CanonicalName getChildFromFieldSetterWithName(Name name) {
-    return getChild('@setters').getChildFromQualifiedName(name);
+    return getChild('@=fields').getChildFromQualifiedName(name);
   }
 
   CanonicalName getChildFromTypedef(Typedef typedef_) {

pkg/kernel/test/convert_field_to_setter_getter.dart

@@ -37,14 +37,12 @@ main() {
   List<int> writtenBytesFieldOriginal = serialize(lib1, lib2);
   // Canonical names are now set: Verify that the field is marked as such,
   // canonical-name-wise.
-  String getterCanonicalName = '${field.getterCanonicalName}';
-  if (field.getterCanonicalName.parent.name != "@getters") {
-    throw "Expected @getters parent, but had "
+  if (field.getterCanonicalName.parent.name != "@fields") {
+    throw "Expected @fields parent, but had "
         "${field.getterCanonicalName.parent.name}";
   }
-  String setterCanonicalName = '${field.setterCanonicalName}';
-  if (field.setterCanonicalName.parent.name != "@setters") {
-    throw "Expected @setters parent, but had "
+  if (field.setterCanonicalName.parent.name != "@=fields") {
+    throw "Expected @=fields parent, but had "
         "${field.setterCanonicalName.parent.name}";
   }
 
@@ -82,18 +80,10 @@ main() {
     throw "Expected @getters parent, but had "
         "${getter.canonicalName.parent.name}";
   }
-  if ('${getter.canonicalName}' != getterCanonicalName) {
-    throw "Unexpected getter canonical name. "
-        "Expected $getterCanonicalName, actual ${getter.canonicalName}.";
-  }
   if (setter.canonicalName.parent.name != "@setters") {
     throw "Expected @setters parent, but had "
         "${setter.canonicalName.parent.name}";
   }
-  if ('${setter.canonicalName}' != setterCanonicalName) {
-    throw "Unexpected setter canonical name. "
-        "Expected $setterCanonicalName, actual ${setter.canonicalName}.";
-  }
 
   // Replace getter/setter with field.
   lib1.procedures.remove(getter);
@@ -111,12 +101,12 @@ main() {
   List<int> writtenBytesFieldNew = serialize(lib1, lib2);
   // Canonical names are now set: Verify that the field is marked as such,
   // canonical-name-wise.
-  if (fieldReplacement.getterCanonicalName.parent.name != "@getters") {
-    throw "Expected @getters parent, but had "
+  if (fieldReplacement.getterCanonicalName.parent.name != "@fields") {
+    throw "Expected @fields parent, but had "
         "${fieldReplacement.getterCanonicalName.parent.name}";
   }
-  if (fieldReplacement.setterCanonicalName.parent.name != "@setters") {
-    throw "Expected @setters parent, but had "
+  if (fieldReplacement.setterCanonicalName.parent.name != "@=fields") {
+    throw "Expected @=fields parent, but had "
         "${fieldReplacement.setterCanonicalName.parent.name}";
   }
 

pkg/kernel/test/text_serializer_from_kernel_nodes_test.dart

@@ -133,7 +133,7 @@ void test() {
           name: '/* suppose top-level: dynamic field; */ field;',
           node: new ExpressionStatement(new StaticGet(field)),
           expectation: ''
-              '(expr (get-static "package:foo/bar.dart::@getters::field"))',
+              '(expr (get-static "package:foo/bar.dart::@fields::field"))',
           makeSerializationState: () => new SerializationState(null),
           makeDeserializationState: () =>
               new DeserializationState(null, component.root),
@@ -151,7 +151,7 @@ void test() {
           name: '/* suppose top-level: dynamic field; */ field;',
           node: new ExpressionStatement(new StaticGet(field)),
           expectation: ''
-              '(expr (get-static "package:foo/bar.dart::@getters::field"))',
+              '(expr (get-static "package:foo/bar.dart::@fields::field"))',
           makeSerializationState: () => new SerializationState(null),
           makeDeserializationState: () =>
               new DeserializationState(null, component.root),
@@ -171,7 +171,7 @@ void test() {
               new ExpressionStatement(new StaticSet(field, new IntLiteral(1))),
           expectation: ''
               '(expr'
-              ' (set-static "package:foo/bar.dart::@setters::field" (int 1)))',
+              ' (set-static "package:foo/bar.dart::@=fields::field" (int 1)))',
           makeSerializationState: () => new SerializationState(null),
           makeDeserializationState: () =>
               new DeserializationState(null, component.root),

pkg/vm/lib/transformations/ffi_definitions.dart

@@ -510,29 +510,17 @@ class _FfiDefinitionTransformer extends FfiTransformer {
 
   List<Procedure> _generateMethodsForField(Field field, NativeTypeCfe type,
       Map<Abi, int> offsets, IndexedClass indexedClass) {
-    // TODO(johnniwinther): Avoid passing [indexedClass]. When compiling
-    // incrementally, [field] should already carry the references from
-    // [indexedClass].
     final getterStatement = type.generateGetterStatement(
         field.type, field.fileOffset, offsets, this);
-    Reference getterReference =
-        indexedClass?.lookupGetterReference(field.name) ??
-            field.getterReference;
-    assert(getterReference == field.getterReference,
-        "Unexpected getter reference for ${field}, found $getterReference.");
     final Procedure getter = Procedure(field.name, ProcedureKind.Getter,
         FunctionNode(getterStatement, returnType: field.type),
-        fileUri: field.fileUri, reference: getterReference)
+        fileUri: field.fileUri,
+        reference: indexedClass?.lookupGetterReference(field.name))
       ..fileOffset = field.fileOffset
       ..isNonNullableByDefault = field.isNonNullableByDefault;
 
     Procedure setter = null;
     if (!field.isFinal) {
-      Reference setterReference =
-          indexedClass?.lookupSetterReference(field.name) ??
-              field.setterReference;
-      assert(setterReference == field.setterReference,
-          "Unexpected setter reference for ${field}, found $setterReference.");
       final VariableDeclaration argument =
           VariableDeclaration('#v', type: field.type)
             ..fileOffset = field.fileOffset;
@@ -544,7 +532,7 @@ class _FfiDefinitionTransformer extends FfiTransformer {
           FunctionNode(setterStatement,
               returnType: VoidType(), positionalParameters: [argument]),
           fileUri: field.fileUri,
-          reference: setterReference)
+          reference: indexedClass?.lookupSetterReference(field.name))
         ..fileOffset = field.fileOffset
         ..isNonNullableByDefault = field.isNonNullableByDefault;
     }

pkg/vm/lib/transformations/type_flow/transformer.dart

@@ -1324,12 +1324,7 @@ class _TreeShakerPass2 extends RemovingTransformer {
     if (!shaker.isMemberUsed(node) && !_preserveSpecialMember(node)) {
       // Ensure that kernel file writer will not be able to
       // write a dangling reference to the deleted member.
-      if (node is Field) {
-        node.getterCanonicalName?.reference = null;
-        node.setterCanonicalName?.reference = null;
-      } else {
-        node.canonicalName?.reference = null;
-      }
+      node.reference.canonicalName = null;
       Statistics.membersDropped++;
       return removalSentinel;
     }

runtime/vm/compiler/frontend/constant_reader.cc

@@ -264,8 +264,7 @@ InstancePtr ConstantReader::ReadConstantInternal(intptr_t constant_offset) {
       Field& field = Field::Handle(Z);
       Instance& constant = Instance::Handle(Z);
       for (intptr_t j = 0; j < number_of_fields; ++j) {
-        field = H.LookupFieldByKernelGetterOrSetter(
-            reader.ReadCanonicalNameReference());
+        field = H.LookupFieldByKernelField(reader.ReadCanonicalNameReference());
         // Recurse into lazily evaluating all "sub" constants
         // needed to evaluate the current constant.
         const intptr_t entry_offset = reader.ReadUInt();

runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc

@@ -237,7 +237,7 @@ Fragment StreamingFlowGraphBuilder::BuildInitializers(
         ReadBool();
         const NameIndex field_name = ReadCanonicalNameReference();
         const Field& field =
-            Field::Handle(Z, H.LookupFieldByKernelGetterOrSetter(field_name));
+            Field::Handle(Z, H.LookupFieldByKernelField(field_name));
         initializer_fields[i] = &field;
         SkipExpression();
         continue;
@@ -2180,7 +2180,8 @@ Fragment StreamingFlowGraphBuilder::BuildPropertyGet(TokenPosition* p) {
   const Function* tearoff_interface_target = &Function::null_function();
   const NameIndex itarget_name =
       ReadInterfaceMemberNameReference();  // read interface_target_reference.
-  if (!H.IsRoot(itarget_name) && H.IsGetter(itarget_name)) {
+  if (!H.IsRoot(itarget_name) &&
+      (H.IsGetter(itarget_name) || H.IsField(itarget_name))) {
     interface_target = &Function::ZoneHandle(
         Z,
         H.LookupMethodByMember(itarget_name, H.DartGetterName(itarget_name)));
@@ -2548,11 +2549,10 @@ Fragment StreamingFlowGraphBuilder::BuildStaticGet(TokenPosition* p) {
       inferred_type_metadata_helper_.GetInferredType(offset);
 
   NameIndex target = ReadCanonicalNameReference();  // read target_reference.
-  ASSERT(H.IsGetter(target));
 
-  const Field& field = Field::ZoneHandle(
-      Z, H.LookupFieldByKernelGetterOrSetter(target, /*required=*/false));
-  if (!field.IsNull()) {
+  if (H.IsField(target)) {
+    const Field& field =
+        Field::ZoneHandle(Z, H.LookupFieldByKernelField(target));
     if (field.is_const()) {
       // Since the CFE inlines all references to const variables and fields,
       // it never emits a StaticGet of a const field.
@@ -2605,39 +2605,44 @@ Fragment StreamingFlowGraphBuilder::BuildStaticSet(TokenPosition* p) {
   if (p != NULL) *p = position;
 
   NameIndex target = ReadCanonicalNameReference();  // read target_reference.
-  ASSERT(H.IsSetter(target));
-
-  // Evaluate the expression on the right hand side.
-  Fragment instructions = BuildExpression();  // read expression.
 
-  // Look up the target as a setter first and, if not present, as a field
-  // second. This order is needed to avoid looking up a final field as the
-  // target.
-  const Function& function = Function::ZoneHandle(
-      Z, H.LookupStaticMethodByKernelProcedure(target, /*required=*/false));
+  if (H.IsField(target)) {
+    const Field& field =
+        Field::ZoneHandle(Z, H.LookupFieldByKernelField(target));
+    const Class& owner = Class::Handle(Z, field.Owner());
+    const String& setter_name = H.DartSetterName(target);
+    const Function& setter =
+        Function::ZoneHandle(Z, owner.LookupStaticFunction(setter_name));
+    Fragment instructions = BuildExpression();  // read expression.
+    if (NeedsDebugStepCheck(stack(), position)) {
+      instructions = DebugStepCheck(position) + instructions;
+    }
+    LocalVariable* variable = MakeTemporary();
+    instructions += LoadLocal(variable);
+    if (!setter.IsNull() && field.NeedsSetter()) {
+      instructions += StaticCall(position, setter, 1, ICData::kStatic);
+      instructions += Drop();
+    } else {
+      instructions += StoreStaticField(position, field);
+    }
+    return instructions;
+  } else {
+    ASSERT(H.IsProcedure(target));
 
-  if (!function.IsNull()) {
+    // Evaluate the expression on the right hand side.
+    Fragment instructions = BuildExpression();  // read expression.
     LocalVariable* variable = MakeTemporary();
 
     // Prepare argument.
     instructions += LoadLocal(variable);
 
     // Invoke the setter function.
+    const Function& function =
+        Function::ZoneHandle(Z, H.LookupStaticMethodByKernelProcedure(target));
     instructions += StaticCall(position, function, 1, ICData::kStatic);
 
     // Drop the unused result & leave the stored value on the stack.
     return instructions + Drop();
-  } else {
-    const Field& field =
-        Field::ZoneHandle(Z, H.LookupFieldByKernelGetterOrSetter(target));
-    ASSERT(!field.NeedsSetter());
-    if (NeedsDebugStepCheck(stack(), position)) {
-      instructions = DebugStepCheck(position) + instructions;
-    }
-    LocalVariable* variable = MakeTemporary();
-    instructions += LoadLocal(variable);
-    instructions += StoreStaticField(position, field);
-    return instructions;
   }
 }
 
@@ -2765,7 +2770,8 @@ Fragment StreamingFlowGraphBuilder::BuildMethodInvocation(TokenPosition* p) {
   // TODO(dartbug.com/34497): Once front-end desugars calls via
   // fields/getters, filtering of field and getter interface targets here
   // can be turned into assertions.
-  if (!H.IsRoot(itarget_name) && !H.IsGetter(itarget_name)) {
+  if (!H.IsRoot(itarget_name) && !H.IsField(itarget_name) &&
+      !H.IsGetter(itarget_name)) {
     interface_target = &Function::ZoneHandle(
         Z, H.LookupMethodByMember(itarget_name,
                                   H.DartProcedureName(itarget_name)));