Skip to content

Commit ebf9df9

Browse files
MarkzipanCommit Queue
Markzipan
authored and
Commit Queue
committed
[ddc] Avoid overriding LegacyJavaScriptObject type rules across link phases.
This prevents libraries from accidentally clobbering LegacyJavaScriptObject rules when linking. Change-Id: Ia7465013633a34907ca6ab9d1d5bfcdf99ffa13c Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/395161 Reviewed-by: Srujan Gaddam <[email protected]> Commit-Queue: Mark Zhou <[email protected]> Reviewed-by: Nicholas Shahan <[email protected]>
1 parent e4e8360 commit ebf9df9

File tree

3 files changed

+135
-42
lines changed

3 files changed

+135
-42
lines changed

‎pkg/dev_compiler/lib/src/kernel/compiler.dart‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -662,6 +662,11 @@ class ProgramCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
662662
var universeClass =
663663
_rtiLibrary.classes.firstWhere((cls) => cls.name == '_Universe');
664664
var typeRules = _typeRecipeGenerator.liveInterfaceTypeRules;
665+
var legacyJavaScriptObjectRecipe = _typeRecipeGenerator.interfaceTypeRecipe(
666+
_coreTypes.index
667+
.getClass('dart:_interceptors', 'LegacyJavaScriptObject'));
668+
var legacyJavaScriptObjectRules =
669+
typeRules.remove(legacyJavaScriptObjectRecipe);
665670
if (typeRules.isNotEmpty) {
666671
var template = '#._Universe.#(#, JSON.parse(#))';
667672
var addRulesStatement = js.call(template, [
@@ -672,6 +677,22 @@ class ProgramCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
672677
]).toStatement();
673678
_moduleItems.add(addRulesStatement);
674679
}
680+
if (legacyJavaScriptObjectRules != null) {
681+
var legacyJavaScriptObjectAddRules = {
682+
legacyJavaScriptObjectRecipe: legacyJavaScriptObjectRules
683+
};
684+
// The recipe for 'LegacyJavaScriptObject' is updated during the lifetime
685+
// of the program and should be emitted with 'addOrUpdateRules' to avoid
686+
// clobbering its previous state.
687+
var updateRulesStatement =
688+
js.statement('#._Universe.#(#, JSON.parse(#))', [
689+
_emitLibraryName(_rtiLibrary),
690+
_emitMemberName('addOrUpdateRules', memberClass: universeClass),
691+
_runtimeCall('typeUniverse'),
692+
js.string(jsonEncode(legacyJavaScriptObjectAddRules), "'")
693+
]);
694+
_moduleItems.add(updateRulesStatement);
695+
}
675696
// Update type rules for `LegacyJavaScriptObject` to add all interop
676697
// types in this module as a supertype.
677698
var updateRules = _typeRecipeGenerator.updateLegacyJavaScriptObjectRules;

‎pkg/dev_compiler/lib/src/kernel/compiler_new.dart‎

