[vm] Remove unused kernel bytes in case of compile-time errors

In case of compile-time errors kernel service has been serializing kernel AST anyway and has been passing kernel bytes to the VM.
VM does not use those kernel bytes except a few unit tests.

This change removes the unnecessary kernel serialization and freeing
of kernel bytes and cleans up unit tests.

TEST=ci

Change-Id: Ic2464e50f56227bc998df53dd2b594c1abcf9468
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/436360
Commit-Queue: Alexander Markov <[email protected]>
Reviewed-by: Ryan Macnak <[email protected]>

Author: alexmarkov

pkg/vm/bin/kernel_service.dart

@@ -643,7 +643,7 @@ Future _processExpressionCompilationRequest(request) async {
             port.send(
               new CompilationResult.errors([
                 "No platform found to initialize incremental compiler.",
-              ], null).toResponse(),
+              ]).toResponse(),
             );
             return;
           }
@@ -687,7 +687,7 @@ Future _processExpressionCompilationRequest(request) async {
           new CompilationResult.errors([
             "Error when trying to create a compiler for expression compilation: "
                 "'$e'.",
-          ], null).toResponse(),
+          ]).toResponse(),
         );
         return;
       }
@@ -698,7 +698,7 @@ Future _processExpressionCompilationRequest(request) async {
     port.send(
       new CompilationResult.errors([
         "No incremental compiler available for this isolate.",
-      ], null).toResponse(),
+      ]).toResponse(),
     );
     return;
   }
@@ -724,9 +724,7 @@ Future _processExpressionCompilationRequest(request) async {
     );
 
     if (procedure == null) {
-      port.send(
-        new CompilationResult.errors(["Invalid scope."], null).toResponse(),
-      );
+      port.send(new CompilationResult.errors(["Invalid scope."]).toResponse());
       return;
     }
 
@@ -736,7 +734,7 @@ Future _processExpressionCompilationRequest(request) async {
     if (compiler.errorsPlain.isNotEmpty) {
       // TODO(sigmund): the compiler prints errors to the console, so we
       // shouldn't print those messages again here.
-      result = new CompilationResult.errors(compiler.errorsPlain, null);
+      result = new CompilationResult.errors(compiler.errorsPlain);
     } else {
       Component component = createExpressionEvaluationComponent(procedure);
       result = new CompilationResult.ok(serializeComponent(component));
@@ -899,7 +897,7 @@ Future _processLoadRequest(request) async {
       port.send(
         new CompilationResult.errors([
           "No incremental compiler available for this isolate.",
-        ], null).toResponse(),
+        ]).toResponse(),
       );
       return;
     }
@@ -1024,19 +1022,7 @@ Future _processLoadRequest(request) async {
         ...(enableColors) ? compiler.errorsColorized : compiler.errorsPlain,
         ...nativeAssetsErrors.map((e) => e.message),
       ];
-      final component = compilerResult.component;
-      if (component != null) {
-        result = new CompilationResult.errors(
-          errors,
-          serializeComponent(
-            component,
-            filter: (lib) => !loadedLibraries.contains(lib),
-            nativeAssetsComponent: nativeAssetsComponent,
-          ),
-        );
-      } else {
-        result = new CompilationResult.errors(errors, null);
-      }
+      result = new CompilationResult.errors(errors);
     } else {
       // We serialize the component excluding vm_platform.dill because the VM has
       // these sources built-in. Everything loaded as a summary in
@@ -1076,7 +1062,7 @@ Future _processLoadRequest(request) async {
       inputFileUri,
       inputFileUri,
       null,
-      new CompilationResult.errors(<String>["unknown tag"], null).payload,
+      new CompilationResult.errors(<String>["unknown tag"]).payload,
     ]);
   }
 }
