Document implications of tracking widget creation, and use of --no-track-widget-creation flag

[ghost]

@2ZeroSix commented on Apr 30, 2020, 7:51 PM UTC:

using dart sdk shipped with flutter:

Flutter (Channel master, 1.18.0-9.0.pre.74, on Mac OS X 10.15.3 19D76,
    locale en-RU)
    • Flutter version 1.18.0-9.0.pre.74 at /Users/iam/development/flutter
    • Framework revision 0d452b8305 (42 minutes ago), 2020-04-30 11:20:14 -0700
    • Engine revision 2db3276573
    • Dart version 2.9.0 (build 2.9.0-3.0.dev 726d3c7725)

this code works differently only in debug build

import 'package:flutter/widgets.dart';

void main() {
  final generatedListOfConst = [
    ...{
      const SizedBox(),
      const SizedBox(),
    },
  ];
  print(generatedListOfConst);
}

expected (dev_compiler, aot, ...): [SizedBox] actual (Dart VM): [SizedBox, SizedBox]

same result for any other Flutter widget in place of SizedBox, but for some reason it does work as expected for other objects

class EmptyWidget extends StatelessWidget {
  const EmptyWidget({Key key}) : super(key: key);

  @override
  Widget build(BuildContext context) {
    return Container();
  }
}

class Empty {
  const Empty();
}

void main() {
  final generatedListOfConstWidgets = [
    ...{
      const EmptyWidget(),
      const EmptyWidget(),
    },
  ];
  print(generatedListOfConstWidgets);

  final generatedListOfConst = [
    ...{
      const Empty(),
      const Empty(),
    },
  ];
  print(generatedListOfConst);

  final generatedListOfStr = [
    ...{
      'a',
      'a',
    },
  ];
  print(generatedListOfStr);
}

Dart VM output:

[EmptyWidget, EmptyWidget]
[Instance of 'Empty']
[a]

This issue was moved by keertip from dart-lang/sdk#41728.

[ghost]

@2ZeroSix commented on Apr 30, 2020, 8:05 PM UTC:

Oh, it's one level lower, it's LinkedHashSet inconsistency, everything above works the same without "Control Flow Collections" syntax, but with bare default sets

every [...{_list_}] may be replaced with {_list_}

[ghost]

@pcsosinski commented on Apr 30, 2020, 11:38 PM UTC:

/cc @mraleph @franklinyow

[ghost]

@jonahwilliams commented on May 1, 2020, 12:07 AM UTC:

isn't this just the track-widget-creation transformer at work?

[Hixie]

Yeah, that would make sense. @2ZeroSix does it still repro if you run with --no-track-widget-creation?

[mraleph]

Should toString() on the classes processed by widget creation tracking maybe changed to make this more obvious? I think this is not the first time people are confused by this. Or maybe widget creation tracking should be implemented in way that does not interfere with object identity. (though that would require building such a mechanism into the language).

[mraleph]

/cc @jacob314 for visibility

[2ZeroSix]

There is a mistake in my initial description, this issue applies for both dev_compiler (debug for web) and dart vm (debug for mobile).

---

I've localized this issue a bit more:

  print('${const SizedBox() == const SizedBox()}');

in debug: false in profile/release: true

---

Yeah, that would make sense. @2ZeroSix does it still repro if you run with --no-track-widget-creation?

@Hixie With --no-track-widget-creation dev_compiler behavior changed, but not dart vm: in debug(mobile): false in debug(web)/profile/release: true

---

And the funniest part: hot restart changes behavior of mobile debug build with --no-track-widget-creation

1. flutter run --debug (mobile) output: false

2. R (hot restart) output: false

3. flutter run --debug --no-track-widget-creation (mobile) output: false

4. R (hot restart) output: true

[2ZeroSix]

1. flutter run --debug --no-track-widget-creation (mobile) output: false
2. R (hot restart) output: true

It seems that flag is ignored somehow on the initial run:

after hot restart

So it should be another issue

[jacob314]

A middle ground which might be more confusing would be to override the equals and hashcode for widgets so that you still get the correct equality behavior even though you would not get the correct identity behavior. The equality operator for Widget is annotated as not being override-able so this would be simpler than it once was to implement. Experimentation would be needed to test the performance impact.

[jonahwilliams]

@2ZeroSix fixed the bug with the inconsistency (Thank you!), what other issues is this bug tracking?

[2ZeroSix]

@jonahwilliams current equality behavior of const widgets might cause bugs not reproducible in common debug mode, but only in release or debug with explicitly disabled tracking.

It should be either clearly documented or be consistent.

[jonahwilliams]

Makes sense. at any rate, I'm removing the tool designation unless we need more tooling behavior changes

[Hixie]

@2ZeroSix where did you look for documentation about this? (I ask to figure out where to add that documentation, it's not clear to me where we would document it.)

[2ZeroSix]

@Hixie I think the best solution: workaround this behavior somehow as @jacob314 proposed.

As a temporary solution it would be nice to warn users if they call operator == on const widgets in user code. But this might be harder than appropriate fix.

There is a few places where this might be highlighted in website docs:

• https://flutter.dev/docs/testing/debugging - probably the first place where I looked when I faced this problem.
• https://flutter.dev/docs/development/tools/devtools/inspector#track-widget-creation - there is an outdated info: flag is enabled by default in debug mode. (don't know since which version)

Maybe it can also be added in flutter cli help output: for example flutter run --help. But this approach can introduce unwanted noise in the output, so it may become unusable.

[Hixie]

@jacob314 is it feasible to reproduce the const == behavior in the kernel transformer? e.g. can it label all the instances that would have been == with the same unique ID and then have operator == compare that ID?

@2ZeroSix thanks, those are useful places for us to start.

[Hixie]

Docs PRs: https://github.com/flutter/website/pull/4105, https://github.com/flutter/website/pull/4106

[jacob314]

@Hixie that is possible. Hopefully the overhead of doing it wouldn't be too much. We could reproduce the const == and hashcode behavior like that in the CFE now that const canonicalization takes place in the CFE anyway. It will require a bit more than the typical kernel transformer work I'm familiar with as it will need to interact with existing const canonicalization code rather than performing a simpler ast to ast transform. identical would still not return that the objects were identical but it is definitely an improvement over what we do today.