dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.08k stars 1.56k forks source link

Dart 2 class generics bug? #32425

Closed brianegan closed 6 years ago

brianegan commented 6 years ago

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 commented 6 years ago

What Dart/Flutter version are you using?

mraleph commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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 :)

brianegan commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

brianegan commented 6 years ago

Thanks for the fast response and fix :)

crelier commented 6 years ago

You are welcome. Thanks for the report.

brianegan commented 6 years ago

@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.

https://github.com/brianegan/flutter_redux/issues/30

eernstg commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

--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 commented 6 years ago

@eernstg Thanks much -- appreciate the info!

kyle-berry-perficient commented 6 years ago

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)

01 pc 000000000008a601 /system/lib64/libc.so (pthread_kill+65)

02 pc 0000000000030241 /system/lib64/libc.so (raise+17)

03 pc 000000000002877d /system/lib64/libc.so (abort+77)

04 pc 000000000078c695 /data/app/com.example.helloworld-1/lib/x86_64/libflutter.so

05 pc 0000000000a8078a /data/app/com.example.helloworld-1/lib/x86_64/libflutter.so

06 pc 0000000000750ed2 /data/app/com.example.helloworld-1/lib/x86_64/libflutter.so

07 pc 0000000000704cf6 /data/app/com.example.helloworld-1/lib/x86_64/libflutter.so

08 pc 000000000071e48f /data/app/com.example.helloworld-1/lib/x86_64/libflutter.so

09 pc 000000000082ea2c /data/app/com.example.helloworld-1/lib/x86_64/libflutter.so

10 pc 000000000000063a

11 pc 0000000000004b3f

12 pc 00000000000043c0

13 pc 00000000000285b3

14 pc 000000000002d7e3

15 pc 000000000002c94c

16 pc 000000000002c258

17 pc 0000000000025686

18 pc 000000000002bb3f

19 pc 000000000002a772

20 pc 000000000002996f

21 pc 000000000002db3b

22 pc 000000000002c94c

23 pc 000000000002c258

24 pc 000000000002bb3f

25 pc 000000000002a772

26 pc 000000000002996f

27 pc 0000000000017957

28 pc 000000000002a772

29 pc 000000000002996f

30 pc 0000000000017957

31 pc 000000000002a772

32 pc 000000000002996f

33 pc 0000000000017957

34 pc 000000000002a772

35 pc 000000000002996f

36 pc 0000000000017957

37 pc 000000000002a772

38 pc 000000000002996f

39 pc 000000000002db3b

40 pc 000000000002c94c

41 pc 000000000002c258

42 pc 0000000000025686

43 pc 000000000002bb3f

44 pc 000000000002a772

45 pc 000000000002996f

46 pc 000000000002db3b

47 pc 000000000002c94c

48 pc 000000000002c258

49 pc 000000000002bb3f

50 pc 000000000002a772

51 pc 000000000002996f

52 pc 000000000002db3b

53 pc 000000000002c94c

54 pc 000000000002c258

55 pc 0000000000025686

56 pc 000000000002bb3f

57 pc 000000000002a772

58 pc 000000000002996f

59 pc 0000000000017957

60 pc 000000000002a772

61 pc 000000000002996f

62 pc 000000000002db3b

63 pc 000000000002c94c

crelier commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

This code does not fail on startup, but during hot reload.

//-------------------------------------------------------------------
import 'package:flutter/material.dart';

void main() => runApp(MyApp());

class MyApp extends StatelessWidget {
  final String title;

  MyApp({Key key, this.title}) : super(key: key);

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: "Flutter Redux Demo",
      theme: ThemeData(
        primarySwatch: Colors.red,
      ),
      home: MyHomePage(title: 'Flutter Demo Home Page'),
    );
  }
}

class AppState {

}

class Provider<T> {

}

abstract class PageState<W extends StatefulWidget, S> extends State<W> {
  // Workaround to capture generics
  static Type _typeOf<T>() => T;

  void init() {
    final type1 = _typeOf<Provider<AppState>>(); // works
    final type2 = _typeOf<Provider<S>>(); // DOES NOT WORK - object.cc: 16408: error: unreachable code
  }
}

class MyHomePage extends StatefulWidget {
  final String title;
  MyHomePage({Key key, this.title}) : super(key: key);

  @override
  _MyHomePageState createState() => new _MyHomePageState();
}

class _MyHomePageState extends PageState<MyHomePage, AppState> {
  @override
  Widget build(BuildContext context) {
    super.init(); // calls into method that causes failure

    return Scaffold(
      appBar: AppBar(
        title: Text(widget.title),
      ),
      body: Center(
        child: Column(

        ),
      ),
    );
  }
}

// ----------------------------------------------------------------- Initializing hot reload... E/DartVM ( 3758): ../../third_party/dart/runtime/vm/object.cc: 16408: error: unreachable code E/DartVM ( 3758): Dumping native stack trace for thread ec6 E/DartVM ( 3758): [0x0000736d9c49df67] Unknown symbol E/DartVM ( 3758): [0x0000736d9c49df67] Unknown symbol E/DartVM ( 3758): [0x0000736d9c739786] Unknown symbol E/DartVM ( 3758): -- End of DumpStackTrace F/libc ( 3758): Fatal signal 6 (SIGABRT), code -6 in tid 3782 (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: 3758, tid: 3782, name: Thread-2 >>> com.example.helloworld <<< signal 6 (SIGABRT), code -6 (SI_TKILL), fault addr -------- rax 0000000000000000 rbx 0000736d9b5654f8 rcx ffffffffffffffff rdx 0000000000000006 rsi 0000000000000ec6 rdi 0000000000000eae r8 0000000000000000 r9 000000000000001f r10 0000000000000008 r11 0000000000000202 r12 0000000000000ec6 r13 0000000000000006 r14 0000736d9c7a2837 r15 0000736d9b55dbd0 cs 0000000000000033 ss 000000000000002b rip 0000736db7c93b67 rbp 0000000000000000 rsp 0000736d9b55da38 eflags 0000000000000202 backtrace:

