[ddc,frontend_server] Fix expression compilation after recompile

nshahan · Commit Queue · commit 559b8552a8ae · 2025-06-24T14:14:42.000-07:00
Update the last known good component after a recompile reject back to the expected state before the recompile. Adds tests for expression compilation after recompile accept and recompile reject cases with `--target=dartdevc`. Similar to existing tests for the VM. Removes calls to `_generator.accept()` and `component.computeCanonicalNames()` from `compileExpressionToJs()`. These were added very early in the prototype implementation and it is no longer clear why they would be needed. Added some additional test cases for expression compilation involving import resolution because the change that originally added the `component.computeCanonicalNames()` call implies it was needed for that reason. See: https://dart-review.googlesource.com/c/sdk/+/138010 TEST=pkg/frontend_server/test/frontend_server_test.dart,pkg/dev_compiler/test/expression_compiler/expression_compiler_test.dart Change-Id: I9c04bd46e56b33dc654d00fb330de29c3c643a5b Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/434522 Reviewed-by: Alexander Markov <alexmarkov@google.com> Commit-Queue: Nicholas Shahan <nshahan@google.com> Reviewed-by: Nate Biggs <natebiggs@google.com> Reviewed-by: Jens Johansen <jensj@google.com>
diff --git a/pkg/dev_compiler/test/expression_compiler/expression_compiler_suite.dart b/pkg/dev_compiler/test/expression_compiler/expression_compiler_suite.dart
@@ -9,45 +9,72 @@ import 'package:test/test.dart';
 import '../shared_test_options.dart';
 import 'test_compiler.dart';
 
+typedef AdditionalLibrary = ({String name, String source});
+
 class ExpressionCompilerTestDriver {
   final SetupCompilerOptions setup;
-  late Directory testDir;
+  late Directory _testRootDirectory;
   String source;
   late Uri input;
   late Uri output;
   late Uri packages;
   late int line;
+  final List<AdditionalLibrary> additionalLibraries;
 
-  ExpressionCompilerTestDriver(this.setup, this.source) {
+  ExpressionCompilerTestDriver(
+    this.setup,
+    this.source, {
+    this.additionalLibraries = const [],
+  }) {
     line = _getEvaluationLine(source);
     var systemTempDir = Directory.systemTemp;
-    testDir = systemTempDir.createTempSync('foo bar');
+    _testRootDirectory = systemTempDir.createTempSync('expression_eval_test');
+    var testPackageDirectory = Directory.fromUri(
+      _testRootDirectory.uri.resolve('test_package/'),
+    );
 
-    output = testDir.uri.resolve('test.js');
-    input = testDir.uri.resolve('test.dart');
+    output = testPackageDirectory.uri.resolve('test.js');
+    input = testPackageDirectory.uri.resolve('test.dart');
     File.fromUri(input)
-      ..createSync()
+      ..createSync(recursive: true)
       ..writeAsStringSync(source);
 
-    packages = testDir.uri.resolve('package_config.json');
+    packages = _testRootDirectory.uri.resolve('package_config.json');
+    var packageDescriptors = [_createPackageDescriptor('test_package')];
+    for (var library in additionalLibraries) {
+      var name = library.name;
+      var source = library.source;
+      var additionalPackageDirectory = Directory.fromUri(
+        _testRootDirectory.uri.resolve('$name/'),
+      );
+      var additionalFile = additionalPackageDirectory.uri.resolve('$name.dart');
+      File.fromUri(additionalFile)
+        ..createSync(recursive: true)
+        ..writeAsStringSync(source);
+      packageDescriptors.add(_createPackageDescriptor(name));
+    }
     File.fromUri(packages)
       ..createSync()
       ..writeAsStringSync('''
       {
         "configVersion": 2,
         "packages": [
-          {
-            "name": "foo",
-            "rootUri": "./",
-            "packageUri": "./"
-          }
+          ${packageDescriptors.join(',\n          ')}
         ]
       }
       ''');
   }
 
+  /// Creates a `packages_config.json` entry for a test package called [name].
+  static String _createPackageDescriptor(String name) =>
+      '''{
+            "name": "$name",
+            "rootUri": "./$name",
+            "packageUri": "./"
+          }''';
+
   void delete() {
-    testDir.deleteSync(recursive: true);
+    _testRootDirectory.deleteSync(recursive: true);
   }
 
   Future<TestExpressionCompiler> createCompiler() =>
diff --git a/pkg/dev_compiler/test/expression_compiler/expression_compiler_test.dart b/pkg/dev_compiler/test/expression_compiler/expression_compiler_test.dart
@@ -92,7 +92,7 @@ void runTests(SetupCompilerOptions setup) {
     });
   });
 
