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

Dynamic property access on js object not working on release mode #59837

Closed
MarcoPeli opened this issue Jan 2, 2025 · 3 comments
Closed

Dynamic property access on js object not working on release mode #59837

MarcoPeli opened this issue Jan 2, 2025 · 3 comments
Labels
area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. web-js-interop Issues that impact all js interop

Comments

@MarcoPeli
Copy link

MarcoPeli commented Jan 2, 2025

Flutter 3.27.1 • channel stable • https://github.com/flutter/flutter.git
Framework - revision 17025dd882 - Engine - revision cb4b5fff73 - Tools - Dart 3.6.0 - DevTools 2.40.2

My use case is reading event.target.result from web package indexedDB. But we don't need to go that far to reproduce the error. Check the dartpad code running fine on debug mode: https://dartpad.dev/?id=25d7be1f405f153d24bac3d862adcd58

image

Now you need to run same code on release mode. Dartpad don't allow it but you can check here: https://marcopeli.github.io/temp/ opening the console to see the error bellow:

image

image

image

@MarcoPeli MarcoPeli added the area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. label Jan 2, 2025
@sigmundch
Copy link
Member

Thanks for the question @MarcoPeli

This is actually an intentional choice. Both dart:js_interop and package:web are defined through extension-types. As a result, they do not support virtual or dynamic dispatch. The choice to use extension types was made to address many limitations we had in the past from other JS-interop solutions. For example, they allowed us to verify that type signatures were sound, they helped us support combining packages that may have overlapping definitions without creating conflicts, and they enabled us to provide a more efficient implementation across our compilers.

There are some references to these limitations in some of our docs (when discussing limitations of package-web and comparing dart:js_interop against package:js), but we can highlight them better?

When possible, we recommend to bring the logic back to a statically defined API, as that is generally better for performance and producing smaller compilation artifacts. So for example, if there is an API you expect, you can define an interop type for it and use it:

extension type A._(JSObject _) implements JSObject {
  external Target get target;
}
extension type Target._(JSObject _) implements JSObject {
  external String get result;
}
...

main() {
  ...
  str = (obj as A).target.result;
}

If your goal is to read directly properties in JS that are not well specified ahead of time, then there is also dart:js_interop_unsafe as low-level primitives for dynamically accessing properties and methods from JS directly.

import 'dart:js_interop_unsafe';
...

main() {
  ...
  str = (((obj as JSObject)['target'] as JSObject)['result'] as JSString).toDart;
}

dart:js_interop_unsafe is more cumbersome to use, error prone, and harder to prove safe in large applications, so we usually discourage it unless you really need it.

@sigmundch sigmundch added the web-js-interop Issues that impact all js interop label Jan 3, 2025
@MarcoPeli
Copy link
Author

MarcoPeli commented Jan 3, 2025

@sigmundch sigmundch, I understand your point. But the main problem is: why same code is working on dartpad? Why does it work on debug mode? Isn't that misleading? I worked under the assumption that code running on debug mode would also work on release mode - a natural assumption for all developers on the entire world :P If the code is not intended to work on release mode, make it break on debug mode as well, please.

I really doubt the code breaking only on release mode is an intentional choice.

@sigmundch
Copy link
Member

Excellent question! I share your natural assumption too, btw. Sorry I didn't address this inconsistency in my first comment.

I agree, debug mode should be stricter and disallow these dynamic calls too. Unfortunately, we are in a position where can't fix this until we remove support for the old package:js logic.

Today we are in the uncomfortable state that we have to support both the old and the new mechanisms for a transition period. Since both the interop sit on top of JS values, those JS values can be passed and cast between the old and new interop representations. And, because the old mechanism supports dynamic calls, they may accidentally be available some scenarios. What scenarios make that happen depends on some characteristics of each compilers.

Generally, our release compiler can see the whole program, and can decide which dynamic calls are supported based on the types it sees in the code. This is why you saw the error above and why it's hard to run into this in practice. That said, you can technically also produce a case where the release compiler allows the dynamic calls accidentally. It's a bit contrived, but it requires confusing the compiler so it doesn't know what it can be calling and so that it has the names elsewhere in the program that you happen to be calling too. For example:

import 'dart:js_interop' hide JS;
import 'package:js/js.dart' show JS;

// Note the types here don't matter, we are
// adding these declarations so that the
// compiler believes it needs to support virtual
// and dynamic dispatch of the methods declared
// here.
@JS() class A { external int get target; }
@JS() class B { external int get result; }
@JS('x') external A get a;
@JS('y') external B get b;

// A strange way to confuse our compiler today
// is to conflate many values into a single
// function
@pragma('dart2js:never-inline')
dynamic confuse(dynamic x) => x;

main() {
  confuse(a);
  confuse(b);
  final o = confuse({'target': {'result': 'ok'}}.jsify());
  // Here the release compiler can't tell whether `o` is an interop type
  // of the style of package:js or the new kind of interop.
  // In the end, this happens to print 'ok'!!
  print(o.target.result);
}

This is unlikely in practice though, you'd clearly need to have reasons for the compiler to keep around target and result as members from other parts of the code.

In contrast, our debug compiler doesn't see the whole program. Instead, it makes all decisions in a modular fashion. Since some modules may call JS values dynamically, it supports all dynamic calls by default.

Until we drop support for package:js, we can't make this an error consistently among the two modes. We could consider in the interim a way to configure debug-mode to disallow using package:js so those who are ready can get the new semantics right away.

Since that's not available yet, one thing that many have opted to do is to enable the avoid_dynamic_calls lint rule to help highlight where these dynamic calls may be occurring in the codebase. This helps provide an early signal that something may not match between debug and release mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. web-js-interop Issues that impact all js interop
Projects
None yet
Development

No branches or pull requests

2 participants