Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dart2wasm compiler generates Wasm code with an invalid cast when calling a function with generics. #59901

Closed
gmpassos opened this issue Jan 14, 2025 · 9 comments
Assignees
Labels
area-dart2wasm Issues for the dart2wasm compiler.

Comments

@gmpassos
Copy link
Contributor

gmpassos commented Jan 14, 2025

I created a project to isolate and reproduce the error:

https://github.com/gmpassos/web_attr_issue

Isolating the compilation error was challenging due to tree-shaking and compiler optimizations.

When running the code in the project, the test freezes due to a "crash" in the WebAssembly VM in Chrome:

dart test --platform chrome --compiler dart2wasm

Output:

00:03 +0: [Chrome, Dart2Wasm] loading test/web_attr_issue_test.dart                                                                                                              
Generated wasm module '/private/var/xxx/dart_test_gv9s4j/test_eaSyFi/web_attr_issue_test.dart.browser_test.dart.wasm', and JS init file '/private/var/xxx/dart_test_gv9s4j/test_eaSyFi/web_attr_issue_test.dart.browser_test.dart.mjs'.

00:05 +0: [Chrome, Dart2Wasm] test/web_attr_issue_test.dart: HTMLDivElement no issue                                                                                           
-- div: [object HTMLDivElement]
-- virtualDiv: VirtualDOMNode{tag: DIV, attrs: {class: DOMAttr{container}}}
-- generator: Instance of 'TestVirtualDOMGeneratorNoGenerics'
-- generator.buildElement...
-- element: TestElement{tag: DIV, attrs: {class: null container}}
00:05 +1: [Chrome, Dart2Wasm] test/web_attr_issue_test.dart: HTMLDivElement freeze issue                                                                                       
-- div: [object HTMLDivElement]
-- virtualDiv: VirtualDOMNode{tag: DIV, attrs: {class: DOMAttr{container}}}
-- generator: Instance of 'TestVirtualDOMGenerator'
-- generator.buildElement... (Freeze on Wasm!)
00:35 +1 -1: [Chrome, Dart2Wasm] test/web_attr_issue_test.dart: HTMLDivElement freeze issue [E]                                                                                
  TimeoutException after 0:00:30.000000: Test timed out after 30 seconds. See https://pub.dev/packages/test#timeouts
  web_attr_issue_test.dart.browser_test.dart.wasm 1:503795  module0.closure wrapper at file:///xxx/test_api-0.7.4/lib/src/backend/invoker.dart:336:14 trampoline
  web_attr_issue_test.dart.browser_test.dart.wasm 1:330304  module0._rootRun
  web_attr_issue_test.dart.browser_test.dart.wasm 1:347611  module0._rootRun tear-off trampoline
  web_attr_issue_test.dart.browser_test.dart.wasm 1:340238  module0._CustomZone.run
  web_attr_issue_test.dart.browser_test.dart.wasm 1:503551  module0.Invoker._handleError
  web_attr_issue_test.dart.browser_test.dart.wasm 1:538475  module0.Invoker.heartbeat closure at file:///xxx/test_api-0.7.4/lib/src/backend/invoker.dart:290:42
  
^C
  • NOTE: With dart2js, the test runs and completes successfully.

Dart:

Dart SDK version: 3.6.1 (stable) (Tue Jan 7 09:50:00 2025 -0800) on "macos_x64"

The error also happens with Dart 3.6.0.

@gmpassos gmpassos added the area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. label Jan 14, 2025
@gmpassos
Copy link
Contributor Author

FYI: @kevmoo @askeksa @mkustermann

@kevmoo
Copy link
Member

kevmoo commented Jan 14, 2025

Reproduced with Dart SDK version: 3.7.0-312.0.dev (dev) (Wed Jan 8 16:02:18 2025 -0800) on "macos_arm64"

Thanks for the detailed report and repro! 🙏

@kevmoo kevmoo added the area-dart2wasm Issues for the dart2wasm compiler. label Jan 14, 2025
@gmpassos
Copy link
Contributor Author

Thanks for the detailed report and repro! 🙏

Thanks for the fast response.

I hope to have this fixed on Dart 3.6.2, or I won't be able to finish the "dart:html" to "web" migration before Dart 3.7.

Best regards.

@osa1 osa1 self-assigned this Jan 14, 2025
@osa1
Copy link
Member

osa1 commented Jan 14, 2025

Thanks for the small repro.

This is related to TFA. Relevant parts of post-TFA kernel:

