Kernel outlines do not preserve annotations

[vsmenon]

DDK JS interop relies on @JS annotations being preserved (#36912).

E.g., given this Dart:

@JS('console.log')
external void log(String s);

The kernel file is:

  @js::JS::•("console.log")
  external static abstract method log(core::String s) → void;

We don't generate code for log. Instead, when a caller calls log, it's replaced with console.log based on the JS annotation.

The outline dill is missing the JS annotation, so we're breaking on JS calls across module: we're trying to call a log method that never was generated.

[vsmenon]

@kmillikin - do you have a sense of how hard this is to fix? We've had a few external flutter web users downgrading back to ddc to avoid this.

[kmillikin]

I don't expect this to be hard to fix. Is there a simple reproduction that we can use to verify a fix?

[vsmenon]

Here's a repro:

> git clone https://github.com/vsmenon/b36944.git
> cd b36944
> pub get
> webdev serve --debug --auto restart

You should see a popup alert. Instead, in the console, there is an error message:

Uncaught TypeError: alert$.alert is not a function

If you modify the pubspec.yaml, you can see the build_web_compilers constraint. Below 2.0.0 forces the old DDC.

Basically, the outline dill generated for lib/alert.dart needs to have the JS annotation on it. DDC doesn't actually generate code for functions or classes marked JS. Instead, the caller forwards directly. With this bug, the caller cannot detect the function is actually JavaScript.

[sigmundch]

I've been making progress on creating a modular-test pipeline that we can use to write small unit and regression tests of modular scenarios in the SDK repo.

I was just able to create the same repro with this new framework. Here is how to repro:

• patch cl 103680
• run dart pkg/dev_compiler/test/modular/modular_test.dart --verbose

All steps are invoking the kernel-worker and ddk using sub processes. The test itself is run in d8 to show the failure.

[sigmundch]

The CL has now landed, you can run the test directly via:

dart pkg/dev_compiler/test/modular/modular_test.dart --filter js_interop --verbose

[kmillikin]

1df5fb3d5797a44aa64770bace1573484486d7f4 seems to fix the issue with JS interop in DDC.

I'm going to leave this issue open because we are still missing some outline annotations (typedefs, imports, and perhaps others) that we might want. Otherwise I'm deprioritizing the issue and removing it as a D24 blocker.

[mraleph]

I reverted the change (1df5fb3d5797a44aa64770bace1573484486d7f4) due to regressions in Flutter benchmarks it introduced.

[jakemac53]

Do we need to make this a configurable option or something for now?

We could just enable it for the kernel worker then.

[kmillikin]

@alexmarkov found that annotations from patch files were missing (thanks, Alex!). I'll fix it asap starting Monday.

[vsmenon]

@kmillikin - assigning back to you. If there are perf issues with this (as opposed to correctness issues), we should consider making this a configurable option as Jake suggests to unblock flutter web users (https://github.com/flutter/flutter/issues/32524)/

[kmillikin]

Annotations on classes and procedures from patch files are preserved. This fixes the performance regression, where platform-specific annotations were lost.

We now combine annotations from the original file and the patch file (including keeping the @patch annotation), which should match the previous behavior.

We are still not writing into outlines the annotations on typedefs, imports, and perhaps a few other places.

[vsmenon]

Thanks!

This does work with my local repros now.

@natebosch - Could you sanity check this with firebase (https://github.com/flutter/flutter/issues/32524)?

@sigmundch - I don't think your test is automatically run on the bots. It is passing for me locally now, but I don't see it in the bot logs.

[sigmundch]

@sigmundch - I don't think your test is automatically run on the bots. It is passing for me locally now, but I don't see it in the bot logs.

it's there with the pkg tests. It appears that after the revert, the test was not marked as failing, so the test was making the bots red for a while. Kevin's change made the bot green, but you can see the previous build was red: https://ci.chromium.org/p/dart/builders/ci.sandbox/pkg-linux-release/5846

And it was only because of that modular test: https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket.appspot.com/8911709612652691280/+/steps/test_results/0/logs/unapproved_failing_tests/0

[vsmenon]

Ahh, got it - thanks @sigmundch !

[robbecker-wf]

@aadilmaan or @vsmenon Will fixes for this issue make it into the next release or did it get pushed back?

[aadilmaan]

This issue has been moved out of D24 to the follow up release (D25 - schedule to be finalized).

[jakemac53]

JS interop is almost completely broken with kernel based DDC because of this right now. Are we saying that isn't a release blocker? If that is the case we should consider a patch release quickly following D24, and not waiting for D25.

[vsmenon]

@robbecker-wf @jakemac53 JS interop is fixed and regression tested now (thanks - @sigmundch !). Those fixes will be part of D24.

We're keeping this open for completeness. As @kmillikin says above, annotations are not preserved on typedefs and imports. JS interop shouldn't depend on those today, but they should also be preserved for consistency.

[vsmenon]

The latest dev - https://github.com/dart-lang/sdk/tree/2.3.2-dev.0.1 - should have the fixes. Sanity checking appreciated!

[vsmenon]

Specifically, this is the commit from Kevin that fixes JS interop:

https://github.com/dart-lang/sdk/commit/907f31e6de8eee0b7b0cb0c9e7a7fef5dc8a6794

It's included here:

https://github.com/dart-lang/sdk/commits/2.3.2-dev.0.1

[evanweible-wf]

@vsmenon 2.3.2-dev.0.1 fixes our JS-interop-dependent tests 👍

[vsmenon]

@evanweible-wf - thank you for the quick report!