Lines changed: 68 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -801,53 +801,79 @@ class LibraryCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
801801
}
802802
var universeClass =
803803
_rtiLibrary.classes.firstWhere((cls) => cls.name == '_Universe');
804-
var typeRules = _typeRecipeGenerator.liveInterfaceTypeRules;
805-
if (typeRules.isNotEmpty) {
806-
var template = '#._Universe.#(#, JSON.parse(#))';
807-
var addRulesStatement = js.call(template, [
808-
_emitLibraryName(_rtiLibrary),
809-
_emitMemberName('addRules', memberClass: universeClass),
810-
_runtimeCall('typeUniverse'),
811-
js.string(jsonEncode(typeRules), "'")
812-
]).toStatement();
813-
_typeRuleLinks.add(addRulesStatement);
814-
}
815-
// Reset type rules for all classes with empty type hierarchies. These
816-
// classes implicitly extend `Object` and may have been updated after a
817-
// hot reload. This is unnecessary for types in `liveInterfaceTypeRules`,
818-
// as `addRules` overrides the old rules.
819-
// TODO(57049): Only do this after a hot reload.
820-
var emptyInterfaceTypeRecipes =
821-
Set.from(_typeRecipeGenerator.visitedInterfaceTypeRecipes)
822-
..removeAll(typeRules.keys);
823-
if (emptyInterfaceTypeRecipes.isNotEmpty) {
804+
805+
// Emits either an 'addRules', 'addOrUpdateRules', or 'deleteRules'
806+
// statement for a JSON-serializable [rules] made of RTI type rules.
807+
//
808+
// 'addRules' overrides existing state. Calling this function multiple
809+
// times is safe for types whose hierarchies can be exhaustively
810+
// discovered at compile-time, which is true for all types that aren't
811+
// 'LegacyJavaScriptObject'.
812+
//
813+
// TODO: The above assumption may not hold if a class's hierarchy
814+
// changes after a hot reload. Outdated 'addRules' invocations (during
815+
// linking) may clobber updated type rules.
816+
js_ast.Statement emitRulesStatement(Object? rules,
817+
{required String rulesFunction}) {
824818
var template = '#._Universe.#(#, JSON.parse(#))';
825-
var deleteRulesStatement = js.call(template, [
819+
var rulesExpr = js.call(template, [
826820
_emitLibraryName(_rtiLibrary),
827-
_emitMemberName('deleteRules', memberClass: universeClass),
821+
_emitMemberName(rulesFunction, memberClass: universeClass),
828822
_runtimeCall('typeUniverse'),
829-
js.string(jsonEncode(emptyInterfaceTypeRecipes.toList()), "'")
830-
]).toStatement();
831-
_typeRuleLinks.add(deleteRulesStatement);
832-
}
833-
// Update type rules for `LegacyJavaScriptObject` to add all interop
834-
// types in this module as a supertype.
835-
var updateRules = _typeRecipeGenerator.updateLegacyJavaScriptObjectRules;
836-
if (updateRules.isNotEmpty) {
837-
// All JavaScript interop classes should be mutual subtypes with
838-
// `LegacyJavaScriptObject`. To achieve this the rules are manually
839-
// added here. There is special redirecting rule logic in the dart:_rti
840-
// library for interop types because otherwise they would duplicate
841-
// a lot of supertype information.
842-
var updateRulesStatement =
843-
js.statement('#._Universe.#(#, JSON.parse(#))', [
844-
_emitLibraryName(_rtiLibrary),
845-
_emitMemberName('addOrUpdateRules', memberClass: universeClass),
846-
_runtimeCall('typeUniverse'),
847-
js.string(jsonEncode(updateRules), "'")
823+
js.string(jsonEncode(rules), "'")
848824
]);
849-
_typeRuleLinks.add(updateRulesStatement);
825+
return rulesExpr.toStatement();
826+
}
827+
828+
// We must emit type rules for every interface type encountered by DDC,
829+
// with several caveats:
830+
// 1) 'LegacyJavaScriptObject' has special treatment. Its hierarchy
831+
// accumulates across libraries and must always be emitted in 'append'
832+
// mode ('addOrUpdateRules') to avoid clobbering its previous state.
833+
// 2) We manually add rules for mutual subtype relationships between
834+
// 'LegacyJavaScriptObject' and all JavaScript interop classes. There is
835+
// special redirecting rule logic in the dart:_rti library for interop
836+
// types because otherwise they would duplicate a lot of supertype
837+
// information.
838+
// 3) The RTI treats an empty type hierarchy as implicitly containing
839+
// 'Object'. We explicitly emit 'deleteRules' instructions in case
840+
// a type hierarchy was deleted or edited to extend 'Object' after hot
841+
// reload.
842+
var legacyJavaScriptObjectRecipe = _typeRecipeGenerator.interfaceTypeRecipe(
843+
_coreTypes.index
844+
.getClass('dart:_interceptors', 'LegacyJavaScriptObject'));
845+
var legacyJavaScriptObjectRules = _typeRecipeGenerator
846+
.liveInterfaceTypeRules[legacyJavaScriptObjectRecipe];
847+
var typeRulesExceptLegacyJavaScriptObject = _typeRecipeGenerator
848+
.liveInterfaceTypeRules
849+
..remove(legacyJavaScriptObjectRecipe);
850+
var typesThatOnlyExtendObject =
851+
Set.from(_typeRecipeGenerator.visitedInterfaceTypeRecipes)
852+
..removeAll(typeRulesExceptLegacyJavaScriptObject.keys)
853+
..remove(legacyJavaScriptObjectRecipe);
854+
var legacyJavaScriptObjectMutualSubtypingRules =
855+
_typeRecipeGenerator.updateLegacyJavaScriptObjectRules;
856+
857+
if (typeRulesExceptLegacyJavaScriptObject.isNotEmpty) {
858+
_typeRuleLinks.add(emitRulesStatement(
859+
typeRulesExceptLegacyJavaScriptObject,
860+
rulesFunction: 'addRules'));
850861
}
862+
if (typesThatOnlyExtendObject.isNotEmpty) {
863+
_typeRuleLinks.add(emitRulesStatement(typesThatOnlyExtendObject.toList(),
864+
rulesFunction: 'deleteRules'));
865+
}
866+
if (legacyJavaScriptObjectRules != null) {
867+
_typeRuleLinks.add(emitRulesStatement({
868+
legacyJavaScriptObjectRecipe: legacyJavaScriptObjectRules,
869+
}, rulesFunction: 'addOrUpdateRules'));
870+
}
871+
if (legacyJavaScriptObjectMutualSubtypingRules.isNotEmpty) {
872+
_typeRuleLinks.add(emitRulesStatement(
873+
legacyJavaScriptObjectMutualSubtypingRules,
874+
rulesFunction: 'addOrUpdateRules'));
875+
}
876+
851877
var jsInteropTypeRecipes = _typeRecipeGenerator.visitedJsInteropTypeRecipes;
852878
if (jsInteropTypeRecipes.isNotEmpty) {
853879
// Update the `LegacyJavaScriptObject` class with the type tags for all

‎tests/web/js_interop_generic_cast_test.dart‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file
2+
// for details. All rights reserved. Use of this source code is governed by a
3+
// BSD-style license that can be found in the LICENSE file.
4+
5+
// Tests that type checks work properly for Legacy JS Objects across generic
6+
// boundaries.
7+
8+
import 'dart:html';
9+
import "dart:js" as js;
10+
import 'dart:js_interop';
11+
import 'dart:js_interop_unsafe';
12+
13+
import 'package:js/js.dart' as pkgJs;
14+
15+
class TestGenericClass<X> {
16+
X x;
17+
TestGenericClass(this.x);
18+
19+
directCast(p) {
20+
return p as PackageJSClass<String>;
21+
}
22+
23+
typeArgCast(p) {
24+
return p as X;
25+
}
26+
}
27+
28+
@pkgJs.JS()
29+
class PackageJSClass<T> {
30+
external factory PackageJSClass(T x);
31+
}
32+
33+
void main() {
34+
js.context.callMethod("eval", [
35+
"""
36+
window.PackageJSClass = function PackageJSClass(x) {
37+
this.x = x;
38+
};
39+
"""
40+
]);
41+
42+
var instance =
43+
TestGenericClass<PackageJSClass<JSString>>(PackageJSClass("Hello".toJS));
44+
print(instance.directCast(PackageJSClass("Hello".toJS)));
45+
print(instance.typeArgCast(PackageJSClass("Hello".toJS)));
46+
}

0 commit comments

Comments
 (0)