Tighten a couple type annotations in IterableMixin

Tighten a couple type annotations in IterableMixin

BUG=
R=lrn@google.com

Committed: https://code.google.com/p/dart/source/detail?r=42334

[Jacob, 2014-12-12 01:57:28]

jacobr@google.com changed reviewers:
+ floitsch@google.com

[Jacob, 2014-12-12 01:57:28]

This goes along with https://codereview.chromium.org/794953002
to fix some small inconsistencies in the types for dart:html

[floitsch, 2014-12-12 12:03:51]

floitsch@google.com changed reviewers:
+ lrn@google.com

[floitsch, 2014-12-12 12:03:51]

This is a breaking change, so normally we don't allow them.
I will let Lasse decide.

[Lasse Reichstein Nielsen, 2014-12-12 12:39:26]

I'd say that this is just a bug. The interface of Iterable.firstWhere is written
as returning E and having "orElse" return E too. 

That IterableMixin and IterableBase is different is definitely a bug. They
should have exactly the same interface as Iterable.
The two questions are: 
1) will any code will break if we fix the bug? 
2) Or should we change the interface instead?

In properly written code, variables will be typed as Iterable/List/Set etc, not
in terms of IterableMixin or IterableBase, so you should get warnings if you
rely on non-E return values. 

Also, ListMixin (lib/collection/list.dart) and SetMixin
(lib/collection/set.dart) have the same problem.

As for changing the interface:

If you have a List<Werewolf> and a List<Vampire> you need to return a Monster
(superclass), then you can't actually do:
   
 Monster famousMonster = 
     werewolves.firstWhere((w)=>w.isFamous, 
         orElse: () => vampires.firstWhere((v)=>v.isFamous));

because the orElse doesn't return a Werewolf.

With generic methods, we could pass the return type and write:
  Monster famousMonster = 
      werewolves.firstWhere<Monster>((w)=>w.isFamous,
          orElse: ()=> vampires.firstWhere<Monster(
                           (v)=>v.isFamous));

Since we don't have generic methods, we should replace the places where we would
use them with the type parameter limit, which would be dynamic here - the
interface of firstWhere should be:
   T firstWhere<T>(bool test(E element), {T orElse()})
(it can't be <T extends E> since you might want:
   firstWhere<T>((e)=>e is T)
where T is some unrelated type that some E might implement).

So - I'm on the fence. It's a bug, but likely a bug that was introduced because
it made just as much sense as the interface's version. 

I think we should fix it.
I favor changing the interface, but changing Iterable at this point is not
likely to happen without opposition. 
So, I think we should fix this (and other cases where firstWhere doesn't match
the interface).

[floitsch, 2014-12-12 12:42:59]

On 2014/12/12 12:39:26, Lasse Reichstein Nielsen wrote:
> I'd say that this is just a bug. The interface of Iterable.firstWhere is
written
> as returning E and having "orElse" return E too. 
> 
> That IterableMixin and IterableBase is different is definitely a bug. They
> should have exactly the same interface as Iterable.
> The two questions are: 
> 1) will any code will break if we fix the bug? 
> 2) Or should we change the interface instead?
> 
> In properly written code, variables will be typed as Iterable/List/Set etc,
not
> in terms of IterableMixin or IterableBase, so you should get warnings if you
> rely on non-E return values. 
> 
> Also, ListMixin (lib/collection/list.dart) and SetMixin
> (lib/collection/set.dart) have the same problem.
> 
> As for changing the interface:
> 
> If you have a List<Werewolf> and a List<Vampire> you need to return a Monster
> (superclass), then you can't actually do:
>    
>  Monster famousMonster = 
>      werewolves.firstWhere((w)=>w.isFamous, 
>          orElse: () => vampires.firstWhere((v)=>v.isFamous));
> 
> because the orElse doesn't return a Werewolf.
> 
> With generic methods, we could pass the return type and write:
>   Monster famousMonster = 
>       werewolves.firstWhere<Monster>((w)=>w.isFamous,
>           orElse: ()=> vampires.firstWhere<Monster(
>                            (v)=>v.isFamous));
> 
> Since we don't have generic methods, we should replace the places where we
would
> use them with the type parameter limit, which would be dynamic here - the
> interface of firstWhere should be:
>    T firstWhere<T>(bool test(E element), {T orElse()})
> (it can't be <T extends E> since you might want:
>    firstWhere<T>((e)=>e is T)
> where T is some unrelated type that some E might implement).
> 
> So - I'm on the fence. It's a bug, but likely a bug that was introduced
because
> it made just as much sense as the interface's version. 
> 
> I think we should fix it.
> I favor changing the interface, but changing Iterable at this point is not
> likely to happen without opposition. 
> So, I think we should fix this (and other cases where firstWhere doesn't match
> the interface).

