Allow Object when doing lookups.

Allow Object when doing lookups.

According to our typing rules a Set<Apple> is a Set<Fruit>. However, before this change, we couldn't use it as a Set<Fruit>:

===
bool foo(Set<Fruit> fruits) {
  // Would yield a type-error since we actually got a Set<Apple>.
  return fruits.contains(new Banana());
}

foo(new Set<Apple>());
===

R=ajohnsen@google.com, blois@google.com, lrn@google.com

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

[floitsch, 2013-04-16 15:32:47]

This looks like something we might want to get into M4.

[Anders Johnsen, 2013-04-16 15:37:07]

LGTM with the collowing comment:

The implementation should never throw an error, or we are back to square 1. In
some of the implementations you use '<', etc, and that may throw a type error.

[Lasse Reichstein Nielsen, 2013-04-24 09:11:18]

https://codereview.chromium.org/14246008/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/lib/collection_patch.dart
(right):

https://codereview.chromium.org/14246008/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/lib/collection_patch.dart:45: return
(strings == null) ? false : _hasTableEntry(strings, key);
It always pains me to see ?: expressions with a boolean constant branch.
This could be 
  return (strings != null) && _hasTableEntry(strings, key);
Likewise below.

https://codereview.chromium.org/14246008/diff/1/sdk/lib/core/map.dart
File sdk/lib/core/map.dart (right):

https://codereview.chromium.org/14246008/diff/1/sdk/lib/core/map.dart#newcode79
sdk/lib/core/map.dart:79: // TODO(floitsch): this should return a [Set].
Remove TODO? I doubt we'll do it, as it would be backed by the map.

https://codereview.chromium.org/14246008/diff/1/sdk/lib/core/set.dart
File sdk/lib/core/set.dart (right):

https://codereview.chromium.org/14246008/diff/1/sdk/lib/core/set.dart#newcode48
sdk/lib/core/set.dart:48: void removeAll(Iterable elements);
Iterable<Object>?

https://codereview.chromium.org/14246008/diff/1/sdk/lib/core/set.dart#newcode54
sdk/lib/core/set.dart:54: void retainAll(Iterable elements);
Make this Iterable<Object> to match?

https://codereview.chromium.org/14246008/diff/1/sdk/lib/core/set.dart#newcode74
sdk/lib/core/set.dart:74: Set<E> intersection(Set<E> other);
Should this be Set<Object> other?

[Lasse Reichstein Nielsen, 2013-04-30 07:42:27]

Is SplayTreeMap compatible with this, unchanged? If it is, it's a *little*
scary, since the subclass is not type compatible with the superclass. Shorter
example: 

class C {
  Object foo(Object x) => x;
}

class D<T> extends C {
  T foo(T x) => x;  // Incompatible override signature.
}

main() {
  C c = new D<int>();
  c.foo(42);
  c.foo("ab");
}

https://codereview.chromium.org/14246008/diff/1/sdk/lib/collection/hash_set.dart
File sdk/lib/collection/hash_set.dart (right):

https://codereview.chromium.org/14246008/diff/1/sdk/lib/collection/hash_set.d...
sdk/lib/collection/hash_set.dart:82: external void removeAll(Iterable
objectsToRemove);
Iterable<Object>?

https://codereview.chromium.org/14246008/diff/1/sdk/lib/collection/linked_has...
File sdk/lib/collection/linked_hash_map.dart (right):

https://codereview.chromium.org/14246008/diff/1/sdk/lib/collection/linked_has...
sdk/lib/collection/linked_hash_map.dart:29: external V remove(K key);
remove too.

[floitsch, 2013-06-17 18:25:34]

I think we already discussed that the splay-tree map doesn't have any problems.

PTAL.

I kept some Maps, Lists, Sets unchanged if the type was int or String. In these
cases it's much more likely that a wrong type is a bug. It violates the base
contract, but the List already does that all over the place. So I'm fine with
catching more bugs for the sake of theory.

https://codereview.chromium.org/14246008/diff/1/sdk/lib/collection/hash_set.dart
File sdk/lib/collection/hash_set.dart (right):

https://codereview.chromium.org/14246008/diff/1/sdk/lib/collection/hash_set.d...
sdk/lib/collection/hash_set.dart:82: external void removeAll(Iterable
objectsToRemove);
On 2013/04/30 07:42:27, Lasse Reichstein Nielsen wrote:
> Iterable<Object>?

Done.

https://codereview.chromium.org/14246008/diff/1/sdk/lib/collection/linked_has...
File sdk/lib/collection/linked_hash_map.dart (right):

https://codereview.chromium.org/14246008/diff/1/sdk/lib/collection/linked_has...
sdk/lib/collection/linked_hash_map.dart:29: external V remove(K key);
On 2013/04/30 07:42:27, Lasse Reichstein Nielsen wrote:
> remove too.

Done.

https://codereview.chromium.org/14246008/diff/1/sdk/lib/core/map.dart
File sdk/lib/core/map.dart (right):

