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

Iterable.whereType returns everything in VM unless --preview-dart-2 #32423

Closed
srawlins opened this issue Mar 6, 2018 · 17 comments
Closed

Iterable.whereType returns everything in VM unless --preview-dart-2 #32423

srawlins opened this issue Mar 6, 2018 · 17 comments

Comments

@srawlins
Copy link
Member

srawlins commented Mar 6, 2018

I cannot get whereType to work on the VM. I haven't tried using dart2js or DDC. I tried with Lists, Sets, built-in types, and my own classes:

void main() {
  var listOfNum = <num>[1, 2.2, 3, 4.4, 5];
  print(listOfNum.whereType<int>());
  print(listOfNum.whereType<double>());
  print(listOfNum.whereType<String>());

  var setOfNum = new Set<num>.from(listOfNum);
  print(setOfNum.whereType<int>());

  var a1 = new A();
  var a2 = new A();
  var b1 = new B();
  print([a1, a2, b1].whereType<B>());
  print(new Set.from([a1, a2, b1]).whereType<B>());
}

class A {}

class B extends A {}
$ dart --version
Dart VM version: 2.0.0-dev.32.0 (Thu Mar 1 18:39:53 2018 +0100) on "macos_x64"

$ dart a.dart
(1, 2.2, 3, 4.4, 5)  # I expected (1, 3, 5)
(1, 2.2, 3, 4.4, 5)  # I expected (2.2, 4.4)
(1, 2.2, 3, 4.4, 5)  # I expected ()
(1, 2.2, 3, 4.4, 5)  # I expected (1, 3, 5)
(Instance of 'A', Instance of 'A', Instance of 'B')  # I expected (Instance of 'B')
(Instance of 'A', Instance of 'A', Instance of 'B')  # I expected (Instance of 'B')
@matanlurey
Copy link
Contributor

@srawlins I think it only works in --preview-dart-2 (and DDC/DDK).

... which is really confusing, to me. Owch! @leafpetersen

@srawlins
Copy link
Member Author

srawlins commented Mar 6, 2018

Oh thank goodness.

$ dart --preview-dart-2 a.dart
(1, 3, 5)
(2.2, 4.4)
()
(1, 3, 5)
(Instance of 'B')
(Instance of 'B')

Can I re-use this bug then to express my astonishment that SOME core lib 2 methods only work with --preview-dart-2?!?!

followedBy, singleWhere, etc seem to work. I haven't tried cast or retype yet...

Can we maybe write something in CHANGELOG if we all have to live with this for a few months?

@srawlins srawlins changed the title Iterable.whereType always returns everything in VM Iterable.whereType returns everything in VM unless --preview-dart-2 Mar 6, 2018
@matanlurey
Copy link
Contributor

I think .whereType is the only one that explicitly fails. I believe the others just fall back to returning C<dynamic> instead of C<T>. If you asserted against the .runtimeType it wouldn't be right, for example.

@mraleph
Copy link
Member

mraleph commented Mar 6, 2018

We can also turn on reified generics outside of Dart 2 mode as a transitionary measure.

WDYT @a-siva?

@a-siva
Copy link
Contributor

a-siva commented Mar 6, 2018

you mean reified generic methods I guess, we could there maybe some surprises without type inference but otherwise it would be ok.
@srawlins you could try your test as follows -
dart --reify-generic-functions /tmp/junk.dart

@lrhn
Copy link
Member

lrhn commented Mar 6, 2018

The cast methods also fail entirely without reified generics.

We will not have reified generics until Dart 2, but the current SDK is a Dart 2.0 development branch, so there are Dart 2 specific methods in it.

Turning on reified generics in the VM only is likely to be confusing. It won't help code that gets compiled to JavaScript by dart2js, and it might even introduce errors in some cases. I'd just wait until we switch on the common front end so we have a level playing field for all backends.

AFAIK we plan to change the default to enable Dart 2 features some time this month, so it shouldn't be long now.

@srawlins
Copy link
Member Author

srawlins commented Mar 6, 2018

OK I don't want to complicate things. If --preview-dart-2 will become default on in a few weeks, maybe we just live with the surprise. Is it worth it to put something in the CHANGELOG? Will this confuse Flutter users?

@leafpetersen
Copy link
Member

If you think it best, go ahead and put something in the CHANGELOG, though presumably we will want to take it out for the release (we're in a really weird state with the CHANGELOG). Alternatively, we could put an interim comment in the doc comment for whereType.

@leafpetersen
Copy link
Member

@jakemac53 ran into this too, and in talking to him I got a little worried, not about the VM very much, but about dart2js. Externally, most people are running tests and running their code in the same mode, so if whereType doesn't work they'll see it in tests. But internally, people are testing with DDC and running with dart2js, which seems like a recipe for badness.

@sigmundch How long before reification of generics is turned on in dart2js?

@lrhn What do you think about making this method external and patching in a version which throws on dart2js? Alternatively, maybe we should just make it throw always for now?

@jakemac53
Copy link
Contributor

jakemac53 commented Mar 6, 2018

Ya this was pretty surprising to me when I ran across it.

I have a somewhat crazy idea... would it be possible to add a check for T == dynamic inside the constructor for WhereTypeIterable and throw if that is true? That way at least we could detect somebody trying to use it in a non-dart2 context and throw a good error message, instead of returning a non-filtered list which could have untold consequences. I don't think whereType<dynamic> is a useful thing anyways, and we could remove the check once the flag is flipped?

@sigmundch
Copy link
Member

@sigmundch How long before reification of generics is turned on in dart2js?

It's about a month out or a bit more.

@srawlins
Copy link
Member Author

srawlins commented Mar 6, 2018

@lrhn What do you think about making this method external and patching in a version which throws on dart2js? Alternatively, maybe we should just make it throw always for now?

This would be my preference: if whereType is not wired up right (read: not implemented) when executing dart a.dart, then it should throw Unimplemented, assuming its just a few temporary lines to change, and doesn't require changing (many lines in) status files.

@jakemac53
Copy link
Contributor

What do you think about making this method external and patching in a version which throws on dart2js?

That still leaves the vm in a really weird state which is why I suggested the other T == dynamic hack - it should a more generic solution... and while its absolutely a hack I think its a relatively safe one.

@jakemac53
Copy link
Contributor

jakemac53 commented Mar 8, 2018

Just hit our first real world bug caused as a result of this, dart-lang/build#1123. (I added a usage of this before I was aware of how completely broken it is)

@jakemac53
Copy link
Contributor

cc @kevmoo

@leafpetersen
Copy link
Member

I landed the CL disabling this whereType for now, and filed #32463 to track re-enabling it.

Closing this.

@jakemac53
Copy link
Contributor

Thanks @leafpetersen !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants