Add version validation for LookupMap, also add unittest directly in LookupMap (take 2)

BUG=
R=sra@google.com

Review URL: https://codereview.chromium.org//1308993008 .

pkg/compiler/lib/src/common/backend_api.dart

@@ -343,6 +343,9 @@ abstract class Backend {
   /// times, but [onQueueClosed] is only called once.
   void onQueueClosed() {}
 
+  /// Called when the compiler starts running the codegen enqueuer.
+  void onCodegenStart() {}
+
   /// Called after [element] has been resolved.
   // TODO(johnniwinther): Change [TreeElements] to [Registry] or a dependency
   // node. [elements] is currently unused by the implementation.

pkg/compiler/lib/src/compiler.dart

@@ -1153,6 +1153,7 @@ abstract class Compiler implements DiagnosticListener {
 
     log('Compiling...');
     phase = PHASE_COMPILING;
+    backend.onCodegenStart();
     // TODO(johnniwinther): Move these to [CodegenEnqueuer].
     if (hasIsolateSupport) {
       backend.enableIsolateSupport(enqueuer.codegen);

pkg/compiler/lib/src/diagnostics/messages.dart

@@ -421,6 +421,7 @@ enum MessageKind {
   UNIMPLEMENTED_SETTER,
   UNIMPLEMENTED_SETTER_ONE,
   UNMATCHED_TOKEN,
+  UNRECOGNIZED_VERSION_OF_LOOKUP_MAP,
   UNSUPPORTED_BANG_EQ_EQ,
   UNSUPPORTED_EQ_EQ_EQ,
   UNSUPPORTED_LITERAL_SYMBOL,
@@ -3295,6 +3296,10 @@ $IMPORT_EXPERIMENTAL_MIRRORS_PADDING#{importChain}
           "more code and prevents the compiler from doing some optimizations.",
           howToFix: "Consider removing this 'noSuchMethod' implementation."),
 
+      MessageKind.UNRECOGNIZED_VERSION_OF_LOOKUP_MAP:
+        const MessageTemplate(MessageKind.UNRECOGNIZED_VERSION_OF_LOOKUP_MAP,
+          "Unsupported version of package:lookup_map.",
+          howToFix: DONT_KNOW_HOW_TO_FIX),
 
   }; // End of TEMPLATES.
 

pkg/compiler/lib/src/js_backend/backend.dart

@@ -2159,7 +2159,7 @@ class JavaScriptBackend extends Backend {
       } else if (uri == Uris.dart_html) {
         htmlLibraryIsLoaded = true;
       } else if (uri == PACKAGE_LOOKUP_MAP) {
-        lookupMapAnalysis.initRuntimeClass(find(library, 'LookupMap'));
+        lookupMapAnalysis.init(library);
       }
       annotations.onLibraryScanned(library);
     });
@@ -2605,7 +2605,13 @@ class JavaScriptBackend extends Backend {
     return true;
   }
 