-  group('Expression compiler import tests', () {
+  group('Expression compiler dart: import tests', () {
     var source = '''
       import 'dart:io' show Directory;
       import 'dart:io' as p;
@@ -146,4 +146,65 @@ void runTests(SetupCompilerOptions setup) {
       },
     );
   });
+  group('Expression compiler package: import tests', () {
+    var source = '''
+      import 'package:a/a.dart' show topLevelMethod;
+      import 'package:a/a.dart' as prefix;
+      import 'package:b/b.dart' as prefix;
+
+      main() {
+        print(topLevelMethod(99));
+        print(prefix.topLevelMethod(99));
+        print(prefix.anotherTopLevelMethod('hello'));
+      }
+
+      void foo() {
+        // Breakpoint
+      }
+      ''';
+
+    var a = 'bool topLevelMethod(int i) => i.isEven;';
+    var b = 'int anotherTopLevelMethod(String s) => s.length;';
+
+    late ExpressionCompilerTestDriver driver;
+
+    setUp(() {
+      driver = ExpressionCompilerTestDriver(
+        setup,
+        source,
+        additionalLibraries: [(name: 'a', source: a), (name: 'b', source: b)],
+      );
+    });
+
+    tearDown(() {
+      driver.delete();
+    });
+
+    test('expression referencing unnamed import', () async {
+      await driver.check(
+        scope: <String, String>{},
+        expression: 'topLevelMethod(99)',
+        expectedResult: contains('return a.topLevelMethod(99);'),
+      );
+    });
+
+    test('expression referencing named import', () async {
+      await driver.check(
+        scope: <String, String>{},
+        expression: 'prefix.topLevelMethod(99)',
+        expectedResult: contains('return a.topLevelMethod(99);'),
+      );
+    });
+
+    test(
+      'expression referencing another library with the same named import',
+      () async {
+        await driver.check(
+          scope: <String, String>{},
+          expression: 'prefix.anotherTopLevelMethod("hello")',
+          expectedResult: contains('return b.anotherTopLevelMethod("hello")'),
+        );
+      },
+    );
+  });
 }
diff --git a/pkg/frontend_server/lib/frontend_server.dart b/pkg/frontend_server/lib/frontend_server.dart
@@ -1146,9 +1146,7 @@ class FrontendCompiler implements CompilerInterface {
       Map<String, String> jsModules,
       Map<String, String> jsFrameValues,
       String expression) async {
-    _generator.accept();
     errors.clear();
-
     if (_bundler == null) {
       reportError('JavaScript bundler is null');
       return;
@@ -1166,9 +1164,7 @@ class FrontendCompiler implements CompilerInterface {
     final Compiler kernel2jsCompiler = cachedProgramCompilers[libraryUri]!;
     IncrementalCompilerResult compilerResult = _generator.lastKnownGoodResult!;
     Component component = compilerResult.component;
-    component.computeCanonicalNames();
-
-    _processedOptions.ticker.logMs('Computed component');
+    _processedOptions.ticker.logMs('Retrieved cached component');
 
     ModuleFormat moduleFormat =
         parseModuleFormat(_options['dartdevc-module-format'] as String);
diff --git a/pkg/frontend_server/test/frontend_server_test.dart b/pkg/frontend_server/test/frontend_server_test.dart
@@ -1095,6 +1095,204 @@ extension type Foo(int value) {
       frontendServer.close();
     }, timeout: new Timeout.factor(100));
 
+    test('compile expression to JavaScript when delta is rejected', () async {
+      File fileLib = new File('${tempDir.path}/lib.dart')..createSync();
+      fileLib.writeAsStringSync("foo() => 42;\n");
+      File file = new File('${tempDir.path}/foo.dart')..createSync();
+      // Initial compile contains a generic class so an edit can be made later
+      // that is known to be rejected.
+      file.writeAsStringSync(
+          "import 'lib.dart'; class A<T, S> {} main1() => print(foo);\n");
+      File packageConfig =
+          new File('${tempDir.path}/.dart_tool/package_config.json')
+            ..createSync(recursive: true)
+            ..writeAsStringSync('''
+{
+  "configVersion": 2,
+  "packages": [
+    {
+      "name": "hello",
+      "rootUri": "../",
+      "packageUri": "./"
+    }
+  ]
+}
+''');
+      File dillFile = new File('${tempDir.path}/app.dill');
+      expect(dillFile.existsSync(), equals(false));
+      final List<String> args = <String>[
+        '--sdk-root=${sdkRoot.toFilePath()}',
+        '--incremental',
+        '--platform=${ddcPlatformKernel.path}',
+        '--output-dill=${dillFile.path}',
+        '--target=dartdevc',
+        // TODO(nshahan): Remove these two flags when library bundle format is
+        // the default without passing --dartdevc-canary.
+        '--dartdevc-module-format=ddc',
+        '--dartdevc-canary',
+        '--packages=${packageConfig.path}',
+      ];
+
+      FrontendServer frontendServer = new FrontendServer();
+      final Future<int> result = frontendServer.open(args);
+      frontendServer.compile(file.path);
+      int count = 0;
+      frontendServer.listen((Result compiledResult) {
+        CompilationResult result =
+            new CompilationResult.parse(compiledResult.status);
+        if (count == 0) {
+          // First request was to 'compile', which resulted in full kernel file.
+          expect(result.errorsCount, 0);
+          expect(dillFile.existsSync(), equals(true));
+          expect(result.filename, dillFile.path);
+          frontendServer.accept();
+
+          frontendServer.compileExpressionToJs(
+            expression: 'main1',
+            libraryUri: 'package:hello/foo.dart',
+            line: 1,
+            column: 1,
+          );
+          count += 1;
+        } else if (count == 1) {
+          // Second request was to 'compile-expression', which resulted in
+          // kernel file with a function that wraps compiled expression.
+          expect(result.errorsCount, 0);
+          File outputFile = new File(result.filename);
+          expect(outputFile.existsSync(), equals(true));
+          expect(outputFile.lengthSync(), isPositive);
+
+          // Removing a generic class type argument is known to cause a reload
+          // rejection.
+          file.writeAsStringSync(
+              "import 'lib.dart'; class A<T> {} main() => foo();\n");
+
+          frontendServer.recompile(file.uri, entryPoint: file.path);
+          count += 1;
+        } else if (count == 2) {
+          // Third request was to recompile the script after renaming a
+          // function.
+          expect(result.errorsCount, 1);
+          frontendServer.reject();
+          count += 1;
+        } else if (count == 3) {
+          // Fourth request was to reject the compilation results.
+          expect(result.errorsCount, 0);
+          frontendServer.compileExpressionToJs(
+            expression: 'main1',
+            libraryUri: 'package:hello/foo.dart',
+            line: 1,
+            column: 1,
+          );
+          count += 1;
+        } else {
+          expect(count, 4);
+          // Fifth request was to 'compile-expression' that references original
+          // function, which should still be successful.
+          expect(result.errorsCount, 0);
+          frontendServer.quit();
+        }
+      });
+
+      expect(await result, 0);
+      frontendServer.close();
+    }, timeout: new Timeout.factor(100));
+
+    test('compile expression to JavaScript when delta is accepted', () async {
+      File fileLib = new File('${tempDir.path}/lib.dart')..createSync();
+      fileLib.writeAsStringSync("foo() => 42;\n");
+      File file = new File('${tempDir.path}/foo.dart')..createSync();
+      file.writeAsStringSync("import 'lib.dart'; main1() => print(foo);\n");
+      File packageConfig =
+          new File('${tempDir.path}/.dart_tool/package_config.json')
+            ..createSync(recursive: true)
+            ..writeAsStringSync('''
+{
+  "configVersion": 2,
+  "packages": [
+    {
+      "name": "hello",
+      "rootUri": "../",
+      "packageUri": "./"
+    }
+  ]
+}
+''');
+      File dillFile = new File('${tempDir.path}/app.dill');
+      expect(dillFile.existsSync(), equals(false));
+      final List<String> args = <String>[
+        '--sdk-root=${sdkRoot.toFilePath()}',
+        '--incremental',
+        '--platform=${ddcPlatformKernel.path}',
+        '--output-dill=${dillFile.path}',
+        '--target=dartdevc',
+        // TODO(nshahan): Remove these two flags when library bundle format is
+        // the default without passing --dartdevc-canary.
+        '--dartdevc-module-format=ddc',
+        '--dartdevc-canary',
+        '--packages=${packageConfig.path}',
+      ];
+
+      FrontendServer frontendServer = new FrontendServer();
+      final Future<int> result = frontendServer.open(args);
+      frontendServer.compile(file.path);
+      int count = 0;
+      frontendServer.listen((Result compiledResult) {
+        CompilationResult result =
+            new CompilationResult.parse(compiledResult.status);
+        if (count == 0) {
+          // First request was to 'compile', which resulted in full kernel file.
+          expect(result.errorsCount, 0);
+          expect(dillFile.existsSync(), equals(true));
+          expect(result.filename, dillFile.path);
+          frontendServer.accept();
+
+          frontendServer.compileExpressionToJs(
+            expression: 'main1',
+            libraryUri: 'package:hello/foo.dart',
+            line: 1,
+            column: 1,
+          );
+          count += 1;
+        } else if (count == 1) {
+          // Second request was to 'compile-expression', which resulted in
+          // kernel file with a function that wraps compiled expression.
+          expect(result.errorsCount, 0);
+          File outputFile = new File(result.filename);
+          expect(outputFile.existsSync(), equals(true));
+          expect(outputFile.lengthSync(), isPositive);
+
+          file.writeAsStringSync("import 'lib.dart'; main() => foo();\n");
+          frontendServer.recompile(file.uri, entryPoint: file.path);
+          count += 1;
+        } else if (count == 2) {
+          // Third request was to recompile the script after renaming a
+          // function.
+          expect(result.errorsCount, 0);
+          File dillIncFile = new File('${dillFile.path}.incremental.dill');
+          expect(dillIncFile.existsSync(), equals(true));
+          expect(result.filename, dillIncFile.path);
+          frontendServer.accept();
+          frontendServer.compileExpressionToJs(
+            expression: 'main',
+            libraryUri: 'package:hello/foo.dart',
+            line: 1,
+            column: 1,
+          );
+          count += 1;
+        } else {
+          expect(count, 3);
+          // Fourth request was to 'compile-expression' that references the new
+          // function, which should still be successful.
+          expect(result.errorsCount, 0);
+          frontendServer.quit();
+        }
+      });
+
+      expect(await result, 0);
+      frontendServer.close();
+    }, timeout: new Timeout.factor(100));
+
     test('reject - recreate issue 55357', () async {
       File file = new File('${tempDir.path}/foo.dart')..createSync();
       file.writeAsStringSync(
@@ -2342,6 +2540,8 @@ e() {
             '--output-dill=${dillFile.path}',
             '--target=dartdevc',
             // Errors are only emitted with the DDC library bundle format.
+            // TODO(nshahan): Remove these two flags when library bundle format
+            // is the default without passing --dartdevc-canary.
             '--dartdevc-module-format=ddc',
             '--dartdevc-canary',
             '--packages=${packageConfig.path}',
diff --git a/pkg/vm/lib/incremental_compiler.dart b/pkg/vm/lib/incremental_compiler.dart
