Skip to content

Commit

Permalink
Reland "[frontend_server] Soft-deprecate moduleName for compileExpres…
Browse files Browse the repository at this point in the history 
…sionToJS"

This reverts commit 817306a.

Reason for revert: Later patchset includes fix for `frontend_server_circular_evaluate_test`.

The fix is to cache the bundle/program compiler for all the
libraries in the strongly connected component, and not just
for the initial one that determines the component uri.

Original change's description:
> Revert "[frontend_server] Soft-deprecate moduleName for compileExpressionToJS"
>
> This reverts commit 294a50f.
>
> Reason for revert: This breaks `frontend_server_circular_evaluate_test` in DWDS.
>
> Original change's description:
> > [frontend_server] Soft-deprecate moduleName for compileExpressionToJS
> >
> > #58265
> >
> > The DDC library bundle format does not give names to modules.
> > Therefore the frontend server should try and find the compiler
> > associated with the library and not the module to be consistent.
> > This then means that moduleName becomes entirely unused, and
> > therefore we can soft-deprecate it.
> >
> > Change-Id: I241c63a346d046405599384409a373ab12be2654
> > Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/396102
> > Commit-Queue: Srujan Gaddam <[email protected]>
> > Reviewed-by: Johnni Winther <[email protected]>
>
> Change-Id: I74b096f7ebc322c9d4428d236ab226ef1adcb6b9
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/396567
> Reviewed-by: Nicholas Shahan <[email protected]>
> Reviewed-by: Johnni Winther <[email protected]>
> Bot-Commit: Rubber Stamper <[email protected]>
> Commit-Queue: Srujan Gaddam <[email protected]>

Change-Id: I45852c29a0b5735976f6a5f649f3ee84b26ef76c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/396572
Reviewed-by: Nicholas Shahan <[email protected]>
Reviewed-by: Johnni Winther <[email protected]>
Commit-Queue: Srujan Gaddam <[email protected]>
  • Loading branch information
@srujzs
srujzs authored and Commit Queue committed Nov 25, 2024
1 parent 6a1d3de commit 32565d1
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 60 deletions.
31 changes: 14 additions & 17 deletions pkg/frontend_server/lib/frontend_server.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,7 @@ abstract class CompilerInterface {
bool isStatic);

/// Compiles [expression] in library [libraryUri] and file [scriptUri]
/// at [line]:[column] to JavaScript in [moduleName].
/// at [line]:[column] to JavaScript.
///
/// [libraryUri] and [scriptUri] can be the same, but if for instance
/// evaluating expressions in a part file the [libraryUri] will be the uri of
Expand All @@ -368,7 +368,6 @@ abstract class CompilerInterface {
/// expression.
///
/// Example values of parameters:
/// [moduleName] is of the form '/packages/hello_world_main.dart'
/// [jsFrameValues] is a map from js variable name to its primitive value
/// or another variable name, for example
/// { 'x': '1', 'y': 'y', 'o': 'null' }
Expand All @@ -383,7 +382,6 @@ abstract class CompilerInterface {
int column,
Map<String, String> jsModules,
Map<String, String> jsFrameValues,
String moduleName,
String expression);

/// Communicates an error [msg] to the client.
Expand Down Expand Up @@ -1107,10 +1105,10 @@ class FrontendCompiler implements CompilerInterface {
}
}

/// Program compilers per module.
/// Mapping of libraries to their program compiler.
///
/// Produced during initial compilation of the module to JavaScript,
/// cached to be used for expression compilation in [compileExpressionToJs].
/// Produced during initial compilation of the component to JavaScript, cached
/// to be used for expression compilation in [compileExpressionToJs].
final Map<String, Compiler> cachedProgramCompilers = {};

