Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dart 2 class generics bug? #32425

Closed
brianegan opened this issue Mar 6, 2018 · 27 comments
Closed

Dart 2 class generics bug? #32425

brianegan opened this issue Mar 6, 2018 · 27 comments
Assignees
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. customer-flutter P1 A high priority bug; for example, a single project is unusable or has many test failures vm-hot-reload
Milestone

Comments

@brianegan
Copy link

brianegan commented Mar 6, 2018

Hey all,

I've run into a funny issue. What I'm trying to accomplish: Using Generics within a Type. I might be approaching this the wrong way!

What I'd like to be able to do, but doesn't work:

// From Flutter
class StoreProvider<S> {

  factory StoreProvider.of() {
    return (context.inheritFromWidgetOfExactType(StoreProvider<S>) as StoreProvider<S>)
        .store
  }
}

I have a class like this to get it working:

class StoreFinder<S> {
  Store<S> of(BuildContext context) {
    // Use the class' capture S type to create a new StoreProvider with the correct generic and capture the type
    final type = new StoreProvider<S>().runtimeType;

    return (context.inheritFromWidgetOfExactType(type) as StoreProvider<S>);
  }
}

The problem: If I have the following class and call it like so: new StoreCaptor<String>(), everything works!

// ignore: must_be_immutable
class StoreCaptor<S> extends StatelessWidget {
  static const Key captorKey = const Key("StoreCaptor");

  Store<S> store;

  StoreCaptor() : super(key: captorKey);

  @override
  Widget build(BuildContext context) {
    store = new StoreFinder<S>().of(context);

    return new Container();
  }
}

If I have a class like this (with one extra generic), it fails: new StoreCaptor<String, String>()

// ignore: must_be_immutable
class StoreCaptor<S, A> extends StatelessWidget {
  static const Key captorKey = const Key("StoreCaptor");

  Store<S> store;

  StoreCaptor() : super(key: captorKey);

  @override
  Widget build(BuildContext context) {
    store = new StoreFinder<S>().of(context);

    return new Container();
  }
}

However! If I slightly change the second StoreCaptor class called in the same way to use A as the generic instead of S, everything works?

// ignore: must_be_immutable
class StoreCaptor<S, A> extends StatelessWidget {
  static const Key captorKey = const Key("StoreCaptor");

  Store<A> store;

  StoreCaptor() : super(key: captorKey);

  @override
  Widget build(BuildContext context) {
    // StoreFinder works fine and gives me back a Store!
    store = new StoreFinder<A>().of(context);

    return new Container();
  }
}

I have no idea why, seems really odd that simply changing the order of the generics in the class will cause my code to work :( My class that calls StoreFinder needs to support 2 generics, so I can't simply support one generic. In addition, changing those params around would be a bit weird from an API perspective, so I'd like to keep them in their current order.

@zoechi
Copy link
Contributor

zoechi commented Mar 6, 2018

What Dart/Flutter version are you using?

@mraleph
Copy link
Member

mraleph commented Mar 6, 2018

If I have a class like this (with one extra generic), it fails: new StoreCaptor<String, String>()

@brianegan your code under this line says:

    // StoreFinder works fine and gives me back a Store!
    store = new StoreFinder<S>().of(context);

Should it instead say StoreFinder _does not work_ and _does not_ give me back a Store!?

@brianegan
Copy link
Author

brianegan commented Mar 6, 2018

@mraleph Indeed it should. Meant only for the second example. Thanks for catching that!

This was on Dart 2.0.0-edge.0d5cf900 from the Flutter beta channel.

@lrhn
Copy link
Member

lrhn commented Mar 6, 2018

I can't see anything obvious in the example, so I'll just give you a different workaround.

You can't write:

Type x = StoreFinder<S>;

but you can use a helper class (or in Dart 2, even a helper function) to get that type:

class _TypeOf<T> { Type get type => T; }
Type _typeOf<T>() => T;
...
Type x = new _TypeOf<StoreProvider<S>>().type;
Type y = _typeOf<StoreProvider<S>>();

That might be easier than creating an entire extra class, doing instantiation and using runtimeType (never use runtimeType, it's bad for you!)

With that, you can write:

factory StoreProvider.of() {
  Type storeProviderType = _typeOf<StoreProvider<S>>(): // or use the class in Dart 1.
  return (context.inheritFromWidgetOfExactType(storeProviderType) as StoreProvider<S>)
      .store
}

which is almost what you originally wanted.

@brianegan
Copy link
Author

Thanks @lrhn, I'll give that a shot and report back! I was definitely trying to avoid runtimeType but couldn't see a workaround :)

@brianegan
Copy link
Author

brianegan commented Mar 6, 2018

@lrhn Worked like a charm. I'll go with that approach, thanks so much!

General Q: This feels like a bit of a workaround, will something like this be supported in Future<Dart>?

@mraleph
Copy link
Member

mraleph commented Mar 6, 2018

Reduction

class A<T> { }

final map = {};

class B<T> {
  void foo() {
    print(map[new A<T>().runtimeType]);
  }
}

class C<T, U> {
  void build() {
    new B<T>().foo();
    new B<U>().foo();
  }
}

void main() {
  map[new A<String>().runtimeType] = 42;
  new C<String, String>().build();
}
$ pkg/vm/tool/dart2 /tmp/x.dart
null
42

@jensjoha or @crelier: could one of you take a look if you have time?

@lrhn
Copy link
Member

lrhn commented Mar 6, 2018

General Q: This feels like a bit of a workaround, will something like this be supported in Future?

I hope so. I intend so. Let's see if I succeed :)

@mraleph mraleph added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. P1 A high priority bug; for example, a single project is unusable or has many test failures customer-flutter labels Mar 6, 2018
@mraleph mraleph added this to the I/O Beta 2 milestone Mar 6, 2018
@brianegan
Copy link
Author

Yay! If not, maybe even a global helper / standard pattern for this type of thing would be great.

@mraleph Thanks for reducing it to a good test case :)