abstract class VirtualDOMGenerator<E extends core::Object? = dynamic> extends core::Object {
  method setAttributes([@vm.inferred-arg-type.metadata=library package:web_attr_issue/web_attr_issue.dart::VirtualDOMNode] final web2::VirtualDOMNode node, [@vm.inferred-arg-type.metadata=library package:web_attr_issue/web_attr_issue.dart::TestElement] covariant-by-class final web2::VirtualDOMGenerator::E% element) → void {
  {
    final synthesized core::Iterator<core::String> #forIterator = [@vm.direct-call.metadata=dart._list::GrowableList.iterator] [@vm.inferred-type.metadata=dart._list::_GrowableListIterator<dart.core::String>] [@vm.direct-call.metadata=dart.core::Iterable.toList] [@vm.inferred-type.metadata=dart._list::GrowableList<dart.core::String> (skip check)] [@vm.direct-call.metadata=library dart:_compact_hash::_DefaultMap&_HashFieldBase&MapMixin&_HashBase&_OperatorEqualsAndHashCode&_LinkedHashMapMixin.keys] [@vm.inferred-type.metadata=library dart:_compact_hash::_CompactKeysIterable<dart.core::String>] [@vm.direct-call.metadata=library package:web_attr_issue/web_attr_issue.dart::VirtualDOMNode.attrs] [@vm.inferred-type.metadata=library dart:_compact_hash::DefaultMap<dart.core::String, library package:web_attr_issue/web_attr_issue.dart::DOMAttr>] node.{web2::VirtualDOMNode::attrs}{core::Map<core::String, web2::DOMAttr>}.{core::Map::keys}{core::Iterable<core::String>}.{core::Iterable::toList}(){({growable: core::bool}) → core::List<core::String>}.{core::Iterable::iterator}{core::Iterator<core::String>};
    for (; [@vm.direct-call.metadata=dart._list::_GrowableListIterator.moveNext] [@vm.inferred-type.metadata=dart.core::bool (skip check)] #forIterator.{core::Iterator::moveNext}(){() → core::bool}; ) {
      final core::String name = [@vm.direct-call.metadata=dart._list::_GrowableListIterator.current] [@vm.inferred-type.metadata=!] #forIterator.{core::Iterator::current}{core::String};
      {
        final web2::DOMAttr domAttr = [@vm.direct-call.metadata=library dart:_compact_hash::_DefaultMap&_HashFieldBase&MapMixin&_HashBase&_OperatorEqualsAndHashCode&_LinkedHashMapMixin.[]] [@vm.inferred-type.metadata=library package:web_attr_issue/web_attr_issue.dart::DOMAttr? (skip check)] [@vm.direct-call.metadata=library package:web_attr_issue/web_attr_issue.dart::VirtualDOMNode.attrs] [@vm.inferred-type.metadata=library dart:_compact_hash::DefaultMap<dart.core::String, library package:web_attr_issue/web_attr_issue.dart::DOMAttr>] node.{web2::VirtualDOMNode::attrs}{core::Map<core::String, web2::DOMAttr>}.{core::Map::[]}(name){(core::Object?) → web2::DOMAttr?}!;
        final core::String value = [@vm.direct-call.metadata=library package:web_attr_issue/web_attr_issue.dart::DOMAttr.value] [@vm.inferred-type.metadata=!] domAttr.{web2::DOMAttr::value}{core::String};
        if([@vm.inferred-type.metadata=! (receiver not int)] name =={core::String::==}{(core::Object) → core::bool} "class") {
          final core::String? prev = [@vm.direct-call.metadata=library package:web_attr_issue/web_attr_issue.dart::TestVirtualDOMGenerator.getAttribute] [@vm.inferred-type.metadata=!? (skip check)] this.{web2::VirtualDOMGenerator::getAttribute}(element, name){(web2::VirtualDOMGenerator::E%, core::String) → core::String?};
          [@vm.direct-call.metadata=library package:web_attr_issue/web_attr_issue.dart::TestVirtualDOMGenerator.setAttribute] [@vm.inferred-type.metadata=!? (skip check)] this.{web2::VirtualDOMGenerator::setAttribute}(element, name, "${prev} ${value}"){(web2::VirtualDOMGenerator::E%, core::String, core::String) → core::String?};
        }
        else {
          [@vm.direct-call.metadata=library package:web_attr_issue/web_attr_issue.dart::TestVirtualDOMGenerator.setAttribute] [@vm.inferred-type.metadata=!? (skip check)] this.{web2::VirtualDOMGenerator::setAttribute}(element, name, value){(web2::VirtualDOMGenerator::E%, core::String, core::String) → core::String?};
        }
      }
    }
  }
}

Here type of name is String (with no more specific inferred type), and name is passed to TestVirtualDOMGenerator.getAttribute, whose inferred argument type is OneByteString:

class TestVirtualDOMGenerator extends web2::VirtualDOMGenerator<web2::TestElement> {
  method getAttribute([@vm.inferred-arg-type.metadata=library package:web_attr_issue/web_attr_issue.dart::TestElement (skip check)] covariant-by-class final web2::TestElement element, [@vm.inferred-arg-type.metadata=library dart:_string::OneByteString (value: "class")] final core::String name) → core::String? {
    ...
  }
}

It looks like when TFA propagates the string value based on the conditional name == "class" it also decides on the implementation class based on concreteStringLiteralClass("class"), which returns OneByteString. However, unlike in VM where a string that consists of one one-byte code units will always be OneByteString, in dart2wasm it can be anything. (OneByteString, TwoByteString, JSStringImpl). In the test the string is JSStringImpl, so the kernel has an invalid type cast from JSStringImpl to OneByteString.

I think we should make TFA not assume that a string will always be represented as concreteStringLiteralClass(<string>) when used in dart2wasm.

@mkustermann @alexmarkov any thoughts on this?

@alexmarkov
Copy link
Contributor

Target.concreteStringLiteralClass should either return a concrete class of a string literal if it is known, or null if it is not known at compile time. So, it looks like dart2wasm implementation of concreteStringLiteralClass can just return null. Note that TFA can only propagate constants if their class is known.

If concrete class of literal is actually known and the problem is that inference of variable value after v == c is incorrect when c is a string literal (not sure I understand why these string values are not interchangeable), then we can customize the logic in

(rhs is StringLiteral &&
_isSubtype(lhs.variable.type,
_environment.coreTypes.stringNullableRawType)) ||

to add a target-specific predicate, something along the lines