@override
Expand All @@ -1121,7 +1119,6 @@ class FrontendCompiler implements CompilerInterface {
int column,
Map<String, String> jsModules,
Map<String, String> jsFrameValues,
String moduleName,
String expression) async {
_generator.accept();
errors.clear();
Expand All @@ -1130,18 +1127,17 @@ class FrontendCompiler implements CompilerInterface {
reportError('JavaScript bundler is null');
return;
}
if (!cachedProgramCompilers.containsKey(moduleName)) {
reportError('Cannot find kernel2js compiler for $moduleName.');
if (!cachedProgramCompilers.containsKey(libraryUri)) {
reportError('Cannot find kernel2js compiler for $libraryUri.');
return;
}

final String boundaryKey = generateV4UUID();
_outputStream.writeln('result $boundaryKey');

_processedOptions.ticker
.logMs('Compiling expression to JavaScript in $moduleName');
.logMs('Compiling expression to JavaScript in $libraryUri');

final Compiler kernel2jsCompiler = cachedProgramCompilers[moduleName]!;
final Compiler kernel2jsCompiler = cachedProgramCompilers[libraryUri]!;
IncrementalCompilerResult compilerResult = _generator.lastKnownGoodResult!;
Component component = compilerResult.component;
component.computeCanonicalNames();
Expand Down Expand Up @@ -1370,7 +1366,6 @@ class _CompileExpressionToJsRequest {
late int column;
Map<String, String> jsModules = <String, String>{};
Map<String, String> jsFrameValues = <String, String>{};
late String moduleName;
late String expression;
}

Expand Down Expand Up @@ -1594,7 +1589,9 @@ StreamSubscription<String> listenAndCompile(CompilerInterface compiler,
}
break;
case _State.COMPILE_EXPRESSION_TO_JS_MODULENAME:
compileExpressionToJsRequest.moduleName = string;
// TODO(https://github.com/dart-lang/sdk/issues/58265): `moduleName` is
// soft-deprecated, so we still need to consume the string value, but it
// is unused.
state = _State.COMPILE_EXPRESSION_TO_JS_EXPRESSION;
break;
case _State.COMPILE_EXPRESSION_TO_JS_EXPRESSION:
Expand All @@ -1606,7 +1603,6 @@ StreamSubscription<String> listenAndCompile(CompilerInterface compiler,
compileExpressionToJsRequest.column,
compileExpressionToJsRequest.jsModules,
compileExpressionToJsRequest.jsFrameValues,
compileExpressionToJsRequest.moduleName,
compileExpressionToJsRequest.expression);
state = _State.READY_FOR_INSTRUCTION;
break;
Expand Down Expand Up @@ -1744,12 +1740,14 @@ Future<void> processJsonInput(
int column = getValue<int>("column") ?? -1;
Map<String, String> jsModules = getMap("jsModules") ?? {};
Map<String, String> jsFrameValues = getMap("jsFrameValues") ?? {};
String moduleName = getValue<String>("moduleName") ?? "";

if (errorMessages.isNotEmpty) {
compiler.reportError("Errors: $errorMessages.");
return;
}
// TODO(https://github.com/dart-lang/sdk/issues/58265): `moduleName` is
// soft-deprecated, and therefore is unused.
unusedKeys.remove("moduleName");
if (unusedKeys.isNotEmpty) {
compiler.reportError("Errors: Send over unused data: $unusedKeys.");
}
Expand All @@ -1761,7 +1759,6 @@ Future<void> processJsonInput(
column,
jsModules,
jsFrameValues,
moduleName,
expression,
);
} else {
Expand Down
9 changes: 5 additions & 4 deletions pkg/frontend_server/lib/src/javascript_bundle.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ class IncrementalJavaScriptBundler {
int metadataOffset = 0;
int symbolsOffset = 0;
final Map<String, Map<String, List<int>>> manifest = {};
final Set<Uri> visited = {};
final Map<Uri, Compiler> visited = {};
final Map<String, Compiler> kernel2JsCompilers = {};

for (Library library in _currentComponent.libraries) {
Expand All @@ -189,10 +189,10 @@ class IncrementalJavaScriptBundler {
}
final Uri moduleUri =
_strongComponents.moduleAssignment[library.importUri]!;
if (visited.contains(moduleUri)) {
if (visited.containsKey(moduleUri)) {
kernel2JsCompilers[library.importUri.toString()] = visited[moduleUri]!;
continue;
}
visited.add(moduleUri);

final Component summaryComponent = _uriToComponent[moduleUri]!;

Expand Down Expand Up @@ -231,7 +231,8 @@ class IncrementalJavaScriptBundler {
final Program jsModule = compiler.emitModule(summaryComponent);

// Save program compiler to reuse for expression evaluation.
kernel2JsCompilers[moduleName] = compiler;
kernel2JsCompilers[library.importUri.toString()] = compiler;
visited[moduleUri] = compiler;

String? sourceMapBase;
if (moduleUri.isScheme('package')) {
Expand Down
45 changes: 10 additions & 35 deletions pkg/frontend_server/test/frontend_server_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -795,7 +795,6 @@ extension type Foo(int value) {
}
''');
String library = 'package:hello/foo.dart';
String module = 'packages/hello/foo.dart';

File dillFile = new File('${tempDir.path}/foo.dart.dill');
File sourceFile = new File('${dillFile.path}.sources');
Expand Down Expand Up @@ -835,7 +834,6 @@ extension type Foo(int value) {
libraryUri: library,
line: 5,
column: 3,
moduleName: module,
scriptUri: file.uri,
jsFrameValues: {"f": "42"});
count += 1;
Expand All @@ -850,7 +848,6 @@ extension type Foo(int value) {
libraryUri: library,
line: 11,
column: 5,
moduleName: module,
scriptUri: file.uri,
jsFrameValues: {r"$this": "42"});
count += 1;
Expand Down Expand Up @@ -891,7 +888,6 @@ extension type Foo(int value) {
];

String library = 'package:hello/foo.dart';
String module = 'packages/hello/foo.dart';

FrontendServer frontendServer = new FrontendServer();
Future<int> result = frontendServer.open(args);
Expand All @@ -918,11 +914,7 @@ extension type Foo(int value) {
expect(outputFile.lengthSync(), isPositive);

frontendServer.compileExpressionToJs(
expression: '',
libraryUri: library,
line: 1,
column: 1,
moduleName: module);
expression: '', libraryUri: library, line: 1, column: 1);
count += 1;
} else if (count == 2) {
// Third request is to 'compile-expression-to-js' that fails
Expand Down Expand Up @@ -2818,7 +2810,6 @@ e() {
''');

String library = 'package:hello/foo.dart';
String module = 'packages/hello/foo.dart';

File dillFile = new File('${tempDir.path}/foo.dart.dill');
File sourceFile = new File('${dillFile.path}.sources');
Expand Down Expand Up @@ -2854,11 +2845,7 @@ e() {
frontendServer.accept();

frontendServer.compileExpressionToJs(
expression: '',
libraryUri: library,
line: 2,
column: 1,
moduleName: module);
expression: '', libraryUri: library, line: 2, column: 1);
count += 1;
} else if (count == 1) {
// Second request is to 'compile-expression-to-js' that fails
Expand All @@ -2869,11 +2856,7 @@ e() {
});

frontendServer.compileExpressionToJs(
expression: '2+2',
libraryUri: library,
line: 2,
column: 1,
moduleName: module);
expression: '2+2', libraryUri: library, line: 2, column: 1);
count += 1;
} else if (count == 2) {
expect(result.errorsCount, equals(0));
Expand Down Expand Up @@ -2933,7 +2916,6 @@ e() {
''');

String library = 'package:hello/foo.dart';
String module = 'packages/hello/foo.dart';

File dillFile = new File('${tempDir.path}/foo.dart.dill');
File sourceFile = new File('${dillFile.path}.sources');
Expand Down Expand Up @@ -2979,8 +2961,7 @@ e() {
expression: 'const bool.fromEnvironment("dart.library.html")',
libraryUri: library,
line: 2,
column: 1,
moduleName: module);
column: 1);
count += 1;
} else {
expect(count, 1);
Expand Down Expand Up @@ -3030,7 +3011,6 @@ e() {
}
''');
String library = 'package:hello/foo.dart';
String module = 'packages/hello/foo.dart';

File dillFile = new File('${tempDir.path}/foo.dart.dill');
File sourceFile = new File('${dillFile.path}.sources');
Expand Down Expand Up @@ -3067,11 +3047,7 @@ e() {
frontendServer.accept();

frontendServer.compileExpressionToJs(
expression: '2+2',
libraryUri: library,
line: 2,
column: 1,
moduleName: module);
expression: '2+2', libraryUri: library, line: 2, column: 1);
count += 1;
} else if (count == 1) {
expect(result.errorsCount, equals(0));
Expand All @@ -3092,11 +3068,7 @@ e() {
expect(outputFile.lengthSync(), isPositive);

frontendServer.compileExpressionToJs(
expression: '2+2',
libraryUri: library,
line: 2,
column: 1,
moduleName: module);
expression: '2+2', libraryUri: library, line: 2, column: 1);
count += 1;
} else if (count == 3) {
expect(result.errorsCount, equals(0));
Expand Down Expand Up @@ -3739,10 +3711,13 @@ class FrontendServer {
required String libraryUri,
required int line,
required int column,
required String moduleName,
Uri? scriptUri,
Map<String, String>? jsFrameValues,
String boundaryKey = 'abc'}) {
// TODO(https://github.com/dart-lang/sdk/issues/58265): `moduleName` is
// soft-deprecated, so even though it's unused, the request should still
// contain it so it can be parsed correctly.
final String moduleName = 'unused';
if (useJsonForCommunication) {
outputParser.expectSources = false;
inputStreamController.add('JSON_INPUT\n'.codeUnits);
Expand Down
5 changes: 1 addition & 4 deletions pkg/frontend_server/test/src/javascript_bundle_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -215,10 +215,7 @@ void main() {
expect(code, contains('sourceMappingURL=c.dart.lib.js.map'));

// Verify program compilers are created.
final String moduleUrl = javaScriptBundler.urlForComponentUri(
library.importUri, packageConfig);
final String moduleName = javaScriptBundler.makeModuleName(moduleUrl);
expect(compilers.keys, equals([moduleName]));
expect(compilers.keys, equals([library.importUri.toString()]));
});

test('converts package: uris into /packages/ uris', () async {
Expand Down

0 comments on commit 32565d1

Please sign in to comment.