Revert "Use track-widget-creation transformer included in the sdk. (#9085)"

[jason-simmons]

This reverts commit 651c904.

With this change applied, test/widgets/widget_inspector_test.dart is failing
when run by the dev/bots/test.dart script in the framework tree.

[jason-simmons]

The change to the track-widget-creation transformer was causing failures in the framework test suite.
(example: https://api.cirrus-ci.com/v1/task/5921277928275968/logs/test.log)

To reproduce:

• update bin/internal/engine.version to a commit that includes 651c904
• delete packages/flutter/build/testfile.dill.track.dill if it exists from a previous run of the test suite
• run dart --enable-asserts dev/bots/test.dart

test/widgets/widget_inspector_test.dart will show errors.

Interestingly, if you run dev/bots/test.dart again (with the testfile.dill.track.dill left over from the previous run) the test will succeed.

The test also runs as expected if you just run flutter test --track-widget-creation test/widgets/widget_inspector_test.dart instead of the full suite.

@terrylucas @jacob314 @aam

[jacob314]

Fyi @askeksa-google. I thought Terry landed this change 5 days ago. Did something change on the dart:sdk side that is causing it to fail now?
For example, did the flag to enable const canonicalization turn on?

[jacob314]

Try removing the const
at
line 799 of widget_inspector_test.dart
and 859 of widget_inspector_test.dart
if removing those const fixes the two tests then that means something is not quite right with the kernel transformer running before the const evaluation assuming const evaluation was turned on in the cfe.

[jason-simmons]

I believe the affected test started failing as soon as 651c904 landed. However, we didn't notice for a few days due to the weekend and because the engine->framework roller was failing earlier this week due to some unrelated CI errors.

[jason-simmons]

The widget_inspector_test error still occurs if I remove const on those lines

[terrylucas]

Jason I can't find a version in the repository of bin/internal/engine.version that has a commit that includes 651c904 <651c904> Here are the commits to engine.version (May 22 doesn't have the 651c904 commit and May 29 has the commit and reverted too). commit abca503 Author: engine-flutter-autoroll <[email protected]> Date: Wed May 29 23:28:18 2019 -0400 Roll engine 8dc3a4c..4c4c0f8 (47 commits) (#33546) 8dc3a4c...4c4c0f8 git log 8dc3a4c..4c4c0f8 --no-merges --oneline 4c4c0f8 Add plugin shim to facilitate old plugins in new embedding (#33478). (flutter/engine#9120) ... 44f1b44 Revert &#34;Use track-widget-creation transformer included in the sdk. (#9085)&#34; (flutter/engine#9134) ae14c5a Roll src/third_party/dart fee615c..d5405d0 (21 commits) (flutter/engine#9127) ... 651c904 Use track-widget-creation transformer included in the sdk. (flutter/engine#9085) ... 9d2d58a Add mouse button support to the macOS shell (flutter/engine#9054) ... commit 018df5e Author: Jonah Williams <[email protected]> Date: Wed May 22 16:12:02 2019 -0700 Tried editing engine.version to my local SHA change (that has the CL patched in) but that doesn't work either. Tried three ways: 1. After a build that works (flutter run --local-engine=android_debug_unopt works) with an unedited engine.version, edited engine.version to point to my local SHA (patched in CL) doesn't work. 2. Edit engine.version to point to my local SHA (patched in CL), delete out directory and rebuilt still fails. 3. Also tried rebuilding with untouched engine.version and a patched in the CL but "flutter test test/widgets/widget_inspector_test.dart" doesn't fail Running flutter test with --local-engine doesn't work (although flutter run with --local-engine works). flutter test --local-engine=android_debug_unopt test/widgets/widget_inspector_test.dart ! sky_engine 0.0.99 from path /Users/terry/flutter-engine2-git/engine/src/out/host_debug_unopt/gen/dart-pkg/sky_engine ! sky_services 0.0.99 from path /Users/terry/flutter-engine2-git/engine/src/out/host_debug_unopt/gen/dart-pkg/sky_services Running "flutter pub get" in flutter... 0.5s Cannot find Flutter shell at /Users/terry/flutter-engine2-git/engine/src/out/android_debug_unopt/flutter_tester Is there something else I need to do?

…

On Thu, May 30, 2019 at 12:57 PM Jason Simmons ***@***.***> wrote: The widget_inspector_test error still occurs if I remove const on those lines — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9134?email_source=notifications&email_token=AAPV5QMRQIYS6M47NSWWD3TPYAWT5A5CNFSM4HQYENJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWTLHFA#issuecomment-497464212>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAPV5QPCGX44RCOPF3JXLVLPYAWT5ANCNFSM4HQYENJA> .

[jason-simmons]

You can set bin/internal/engine.version to 651c90409ee7b8f43da7d1c60fcb172f1ad51901 to download an engine that includes that patch.

bin/internal/engine.version can be set locally to any commit from https://github.com/flutter/engine where an engine build completed successfully and was posted to cloud storage. Updates to bin/internal/engine.version will only be rolled into https://github.com/flutter/flutter if they passed the CI test suite. In this case, 651c90409ee7b8f43da7d1c60fcb172f1ad51901 was not committed to https://github.com/flutter/flutter because the widget_inspector_test failed.

[askeksa-google]

From the test failures it looks like the widget objects are not properly transformed. Is the track-widget-creation / trackWidgetCreation flag correctly passed when the front end is run from the widget inspector test?

Maybe the Flutter change did not account for this SDK change: dart-lang/sdk@b08397c

[jacob314]

@askeksa-google that patch from @mraleph should only apply to flutter engine b48c8b1 so I don't think that is it.
Additionally, the widget_inspector.dart test determines that --track-widget-creation is enabled by checking whether class Widget. Thus the test failure described suggests that package:flutter was compiled with --track-widget-creation enabled but the specific test case for some reason wasn't compiled with --track-widget-creation

Code used to determine whether to run tests that require --track-widget-creaiton.

  /// Returns whether [Widget] creation locations are available.
  ///
  /// [Widget] creation locations are only available for debug mode builds when
  /// the `--track-widget-creation` flag is passed to `flutter_tool`. Dart 2.0
  /// is required as injecting creation locations requires a
  /// [Dart Kernel Transformer](https://github.com/dart-lang/sdk/wiki/Kernel-Documentation).
  bool isWidgetCreationTracked() {
    _widgetCreationTracked ??= _WidgetForTypeTests() is _HasCreationLocation;
    return _widgetCreationTracked;
  }

That patch shouldn't

[terrylucas]

Aske is there anything in the CFE/kernel transformer AST that might be in the dill file (or something that's not computed until there is a dill file?
As I run the failing test (first time the dill file is deleted) the tests will fail. After this failure all tests pass. As I debug the tests, when there's success, the location object is the file name of the widget that was created.

Vijay mentioned there a dill dump tool - I'm going to look at the dump. If you think it's interesting to see this dill file I'll attach it too.

[terrylucas]

====================================
Run #1

Flutter test run w/o --track-widget-creation
cd flutter/packages/flutter
flutter test

inside of widget_inspector_test.dart the WidgetInspectorService's isWidgetCreationTracked() is false therefore this test is not compiled w/ track-widget-creation from the kernel transformer point of view.

====================================
Run #2

flutter test --track-widget-creation

inside of the widget_inspector_test.dart the WidgetInspectorService's isWidgetCreationTracked() is true so this test should have been compiled w/ track-widget-creation

There is a dill file generated testfile.dill.track.dill however the test fails because no creationLocation entry can be found (at the top-level same level as children) the getSelectedWidget JSON payload is:

  description: Text,
  type: _DiagnosticableTreeNode,
  style: dense,
  hasChildren: true,
  allowWrap: false,
  objectId: inspector-0,
  valueId: inspector-1,
  widgetRuntimeType: Text,
  children: [
    {description: RichText,
    type: _DiagnosticableTreeNode,
    style: dense,
    allowWrap: false,
    objectId: inspector-2,
    valueId: inspector-3,
    widgetRuntimeType: RichText,
    locationId: 1,
    creationLocation: {
      file: file:///Users/terry/flutter-engine2-git/flutter/packages/flutter/lib/src/widgets/text.dart,
      line: 386,
      column: 21,
      parameterLocations: [
        {file: null, line: 387, column: 7, name: textAlign},
        {file: null, line: 388, column: 7, name: textDirection},
        {file: null, line: 389, column: 7, name: locale},
        {file: null, line: 390, column: 7, name: softWrap},
        {file: null, line: 391, column: 7, name: overflow},
        {file: null, line: 392, column: 7, name: textScaleFactor},
        {file: null, line: 393, column: 7, name: maxLines},
        {file: null, line: 394, column: 7, name: strutStyle},
        {file: null, line: 395, column: 7, name: textWidthBasis},
        {file: null, line: 396, column: 7, name: text}]},
        children: []}
      ]}

====================================
Run #3

When the dill file is generated all run with --track-widget-creation works, a dump of the dill file shows that creationLocation field exist so the tests all run successfully e.g.,

  final wid::_Location creationLocation = wid::_getCreationLocation(value);

Notice after the dill is generated there is a creationLocation (at the same level as children prefix with **)

description: Text,
type: _DiagnosticableTreeNode,
style: dense,
hasChildren: true,
allowWrap: false,
objectId: inspector-0, 
valueId: inspector-1, 
widgetRuntimeType: Text, 
locationId: 10, 

** creationLocation: {file: file:///Users/terry/flutter-engine2-git/flutter/packages/flutter/test/widgets/widget_inspector_test.dart,
  line: 803,
  column: 15, 
  parameterLocations: [
    {file: null,
     line: 803, 
     column: 20, 
     name: data}
  ]
},

children: [
  {description: RichText, 
  type: _DiagnosticableTreeNode,
  style: dense,
  allowWrap: false,
  objectId: inspector-2,
  valueId: inspector-3,
  widgetRuntimeType: RichText,
  locationId: 4,
  creationLocation: {
    file: file:///Users/terry/flutter-engine2-git/flutter/packages/flutter/lib/src/widgets/text.dart,
    line: 386,
    column: 21,
    parameterLocations: [
      {file: null, line: 387, column: 7, name: textAlign},
      {file: null, line: 388, column: 7, name: textDirection},
      {file: null, line: 389, column: 7, name: locale},
      {file: null, line: 390, column: 7, name: softWrap},
      {file: null, line: 391, column: 7, name: overflow},
      {file: null, line: 392, column: 7, name: textScaleFactor},
      {file: null, line: 393, column: 7, name: maxLines},
      {file: null, line: 394, column: 7, name: strutStyle},
      {file: null, line: 395, column: 7, name: textWidthBasis},
      {file: null, line: 396, column: 7, name: text}]}, children: []}
    ]}

[askeksa-google]

Is there a plan to re-land this, now that the SDK issue is fixed by https://dart-review.googlesource.com/c/sdk/+/105661 ?

[terrylucas]

Flutter 1.7 is locked into the Dart 2.4 and this fix <https://dart-review.googlesource.com/c/sdk/+/105661> wont be cherry-picked into 2.4. Jacob's PR, to use the new kernel transformer, will land in the Flutter engine when the next > 2.4 Dart is rolled into Flutter and an engine is built. This will happen sometime after June 24 when Flutter 1.7 stabilizes.

…

On Fri, Jun 14, 2019 at 4:38 AM Aske Simon Christensen < ***@***.***> wrote: Is there a plan to re-land this, now that the SDK issue is fixed by https://dart-review.googlesource.com/c/sdk/+/105661 ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9134?email_source=notifications&email_token=AAPV5QOM4QJ4U7LZRBZQXPLP2N7MXA5CNFSM4HQYENJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXWRFWI#issuecomment-502076121>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAPV5QLEBI53HEABHSWL4JDP2N7MXANCNFSM4HQYENJA> .