@crelier
Copy link
Contributor

crelier commented Mar 6, 2018

Thanks for the report.
This is the case of an optimization coming back to bite us.
The VM runtime is reusing type argument vectors when possible, to avoid reallocation, even if the vector happens to be longer than needed. Extra type arguments in the vector are simply ignored (well, not always, that's the bug).
This optimization applies on this line:
new B().foo();
The vector <T, U> of class C (or more exactly its subvector of length 1, i.e. ) is compatible with the required vector of class A.
The new instance of B shares the type argument vector of its instantiator and ignores the second type argument.
This optimization does not apply on this line, however:
new B().foo();
because <T, U> does not overlap with at index 0.

Now, the problem happens when runtimeType canonicalizes the types. Canonicalization fails to ignore the extra type argument and two different "canonical" types exists for A.
I'll work on a fix and ensure that type canonicalization is reallocating vectors with extra arguments.
Note that the native call Object_haveSameRuntimeType has the same bug. I'll fix it too.

@crelier
Copy link
Contributor

crelier commented Mar 7, 2018

CL is still waiting for a review: https://dart-review.googlesource.com/c/sdk/+/45340

@brianegan
Copy link
Author

Thanks for the fast response and fix :)

@crelier
Copy link
Contributor

crelier commented Mar 8, 2018

You are welcome. Thanks for the report.

@brianegan
Copy link
Author

brianegan commented Mar 24, 2018

@lrhn Do you all know if this Type _typeOf<T>() => T; is fully working in Dart 2? Are there bugs around this?

I changed my code to using Generic Types on Methods / Functions when I upgraded my lib to support Dart 2, and since I've had a ton of bug reports on my repo using this technique. Using the class solution class _TypeOf<T> { Type get type => T; } seems to fix the problem for some of my users. Is the generic methods / functions feature complete and working? Should we rely on it?

I've got a test suite that relies on this method and it works, but for others it doesn't.

brianegan/flutter_redux#30

@eernstg
Copy link
Member

eernstg commented Mar 26, 2018

In Dart 2, Type _typeOf<T>() => T; will definitely return the actual type argument of _typeOf as an instance of Type, but the question is to which extent your actual tool chain implements Dart 2 at this point in time, and going ahead. The support in the vm for reification of the actual arguments of generic function invocations has been controlled by the flag --reify-generic-functions for a while, so one thing you could try would be to use that.

@brianegan
Copy link
Author

brianegan commented Mar 26, 2018

@eernstg Thanks for the heads up! I'll try that out.

Do you happen to know if the --refiy-generic-functions option is enabled by default when the --preview-dart-2 flag is added?

@eernstg
Copy link
Member

eernstg commented Mar 26, 2018

