dart2js PPW now fails with Dart 2.8.0-dev.8

[kevmoo]

Dart2Js finished with:

packages/stagexl/src/events/event_stream.dart:199:28:
Error: A value of type 'T' can't be assigned to a variable of type 'InputEvent'.
 - 'InputEvent' is from 'package:stagexl/src/events.dart' ('packages/stagexl/src/events.dart').
      InputEvent.current = inputEvent;
                           ^
Error: Compilation failed.

This has worked for ever. The code in stagexl seems fine. 🤷‍♂

[sigmundch]

can you share the full repro steps?

This sounds likely from a CFE change.

[kevmoo]

1. sync https://github.com/dart-lang/sample-pop_pop_win/
2. pub get
3. pub run build_runner build

Precompiling executable... (9.0s)
Precompiled build_runner:build_runner.
[INFO] Generating build script completed, took 276ms
[INFO] Reading cached asset graph completed, took 211ms
[INFO] Checking for updates since last build completed, took 632ms
[SEVERE] build_web_compilers:ddc on package:stagexl/src/animation.ddc.module:
Error compiling dartdevc module:stagexl|lib/src/animation.ddc.js

packages/stagexl/src/events/event_stream.dart:199:28: Error: A value of type 'T' can't be assigned to a variable of type 'InputEvent'.
 - 'InputEvent' is from 'package:stagexl/src/events.dart' ('packages/stagexl/src/events.dart').
      InputEvent.current = inputEvent;
                           ^

[WARNING] build_web_compilers:entrypoint on web/main.dart:
Unable to read stagexl|lib/src/animation.ddc.js, check your console or the `.dart_tool/build/generated/stagexl/lib/src/animation.ddc.js.errors` log file.
[SEVERE] build_web_compilers:entrypoint on web/main.dart:

AssetNotFoundException: stagexl|lib/src/animation.ddc.js
[INFO] Running build completed, took 4.5s
[INFO] Caching finalized dependency graph completed, took 131ms
[SEVERE] Failed after 4.6s

[sigmundch]

@kevmoo it appears this is a long standing bug that appeared in both analyzer and CFE that finally got fixed in the CFE. Promoted types were used by inference of local variables when it was not always correct (see #40413 for context).

[kevmoo]

@sigmundch "long standing bug" hurts my head – the code seems to run fine – and has for a long time.

I'm guessing there are corner cases that would break that we're not hitting?

So really the bug is in stagexl – we just didn't know it? Analysis is clean on stagexl – likely because they haven't fixed things?

[sigmundch]

Correct. This is the code that shows the issue:
https://github.com/bp74/StageXL/blob/master/lib/src/events/event_stream.dart#L199

  void _dispatchEventInternal(
      T event, EventDispatcher target, EventPhase eventPhase) {
    var subscriptions = _subscriptions;
    var isCapturing = eventPhase == EventPhase.CAPTURING_PHASE;
    var inputEvent = event is InputEvent ? event : null;

    for (var i = 0; i < subscriptions.length; i++) {
      var subscription = subscriptions[i];
      if (subscription.isCanceled ||
          subscription.isPaused ||
          subscription.isCapturing != isCapturing) continue;

      event._target = target;
      event._currentTarget = this.target;
      event._eventPhase = eventPhase;

      InputEvent.current = inputEvent; // <----- ERROR REPORTED HERE
      subscription.eventListener(event);
      InputEvent.current = null;

      if (event.isImmediatePropagationStopped) return;
    }
  }

@johnniwinther - seems like the fix is to not promote on all cases like this. Could we allow it on variables that are not reassigned (as in the example above)?

[johnniwinther]

With the better flow analysis (in nnbd) we could auto promote on initialization. It would require more work though. Such a change cannot be easily be done in the pre-nnbd type promotion.

Without knowledge of how the variable is used (even knowing that the variable is not reassigned) we need to ascribe an intersection type to the variable which we can only do through promotion:

method<T>(T a, T b) {
  if (a is String) {
    var o = a;
    o.length; // We need to know `o` is a `String`.
    b = o; // We need to know `o` is a `T`.
  }
}

In the problem code, the we can type the inputEvent as InputEvent because we never rely in its T-ishness.

[kevmoo]

Have we decided the plan here? Is this a bug in stagexl or something we could fix?

I'm guessing (given frequent rolls internally) this is a pretty weird/rare case

[sigmundch]

We chatted a bit yesterday about this. We are contemplating whether we can reduce the cases where the breaking change needs to happen (e.g. do it only when a variable is initialized and re-initialized).

If so, the stagexl code will continue to work. If not, the necessary change would be to declare the type of inputEvent explicitly as InputEvent, or add a cast on the assignment where the error is reported.

[kevmoo]

Any update here?

Either the CFE should be changed or analyzer – at the moment they disagree!

[sigmundch]

See #40413

[franklinyow]

#40413 is closed, is this still repro?

[sigmundch]

I believe it does. Note that the breaking change is a bug fix. It may not qualify as a breaking change request, but it needs to be announced ahead of the next release (and in the CHANGELOG)

[kevmoo]

There was a change in behavior.

It should (likely) be announced as a breaking change.
Looks like the analyzer fixed their side!

[johnniwinther]

Closing since the breaking issue #40675 has been closed.