I actually misread the CL (thinking that Iterable was advocating a dynamic
`orElse`).
I think this is just a bug, and was probably introduced when we removed the
IterableMixinWorkaround (which didn't have generic types).
Ok with me to fix the bug (especially if the Iterable interface does the right
thing).

[Lasse Reichstein Nielsen, 2014-12-12 12:45:40]

I guess that's a LGTM, and if you want to do ListMixin and SetMixin too, feel
free (otherwise I'll do it later).
Thanks!

[Jacob, 2014-12-12 17:55:33]

On 2014/12/12 12:45:40, Lasse Reichstein Nielsen wrote:
> I guess that's a LGTM, and if you want to do ListMixin and SetMixin too, feel
> free (otherwise I'll do it later).
> Thanks!

I've fixed ListMixin and SetMixin as well. Lasse's example case is interesting
but I looked over existing code and all cases I saw work fine with this change.
I agree that when/if Dart supports generic methods this method should be changed
to allow the case Lasse suggested. Without generic methods I think the current
type signature in Iterable is better as it matches the type signature required
by all examples of this call I have seen in the wild and matches the signatures
for the related "first" and "last" methods.

[Jacob, 2014-12-12 18:01:00]

Committed patchset #2 (id:20001) manually as 42334 (presubmit successful).

[Jacob, 2014-12-12 20:35:57]

This CL broke a CO19 test in Dartium checked mode. The CO19 test would have
failed if instead of being given a LinkedList it had been given an Iterable that
implemented the lastWhere method with the signature specified in "class
Iterable".
I can proceed by marking the CO19 test as skipped or we can rethink whether we
want to allow this change given that it could be a breaking change in Dartium
checked mode.


Broken version of CO19 test:
We can fix the test by changing noneMatches to 4444444444 instead of new
Object().



/*
 * Copyright (c) 2011, the Dart project authors.  Please see the AUTHORS file
 * for details. All rights reserved. Use of this source code is governed by a
 * BSD-style license that can be found in the LICENSE file.
 */
/**
 * @assertion dynamic lastWhere(bool test(E value), {Object orElse()})
 * Returns the last element that satisfies the given predicate test.
 * If none matches, the result of invoking the orElse function is returned.
 * By default, when orElse is null, a StateError is thrown.
 * @description Checks that If none matches, the result of invoking the orElse
 * function is returned.
 * @author kaigorodov
 */
import "../../../Utils/expect.dart";
import "dart:collection";
import "LinkedList.lib.dart";

var noneMatches=new Object();

void check(LinkedList a, var element) {
  Expect.identical(noneMatches,
    a.lastWhere(
      (MyLinkedListEntry entry)=>entry.value==element,
      orElse: ()=>noneMatches)
    );
}

main() {
  LinkedList a = toLinkedList([42, 0, -1, 42, -1, 6031769, 0]);
  check(a, 43);
  check(a, 2);
  check(a, -2);
  check(a, 6031768);
}

[floitsch, 2014-12-13 16:56:45]

On 2014/12/12 20:35:57, Jacob wrote:
> This CL broke a CO19 test in Dartium checked mode. The CO19 test would have
> failed if instead of being given a LinkedList it had been given an Iterable
that
> implemented the lastWhere method with the signature specified in "class
> Iterable".
> I can proceed by marking the CO19 test as skipped
There are extremely few cases where you want to use "Skip" in a status file.
Always prefer Fail (or Fail, Pass, ...) instead.

Yes: I would mark it as Fail and file a bug on Co19.

> or we can rethink whether we
> want to allow this change given that it could be a breaking change in Dartium
> checked mode.
> 
> 
> Broken version of CO19 test:
> We can fix the test by changing noneMatches to 4444444444 instead of new
> Object().
> 
> 
> 
> /*
>  * Copyright (c) 2011, the Dart project authors.  Please see the AUTHORS file
>  * for details. All rights reserved. Use of this source code is governed by a
>  * BSD-style license that can be found in the LICENSE file.
>  */
> /**
>  * @assertion dynamic lastWhere(bool test(E value), {Object orElse()})
>  * Returns the last element that satisfies the given predicate test.
>  * If none matches, the result of invoking the orElse function is returned.
>  * By default, when orElse is null, a StateError is thrown.
>  * @description Checks that If none matches, the result of invoking the orElse
>  * function is returned.
>  * @author kaigorodov
>  */
> import "../../../Utils/expect.dart";
> import "dart:collection";
> import "LinkedList.lib.dart";
> 
> var noneMatches=new Object();
> 
> void check(LinkedList a, var element) {
>   Expect.identical(noneMatches,
>     a.lastWhere(
>       (MyLinkedListEntry entry)=>entry.value==element,
>       orElse: ()=>noneMatches)
>     );
> }
> 
> main() {
>   LinkedList a = toLinkedList([42, 0, -1, 42, -1, 6031769, 0]);
>   check(a, 43);
>   check(a, 2);
>   check(a, -2);
>   check(a, 6031768);
> }