Bad JS generated with staticInterop and DDC

[kevmoo]

Code defined in import 'package:google_identity_services_web/oauth2.dart' as gis;

/// The Dart definition of the `google.accounts.oauth2` global.
@JS()
@staticInterop
abstract class GoogleAccountsOauth2 {}

/// The `google.accounts.oauth2` methods
extension GoogleAccountsOauth2Extension on GoogleAccountsOauth2 {
reference#google.accounts.oauth2.initTokenClient
  external TokenClient initTokenClient(TokenClientConfig config);
...

Invoking code in Dart

  final client = gis.oauth2.initTokenClient(config);

Generated JS (that fails)

      let config = {callback: js.allowInterop(T.TokenResponseTovoid(), callback), client_id: clientId, scope: scopes[$toSet]()[$join](" "), prompt: prompt};
      let client = dart.global['GoogleAccountsOauth2Extension|initTokenClient'](dart.global.google.accounts.oauth2, config);
      dart.global['TokenClientExtension|requestAccessToken'](client);
      return completer.future;

This is on my machine w/ the Dart SDK. @ditman using Flutter get what seems to be the right code

CC @srujzs @nshahan

[nshahan]

There are three parts that make the target method being called: the receiver object, the method name and the form of access.

In the compiled code that @kevmoo showed me we see
dart.global['GoogleAccountsOauth2Extension|initTokenClient'](dart.global.google.accounts.oauth2, config);
receiver object: dart.global
method name: 'GoogleAccountsOauth2Extension|initTokenClient'
access form: property get receiver[property_name]

It seems like there are a few things that are not matching up correctly.

If the receiver is correct, then the name and access form are probably wrong. I think it should be something more like dart.global.google.accounts.oauth2.initTokenClient(config).

If the method name and access form are correct then it should be accessing the property on some compiled module to get the top level method. module_name['GoogleAccountsOauth2Extension|initTokenClient'].

[ditman]

Try importing it like this:

import 'package:google_identity_services_web/oauth2.dart';

That exports an oauth2 namespace JS-Interop getter of an object (google.accounts.oauth2) that contains an initTokenClient method.

This is my dart code:

And this is the generated one:

Full dart code here.

(I just noticed that what needs to be imported is quite unclear in the API docs from pub, that list the "libraries" but they're hard to tell apart from the entrypoints of the library, I guess I should only have a google_identity_services_web.dart as the import, and reexport everything from there?)

[ditman]

(I guess the js-interop bug is that the invocation over a named import (import blah as gis), and then gis.oauth2.whatever doesn't work as expected.)

oauth2 is this:

https://github.com/flutter/packages/blob/c68bf70211b278139b6fe7231dd1b692c549df5e/packages/google_identity_services_web/lib/src/js_interop/google_accounts_oauth2.dart#L20-L24

[srujzs]

Nick and I chatted offline, the oddity is that we're using dart.global instead of the library name (google_accounts_oauth2). It's weird that adding an alias for the import makes a difference.

@nshahan Based on dit's comments, I tried out the smaller example we used with namespacing e.g. import blah as gis, but couldn't reproduce the use of dart.global. It's possible the packaging (versus the local modules we were trying) might make the difference, but I'm not sure.

@kevmoo Can you share with us your entire Dart code that repro's the issue? Is it just that one line of code + the import?

[kevmoo]

google/googleapis.dart#491

It's weird, though. In test_integration you need to run dart tool/run_server.dart. This expects a ClientID from a file. If you just hard-code that as garbage, it should be fine though and you'll see the problem.

So this uses build_web_compilers – maybe that's the issue?

[srujzs]

Copy-pasting from chat for prosperity:

Okay, I think I know what's going wrong. I think it has to do with modular compiles of some sort. When compiling the module that contains the invocation of the external extension member, we haven't necessarily visited the library that contains that member, so we might not have done the lowering yet where we mark it non-external. So, DDC sees an external member still, and emits code using dart.global instead of the module name.

I confirmed this in two ways:

1. Disable the lowering and just compile a test file calling an external extension member. This gives us the dart.global[....] syntax since we're using the JS top level name.
2. I compiled DDC where I exclude external extension members from being counted as using JS interop (see pkg/dev_compiler/lib/src/kernel/js_interop.dart:usesJSInterop). I built DDC using that change and tried out Kevin's repro. The output code now looks like let client = google_accounts_oauth2['GoogleAccountsOauth2Extension|initCodeClient'](dart.global.google.accounts.oauth2, config);, which is correct.

This means we'll have to add a slightly annoying change to DDC. We lower not only external extension members but also other external members for @staticInterop classes (like external static members). So, we'll need to add logic to DDC to exclude all the external members that we lower, which requires a bit of extra checking. It also means we need to change DDC whenever we add or remove lowerings. I don't think we have to worry about this for inline classes, as that's new syntax and DDC can always assume that syntax is lowered. Similarly, I don't think dart2js has to worry about this as it's non-modular.

[nshahan]

When compiling the module that contains the invocation of the external extension member, we haven't necessarily visited the library that contains that member, so we might not have done the lowering yet where we mark it non-external.

This is a good explanation but it is also a little surprising to me. When does the lowering occur and how is it that we are compiling the use before we have applied the lowering in the imported library? Is this another case where the summary .dill file doesn't include the modifications made by the lowering?

[srujzs]

It's also surprising to me, as I'd expect the library that contains the member to be compiled first. And indeed, when I print out the modules, modular transformations occur first on googleapis_auth, where the extension member is declared, before token_model.dart, where the member is used.

Is this another case where the summary .dill file doesn't include the modifications made by the lowering?

I thought modular transformations persist until the module is recompiled, and I don't think it's getting recompiled here, so I'm not exactly sure where the modular case pops up.

[nshahan]

I'm just throwing out possible ideas: Since we couldn't reproduce it using ddb to compile a similar pattern and it works in a flutter build, I wonder if this is caused by a build_web_compilers ordering issue. Flutter builds use a different tool to determine modules, ordering, and a different method to compile with DDC.

We should try to make a minimal reproduction with a build_web_compilers build with multiple packages.

[srujzs]

I think you're right. I really want to verify that this is a case that we should support with modular compilation, and not just an unusual ordering by build_web_compilers. We could also try our modular test suite to see if we can reproduce the ordering there. I'll respond back when I have try out some stuff.

[sigmundch]

Just a couple thoughts:

• could we repro this by chance using a modular test suite?
• is it possible that the transform is happening on the full.dill but not on the outline?

[KealJones]

I am running into a similar issue where setting an external field from an extension on a @staticInterop class is getting compiled to a lookup on global.dart instead of the extension array it should be from.

See screenshot of compiled DDC:

https://github.com/KealJones/reactor/blob/dart3-ddc-issue/web/index.dart

I didnt realize @staticInterop was going to be deprecated in favor of inline classes... so nevermind? #49353 (comment)

[srujzs]

FYI - the plan is not to deprecate for a while. We still need to release inline classes and make sure all existing interop users have a path forward, so we still want to fix any bugs with @staticInterop.

There's a very recent change to lower interop calls at the call-site instead of in the declaration that might make this bug obsolete: d4adfcc

I can't seem to get your project working with the bleeding-edge SDK, however:

Resolving dependencies...
The current Dart SDK version is 3.1.0-edge.be5513a9e0bcdfcd23bff355b81d930b76a4346a.

Because reactor depends on react_testing_library any which doesn't support null safety, version solving failed.

The lower bound of "sdk: '>=2.2.2 <3.0.0'" must be 2.12.0 or higher to enable null safety.
For details, see https://dart.dev/null-safety

[KealJones]

@srujzs Sorry, master is trash, try using the ‘dart3-ddc-issue’ branch which is a snapshot of the ‘dart3-explore’ branch (it is what I used for the screenshots above) those branches are where I started using Dart 3.0.2 and switched to using @staticInterop. I am traveling at the moment but I can try the latest dev release tomorrow. I just realized you are using an edge release which I don’t think I have access too but regardless the fix sounds like it should resolve most of the issues that I have been experiencing. 🤞

[KealJones]

@srujzs I actually had a chance to test it just now, and the issue seems to be resolved in Dart 3.1.0-155.0.dev

woo thanks for the info and help! :)

[srujzs]

I'm not sure what went in to fix your specific issue, but I'll take it. :)

@kevmoo, are you still coming across this issue? It seems like this package has been updated since the last time I took a look.

[kevmoo]

@srujzs – no! it works now. Forgot to close out!