-  void onQueueClosed() => lookupMapAnalysis.onQueueClosed();
+  void onQueueClosed() {
+    lookupMapAnalysis.onQueueClosed();
+  }
+
+  void onCodegenStart() {
+    lookupMapAnalysis.onCodegenStart();
+  }
 
   void onElementResolved(Element element, TreeElements elements) {
     if ((element.isFunction || element.isGenerativeConstructor) &&

pkg/compiler/lib/src/js_backend/lookup_map_analysis.dart

@@ -7,6 +7,7 @@ library compiler.src.js_backend.lookup_map_analysis;
 
 import '../common/registry.dart' show Registry;
 import '../compiler.dart' show Compiler;
+import '../diagnostics/messages.dart' show MessageKind;
 import '../constants/values.dart' show
      ConstantValue,
      ConstructedConstantValue,
@@ -14,8 +15,15 @@ import '../constants/values.dart' show
      NullConstantValue,
      TypeConstantValue;
 import '../dart_types.dart' show DartType;
-import '../elements/elements.dart' show Elements, Element, ClassElement,
-     FieldElement, FunctionElement, FunctionSignature;
+import '../elements/elements.dart' show
+    ClassElement,
+    Element,
+    Elements,
+    FieldElement,
+    FunctionElement,
+    FunctionSignature,
+    LibraryElement,
+    VariableElement;
 import '../enqueue.dart' show Enqueuer;
 import 'js_backend.dart' show JavaScriptBackend;
 import '../dart_types.dart' show DynamicType, InterfaceType;
@@ -63,6 +71,13 @@ class LookupMapAnalysis {
   /// discover that a key in a map is potentially used.
   final JavaScriptBackend backend;
 
+  /// The resolved [VariableElement] associated with the top-level `_version`.
+  VariableElement lookupMapVersionVariable;
+
+  /// The resolved [LibraryElement] associated with
+  /// `package:lookup_map/lookup_map.dart`.
+  LibraryElement lookupMapLibrary;
+
   /// The resolved [ClassElement] associated with `LookupMap`.
   ClassElement typeLookupMapClass;
 
@@ -101,9 +116,7 @@ class LookupMapAnalysis {
   final _pending = <ConstantValue, List<_LookupMapInfo>>{};
 
   /// Whether the backend is currently processing the codegen queue.
-  // TODO(sigmund): is there a better way to do this. Do we need to plumb the
-  // enqueuer on each callback?
-  bool get _inCodegen => backend.compiler.phase == Compiler.PHASE_COMPILING;
+  bool _inCodegen = false;
 
   LookupMapAnalysis(this.backend);
 
@@ -114,14 +127,53 @@ class LookupMapAnalysis {
     return typeLookupMapClass != null;
   }
 
-  /// Initializes this analysis by providing the resolver information of
-  /// `LookupMap`.
-  void initRuntimeClass(ClassElement cls) {
+  /// Initializes this analysis by providing the resolved library. This is
+  /// invoked during resolution when the `lookup_map` library is discovered.
+  void init(LibraryElement library) {
+    lookupMapLibrary = library;
+    // We will enable the lookupMapAnalysis as long as we get a known version of
+    // the lookup_map package. We otherwise produce a warning.
+    lookupMapVersionVariable = library.implementation.findLocal('_version');
+    if (lookupMapVersionVariable == null) {
+      backend.compiler.reportInfo(library,
+          MessageKind.UNRECOGNIZED_VERSION_OF_LOOKUP_MAP);
+    } else {
+      backend.compiler.enqueuer.resolution.addToWorkList(
+          lookupMapVersionVariable);
+    }
+  }
+
+  /// Checks if the version of lookup_map is valid, and if so, enable this
+  /// analysis during codegen.
+  void onCodegenStart() {
+    _inCodegen = true;
+    if (lookupMapVersionVariable == null) return;
+
+    // At this point, the lookupMapVersionVariable should be resolved and it's
+    // constant value should be available.
+    ConstantValue value =
+        backend.constants.getConstantValueForVariable(lookupMapVersionVariable);
+    if (value == null) {
+      backend.compiler.reportInfo(lookupMapVersionVariable,
+          MessageKind.UNRECOGNIZED_VERSION_OF_LOOKUP_MAP);
+      return;
+    }
+
+    // TODO(sigmund): add proper version resolution using the pub_semver package
+    // when we introduce the next version.
+    String version = value.primitiveValue.slowToString();
+    if (version != '0.0.1') {
+      backend.compiler.reportInfo(lookupMapVersionVariable,
+          MessageKind.UNRECOGNIZED_VERSION_OF_LOOKUP_MAP);
+      return;
+    }
+
+    ClassElement cls = lookupMapLibrary.findLocal('LookupMap');
     cls.computeType(backend.compiler);
     entriesField = cls.lookupMember('_entries');
     keyField = cls.lookupMember('_key');
     valueField = cls.lookupMember('_value');
-    // TODO(sigmund): Maybe inline nested maps make the output code smaller?
+    // TODO(sigmund): Maybe inline nested maps to make the output code smaller?
     typeLookupMapClass = cls;
   }
 

pkg/lookup_map/lib/lookup_map.dart

@@ -91,3 +91,10 @@ class LookupMap<K, V> {
 /// An expando that stores a flatten version of a [LookupMap], this is
 /// computed and stored the first time the map is accessed.
 final _flatMap = new Expando('_flat_map');
+
+/// Internal constant that matches the version in the pubspec. This is used by
+/// dart2js to ensure that optimizations are only enabled on known versions of
+/// this code.
+// Note: this needs to be kept in sync with the pubspec, otherwise
+// test/version_check_test would fail.
+final _version = '0.0.1';

pkg/lookup_map/pubspec.yaml

@@ -1,3 +1,8 @@
 name: lookup_map
 description: a lookup-only map that can be tree-shaken by dart2js
+# Note: the version needs to be kept in sync with the string in
+# lib/lookup_map.dart, otherwise test/version_check_test would fail.
 version: 0.0.1
+dev_dependencies:
+  test: ^0.12.3+8
+  yaml: ^2.1.2

pkg/lookup_map/test/lookup_map_test.dart

@@ -0,0 +1,74 @@
+// Copyright (c) 2015, 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:lookup_map/lookup_map.dart';
+
+import 'package:test/test.dart';
+
+class Key {
+  final int id;
+  const Key(this.id);
+}
+
+class A{}
+const B = const Key(1);
+class C{}
+
+main() {
+  test('entries constructor', () {
+    var m = const LookupMap(const [
+        A, "the-text-for-A",
+        B, "the-text-for-B",
+        1.2, "the-text-for-1.2"]);
+    expect(m[A], 'the-text-for-A');
+    expect(m[B], 'the-text-for-B');
+    expect(m[1.2], 'the-text-for-1.2');
+    expect(m[C], null);
+    expect(m[1.3], null);
+  });
+
+  test('pair constructor', () {
+    var m = const LookupMap.pair(A, "the-text-for-A");
+    expect(m[A], 'the-text-for-A');
+    expect(m[B], null);
+  });
+
+  test('nested lookup', () {
+    var m = const LookupMap(const [],
+        const [const LookupMap.pair(A, "the-text-for-A")]);
+    expect(m[A], 'the-text-for-A');
+    expect(m[B], null);
+  });
+
+  test('entry shadows nested maps', () {
+    var m = const LookupMap(const [
+      A, "the-text-for-A2",
+    ], const [
+      const LookupMap.pair(A, "the-text-for-A1"),
+    ]);
+    expect(m[A], 'the-text-for-A2');
+  });
+
+  test('nested maps shadow in order', () {
+    var m = const LookupMap(const [ ], const [
+      const LookupMap.pair(A, "the-text-for-A1"),
+      const LookupMap.pair(B, "the-text-for-B2"),
+      const LookupMap.pair(A, "the-text-for-A2"),
+      const LookupMap.pair(B, "the-text-for-B1"),
+    ]);
+    expect(m[A], 'the-text-for-A2');
+    expect(m[B], 'the-text-for-B1');
+  });
+
+  // This test would fail if dart2js has a bug, but we keep it here for our
+  // sanity.
+  test('reachable lookups are not tree-shaken', () {
+    var m = const LookupMap(const [
+      A, B,
+      B, C,
+      C, 3.4,
+    ]);
+    expect(m[m[m[A]]], 3.4);
+  });
+}

pkg/lookup_map/test/version_check_test.dart

@@ -0,0 +1,21 @@
+// Copyright (c) 2015, 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 'dart:io';
+import 'dart:mirrors';
+import 'package:lookup_map/lookup_map.dart'; // accessed via mirrors;
+import 'package:test/test.dart';
+import 'package:yaml/yaml.dart';
+
+/// This dartdoc helps remove a warning for the unused import on [LookupMap].
+main() {
+  test('validate version number matches', () {
+    var pubspecPath = Platform.script.resolve('../pubspec.yaml').path;
+    var yaml = loadYaml(new File(pubspecPath).readAsStringSync());
+    var version1 = yaml['version'];
+    var library = currentMirrorSystem().findLibrary(#lookup_map);
+    var version2 = library.getField(new Symbol('_version')).reflectee;
+    expect(version1, version2);
+  });
+}

pkg/pkg.status

@@ -107,6 +107,7 @@ analyzer/test/src/task/incremental_element_builder_test: Pass, Slow # Issue 2162
 analyzer/test/src/task/inputs_test: Pass, Slow # Issue 21628
 analyzer/test/src/task/manager_test: Pass, Slow # Issue 21628
 analyzer/test/src/task/model_test: Pass, Slow # Issue 21628
+lookup_map/test/version_check_test: SkipByDesign # Only meant to run in vm.
 
 [ $runtime == jsshell ]
 async/test/stream_zip_test: RuntimeError, OK # Timers are not supported.

tests/compiler/dart2js/mock_libraries.dart

@@ -431,4 +431,5 @@ const Map<String, String> DEFAULT_LOOKUP_MAP_LIBRARY = const <String, String>{
         : _entries = const [], _nestedMaps = const [];
     V operator[](K k) => null;
   }''',
+  '_version': 'const _version = "0.0.1";',
 };