https://codereview.chromium.org/14246008/diff/1/sdk/lib/core/map.dart#newcode79
sdk/lib/core/map.dart:79: // TODO(floitsch): this should return a [Set].
On 2013/04/24 09:11:18, Lasse Reichstein Nielsen wrote:
> Remove TODO? I doubt we'll do it, as it would be backed by the map.

Done.

https://codereview.chromium.org/14246008/diff/1/sdk/lib/core/set.dart
File sdk/lib/core/set.dart (right):

https://codereview.chromium.org/14246008/diff/1/sdk/lib/core/set.dart#newcode48
sdk/lib/core/set.dart:48: void removeAll(Iterable elements);
On 2013/04/24 09:11:18, Lasse Reichstein Nielsen wrote:
> Iterable<Object>?

Done.

https://codereview.chromium.org/14246008/diff/1/sdk/lib/core/set.dart#newcode54
sdk/lib/core/set.dart:54: void retainAll(Iterable elements);
On 2013/04/24 09:11:18, Lasse Reichstein Nielsen wrote:
> Make this Iterable<Object> to match?

Done.

https://codereview.chromium.org/14246008/diff/1/sdk/lib/core/set.dart#newcode74
sdk/lib/core/set.dart:74: Set<E> intersection(Set<E> other);
On 2013/04/24 09:11:18, Lasse Reichstein Nielsen wrote:
> Should this be Set<Object> other?

Done.

[floitsch, 2013-06-17 18:39:41]

Fix small type error.

[Lasse Reichstein Nielsen, 2013-06-18 14:13:08]

lgtm

https://codereview.chromium.org/14246008/diff/15001/runtime/lib/growable_arra...
File runtime/lib/growable_array.dart (right):

https://codereview.chromium.org/14246008/diff/15001/runtime/lib/growable_arra...
runtime/lib/growable_array.dart:215: int indexOf(T element, [int start = 0]) {
Why not accept Object here? and in lastIndexOf?

https://codereview.chromium.org/14246008/diff/15001/sdk/lib/collection/splay_...
File sdk/lib/collection/splay_tree.dart (right):

https://codereview.chromium.org/14246008/diff/15001/sdk/lib/collection/splay_...
sdk/lib/collection/splay_tree.dart:260: if (key is! K) return null;
Per earlier discussion, could we keep this as "K"?
A TreeMap<K,V> is not really castable to any TreeMap<K2,V> with K superlass of
K.

[floitsch, 2013-06-20 15:52:50]

Changed indexOf too.
Added vm reviewers.
Will commit tomorrow.

https://codereview.chromium.org/14246008/diff/15001/runtime/lib/growable_arra...
File runtime/lib/growable_array.dart (right):

https://codereview.chromium.org/14246008/diff/15001/runtime/lib/growable_arra...
runtime/lib/growable_array.dart:215: int indexOf(T element, [int start = 0]) {
On 2013/06/18 14:13:08, Lasse Reichstein Nielsen wrote:
> Why not accept Object here? and in lastIndexOf?

Done.

https://codereview.chromium.org/14246008/diff/15001/sdk/lib/collection/splay_...
File sdk/lib/collection/splay_tree.dart (right):

https://codereview.chromium.org/14246008/diff/15001/sdk/lib/collection/splay_...
sdk/lib/collection/splay_tree.dart:260: if (key is! K) return null;
On 2013/06/18 14:13:08, Lasse Reichstein Nielsen wrote:
> Per earlier discussion, could we keep this as "K"?
> A TreeMap<K,V> is not really castable to any TreeMap<K2,V> with K superlass of
> K.

As discussed in person: leaving as is.

[floitsch, 2013-06-20 15:53:14]

CC +blois.

[blois, 2013-06-20 15:58:58]

lgtm, with one nit.

https://codereview.chromium.org/14246008/diff/25001/sdk/lib/html/html_common/...
File sdk/lib/html/html_common/filtered_element_list.dart (right):

https://codereview.chromium.org/14246008/diff/25001/sdk/lib/html/html_common/...
sdk/lib/html/html_common/filtered_element_list.dart:63: if (needle != Element)
return false;
is!?

[floitsch, 2013-06-20 16:00:05]

https://codereview.chromium.org/14246008/diff/25001/sdk/lib/html/html_common/...
File sdk/lib/html/html_common/filtered_element_list.dart (right):

https://codereview.chromium.org/14246008/diff/25001/sdk/lib/html/html_common/...
sdk/lib/html/html_common/filtered_element_list.dart:63: if (needle != Element)
return false;
On 2013/06/20 15:58:58, blois wrote:
> is!?

argh. good catch. thanks.
done.

[floitsch, 2013-06-21 16:09:40]

Had to change firstWhere and lastWhere, too.

[floitsch, 2013-06-24 12:45:45]

Committed patchset #8 manually as r24326 (presubmit successful).