Split CanonicalNameError, no warning if CanonicalNameSdkError

Prior to this CL, we would issue a warning whenever a CanonicalNameError
was encountered.
This is in principal a good thing, but because we currently have no way
to detect if the sdk we get is the one we expect
(by any other measure than when it issues a CanonicalNameError) we often
issue these warnings for no "real reason" whenever, for instance,
the flutter sdk changes.

This CL splits the CanonicalNameError in two such that errors with
references to the sdk ("dart:" libraries) issue CanonicalNameSdkError
instead, an we then handle that differently. Namely we silently ignore
the error (i.e. don't issue a warning) and just don't initialize from
dill.

This should remedy the situation and be strictly better than to always
swallow CanonicalNameErrors.

Bug: 36032
Change-Id: Idbae0b5ee5b9843a5dbeb49b3c65ae25f5962e36
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/105240
Commit-Queue: Jens Johansen <[email protected]>
Reviewed-by: Kevin Millikin <[email protected]>

Author: jensjoha

pkg/front_end/lib/src/fasta/incremental_compiler.dart

@@ -9,7 +9,11 @@ import 'dart:async' show Future;
 import 'package:kernel/binary/ast_from_binary.dart' show BinaryBuilder;
 
 import 'package:kernel/binary/ast_from_binary.dart'
-    show BinaryBuilder, CanonicalNameError, InvalidKernelVersionError;
+    show
+        BinaryBuilder,
+        CanonicalNameError,
+        CanonicalNameSdkError,
+        InvalidKernelVersionError;
 
 import 'package:kernel/class_hierarchy.dart' show ClassHierarchy;
 
@@ -161,7 +165,9 @@ class IncrementalCompiler implements IncrementalKernelGenerator {
               bytesLength =
                   prepareSummary(summaryBytes, uriTranslator, c, data);
 
-              if (e is InvalidKernelVersionError || e is PackageChangedError) {
+              if (e is InvalidKernelVersionError ||
+                  e is PackageChangedError ||
+                  e is CanonicalNameSdkError) {
                 // Don't report any warning.
               } else {
                 Uri gzInitializedFrom;

pkg/front_end/test/incremental_load_from_invalid_dill_test.dart

@@ -40,7 +40,7 @@ import 'package:front_end/src/fasta/kernel/utils.dart' show serializeComponent;
 
 import 'package:front_end/src/fasta/severity.dart' show Severity;
 
-import 'package:kernel/kernel.dart' show Component;
+import 'package:kernel/kernel.dart' show Component, Library;
 
 import 'incremental_load_from_dill_test.dart' show getOptions;
 
@@ -56,24 +56,26 @@ class Tester {
   Uri sdkSummary;
   Uri initializeFrom;
   Uri helperFile;
+  Uri helper2File;
   Uri entryPoint;
+  Uri entryPointImportDartFoo;
   Uri platformUri;
   List<int> sdkSummaryData;
   List<DiagnosticMessage> errorMessages;
   List<DiagnosticMessage> warningMessages;
   MemoryFileSystem fs;
   CompilerOptions options;
+  IncrementalCompiler compiler;
 
-  compileExpectInitializeFailAndSpecificWarning(
+  void compileExpectInitializeFailAndSpecificWarning(
       Code expectedWarningCode, bool writeFileOnCrashReport) async {
     errorMessages.clear();
     warningMessages.clear();
     options.writeFileOnCrashReport = writeFileOnCrashReport;
-    DeleteTempFilesIncrementalCompiler compiler =
-        new DeleteTempFilesIncrementalCompiler(
-            new CompilerContext(
-                new ProcessedOptions(options: options, inputs: [entryPoint])),
-            initializeFrom);
+    compiler = new DeleteTempFilesIncrementalCompiler(
+        new CompilerContext(
+            new ProcessedOptions(options: options, inputs: [entryPoint])),
+        initializeFrom);
     await compiler.computeDelta();
     if (compiler.initializedFromDill) {
       Expect.fail("Expected to not be able to initialized from dill, but did.");
@@ -91,13 +93,40 @@ class Tester {
     }
   }
 
+  Future<Component> compileExpectOk(
+      bool initializedFromDill, Uri compileThis) async {
+    errorMessages.clear();
+    warningMessages.clear();
+    options.writeFileOnCrashReport = false;
+    compiler = new DeleteTempFilesIncrementalCompiler(
+        new CompilerContext(
+            new ProcessedOptions(options: options, inputs: [compileThis])),
+        initializeFrom);
+    Component component = await compiler.computeDelta();
+
+    if (compiler.initializedFromDill != initializedFromDill) {
+      Expect.fail("Expected initializedFromDill to be $initializedFromDill "
+          "but was ${compiler.initializedFromDill}");
+    }
+    if (errorMessages.isNotEmpty) {
+      Expect.fail("Got unexpected errors: " + joinMessages(errorMessages));
+    }
+    if (warningMessages.isNotEmpty) {
+      Expect.fail("Got unexpected warnings: " + joinMessages(warningMessages));
+    }
+
+    return component;
+  }
+
   initialize() async {
     sdkRoot = computePlatformBinariesLocation(forceBuildDir: true);
     base = Uri.parse("org-dartlang-test:///");
     sdkSummary = base.resolve("vm_platform.dill");
     initializeFrom = base.resolve("initializeFrom.dill");
     helperFile = base.resolve("helper.dart");
+    helper2File = base.resolve("helper2.dart");
     entryPoint = base.resolve("small.dart");
+    entryPointImportDartFoo = base.resolve("small_foo.dart");
     platformUri = sdkRoot.resolve("vm_platform_strong.dill");
     sdkSummaryData = await new File.fromUri(platformUri).readAsBytes();
     errorMessages = <DiagnosticMessage>[];
@@ -108,6 +137,7 @@ class Tester {
     options.fileSystem = fs;
     options.sdkRoot = null;
     options.sdkSummary = sdkSummary;
+    options.omitPlatform = true;
     options.onDiagnostic = (DiagnosticMessage message) {
       if (message.severity == Severity.error) {
         errorMessages.add(message);
@@ -121,17 +151,28 @@ class Tester {
 foo() {
     print("hello from foo");
 }
+""");
+    fs.entityForUri(helper2File).writeAsStringSync("""
+foo2() {
+    print("hello from foo2");
+}
 """);
     fs.entityForUri(entryPoint).writeAsStringSync("""
 import "helper.dart" as helper;
 main() {
     helper.foo();
 }
+""");
+    fs.entityForUri(entryPointImportDartFoo).writeAsStringSync("""
+import "dart:foo" as helper;
+main() {
+    helper.foo2();
+}
 """);
   }
 
   Future<Null> test() async {
-    IncrementalCompiler compiler = new IncrementalCompiler(
+    compiler = new IncrementalCompiler(
         new CompilerContext(
             new ProcessedOptions(options: options, inputs: [entryPoint])),
         initializeFrom);
@@ -140,23 +181,32 @@ main() {
     List<int> dataGood = serializeComponent(componentGood);
     fs.entityForUri(initializeFrom).writeAsBytesSync(dataGood);
 
-    // Initialize from good dill file should be ok.
+    // Create fake "dart:foo" library.
+    options.omitPlatform = false;
     compiler = new IncrementalCompiler(
         new CompilerContext(
-            new ProcessedOptions(options: options, inputs: [entryPoint])),
+            new ProcessedOptions(options: options, inputs: [helper2File])),
         initializeFrom);
-    compiler.invalidate(entryPoint);
-    Component component = await compiler.computeDelta();
-    if (!compiler.initializedFromDill) {
-      Expect.fail(
-          "Expected to have sucessfully initialized from dill, but didn't.");
-    }
-    if (errorMessages.isNotEmpty) {
-      Expect.fail("Got unexpected errors: " + joinMessages(errorMessages));
-    }
-    if (warningMessages.isNotEmpty) {
-      Expect.fail("Got unexpected warnings: " + joinMessages(warningMessages));
+    Component componentHelper = await compiler.computeDelta();
+    Library helper2Lib = componentHelper.libraries
+        .firstWhere((lib) => lib.importUri == helper2File);
+    helper2Lib.importUri = new Uri(scheme: "dart", path: "foo");
+    List<int> sdkWithDartFoo = serializeComponent(componentHelper);
+    options.omitPlatform = true;
+
+    // Compile with our fake sdk with dart:foo should be ok.
+    List<int> orgSdkBytes = await fs.entityForUri(sdkSummary).readAsBytes();
+    fs.entityForUri(sdkSummary).writeAsBytesSync(sdkWithDartFoo);
+    Component component = await compileExpectOk(true, entryPointImportDartFoo);
+    fs.entityForUri(sdkSummary).writeAsBytesSync(orgSdkBytes);
+    if (component.libraries.length != 1) {
+      Expect.fail("Expected 1 library, got ${component.libraries.length}: "
+          "${component.libraries}");
     }
+    List<int> dataLinkedToSdkWithFoo = serializeComponent(component);
+
+    // Initialize from good dill file should be ok.
+    await compileExpectOk(true, entryPoint);
 
     // Create a partial dill file.
     compiler.invalidate(entryPoint);
@@ -181,6 +231,11 @@ main() {
         codeInitializeFromDillUnknownProblem, true);
     await compileExpectInitializeFailAndSpecificWarning(
         codeInitializeFromDillUnknownProblemNoDump, false);
+
+    // Create a dill with a reference to a non-existing sdk thing:
+    // Should be ok (for now), but we shouldn't actually initialize from dill.
+    fs.entityForUri(initializeFrom).writeAsBytesSync(dataLinkedToSdkWithFoo);
+    await compileExpectOk(false, entryPoint);
   }
 }
 

pkg/kernel/lib/binary/ast_from_binary.dart

@@ -39,6 +39,10 @@ class CanonicalNameError {
   CanonicalNameError(this.message);
 }
 
+class CanonicalNameSdkError extends CanonicalNameError {
+  CanonicalNameSdkError(String message) : super(message);
+}
+
 class _ComponentIndex {
   static const numberOfFixedFields = 9;
 
@@ -528,8 +532,8 @@ class BinaryBuilder {
               // OK then.
               checkReferenceNode = false;
             } else {
-              throw new CanonicalNameError(
-                  "Null reference (${child.name}) ($child).");
+              throw buildCanonicalNameError(
+                  "Null reference (${child.name}) ($child).", child);
             }
           }
           if (checkReferenceNode) {
@@ -538,8 +542,8 @@ class BinaryBuilder {
                   "Canonical name and reference doesn't agree.");
             }
             if (child.reference.node == null) {
-              throw new CanonicalNameError(
-                  "Reference is null (${child.name}) ($child).");
+              throw buildCanonicalNameError(
+                  "Reference is null (${child.name}) ($child).", child);
             }
           }
         }
@@ -548,6 +552,17 @@ class BinaryBuilder {
     }
   }
 
+  CanonicalNameError buildCanonicalNameError(
+      String message, CanonicalName problemNode) {
+    // Special-case missing sdk entries as that is probably a change to the
+    // platform - that's something we might want to react differently to.
+    String libraryUri = problemNode?.nonRootTop?.name ?? "";
+    if (libraryUri.startsWith("dart:")) {
+      return new CanonicalNameSdkError(message);
+    }
+    return new CanonicalNameError(message);
+  }
+
   _ComponentIndex _readComponentIndex(int componentFileSize) {
     int savedByteIndex = _byteOffset;