From d17205184ac5e5e9404a376673a4c6b45dd08f37 Mon Sep 17 00:00:00 2001 From: Srujan Gaddam Date: Fri, 21 Jan 2022 01:04:10 +0000 Subject: [PATCH] [dart2js] Handle `Object` members of `dart:html` types `dart:html` types have changed to inherit `JavaScriptObject`. Therefore, the dart2js runtime needs to be changed so that interceptors are still created for those types, and they're properly handled in `toString` calculations. Includes tests on `Object` members that are currently inconsistent between both compilers. This test passes on dart2js with and without making the types in `dart:html` extend `JavaScriptObject`. This test fails in DDC for the following reasons: - `toString` of native types calls the native `toString` - `hashCode` for interop objects are random and not 0 - `runtimeType` of interop objects is `LegacyJavaScriptObject` not `JSObject` Change-Id: Ibf80109174615120df9e64995fa13016f7a1677b Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/228741 Reviewed-by: Stephen Adams Commit-Queue: Srujan Gaddam --- .../lib/src/js_emitter/native_emitter.dart | 9 +++ .../jsinterop/internal_annotations_test.dart | 10 ++- pkg/compiler/test/jsinterop/world_test.dart | 10 ++- .../_internal/js_runtime/lib/js_helper.dart | 14 ++-- tests/web/internal/object_members_test.dart | 80 +++++++++++++++++++ tests/web/web.status | 4 + tests/web_2/internal/object_members_test.dart | 80 +++++++++++++++++++ tests/web_2/web_2.status | 4 + 8 files changed, 203 insertions(+), 8 deletions(-) create mode 100644 tests/web/internal/object_members_test.dart create mode 100644 tests/web_2/internal/object_members_test.dart diff --git a/pkg/compiler/lib/src/js_emitter/native_emitter.dart b/pkg/compiler/lib/src/js_emitter/native_emitter.dart index a3137e7930e..857b67bebcd 100644 --- a/pkg/compiler/lib/src/js_emitter/native_emitter.dart +++ b/pkg/compiler/lib/src/js_emitter/native_emitter.dart @@ -94,6 +94,7 @@ class NativeEmitter { Class objectClass = null; Class jsInterceptorClass = null; + Class jsJavaScriptObjectClass = null; void walk(Class cls) { if (cls.element == _commonElements.objectClass) { @@ -104,6 +105,11 @@ class NativeEmitter { jsInterceptorClass = cls; return; } + // Native classes may inherit either `Interceptor` e.g. `JSBool` or + // `JavaScriptObject` e.g. `dart:html` classes. + if (cls.element == _commonElements.jsJavaScriptObjectClass) { + jsJavaScriptObjectClass = cls; + } if (seen.contains(cls)) return; seen.add(cls); walk(cls.superclass); @@ -215,6 +221,9 @@ class NativeEmitter { // by getNativeInterceptor and custom elements. if (_nativeCodegenEnqueuer.hasInstantiatedNativeClasses) { fillNativeInfo(jsInterceptorClass); + if (jsJavaScriptObjectClass != null) { + fillNativeInfo(jsJavaScriptObjectClass); + } for (Class cls in classes) { if (!cls.isNative || neededClasses.contains(cls)) { fillNativeInfo(cls); diff --git a/pkg/compiler/test/jsinterop/internal_annotations_test.dart b/pkg/compiler/test/jsinterop/internal_annotations_test.dart index 4299a38b83c..62deced1da6 100644 --- a/pkg/compiler/test/jsinterop/internal_annotations_test.dart +++ b/pkg/compiler/test/jsinterop/internal_annotations_test.dart @@ -184,7 +184,15 @@ $mainSource "Expected $name to be indirectly instantiated in `${mainSource}`:" "\n${world.classHierarchy.dump(cls)}"); } - if (!isInstantiated && (name != 'Object' && name != 'Interceptor')) { + // Classes that are expected to be instantiated by default. `Object` and + // `Interceptor` are base types for non-native and native types, and + // `JavaScriptObject` is the base type for `dart:html` types. + var insantiatedBaseClasses = [ + 'Object', + 'Interceptor', + 'JavaScriptObject' + ]; + if (!isInstantiated && !insantiatedBaseClasses.contains(name)) { Expect.isFalse( world.classHierarchy.isInstantiated(cls), "Expected $name to be uninstantiated in `${mainSource}`:" diff --git a/pkg/compiler/test/jsinterop/world_test.dart b/pkg/compiler/test/jsinterop/world_test.dart index 6c3253fec08..230873f716c 100644 --- a/pkg/compiler/test/jsinterop/world_test.dart +++ b/pkg/compiler/test/jsinterop/world_test.dart @@ -178,7 +178,15 @@ $mainSource "Expected $name to be indirectly instantiated in `${mainSource}`:" "\n${world.classHierarchy.dump(cls)}"); } - if (!isInstantiated && (name != 'Object' && name != 'Interceptor')) { + // Classes that are expected to be instantiated by default. `Object` and + // `Interceptor` are base types for non-native and native types, and + // `JavaScriptObject` is the base type for `dart:html` types. + var insantiatedBaseClasses = [ + 'Object', + 'Interceptor', + 'JavaScriptObject' + ]; + if (!isInstantiated && !insantiatedBaseClasses.contains(name)) { Expect.isFalse( world.classHierarchy.isInstantiated(cls), "Expected $name to be uninstantiated in `${mainSource}`:" diff --git a/sdk/lib/_internal/js_runtime/lib/js_helper.dart b/sdk/lib/_internal/js_runtime/lib/js_helper.dart index a7f3ca829a8..d41e5114780 100644 --- a/sdk/lib/_internal/js_runtime/lib/js_helper.dart +++ b/sdk/lib/_internal/js_runtime/lib/js_helper.dart @@ -415,19 +415,21 @@ class Primitives { var interceptor = getInterceptor(object); if (identical(interceptor, JS_INTERCEPTOR_CONSTANT(Interceptor)) || + identical(interceptor, JS_INTERCEPTOR_CONSTANT(JavaScriptObject)) || object is UnknownJavaScriptObject) { // Try to do better. If we do not find something better, fallthrough to - // Dart-type based name that leave the name as 'UnknownJavaScriptObject' - // or 'Interceptor' (or the minified versions thereof). + // Dart-type based name that leave the name as 'UnknownJavaScriptObject', + // 'Interceptor', or 'JavaScriptObject' (or their minified versions). // // When we get here via the UnknownJavaScriptObject test (for JavaScript // objects from outside the program), the object's constructor has a // better name that 'UnknownJavaScriptObject'. // - // When we get here the Interceptor test (for Native classes that are - // declared in the Dart program but have been 'folded' into Interceptor), - // the native class's constructor name is better than the generic - // 'Interceptor' (an abstract class). + // When we get here via either the Interceptor or JavaScriptObject test + // (for Native classes that are declared in the Dart program but have been + // 'folded' into one of those interceptors), the native class's + // constructor name is better than the generic 'Interceptor' or + // 'JavaScriptObject'. // Try the [constructorNameFallback]. This gets the constructor name for // any browser (used by [getNativeInterceptor]). diff --git a/tests/web/internal/object_members_test.dart b/tests/web/internal/object_members_test.dart new file mode 100644 index 00000000000..02092eb1022 --- /dev/null +++ b/tests/web/internal/object_members_test.dart @@ -0,0 +1,80 @@ +// Copyright (c) 2022, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +// Make sure `Object` methods work as expected with `dart:html` and interop +// types. The expectations here aren't guarantees that they should work a +// particular way, but rather a way to monitor regressions/changes. + +@JS() +library object_members_test; + +import 'package:js/js.dart'; +import 'package:expect/minitest.dart'; + +import 'dart:html'; +import 'dart:_interceptors' show JSObject; + +@JS() +external void eval(String code); + +@JS() +class JSClass { + external JSClass(); +} + +void main() { + eval(r''' + function JSClass() {} + '''); + + // `dart:html` type. + var div = document.createElement('div'); + expect(div == div, true); + expect(div == DomPointReadOnly(), false); + // Ensure that we get a random hash for each new instance. It should be + // improbable for this to fail across many runs if the hash is + // non-deterministic. + var hashCode = div.hashCode; + var attempts = 0; + var maxAttempts = 1000; + while (div.hashCode == hashCode && attempts < maxAttempts) { + div = document.createElement('div'); + attempts++; + } + expect(attempts > 0 && attempts != maxAttempts, isTrue); + expect(div.toString, isNotNull); + expect(div.toString(), 'div'); + expect(div.noSuchMethod, isNotNull); + var noSuchMethodErrorThrown = true; + try { + (div as dynamic).triggerNoSuchMethod(); + noSuchMethodErrorThrown = false; + } catch (_) {} + expect(noSuchMethodErrorThrown, isTrue); + expect(div.runtimeType, DivElement); + + // `toString` for `dart:html` types that do not have an overridden `toString` + // should look up the type through the proto. + expect(window.navigator.toString(), "Instance of 'Navigator'"); + + // Interop type. + var js = JSClass(); + expect(js == js, true); + expect(js == JSClass(), false); + // TODO(srujzs): Modify this once interop has random hash codes. + hashCode = js.hashCode; + expect(hashCode, 0); + expect(hashCode, js.hashCode); + expect(js.toString, isNotNull); + // Should forward to underlying `toString` call. + expect(js.toString(), '[object Object]'); + expect(js.noSuchMethod, isNotNull); + noSuchMethodErrorThrown = true; + try { + (js as dynamic).triggerNoSuchMethod(); + noSuchMethodErrorThrown = false; + } catch (_) {} + expect(noSuchMethodErrorThrown, isTrue); + expect(js.runtimeType, JSObject); +} diff --git a/tests/web/web.status b/tests/web/web.status index 481ba3db580..7bdbea2718b 100644 --- a/tests/web/web.status +++ b/tests/web/web.status @@ -27,12 +27,16 @@ deferred_split_test: Slow, Pass # Issue 25940 [ $compiler == dart2js && $runtime == chrome && $csp ] deferred/load_in_correct_order_test: SkipByDesign # Purposely uses `eval` +[ $compiler == dart2js && $runtime == d8 ] +internal/object_members_test: SkipByDesign # Browser test + [ $compiler == dart2js && $runtime == ff && $system == windows ] consistent_index_error_string_test: Slow, Pass # Issue 25940 [ $compiler == dart2js && $csp ] deferred_custom_loader_test: SkipByDesign # Issue 25683 deferred_fail_and_retry_test: SkipByDesign # Uses eval to simulate failed loading. +internal/object_members_test: SkipByDesign # Uses eval for interop [ $compiler == dart2js && !$host_checked ] dummy_compiler_test: Slow, Pass # Issue 32439. self-hosting doesn't work with CFE yet. diff --git a/tests/web_2/internal/object_members_test.dart b/tests/web_2/internal/object_members_test.dart new file mode 100644 index 00000000000..02092eb1022 --- /dev/null +++ b/tests/web_2/internal/object_members_test.dart @@ -0,0 +1,80 @@ +// Copyright (c) 2022, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +// Make sure `Object` methods work as expected with `dart:html` and interop +// types. The expectations here aren't guarantees that they should work a +// particular way, but rather a way to monitor regressions/changes. + +@JS() +library object_members_test; + +import 'package:js/js.dart'; +import 'package:expect/minitest.dart'; + +import 'dart:html'; +import 'dart:_interceptors' show JSObject; + +@JS() +external void eval(String code); + +@JS() +class JSClass { + external JSClass(); +} + +void main() { + eval(r''' + function JSClass() {} + '''); + + // `dart:html` type. + var div = document.createElement('div'); + expect(div == div, true); + expect(div == DomPointReadOnly(), false); + // Ensure that we get a random hash for each new instance. It should be + // improbable for this to fail across many runs if the hash is + // non-deterministic. + var hashCode = div.hashCode; + var attempts = 0; + var maxAttempts = 1000; + while (div.hashCode == hashCode && attempts < maxAttempts) { + div = document.createElement('div'); + attempts++; + } + expect(attempts > 0 && attempts != maxAttempts, isTrue); + expect(div.toString, isNotNull); + expect(div.toString(), 'div'); + expect(div.noSuchMethod, isNotNull); + var noSuchMethodErrorThrown = true; + try { + (div as dynamic).triggerNoSuchMethod(); + noSuchMethodErrorThrown = false; + } catch (_) {} + expect(noSuchMethodErrorThrown, isTrue); + expect(div.runtimeType, DivElement); + + // `toString` for `dart:html` types that do not have an overridden `toString` + // should look up the type through the proto. + expect(window.navigator.toString(), "Instance of 'Navigator'"); + + // Interop type. + var js = JSClass(); + expect(js == js, true); + expect(js == JSClass(), false); + // TODO(srujzs): Modify this once interop has random hash codes. + hashCode = js.hashCode; + expect(hashCode, 0); + expect(hashCode, js.hashCode); + expect(js.toString, isNotNull); + // Should forward to underlying `toString` call. + expect(js.toString(), '[object Object]'); + expect(js.noSuchMethod, isNotNull); + noSuchMethodErrorThrown = true; + try { + (js as dynamic).triggerNoSuchMethod(); + noSuchMethodErrorThrown = false; + } catch (_) {} + expect(noSuchMethodErrorThrown, isTrue); + expect(js.runtimeType, JSObject); +} diff --git a/tests/web_2/web_2.status b/tests/web_2/web_2.status index 481ba3db580..7bdbea2718b 100644 --- a/tests/web_2/web_2.status +++ b/tests/web_2/web_2.status @@ -27,12 +27,16 @@ deferred_split_test: Slow, Pass # Issue 25940 [ $compiler == dart2js && $runtime == chrome && $csp ] deferred/load_in_correct_order_test: SkipByDesign # Purposely uses `eval` +[ $compiler == dart2js && $runtime == d8 ] +internal/object_members_test: SkipByDesign # Browser test + [ $compiler == dart2js && $runtime == ff && $system == windows ] consistent_index_error_string_test: Slow, Pass # Issue 25940 [ $compiler == dart2js && $csp ] deferred_custom_loader_test: SkipByDesign # Issue 25683 deferred_fail_and_retry_test: SkipByDesign # Uses eval to simulate failed loading. +internal/object_members_test: SkipByDesign # Uses eval for interop [ $compiler == dart2js && !$host_checked ] dummy_compiler_test: Slow, Pass # Issue 32439. self-hosting doesn't work with CFE yet.