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

DDC lowers object literal/anonymous factory constructors using the invocation's ordering #55675

Open
srujzs opened this issue May 8, 2024 · 3 comments
Assignees
Labels
area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. web-dev-compiler web-js-interop Issues that impact all js interop

Comments

@srujzs
Copy link
Contributor

srujzs commented May 8, 2024

Given a declaration:

import 'dart:js_interop';

extension type Foo._(JSObject o) {
  external Foo({int a, int b});
}

and an invocation:

Foo(b: 0, a: 0);

DDC lowers the result in JS as {b: 0, a: 0}, whereas dart2wasm and dart2js lower it as {a: 0, b: 0}, respecting the procedure's ordering instead.

FWIW, I think DDC is correct here but, fixing the inconsistency in DDC here is likely less breaking.

This also applies to @anonymous factory lowerings.

cc @nshahan

@srujzs srujzs added web-js-interop Issues that impact all js interop web-dev-compiler area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. labels May 8, 2024
@srujzs srujzs self-assigned this May 8, 2024
@sigmundch
Copy link
Member

I'm curious why this inconsistency became visible :)

FWIW, dart2js does preserve the evaluation order, but will introduce temporary variables if needed to ensure that expressions are evaluated as expected.

import 'dart:js_interop';

extension type Foo._(JSObject o) {
  external Foo({int a, int c, int b});
}

@pragma('dart2js:never-inline')
int effect(int i) {
  print('see $i');
  return i;
}

main() {
  Foo(b: effect(0), c: effect(1), a: effect(2));
}

Prints see 0, see 1 and see 2 in order, even though it does create an object literal with keys in the declaration order a, c, b in this case.

@srujzs
Copy link
Contributor Author

srujzs commented May 9, 2024

I saw some devtools code caring about the order of procedure parameters, and it had me looking at some dart2wasm code and making sure it's consistent with every other compiler. So, it's not a real issue anyone filed. :) You can observe this inconsistency with Object.keys.

The evaluation order is interesting here. I'm not quite sure if we can keep that consistent in every compiler. In dart2wasm, due to the indirection/trampolining, it maintains the same order as the procedure i.e. see 2, see 1, see 0, because it directly transforms the call to a call to a trampoline whose parameters are ordered according to the procedure. This then has the effect that the arguments are evaluated in the procedure order and not the invocation order. DDC does the same, but because its order is invocation-dependent, it follows dart2js' evaluation order currently i.e. see 0, see 1, see 2. If we change the order of the keys to correspond to the behavior in dart2js and dart2wasm like in https://dart-review.googlesource.com/c/sdk/+/365924, this would then become see 2, see 1, see 0.

In general, the order may matter for dictionaries: https://webidl.spec.whatwg.org/#idl-dictionaries.

Dictionaries need to have their members ordered because in some language bindings the behavior observed when passing a dictionary value to a platform object depends on the order the dictionary members are fetched.

So, it seems we're in a weird inconsistency matrix today:

dart2wasm: object key order corresponds to the procedure, and so does the evaluation order

dart2js: object key order corresponds to the procedure, but evaluation order corresponds to the invocation

dartdevc: object key order corresponds to the invocation, and so does the evaluation order

Of course, we could just document this as undefined behavior and recommend users always pass arguments in the order of the parameters.

@sigmundch
Copy link
Member

Ha! That's interesting. I think we can make this consistent among all there, but it requires adding temporaries before the call is made. In essence the dart2js behavior is similar to a kernel transformation of the form:

let t0 = effect(0) in
  let t1 = effect(1) in
    let t2 = effect(2) in
       Foo(a: t2, c: t1, b: t0);

We could do this in a kernel transformation, but I'd be inclined to do it at the codegen stage (DDC already has logic to introduce temporaries and I assume dart2wasm must have a similar mechanism as well).

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-dev-compiler web-js-interop Issues that impact all js interop
Projects
None yet
Development

No branches or pull requests

2 participants