          (rhs is StringLiteral &&
              _isSubtype(lhs.variable.type,
                  _environment.coreTypes.stringNullableRawType) &&
             target.canInferStringValueAfterEqualityComparison()) ||

By default, we can declare canInferStringValueAfterEqualityComparison() to return true and override it in WasmTarget to return false.

@srujzs srujzs removed the area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. label Jan 14, 2025
@mkustermann
Copy link
Member

If concrete class of literal is actually known and the problem is that inference of variable value after v == c is incorrect when c is a string literal (not sure I understand why these string values are not interchangeable), then we can customize the logic in

Two strings in dart2wasm can be equal but don't have to be identical nor have to have same concrete class.

In VM's terminology: Imagine comparing a ExternalOneByteString to a OneByteString they may have same content and compare equal but different class id (maybe we removed external strings from VM by now but you get the idea).

@osa1
Copy link
Member

osa1 commented Jan 15, 2025

I'm trying both approaches.

  • cl/404561 returns null from concreteStringLiteralClass. I suspect this will have a performance impact. I started benchmarking this, link in the CL.
  • cl/404562 updates TFA as suggested by @alexmarkov.

@gmpassos
Copy link
Contributor Author

IMHO, canInferStringClassAfterEqualityComparison is the one that fixes the actual bug, without impacting other platforms.

@osa1
Copy link
Member

osa1 commented Jan 16, 2025

IMHO, canInferStringClassAfterEqualityComparison is the one that fixes the actual bug, without impacting other platforms.

Both of the solutions are specific to dart2wasm, they don't impact other platforms.


Returning null from concreteStringLiteralClass didn't make too many changes in the benchmarks. Mainly some replaceAll benchmarks changed where previously the replaceAll calls were direct calls, which became indirect as we lost the receiver type information.

(I found it somewhat surprising that it didn't make a larger impact, as benchmark code tend to be simpler than real world and more amenable to static analysis and so they can exaggerate results.)

I still sent the CL with the new Target method for reviews as it basically has no downsides for the generated code, just adds one line to TFA.

copybara-service bot pushed a commit that referenced this issue Jan 28, 2025
In dart2wasm, when a comparison like `x == "hello"` is true, we can't
assume that the class of `x` is the same as the class of `"hello"`:

- If `x` is received from JS, it will be `JSStringImpl`.
- If it's a substring of a `TwoByteString`, it will be `TwoByteString`.
- Otherwise it will be `OneByteString`.

Update `Target` with the new method

```
bool get canInferStringClassAfterEqualityComparison => true;
```

to allow TFA to *not* infer classes of string values after comparisons.

Override the method to return `false` in dart2wasm's `Target`
implementation.

Bug: #59901
Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/404562
Cherry-pick-request: #59924
Change-Id: I1a6c8deaf27c54240dd4e821dbd8160914502ad7
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/404842
Reviewed-by: Johnni Winther <[email protected]>
Commit-Queue: Ömer Ağacan <[email protected]>
Reviewed-by: Martin Kustermann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-dart2wasm Issues for the dart2wasm compiler.
Projects
None yet
Development

No branches or pull requests

6 participants