--preview-dart-2 is intended to subsume --reify-generic-functions at some point, but it might not have happened yet. It's a matter of days and exact version numbers...

@brianegan
Copy link
Author

@eernstg Thanks much -- appreciate the info!

@kyle-berry-perficient
Copy link

kyle-berry-perficient commented Apr 19, 2018

From the example above, calling this code crashes the app in Android.

Flutter version
Flutter 0.3.0 • channel dev • https://github.com/flutter/flutter.git
Framework • revision c73b8a7cf6 (7 days ago) • 2018-04-12 16:17:26 -0700
Engine • revision 8a6e64a8ef
Tools • Dart 2.0.0-dev.47.0.flutter-4126459025

Reproduce
Type y = _typeOf<StoreProvider<S>>();

Logs
E/DartVM (29011): ../../third_party/dart/runtime/vm/object.cc: 16408: error: unreachable code
E/DartVM (29011): Dumping native stack trace for thread 7164
E/DartVM (29011): [0x0000785da24a7f67] Unknown symbol
E/DartVM (29011): [0x0000785da24a7f67] Unknown symbol
E/DartVM (29011): [0x0000785da2743786] Unknown symbol
E/DartVM (29011): -- End of DumpStackTrace
F/libc (29011): Fatal signal 6 (SIGABRT), code -6 in tid 29028 (Thread-2)


Build fingerprint: 'Android/sdk_google_phone_x86_64/generic_x86_64:7.0/NYC/4662066:userdebug/dev-keys'
Revision: '0'
ABI: 'x86_64'
pid: 29011, tid: 29028, name: Thread-2 >>> com.example.helloworld <<<
signal 6 (SIGABRT), code -6 (SI_TKILL), fault addr --------
rax 0000000000000000 rbx 0000785da166e4f8 rcx ffffffffffffffff rdx 0000000000000006
rsi 0000000000007164 rdi 0000000000007153
r8 0000000000000000 r9 000000000000001f r10 0000000000000008 r11 0000000000000206
r12 0000000000007164 r13 0000000000000006 r14 0000785da27ac837 r15 0000785da1666bd0
cs 0000000000000033 ss 000000000000002b
rip 0000785dbd97fb67 rbp 0000000000000000 rsp 0000785da1666a38 eflags 0000000000000206
backtrace:
#00 pc 000000000008db67 /system/lib64/libc.so (tgkill+7)
#1 pc 000000000008a601 /system/lib64/libc.so (pthread_kill+65)
#2 pc 0000000000030241 /system/lib64/libc.so (raise+17)
#3 pc 000000000002877d /system/lib64/libc.so (abort+77)
#4 pc 000000000078c695 /data/app/com.example.helloworld-1/lib/x86_64/libflutter.so
#5 pc 0000000000a8078a /data/app/com.example.helloworld-1/lib/x86_64/libflutter.so
#6 pc 0000000000750ed2 /data/app/com.example.helloworld-1/lib/x86_64/libflutter.so
#7 pc 0000000000704cf6 /data/app/com.example.helloworld-1/lib/x86_64/libflutter.so
#8 pc 000000000071e48f /data/app/com.example.helloworld-1/lib/x86_64/libflutter.so
#9 pc 000000000082ea2c /data/app/com.example.helloworld-1/lib/x86_64/libflutter.so
#10 pc 000000000000063a anonymous:0000785d9f040000
#11 pc 0000000000004b3f anonymous:0000785d9ed40000
#12 pc 00000000000043c0 anonymous:0000785d9ed40000
#13 pc 00000000000285b3 anonymous:0000785d93300000
#14 pc 000000000002d7e3 anonymous:0000785d93700000
#15 pc 000000000002c94c anonymous:0000785d93700000
#16 pc 000000000002c258 anonymous:0000785d93700000
#17 pc 0000000000025686 anonymous:0000785d93300000
#18 pc 000000000002bb3f anonymous:0000785d93700000
#19 pc 000000000002a772 anonymous:0000785d93700000
#20 pc 000000000002996f anonymous:0000785d93700000
#21 pc 000000000002db3b anonymous:0000785d93700000
#22 pc 000000000002c94c anonymous:0000785d93700000
#23 pc 000000000002c258 anonymous:0000785d93700000
#24 pc 000000000002bb3f anonymous:0000785d93700000
#25 pc 000000000002a772 anonymous:0000785d93700000
#26 pc 000000000002996f anonymous:0000785d93700000
#27 pc 0000000000017957 anonymous:0000785d92d00000
#28 pc 000000000002a772 anonymous:0000785d93700000
#29 pc 000000000002996f anonymous:0000785d93700000
#30 pc 0000000000017957 anonymous:0000785d92d00000
#31 pc 000000000002a772 anonymous:0000785d93700000
#32 pc 000000000002996f anonymous:0000785d93700000
#33 pc 0000000000017957 anonymous:0000785d92d00000
#34 pc 000000000002a772 anonymous:0000785d93700000
#35 pc 000000000002996f anonymous:0000785d93700000
#36 pc 0000000000017957 anonymous:0000785d92d00000
#37 pc 000000000002a772 anonymous:0000785d93700000
#38 pc 000000000002996f anonymous:0000785d93700000
#39 pc 000000000002db3b anonymous:0000785d93700000
#40 pc 000000000002c94c anonymous:0000785d93700000
#41 pc 000000000002c258 anonymous:0000785d93700000
#42 pc 0000000000025686 anonymous:0000785d93300000
#43 pc 000000000002bb3f anonymous:0000785d93700000
#44 pc 000000000002a772 anonymous:0000785d93700000
#45 pc 000000000002996f anonymous:0000785d93700000
#46 pc 000000000002db3b anonymous:0000785d93700000
#47 pc 000000000002c94c anonymous:0000785d93700000
#48 pc 000000000002c258 anonymous:0000785d93700000
#49 pc 000000000002bb3f anonymous:0000785d93700000
#50 pc 000000000002a772 anonymous:0000785d93700000
#51 pc 000000000002996f anonymous:0000785d93700000
#52 pc 000000000002db3b anonymous:0000785d93700000
#53 pc 000000000002c94c anonymous:0000785d93700000
#54 pc 000000000002c258 anonymous:0000785d93700000
#55 pc 0000000000025686 anonymous:0000785d93300000
#56 pc 000000000002bb3f anonymous:0000785d93700000
#57 pc 000000000002a772 anonymous:0000785d93700000
#58 pc 000000000002996f anonymous:0000785d93700000
#59 pc 0000000000017957 anonymous:0000785d92d00000
#60 pc 000000000002a772 anonymous:0000785d93700000
#61 pc 000000000002996f anonymous:0000785d93700000
#62 pc 000000000002db3b anonymous:0000785d93700000
#63 pc 000000000002c94c anonymous:0000785d93700000

