[VM] Add new SymbolConstant to package:kernel/ast.dart

The canonicalization of symbols needs to take library privacy into
account. Since the Symbol class itself only has a [_name] field but does
not reference in which library the symbol came from, the [_name] must be
mangled.

Mangling is done by backends and so we make a new [SymbolConstant] which
the backends can desugar by doing appropriate mangling and construction
of a [Symbol] instance.

Fixes https://github.com/dart-lang/sdk/issues/34396

Change-Id: I5ddb5331ce79a0b942807929d4b8f1050a9899e7
Reviewed-on: https://dart-review.googlesource.com/73883
Commit-Queue: Martin Kustermann <kustermann@google.com>
Reviewed-by: Alexander Markov <alexmarkov@google.com>
Reviewed-by: Vyacheslav Egorov <vegorov@google.com>

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

@@ -4975,6 +4975,8 @@ class ProgramCompiler extends Object
   @override
   defaultConstant(Constant node) => _emitInvalidNode(node);
   @override
+  visitSymbolConstant(node) => defaultConstant(node);
+  @override
   visitMapConstant(node) => defaultConstant(node);
   @override
   visitListConstant(node) => defaultConstant(node);

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

@@ -303,14 +303,6 @@ class _ConstantsBackend implements ConstantsBackend {
           nativeName, typeArguments, positionalArguments, namedArguments) =>
       throw StateError('unreachable'); // DDC does not use VM native syntax
 
-  @override
-  buildSymbolConstant(StringConstant value) {
-    return InstanceConstant(
-        coreTypes.internalSymbolClass.reference,
-        const <DartType>[],
-        <Reference, Constant>{symbolNameField.reference: value});
-  }
-
   @override
   lowerMapConstant(constant) => constant;
 

pkg/kernel/binary.md