00 pc 000000000008db67 /system/lib64/libc.so (tgkill+7)

#01 pc 000000000008a601  /system/lib64/libc.so (pthread_kill+65)
#02 pc 0000000000030241  /system/lib64/libc.so (raise+17)
#03 pc 000000000002877d  /system/lib64/libc.so (abort+77)
#04 pc 000000000078c695  /data/app/com.example.helloworld-1/lib/x86_64/libflutter.so
#05 pc 0000000000a8078a  /data/app/com.example.helloworld-1/lib/x86_64/libflutter.so
#06 pc 0000000000750ed2  /data/app/com.example.helloworld-1/lib/x86_64/libflutter.so
#07 pc 0000000000704cf6  /data/app/com.example.helloworld-1/lib/x86_64/libflutter.so
#08 pc 000000000071e48f  /data/app/com.example.helloworld-1/lib/x86_64/libflutter.so
#09 pc 000000000082ea2c  /data/app/com.example.helloworld-1/lib/x86_64/libflutter.so
#10 pc 000000000000063a  <anonymous:0000736d98940000>
#11 pc 0000000000020fa0  <anonymous:0000736d8f580000>
#12 pc 0000000000020840  <anonymous:0000736d8f580000>
#13 pc 0000000000002233  <anonymous:0000736d8de00000>
#14 pc 0000000000008523  <anonymous:0000736d8e100000>
#15 pc 000000000000768c  <anonymous:0000736d8e100000>
#16 pc 0000000000006f98  <anonymous:0000736d8e100000>
#17 pc 000000000003f2c6  <anonymous:0000736d8e100000>
#18 pc 000000000000687f  <anonymous:0000736d8e100000>
#19 pc 00000000000054b2  <anonymous:0000736d8e100000>
#20 pc 00000000000046af  <anonymous:0000736d8e100000>
#21 pc 000000000000887b  <anonymous:0000736d8e100000>
#22 pc 000000000000768c  <anonymous:0000736d8e100000>
#23 pc 0000000000006f98  <anonymous:0000736d8e100000>
#24 pc 000000000000687f  <anonymous:0000736d8e100000>
#25 pc 00000000000054b2  <anonymous:0000736d8e100000>
#26 pc 00000000000046af  <anonymous:0000736d8e100000>
#27 pc 0000000000031077  <anonymous:0000736d8db80000>
#28 pc 00000000000054b2  <anonymous:0000736d8e100000>
#29 pc 00000000000046af  <anonymous:0000736d8e100000>
#30 pc 0000000000031077  <anonymous:0000736d8db80000>
#31 pc 00000000000054b2  <anonymous:0000736d8e100000>
#32 pc 00000000000046af  <anonymous:0000736d8e100000>
#33 pc 0000000000031077  <anonymous:0000736d8db80000>
#34 pc 00000000000054b2  <anonymous:0000736d8e100000>
#35 pc 00000000000046af  <anonymous:0000736d8e100000>
#36 pc 0000000000031077  <anonymous:0000736d8db80000>
#37 pc 00000000000054b2  <anonymous:0000736d8e100000>
#38 pc 00000000000046af  <anonymous:0000736d8e100000>
#39 pc 000000000000887b  <anonymous:0000736d8e100000>
#40 pc 000000000000768c  <anonymous:0000736d8e100000>
#41 pc 0000000000006f98  <anonymous:0000736d8e100000>
#42 pc 000000000003f2c6  <anonymous:0000736d8e100000>
#43 pc 000000000000687f  <anonymous:0000736d8e100000>
#44 pc 00000000000054b2  <anonymous:0000736d8e100000>
#45 pc 00000000000046af  <anonymous:0000736d8e100000>
#46 pc 000000000000887b  <anonymous:0000736d8e100000>
#47 pc 000000000000768c  <anonymous:0000736d8e100000>
#48 pc 0000000000006f98  <anonymous:0000736d8e100000>
#49 pc 000000000000687f  <anonymous:0000736d8e100000>
#50 pc 00000000000054b2  <anonymous:0000736d8e100000>
#51 pc 00000000000046af  <anonymous:0000736d8e100000>
#52 pc 000000000000887b  <anonymous:0000736d8e100000>
#53 pc 000000000000768c  <anonymous:0000736d8e100000>
#54 pc 0000000000006f98  <anonymous:0000736d8e100000>
#55 pc 000000000003f2c6  <anonymous:0000736d8e100000>
#56 pc 000000000000687f  <anonymous:0000736d8e100000>
#57 pc 00000000000054b2  <anonymous:0000736d8e100000>
#58 pc 00000000000046af  <anonymous:0000736d8e100000>
#59 pc 0000000000031077  <anonymous:0000736d8db80000>
#60 pc 00000000000054b2  <anonymous:0000736d8e100000>
#61 pc 00000000000046af  <anonymous:0000736d8e100000>
#62 pc 000000000000887b  <anonymous:0000736d8e100000>
#63 pc 000000000000768c  <anonymous:0000736d8e100000>

Lost connection to device.

zoechi commented 6 years ago

@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 commented 6 years ago

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

kgberry83 commented 6 years ago

@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 commented 6 years ago

@affinnen can you please open a new issue?

kgberry83 commented 6 years ago

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