@crelier
Copy link
Contributor

crelier commented Apr 19, 2018

@brianegan, it would be great if you could provide a standalone reproduction, something like:

$ cat ~/bug.dart
Type _typeOf<T>() => T;
class StoreProvider<S> {
  foo() {
    Type y = _typeOf<StoreProvider<S>>();
    print(y);
  }
}
main() {
  new StoreProvider<bool>().foo();
}

This example does not crash:

$ pkg/vm/tool/dart2 ~/bug.dart
StoreProvider<bool>

@brianegan
Copy link
Author

brianegan commented Apr 19, 2018

@crelier Hey there -- sorry, I should have updated this thread. I think the problem was perhaps related to what eernstg mentioned above: whether the reified generic functions feature was enabled or not for certain folks / toolchains. I haven't gotten more bug reports with the latest version of the Flutter beta.

@kyle-berry-perficient
Copy link

kyle-berry-perficient commented Apr 20, 2018 via email

@zoechi
Copy link
Contributor

zoechi commented Apr 20, 2018

@kyle-berry-perficient you can make code and log output easier to read, by wrapping it in ```
You can edit the existing comment and add it.

@crelier crelier assigned a-siva and unassigned crelier Apr 20, 2018
@crelier
Copy link
Contributor

crelier commented Apr 20, 2018

@a-siva, can someone familiar with hot-reload on flutter have a look? Thanks.

@kgberry83
Copy link

@crelier / @a-siva it looks like this issue has been marked as Closed, should I create a separate issue for the "../../third_party/dart/runtime/vm/object.cc: 16408: error: unreachable code" issue?

@dgrove
Copy link
Contributor

dgrove commented Apr 22, 2018

@affinnen can you please open a new issue?

@kgberry83
Copy link

@dgrove / @crelier / @a-siva - a new issue has been opened #32942.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. customer-flutter P1 A high priority bug; for example, a single project is unusable or has many test failures vm-hot-reload
Projects
None yet
Development

No branches or pull requests