@@ -1288,8 +1274,7 @@ abstract class CompilationResult {
 
   factory CompilationResult.ok(Uint8List? bytes) = _CompilationOk;
 
-  factory CompilationResult.errors(List<String> errors, Uint8List? bytes) =
-      _CompilationError;
+  factory CompilationResult.errors(List<String> errors) = _CompilationError;
 
   factory CompilationResult.crash(Object exception, StackTrace stack) =
       _CompilationCrash;
@@ -1332,10 +1317,9 @@ abstract class _CompilationFail extends CompilationResult {
 }
 
 class _CompilationError extends _CompilationFail {
-  final Uint8List? bytes;
   final List<String> errors;
 
-  _CompilationError(this.errors, this.bytes);
+  _CompilationError(this.errors);
 
   @override
   Status get status => Status.error;
@@ -1344,8 +1328,6 @@ class _CompilationError extends _CompilationFail {
   String get errorString => errors.join('\n');
 
   String toString() => "_CompilationError(${errorString})";
-
-  List toResponse() => [status.index, payload, bytes];
 }
 
 class _CompilationCrash extends _CompilationFail {

pkg/vm/test/kernel_service_test.dart

@@ -169,11 +169,10 @@ Future<kernel_service.Status> singleShotCompile(
     }
     return kernel_service.Status.ok;
   } else if (status == kernel_service.Status.error.index) {
-    expectLength(m, 3);
+    expectLength(m, 2);
     final String errors = m[1];
-    final List<int> bytes = m[2];
     if (verbose) {
-      print("Compiled with errors --- $errors and ${bytes.length} bytes dill");
+      print("Compiled with errors --- $errors");
     }
     return kernel_service.Status.error;
   } else if (status == kernel_service.Status.crash.index) {

runtime/bin/dfe.cc

@@ -213,18 +213,15 @@ void DFE::CompileAndReadScript(const char* script_uri,
       *exit_code = 0;
       break;
     case Dart_KernelCompilationStatus_Error:
-      free(result.kernel);
       *error = result.error;  // Copy error message.
       *exit_code = kCompilationErrorExitCode;
       break;
     case Dart_KernelCompilationStatus_Crash:
-      free(result.kernel);
       *error = result.error;  // Copy error message.
       *exit_code = kDartFrontendErrorExitCode;
       break;
     case Dart_KernelCompilationStatus_Unknown:
     case Dart_KernelCompilationStatus_MsgFailed:
-      free(result.kernel);
       *error = result.error;  // Copy error message.
       *exit_code = kErrorExitCode;
       break;

runtime/vm/dart_api_impl_test.cc

@@ -8299,8 +8299,7 @@ TEST_CASE(DartAPI_Multiroot_Valid) {
   int sourcefiles_count = sizeof(sourcefiles) / sizeof(Dart_SourceFile);
   lib = TestCase::LoadTestScriptWithDFE(
       sourcefiles_count, sourcefiles, nullptr, /* finalize= */ true,
-      /* incrementally= */ true, /* allow_compile_errors= */ false,
-      "foo:///main.dart",
+      /* incrementally= */ true, "foo:///main.dart",
       /* multiroot_filepaths= */ "/bar,/baz",
       /* multiroot_scheme= */ "foo");
   EXPECT_VALID(lib);
@@ -8340,8 +8339,7 @@ TEST_CASE(DartAPI_Multiroot_FailWhenUriIsWrong) {
   int sourcefiles_count = sizeof(sourcefiles) / sizeof(Dart_SourceFile);
   lib = TestCase::LoadTestScriptWithDFE(
       sourcefiles_count, sourcefiles, nullptr, /* finalize= */ true,
-      /* incrementally= */ true, /* allow_compile_errors= */ false,
-      "foo1:///main.dart",
+      /* incrementally= */ true, "foo1:///main.dart",
       /* multiroot_filepaths= */ "/bar,/baz",
       /* multiroot_scheme= */ "foo");
   EXPECT_ERROR(lib,

runtime/vm/isolate_reload.cc

@@ -1184,9 +1184,6 @@ char* IsolateGroupReloadContext::CompileToKernel(bool force_reload,
         /*multiroot_scheme=*/nullptr);
   }
   if (retval.status != Dart_KernelCompilationStatus_Ok) {
-    if (retval.kernel != nullptr) {
-      free(retval.kernel);
-    }
     return retval.error;
   }
   *kernel_buffer = retval.kernel;

runtime/vm/isolate_reload_test.cc

@@ -614,17 +614,16 @@ TEST_CASE(IsolateReload_LibraryImportAdded) {
       "  return max(3, 4);\n"
       "}\n";
 
-  const char* kReloadScript =
+  const char* kScript2 =
       "import 'dart:math';\n"
       "main() {\n"
       "  return max(3, 4);\n"
       "}\n";
 
-  Dart_Handle lib = TestCase::LoadTestScriptWithErrors(kScript);
-  EXPECT_VALID(lib);
-  EXPECT_ERROR(SimpleInvokeError(lib, "main"), "max");
+  Dart_Handle lib = TestCase::LoadTestScript(kScript, nullptr);
+  EXPECT_ERROR(lib, "Compilation failed");
 
-  lib = TestCase::ReloadTestScript(kReloadScript);
+  lib = TestCase::LoadTestScript(kScript2, nullptr);
   EXPECT_VALID(lib);
   EXPECT_EQ(4, SimpleInvoke(lib, "main"));
 }
@@ -1124,8 +1123,8 @@ TEST_CASE(IsolateReload_LibraryHide) {
   const char* kImportScript = "importedFunc() => 'a';\n";
   TestCase::AddTestLib("test:lib1", kImportScript);
 
-  // Import 'test:lib1' with importedFunc hidden. Will result in an
-  // error.
+  // Import 'test:lib1' with importedFunc hidden. Will result in a
+  // compile-time error.
   const char* kScript =
       "import 'test:lib1' hide importedFunc;\n"
       "main() {\n"
@@ -1134,18 +1133,17 @@ TEST_CASE(IsolateReload_LibraryHide) {
 
   // Dart_Handle result;
 
-  Dart_Handle lib = TestCase::LoadTestScriptWithErrors(kScript);
-  EXPECT_VALID(lib);
-  EXPECT_ERROR(SimpleInvokeError(lib, "main"), "importedFunc");
+  Dart_Handle lib = TestCase::LoadTestScript(kScript, nullptr);
+  EXPECT_ERROR(lib, "Compilation failed");
 
   // Import 'test:lib1'.
-  const char* kReloadScript =
+  const char* kScript2 =
       "import 'test:lib1';\n"
       "main() {\n"
       "  return importedFunc();\n"
       "}\n";
 
-  lib = TestCase::ReloadTestScript(kReloadScript);
+  lib = TestCase::LoadTestScript(kScript2, nullptr);
   EXPECT_VALID(lib);
   EXPECT_STREQ("a", SimpleInvokeStr(lib, "main"));
 }
@@ -1157,7 +1155,7 @@ TEST_CASE(IsolateReload_LibraryShow) {
   TestCase::AddTestLib("test:lib1", kImportScript);
 
   // Import 'test:lib1' with importedIntFunc visible. Will result in
-  // an error when 'main' is invoked.
+  // a compile-time error.
   const char* kScript =
       "import 'test:lib1' show importedIntFunc;\n"
       "main() {\n"
@@ -1168,17 +1166,12 @@ TEST_CASE(IsolateReload_LibraryShow) {
       "  return importedIntFunc();\n"
       "}\n";
 
-  Dart_Handle lib = TestCase::LoadTestScriptWithErrors(kScript);
-  EXPECT_VALID(lib);
-
-  // Works.
-  EXPECT_EQ(4, SimpleInvoke(lib, "mainInt"));
-  // Results in an error.
-  EXPECT_ERROR(SimpleInvokeError(lib, "main"), "importedFunc");
+  Dart_Handle lib = TestCase::LoadTestScript(kScript, nullptr);
+  EXPECT_ERROR(lib, "Compilation failed");
 
   // Import 'test:lib1' with importedFunc visible. Will result in
-  // an error when 'mainInt' is invoked.
-  const char* kReloadScript =
+  // a compile-time error.
+  const char* kScript2 =
       "import 'test:lib1' show importedFunc;\n"
       "main() {\n"
       "  return importedFunc();\n"
@@ -1188,8 +1181,24 @@ TEST_CASE(IsolateReload_LibraryShow) {
       "  return importedIntFunc();\n"
       "}\n";
 
-  lib = TestCase::ReloadTestScript(kReloadScript);
-  EXPECT_ERROR(lib, "importedIntFunc");
+  lib = TestCase::LoadTestScript(kScript2, nullptr);
+  EXPECT_ERROR(lib, "Compilation failed");
+
+  // Both imports 'test:lib1' are visible.
+  // Should be successful.
+  const char* kScript3 =
+      "import 'test:lib1' show importedFunc, importedIntFunc;\n"
+      "main() {\n"
+      "  return importedFunc();\n"
+      "}\n"
+      "@pragma('vm:entry-point', 'call')\n"
+      "mainInt() {\n"
+      "  return importedIntFunc();\n"
+      "}\n";
+
+  lib = TestCase::LoadTestScript(kScript3, nullptr);
+  EXPECT_VALID(lib);
+  EXPECT_EQ(4, SimpleInvoke(lib, "mainInt"));
 }
 
 // Verifies that we clear the ICs for the functions live on the stack in a way

runtime/vm/kernel_isolate.cc

@@ -988,9 +988,6 @@ class KernelCompilationRequest : public ValueObject {
     if (result_.status == Dart_KernelCompilationStatus_Ok) {
       LoadKernelFromResponse(response[1]);
     } else {
-      if (result_.status == Dart_KernelCompilationStatus_Error) {
-        LoadKernelFromResponse(response[2]);
-      }
       // This is an error.
       ASSERT(response[1]->type == Dart_CObject_kString);
       result_.error = Utils::StrDup(response[1]->value.as_string);

runtime/vm/source_report_test.cc

@@ -10,15 +10,11 @@ namespace dart {
 
 #ifndef PRODUCT
 
-static ObjectPtr ExecuteScript(const char* script, bool allow_errors = false) {
+static ObjectPtr ExecuteScript(const char* script) {
   Dart_Handle lib;
   {
     TransitionVMToNative transition(Thread::Current());
-    if (allow_errors) {
-      lib = TestCase::LoadTestScriptWithErrors(script, nullptr);
-    } else {
-      lib = TestCase::LoadTestScript(script, nullptr);
-    }
+    lib = TestCase::LoadTestScript(script, nullptr);
     EXPECT_VALID(lib);
     Dart_Handle result = Dart_Invoke(lib, NewString("main"), 0, nullptr);
     EXPECT_VALID(result);
@@ -315,59 +311,6 @@ ISOLATE_UNIT_TEST_CASE(SourceReport_Coverage_UnusedClass_ForceCompile) {
       buffer);
 }
 
-ISOLATE_UNIT_TEST_CASE(SourceReport_Coverage_UnusedClass_ForceCompileError) {
-  // WARNING: This MUST be big enough for the serialized JSON string.
-  const int kBufferSize = 1024;
-  char buffer[kBufferSize];
-  const char* kScript =
-      "helper0() {}\n"
-      "class Unused {\n"
-      "  helper1() { helper0()+ }\n"  // syntax error
-      "}\n"
-      "main() {\n"
-      "  helper0();\n"
-      "}";
-
-  Library& lib = Library::Handle();
-  lib ^= ExecuteScript(kScript, true);
-  ASSERT(!lib.IsNull());
-  const Script& script =
-      Script::Handle(lib.LookupScript(String::Handle(String::New("test-lib"))));
-
-  SourceReport report(SourceReport::kCoverage, SourceReport::kForceCompile);
-  JSONStream js;
-  js.set_id_zone(thread->isolate()->EnsureDefaultServiceIdZone());
-  report.PrintJSON(&js, script);
-  const char* json_str = js.ToCString();
-  ASSERT(strlen(json_str) < kBufferSize);
-  ElideJSONSubstring("classes", json_str, buffer);
-  ElideJSONSubstring("libraries", buffer, buffer);
-  EXPECT_STREQ(
-      "{\"type\":\"SourceReport\",\"ranges\":["
-
-      // UnusedClass has a syntax error.
-      "{\"scriptIndex\":0,\"startPos\":30,\"endPos\":53,\"compiled\":false,"
-      "\"error\":{\"type\":\"@Error\",\"_vmType\":\"LanguageError\","
-      "\"kind\":\"LanguageError\",\"id\":\"objects\\/0\\/0\","
-      "\"message\":\"'file:\\/\\/\\/test-lib': error: "
-      "\\/test-lib:3:26: "
-      "Error: This couldn't be parsed.\\n"
-      "  helper1() { helper0()+ }\\n                         ^\"}},"
-
-      // helper0 is compiled.
-      "{\"scriptIndex\":0,\"startPos\":0,\"endPos\":11,\"compiled\":true,"
-      "\"coverage\":{\"hits\":[0],\"misses\":[]}},"
-
-      // One range with two hits (main).
-      "{\"scriptIndex\":0,\"startPos\":57,\"endPos\":79,\"compiled\":true,"
-      "\"coverage\":{\"hits\":[57,68],\"misses\":[]}}],"
-
-      // Only one script in the script table.
-      "\"scripts\":[{\"type\":\"@Script\",\"fixedId\":true,\"id\":\"\","
-      "\"uri\":\"file:\\/\\/\\/test-lib\",\"_kind\":\"kernel\"}]}",
-      buffer);
-}
-
 ISOLATE_UNIT_TEST_CASE(SourceReport_Coverage_LibrariesAlreadyCompiled) {
   // WARNING: This MUST be big enough for the serialized JSON string.
   const int kBufferSize = 1024;