[vm/reload] Reject failed compilation during isolate reload

This is needed so that the expression evaluation works correctly after attempt to reload to bad source code.

Bug: https://github.com/dart-lang/sdk/issues/45846
Change-Id: Ib9fccc54141433bc662cbca923037fd12616ccd2
TEST=bad_reload_test
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/278664
Reviewed-by: Jens Johansen <jensj@google.com>
Commit-Queue: Alexander Aprelev <aam@google.com>

pkg/vm/bin/kernel_service.dart

@@ -66,6 +66,7 @@ const String packageConfigFile = '.dart_tool/package_config.json';
 //       purposes).
 //   5 - List program dependencies (for creating depfiles)
 //   6 - Isolate shutdown that potentially should result in compiler cleanup.
+//   7 - Reject last compilation result.
 const int kCompileTag = 0;
 const int kUpdateSourcesTag = 1;
 const int kAcceptTag = 2;
@@ -73,6 +74,7 @@ const int kTrainTag = 3;
 const int kCompileExpressionTag = 4;
 const int kListDependenciesTag = 5;
 const int kNotifyIsolateShutdownTag = 6;
+const int kRejectTag = 7;
 
 bool allowDartInternalImport = false;
 
@@ -103,7 +105,9 @@ CompilerOptions setupCompilerOptions(
     ..packagesFileUri = packagesUri
     ..sdkSummary = platformKernelPath
     ..verbose = verbose
-    ..omitPlatform = true
+    ..omitPlatform = false // so that compilation results can be rejected,
+                           // which potentially is only relevant for
+                           // incremental, rather than single-shot compilter
     ..explicitExperimentalFlags = parseExperimentalFlags(
         parseExperimentalArguments(expFlags), onError: (msg) {
       errorsPlain.add(msg);
@@ -339,6 +343,7 @@ class IncrementalCompilerWrapper extends Compiler {
   }
 
   void accept() => generator!.accept();
+  Future<void> reject() async => generator!.reject();
   void invalidate(Uri uri) => generator!.invalidate(uri);
 
   Future<IncrementalCompilerWrapper> clone(int isolateGroupId) async {
@@ -406,7 +411,7 @@ class SingleShotCompilerWrapper extends Compiler {
 
     Set<Library> loadedLibraries = createLoadedLibrariesSet(
         compilerResult.loadedComponents, compilerResult.sdkComponent,
-        includePlatform: !options.omitPlatform);
+        includePlatform: false);
 
     return new CompilerResult(compilerResult.component, loadedLibraries,
         compilerResult.classHierarchy, compilerResult.coreTypes);
@@ -814,14 +819,21 @@ Future _processLoadRequest(request) async {
     updateSources(compiler as IncrementalCompilerWrapper, sourceFiles);
     port.send(new CompilationResult.ok(null).toResponse());
     return;
-  } else if (tag == kAcceptTag) {
+  } else if (tag == kAcceptTag || tag == kRejectTag) {
     assert(
-        incremental, "Incremental compiler required for use of 'kAcceptTag'");
+        incremental,
+        "Incremental compiler required for use of 'kAcceptTag' or "
+        "'kRejectTag");
     compiler = lookupIncrementalCompiler(isolateGroupId);
     // There are unit tests that invoke the IncrementalCompiler directly and
     // request a reload, meaning that we won't have a compiler for this isolate.
     if (compiler != null) {
-      (compiler as IncrementalCompilerWrapper).accept();
+      final wrapper = compiler as IncrementalCompilerWrapper;
+      if (tag == kAcceptTag) {
+        wrapper.accept();
+      } else {
+        await wrapper.reject();
+      }
     }
     port.send(new CompilationResult.ok(null).toResponse());
     return;

pkg/vm/lib/incremental_compiler.dart

@@ -29,29 +29,36 @@ class IncrementalCompiler {
   bool initialized = false;
   bool fullComponent = false;
   Uri? initializeFromDillUri;
-  List<Uri> _entryPoints;
+  List<Uri> _latestAcceptedEntryPoints;
+  List<Uri> _latestUsedEntryPoints;
   final bool forExpressionCompilationOnly;
 
-  List<Uri> get entryPoints => _entryPoints;
+  List<Uri> get entryPoints => _latestAcceptedEntryPoints;
   IncrementalKernelGenerator get generator => _generator;
   IncrementalCompilerResult? get lastKnownGoodResult => _lastKnownGood;
 
-  IncrementalCompiler(this._compilerOptions, this._entryPoints,
+  IncrementalCompiler(this._compilerOptions, this._latestAcceptedEntryPoints,
       {this.initializeFromDillUri, bool incrementalSerialization = true})
-      : forExpressionCompilationOnly = false {
+      : forExpressionCompilationOnly = false,
+        _latestUsedEntryPoints = _latestAcceptedEntryPoints {
     if (incrementalSerialization) {
       incrementalSerializer = new IncrementalSerializer();
     }
-    _generator = new IncrementalKernelGenerator(_compilerOptions, _entryPoints,
-        initializeFromDillUri, false, incrementalSerializer);
+    _generator = new IncrementalKernelGenerator(
+        _compilerOptions,
+        _latestAcceptedEntryPoints,
+        initializeFromDillUri,
+        false,
+        incrementalSerializer);
     _pendingDeltas = <IncrementalCompilerResult>[];
   }
 
-  IncrementalCompiler.forExpressionCompilationOnly(
-      Component component, this._compilerOptions, this._entryPoints)
-      : forExpressionCompilationOnly = true {
+  IncrementalCompiler.forExpressionCompilationOnly(Component component,
+      this._compilerOptions, this._latestAcceptedEntryPoints)
+      : forExpressionCompilationOnly = true,
+        _latestUsedEntryPoints = _latestAcceptedEntryPoints {
     _generator = new IncrementalKernelGenerator.forExpressionCompilationOnly(
-        _compilerOptions, _entryPoints, component);
+        _compilerOptions, _latestAcceptedEntryPoints, component);
     _pendingDeltas = <IncrementalCompilerResult>[];
   }
 
@@ -64,9 +71,9 @@ class IncrementalCompiler {
     final task = new TimelineTask();
     try {
       task.start("IncrementalCompiler.compile");
-      _entryPoints = entryPoints ?? _entryPoints;
+      _latestUsedEntryPoints = entryPoints ?? _latestAcceptedEntryPoints;
       IncrementalCompilerResult compilerResult = await _generator.computeDelta(
-          entryPoints: _entryPoints, fullComponent: fullComponent);
+          entryPoints: _latestUsedEntryPoints, fullComponent: fullComponent);
       initialized = true;
       fullComponent = false;
       _pendingDeltas.add(compilerResult);
@@ -150,6 +157,8 @@ class IncrementalCompiler {
       lastKnownGood.component.addMetadataRepository(repo);
     }
     _pendingDeltas.clear();
+
+    _latestAcceptedEntryPoints = _latestUsedEntryPoints;
   }
 
   /// This lets incremental compiler know that results of last [compile] call
@@ -174,9 +183,13 @@ class IncrementalCompiler {
     // loading from it below is basically nonsense, it will just start over).
     _lastKnownGood?.component.relink();
 
-    _generator = new IncrementalKernelGenerator.fromComponent(_compilerOptions,
-        _entryPoints, _lastKnownGood?.component, false, incrementalSerializer);
-    await _generator.computeDelta(entryPoints: _entryPoints);
+    _generator = new IncrementalKernelGenerator.fromComponent(
+        _compilerOptions,
+        _latestAcceptedEntryPoints,
+        _lastKnownGood?.component,
+        false,
+        incrementalSerializer);
+    await _generator.computeDelta(entryPoints: _latestAcceptedEntryPoints);
   }
 
   /// This tells incremental compiler that it needs rescan [uri] file during

pkg/vm/test/incremental_compiler_test.dart

@@ -1240,6 +1240,85 @@ main() {
         expect(lrc.librariesReferenced, equals(<Library>{barLib}));
       }
     });
+
+    test('compile, recompile with new entry, reject, expression compilation',
+        () async {
+      var v1Uri = Uri.file('${mytest.path}/v1.dart');
+      new File(v1Uri.toFilePath()).writeAsStringSync("""import 'dart:isolate';
+          test() => 'apple';
+          main() {
+            RawReceivePort keepAlive = new RawReceivePort();
+            print('spawned isolate running');
+          }
+          """);
+
+      var v2Uri = Uri.file('${mytest.path}/v2.dart');
+      new File(v2Uri.toFilePath()).writeAsStringSync("""import 'dart:isolate';
+          import 'library_isnt_here_man';
+
+          test() => 'apple';
+
+          main() {
+            RawReceivePort keepAlive = new RawReceivePort();
+            print('spawned isolate running');
+          }
+          """);
+
+      CompilerOptions optionsModified = getFreshOptions()
+        ..onDiagnostic = (DiagnosticMessage message) {
+          // sometimes expected on this one.
+          print(message.ansiFormatted);
+        };
+      IncrementalCompiler compiler =
+          new IncrementalCompiler(optionsModified, [v1Uri]);
+
+      print("Compiling v1 uri (should be ok)");
+      await compiler.compile(entryPoints: [v1Uri]);
+
+      print("Accepting.");
+      compiler.accept();
+      {
+        print("Compiling expression.");
+        Procedure? procedure = await compiler.compileExpression(
+            'test()',
+            <String>[],
+            <String>[],
+            <String>[],
+            <String>[],
+            <String>[],
+            v1Uri.toString(),
+            null,
+            null,
+            false);
+        expect(procedure, isNotNull);
+        ReturnStatement returnStmt =
+            procedure!.function.body as ReturnStatement;
+        expect(returnStmt.expression, isA<StaticInvocation>());
+      }
+
+      print("Compiling v2 uri (with error)");
+      await compiler.compile(entryPoints: [v2Uri]);
+      print("Rejecting.");
+      await compiler.reject();
+      {
+        print("Compiling expression.");
+        Procedure? procedure = await compiler.compileExpression(
+            'test()',
+            <String>[],
+            <String>[],
+            <String>[],
+            <String>[],
+            <String>[],
+            v1Uri.toString(),
+            null,
+            null,
+            false);
+        expect(procedure, isNotNull);
+        ReturnStatement returnStmt =
+            procedure!.function.body as ReturnStatement;
+        expect(returnStmt.expression, isA<StaticInvocation>());
+      }
+    });
   });
 
   group('expression evaluation', () {

runtime/observatory/tests/service/bad_reload_test.dart

@@ -35,19 +35,13 @@ Future<String> invokeTest(Isolate isolate) async {
   await isolate.reload();
   Library lib = isolate.rootLibrary;
   await lib.load();
-  Instance result = await lib.evaluate('test()') as Instance;
+  final resultOrError = await lib.evaluate('test()');
+  print('resultOrError: $resultOrError');
+  Instance result = resultOrError as Instance;
   expect(result.isString, isTrue);
   return result.valueAsString as String;
 }
 
-Future<void> invokeTestWithError(Isolate isolate) async {
-  await isolate.reload();
-  Library lib = isolate.rootLibrary;
-  await lib.load();
-  final result = await lib.evaluate('test()');
-  expect(result.isError, isTrue);
-}
-
 var tests = <IsolateTest>[
   // Stopped at 'debugger' statement.
   hasStoppedAtBreakpoint,
@@ -82,7 +76,7 @@ var tests = <IsolateTest>[
     expect(reasonForCancelling['type'], equals('ReasonForCancelling'));
     expect(reasonForCancelling['message'], contains('library_isnt_here_man'));
 
-    await invokeTestWithError(spawnedIsolate);
+    await invokeTest(spawnedIsolate);
   }
 ];
 

runtime/observatory_2/tests/service_2/bad_reload_test.dart

@@ -35,17 +35,11 @@ Future<String> invokeTest(Isolate isolate) async {
   await isolate.reload();
   Library lib = isolate.rootLibrary;
   await lib.load();
-  Instance result = await lib.evaluate('test()');
+  final resultOrError = await lib.evaluate('test()');
+  print('resultOrError: $resultOrError');
+  Instance result = resultOrError as Instance;
   expect(result.isString, isTrue);
-  return result.valueAsString;
-}
-
-Future<void> invokeTestWithError(Isolate isolate) async {
-  await isolate.reload();
-  Library lib = isolate.rootLibrary;
-  await lib.load();
-  final result = await lib.evaluate('test()');
-  expect(result.isError, isTrue);
+  return result.valueAsString as String;
 }
 
 var tests = <IsolateTest>[
@@ -82,7 +76,7 @@ var tests = <IsolateTest>[
     expect(reasonForCancelling['type'], equals('ReasonForCancelling'));
     expect(reasonForCancelling['message'], contains('library_isnt_here_man'));
 
-    await invokeTestWithError(spawnedIsolate);
+    await invokeTest(spawnedIsolate);
   }
 ];
 

runtime/vm/dart_api_impl.cc

@@ -6139,14 +6139,16 @@ Dart_CompileToKernel(const char* script_uri,
       script_uri, platform_kernel, platform_kernel_size, 0, NULL,
       incremental_compile, snapshot_compile, package_config, NULL, NULL,
       verbosity);
-  if (incremental_compile && result.status == Dart_KernelCompilationStatus_Ok) {
-    Dart_KernelCompilationResult accept_result =
-        KernelIsolate::AcceptCompilation();
-    if (accept_result.status != Dart_KernelCompilationStatus_Ok) {
+  if (incremental_compile) {
+    Dart_KernelCompilationResult ack_result =
+        result.status == Dart_KernelCompilationStatus_Ok ?
+            KernelIsolate::AcceptCompilation():
+            KernelIsolate::RejectCompilation();
+    if (ack_result.status != Dart_KernelCompilationStatus_Ok) {
       FATAL1(
-          "An error occurred in the CFE while accepting the most recent"
+          "An error occurred in the CFE while acking the most recent"
           " compilation results: %s",
-          accept_result.error);
+          ack_result.error);
     }
   }
 #endif

runtime/vm/isolate_reload.cc

@@ -656,6 +656,28 @@ static ObjectPtr AcceptCompilation(Thread* thread) {
   return Object::null();
 }
 
+static ObjectPtr RejectCompilation(Thread* thread) {
+  TransitionVMToNative transition(thread);
+  Dart_KernelCompilationResult result = KernelIsolate::RejectCompilation();
+  if (result.status != Dart_KernelCompilationStatus_Ok) {
+    if (result.status != Dart_KernelCompilationStatus_MsgFailed) {
+      FATAL1(
+          "An error occurred while rejecting the most recent"
+          " compilation results: %s",
+          result.error);
+    }
+    TIR_Print(
+        "An error occurred while rejecting the most recent"
+        " compilation results: %s",
+        result.error);
+    Zone* zone = thread->zone();
+    const auto& error_str = String::Handle(zone, String::New(result.error));
+    free(result.error);
+    return ApiError::New(error_str);
+  }
+  return Object::null();
+}
+
 // If [root_script_url] is null, attempt to load from [kernel_buffer].
 bool IsolateGroupReloadContext::Reload(bool force_reload,
                                        const char* root_script_url,
@@ -711,6 +733,8 @@ bool IsolateGroupReloadContext::Reload(bool force_reload,
           AddReasonForCancelling(new Aborted(Z, error));
           ReportReasonsForCancelling();
           CommonFinalizeTail(num_old_libs_);
+
+          RejectCompilation(thread);
           return false;
         }
       }

runtime/vm/kernel_isolate.cc

@@ -52,6 +52,7 @@ DEFINE_FLAG(charp,
 //   4 - Compile expressions in context (used by expression evaluation).
 //   5 - Generate dependencies used to create a dependencies file.
 //   6 - Triggers shutdown of the kernel isolate.
+//   7 - Reject last compilation result.
 const int KernelIsolate::kCompileTag = 0;
 const int KernelIsolate::kUpdateSourcesTag = 1;
 const int KernelIsolate::kAcceptTag = 2;
@@ -59,6 +60,7 @@ const int KernelIsolate::kTrainTag = 3;
 const int KernelIsolate::kCompileExpressionTag = 4;
 const int KernelIsolate::kListDependenciesTag = 5;
 const int KernelIsolate::kNotifyIsolateShutdown = 6;
+const int KernelIsolate::kRejectTag = 7;
 
 const char* KernelIsolate::kName = DART_KERNEL_ISOLATE_NAME;
 Dart_IsolateGroupCreateCallback KernelIsolate::create_group_callback_ = NULL;
@@ -1186,6 +1188,24 @@ Dart_KernelCompilationResult KernelIsolate::AcceptCompilation() {
       Dart_KernelCompilationVerbosityLevel_Error);
 }
 
+Dart_KernelCompilationResult KernelIsolate::RejectCompilation() {
+  // This must be the main script to be loaded. Wait for Kernel isolate
+  // to finish initialization.
+  Dart_Port kernel_port = WaitForKernelPort();
+  if (kernel_port == ILLEGAL_PORT) {
+    Dart_KernelCompilationResult result = {};
+    result.status = Dart_KernelCompilationStatus_MsgFailed;
+    result.error = Utils::StrDup("Error while initializing Kernel isolate");
+    return result;
+  }
+
+  KernelCompilationRequest request;
+  return request.SendAndWaitForResponse(
+      kRejectTag, kernel_port, NULL, NULL, 0, 0, NULL, true, false, NULL, NULL,
+      NULL, experimental_flags_, NULL,
+      Dart_KernelCompilationVerbosityLevel_Error);
+}
+
 Dart_KernelCompilationResult KernelIsolate::CompileExpressionToKernel(
     const uint8_t* platform_kernel,
     intptr_t platform_kernel_size,

runtime/vm/kernel_isolate.h

@@ -31,6 +31,7 @@ class KernelIsolate : public AllStatic {
   static const int kCompileExpressionTag;
   static const int kListDependenciesTag;
   static const int kNotifyIsolateShutdown;
+  static const int kRejectTag;
 
   static void InitializeState();
   static bool Start();
@@ -62,6 +63,7 @@ class KernelIsolate : public AllStatic {
                                const char* original_working_directory);
 
   static Dart_KernelCompilationResult AcceptCompilation();
+  static Dart_KernelCompilationResult RejectCompilation();
   static Dart_KernelCompilationResult UpdateInMemorySources(
       int source_files_count,
       Dart_SourceFile source_files[]);