@@ -131,7 +131,7 @@ type CanonicalName {
 
 type ComponentFile {
   UInt32 magic = 0x90ABCDEF;
-  UInt32 formatVersion = 11;
+  UInt32 formatVersion = 12;
   Library[] libraries;
   UriSource sourceMap;
   List<CanonicalName> canonicalNames;
@@ -865,39 +865,45 @@ type StringConstant extends Constant {
   StringReference value;
 }
 
-type MapConstant extends Constant {
+type SymbolConstant extends Constant {
   Byte tag = 5;
+  Option<LibraryReference> library;
+  StringReference name;
+}
+
+type MapConstant extends Constant {
+  Byte tag = 6;
   DartType keyType;
   DartType valueType;
   List<[ConstantReference, ConstantReference]> keyValueList;
 }
 
 type ListConstant extends Constant {
-  Byte tag = 6;
+  Byte tag = 7;
   DartType type;
   List<ConstantReference> values;
 }
 
 type InstanceConstant extends Constant {
-  Byte tag = 7;
+  Byte tag = 8;
   CanonicalNameReference class;
   List<DartType> typeArguments;
   List<[FieldReference, ConstantReference]> values;
 }
 
 type PartialInstantiationConstant extends Constant {
-  Byte tag = 8;
+  Byte tag = 9;
   ConstantReference tearOffConstant;
   List<DartType> typeArguments;
 }
 
 type TearOffConstant extends Constant {
-  Byte tag = 9;
+  Byte tag = 10;
   CanonicalNameReference staticProcedureReference;
 }
 
 type TypeLiteralConstant extends Constant {
-  Byte tag = 10;
+  Byte tag = 11;
   DartType type;
 }
 

pkg/kernel/lib/ast.dart

@@ -5073,6 +5073,34 @@ class StringConstant extends PrimitiveConstant<String> {
   DartType getType(TypeEnvironment types) => types.stringType;
 }
 
+class SymbolConstant extends Constant {
+  final String name;
+  final Reference libraryReference;
+
+  SymbolConstant(this.name, this.libraryReference);
+
+  visitChildren(Visitor v) {}
+
+  accept(ConstantVisitor v) => v.visitSymbolConstant(this);
+  acceptReference(Visitor v) => v.visitSymbolConstantReference(this);
+
+  String toString() {
+    return libraryReference != null
+        ? '#${libraryReference.asLibrary.importUri}::$name'
+        : '#$name';
+  }
+
+  int get hashCode => name.hashCode ^ libraryReference.hashCode;
+
+  bool operator ==(Object other) =>
+      identical(this, other) ||
+      (other is SymbolConstant &&
+          other.name == name &&
+          other.libraryReference == libraryReference);
+
+  DartType getType(TypeEnvironment types) => types.symbolType;
+}
+
 class MapConstant extends Constant {
   final DartType keyType;
   final DartType valueType;

pkg/kernel/lib/binary/ast_from_binary.dart

@@ -206,6 +206,12 @@ class BinaryBuilder {
         return new DoubleConstant(readDouble());
       case ConstantTag.StringConstant:
         return new StringConstant(readStringReference());
+      case ConstantTag.SymbolConstant:
+        Reference libraryReference;
+        if (readAndCheckOptionTag()) {
+          libraryReference = readLibraryReference();
+        }
+        return new SymbolConstant(readStringReference(), libraryReference);
       case ConstantTag.MapConstant:
         final DartType keyType = readDartType();
         final DartType valueType = readDartType();

pkg/kernel/lib/binary/ast_to_binary.dart

@@ -164,6 +164,10 @@ class BinaryPrinter implements Visitor<void>, BinarySink {
     } else if (constant is StringConstant) {
       writeByte(ConstantTag.StringConstant);
       writeStringReference(constant.value);
+    } else if (constant is SymbolConstant) {
+      writeByte(ConstantTag.SymbolConstant);
+      writeOptionalReference(constant.libraryReference);
+      writeStringReference(constant.name);
     } else if (constant is MapConstant) {
       writeByte(ConstantTag.MapConstant);
       writeDartType(constant.keyType);
@@ -1876,6 +1880,16 @@ class BinaryPrinter implements Visitor<void>, BinarySink {
     throw new UnsupportedError('serialization of StringConstant references');
   }
 
+  @override
+  void visitSymbolConstant(SymbolConstant node) {
+    throw new UnsupportedError('serialization of SymbolConstants');
+  }
+
+  @override
+  void visitSymbolConstantReference(SymbolConstant node) {
+    throw new UnsupportedError('serialization of SymbolConstant references');
+  }
+
   @override
   void visitPartialInstantiationConstant(PartialInstantiationConstant node) {
     throw new UnsupportedError(
@@ -1997,6 +2011,8 @@ class ConstantIndexer extends RecursiveVisitor {
 
     if (constant is StringConstant) {
       stringIndexer.put(constant.value);
+    } else if (constant is SymbolConstant) {
+      stringIndexer.put(constant.name);
     } else if (constant is DoubleConstant) {
       stringIndexer.put('${constant.value}');
     } else if (constant is IntConstant) {

pkg/kernel/lib/binary/tag.dart

@@ -127,7 +127,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 = 11;
+  static const int BinaryFormatVersion = 12;
 }
 
 abstract class ConstantTag {
@@ -136,10 +136,11 @@ abstract class ConstantTag {
   static const int IntConstant = 2;
   static const int DoubleConstant = 3;
   static const int StringConstant = 4;
-  static const int MapConstant = 5;
-  static const int ListConstant = 6;
-  static const int InstanceConstant = 7;
-  static const int PartialInstantiationConstant = 8;
-  static const int TearOffConstant = 9;
-  static const int TypeLiteralConstant = 10;
+  static const int SymbolConstant = 5;
+  static const int MapConstant = 6;
+  static const int ListConstant = 7;
+  static const int InstanceConstant = 8;
+  static const int PartialInstantiationConstant = 9;
+  static const int TearOffConstant = 10;
+  static const int TypeLiteralConstant = 11;
 }

pkg/kernel/lib/transformations/constants.dart

@@ -1025,8 +1025,9 @@ class ConstantEvaluator extends RecursiveVisitor {
   }
 
   visitSymbolLiteral(SymbolLiteral node) {
-    final value = canonicalize(new StringConstant(node.value));
-    return canonicalize(backend.buildSymbolConstant(value));
+    final libraryReference =
+        node.value.startsWith('_') ? libraryOf(node).reference : null;
+    return canonicalize(new SymbolConstant(node.value, libraryReference));
   }
 
   visitInstantiation(Instantiation node) {
@@ -1193,6 +1194,15 @@ class ConstantEvaluator extends RecursiveVisitor {
     }
     return value;
   }
+
+  Library libraryOf(TreeNode node) {
+    // The tree structure of the kernel AST ensures we always have an enclosing
+    // library.
+    while (true) {
+      if (node is Library) return node;
+      node = node.parent;
+    }
+  }
 }
 
 /// Holds the necessary information for a constant object, namely
@@ -1270,8 +1280,6 @@ abstract class ConstantsBackend {
       List<DartType> typeArguments,
       List<Constant> positionalArguments,
       Map<String, Constant> namedArguments);
-  Constant buildSymbolConstant(StringConstant value);
-
   Constant lowerListConstant(ListConstant constant);
   Constant lowerMapConstant(MapConstant constant);
 }

pkg/kernel/lib/visitor.dart

@@ -269,6 +269,7 @@ class ConstantVisitor<R> {
   R visitIntConstant(IntConstant node) => defaultConstant(node);
   R visitDoubleConstant(DoubleConstant node) => defaultConstant(node);
   R visitStringConstant(StringConstant node) => defaultConstant(node);
+  R visitSymbolConstant(SymbolConstant node) => defaultConstant(node);
   R visitMapConstant(MapConstant node) => defaultConstant(node);
   R visitListConstant(ListConstant node) => defaultConstant(node);
   R visitInstanceConstant(InstanceConstant node) => defaultConstant(node);
@@ -321,6 +322,7 @@ class Visitor<R> extends TreeVisitor<R>
   R visitIntConstant(IntConstant node) => defaultConstant(node);
   R visitDoubleConstant(DoubleConstant node) => defaultConstant(node);
   R visitStringConstant(StringConstant node) => defaultConstant(node);
+  R visitSymbolConstant(SymbolConstant node) => defaultConstant(node);
   R visitMapConstant(MapConstant node) => defaultConstant(node);
   R visitListConstant(ListConstant node) => defaultConstant(node);
   R visitInstanceConstant(InstanceConstant node) => defaultConstant(node);
@@ -345,6 +347,8 @@ class Visitor<R> extends TreeVisitor<R>
       defaultConstantReference(node);
   R visitStringConstantReference(StringConstant node) =>
       defaultConstantReference(node);
+  R visitSymbolConstantReference(SymbolConstant node) =>
+      defaultConstantReference(node);
   R visitMapConstantReference(MapConstant node) =>
       defaultConstantReference(node);
   R visitListConstantReference(ListConstant node) =>

pkg/kernel/lib/vm/constants_native_effects.dart

@@ -91,13 +91,6 @@ class VmConstantsBackend implements ConstantsBackend {
     throw 'No native effect registered for constant evaluation: $nativeName';
   }
 
-  Constant buildSymbolConstant(StringConstant value) {
-    return new InstanceConstant(
-        internalSymbolClass.reference,
-        const <DartType>[],
-        <Reference, Constant>{symbolNameField.reference: value});
-  }
-
   Constant lowerMapConstant(MapConstant constant) {
     // The _ImmutableMap class is implemented via one field pointing to a list
     // of key/value pairs -- see runtime/lib/immutable_map.dart!

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

@@ -535,6 +535,10 @@ class SummaryCollector extends RecursiveVisitor<TypeExpr> {
   Type get _stringType =>
       _cachedStringType ??= new Type.cone(_environment.stringType);
 
+  Type _cachedSymbolType;
+  Type get _symbolType =>
+      _cachedSymbolType ??= new Type.cone(_environment.symbolType);
+
   Type _cachedNullType;
   Type get _nullType => _cachedNullType ??= new Type.nullable(new Type.empty());
 
@@ -1258,6 +1262,11 @@ class ConstantAllocationCollector extends ConstantVisitor<Type> {
     return summaryCollector._stringType;
   }
 
+  @override
+  visitSymbolConstant(SymbolConstant constant) {
+    return summaryCollector._symbolType;
+  }
+
   @override
   Type visitMapConstant(MapConstant node) {
     throw 'The kernel2kernel constants transformation desugars const maps!';

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

@@ -939,6 +939,11 @@ class _TreeShakerConstantVisitor extends ConstantVisitor<Null> {
   @override
   visitStringConstant(StringConstant constant) {}
 
+  @override
+  visitSymbolConstant(SymbolConstant constant) {
+    // The Symbol class and it's _name field are always retained.
+  }
+
   @override
   visitMapConstant(MapConstant node) {
     throw 'The kernel2kernel constants transformation desugars const maps!';

runtime/vm/compiler/frontend/constant_evaluator.cc

@@ -1050,19 +1050,33 @@ ConstantHelper::ConstantHelper(Zone* zone,
       const_evaluator_(helper, type_translator, active_class, nullptr),
       translation_helper_(helper->translation_helper_),
       skip_vmservice_library_(skip_vmservice_library),
+      symbol_class_(Class::Handle(zone)),
+      symbol_name_field_(Field::Handle(zone)),
       temp_type_(AbstractType::Handle(zone)),
       temp_type_arguments_(TypeArguments::Handle(zone)),
       temp_type_arguments2_(TypeArguments::Handle(zone)),
       temp_type_arguments3_(TypeArguments::Handle(zone)),
       temp_object_(Object::Handle(zone)),
+      temp_string_(String::Handle(zone)),
       temp_array_(Array::Handle(zone)),
       temp_instance_(Instance::Handle(zone)),
       temp_field_(Field::Handle(zone)),
       temp_class_(Class::Handle(zone)),
+      temp_library_(Library::Handle(zone)),
       temp_function_(Function::Handle(zone)),
       temp_closure_(Closure::Handle(zone)),
       temp_context_(Context::Handle(zone)),
-      temp_integer_(Integer::Handle(zone)) {}
+      temp_integer_(Integer::Handle(zone)) {
+  temp_library_ = Library::InternalLibrary();
+  ASSERT(!temp_library_.IsNull());
+
+  symbol_class_ = temp_library_.LookupClass(Symbols::Symbol());
+  ASSERT(!symbol_class_.IsNull());
+
+  symbol_name_field_ =
+      symbol_class_.LookupInstanceFieldAllowPrivate(Symbols::_name());
+  ASSERT(!symbol_name_field_.IsNull());
+}
 
 const Array& ConstantHelper::ReadConstantTable() {
   const intptr_t number_of_constants = helper_.ReadUInt();
@@ -1111,6 +1125,21 @@ const Array& ConstantHelper::ReadConstantTable() {
             H.Canonicalize(H.DartString(helper_.ReadStringReference()));
         break;
       }
+      case kSymbolConstant: {
+        Tag initializer_tag = helper_.ReadTag();
+        if (initializer_tag == kSomething) {
+          const NameIndex index = helper_.ReadCanonicalNameReference();
+          temp_library_ = H.LookupLibraryByKernelLibrary(index);
+        } else {
+          temp_library_ = Library::null();
+        }
+        const String& symbol =
+            H.DartIdentifier(temp_library_, helper_.ReadStringReference());
+        temp_instance_ = Instance::New(symbol_class_, Heap::kOld);
+        temp_instance_.SetField(symbol_name_field_, symbol);
+        temp_instance_ = H.Canonicalize(temp_instance_);
+        break;
+      }
       case kListConstant: {
         temp_type_arguments_ = TypeArguments::New(1, Heap::kOld);
         const AbstractType& type = type_translator_.BuildType();

runtime/vm/compiler/frontend/constant_evaluator.h

@@ -166,15 +166,19 @@ class ConstantHelper {
   ConstantEvaluator const_evaluator_;
   TranslationHelper& translation_helper_;
   NameIndex skip_vmservice_library_;
+  Class& symbol_class_;
+  Field& symbol_name_field_;
   AbstractType& temp_type_;
   TypeArguments& temp_type_arguments_;
   TypeArguments& temp_type_arguments2_;
   TypeArguments& temp_type_arguments3_;
   Object& temp_object_;
+  String& temp_string_;
   Array& temp_array_;
   Instance& temp_instance_;
   Field& temp_field_;
   Class& temp_class_;
+  Library& temp_library_;
   Function& temp_function_;
   Closure& temp_closure_;
   Context& temp_context_;

runtime/vm/kernel_binary.h

@@ -17,7 +17,7 @@ namespace kernel {
 // package:kernel/binary.md.
 
 static const uint32_t kMagicProgramFile = 0x90ABCDEFu;
-static const uint32_t kBinaryFormatVersion = 11;
+static const uint32_t kBinaryFormatVersion = 12;
 
 // Keep in sync with package:kernel/lib/binary/tag.dart
 #define KERNEL_TAG_LIST(V)                                                     \
@@ -138,12 +138,13 @@ enum ConstantTag {
   kIntConstant = 2,
   kDoubleConstant = 3,
   kStringConstant = 4,
-  kMapConstant = 5,
-  kListConstant = 6,
-  kInstanceConstant = 7,
-  kPartialInstantiationConstant = 8,
-  kTearOffConstant = 9,
-  kTypeLiteralConstant = 10,
+  kSymbolConstant = 5,
+  kMapConstant = 6,
+  kListConstant = 7,
+  kInstanceConstant = 8,
+  kPartialInstantiationConstant = 9,
+  kTearOffConstant = 10,
+  kTypeLiteralConstant = 11,
 };
 
 static const int SpecializedIntLiteralBias = 3;

tests/language_2/vm/regress_34396_helper.dart

@@ -0,0 +1,8 @@
+// Copyright (c) 2018, 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.
+
+library regress_34396_helper;
+
+get privateSymbol => #_privateSymbol;
+get privateSymbolSame => #_privateSymbol;

tests/language_2/vm/regress_34396_test.dart

@@ -0,0 +1,17 @@
+// Copyright (c) 2018, 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.
+
+import 'package:expect/expect.dart';
+
+import 'regress_34396_helper.dart' as helper;
+
+main() {
+  Expect.isFalse(#_privateSymbol == helper.privateSymbol);
+  Expect.isFalse(#_privateSymbol == helper.privateSymbolSame);
+  Expect.isFalse(identical(#_privateSymbol, helper.privateSymbol));
+  Expect.isFalse(identical(#_privateSymbol, helper.privateSymbolSame));
+
+  Expect.isTrue(helper.privateSymbol == helper.privateSymbolSame);
+  Expect.isTrue(identical(helper.privateSymbol, helper.privateSymbolSame));
+}