Allow S->T <: dynamic->T

Allow S->T <: dynamic->T

In followup CLs, I will:
- Fix coercions to limit implicit ones.

R=jmesserly@google.com, leafp@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/8f41f42b26f2ee5f5d865e16b5c602460341096e

[vsm, 2015-03-17 20:45:12]

vsm@google.com changed reviewers:
+ leafp@google.com

[vsm, 2015-03-18 21:27:52]

vsm@google.com changed reviewers:
+ jmesserly@google.com

[vsm, 2015-03-18 21:33:21]

PTAL

This eliminates all invalid overrides in Angular.

John: I added a fix to the codegen to call dinvoke in the (newly) right places.

[Jennifer Messerly, 2015-03-18 21:50:59]

https://codereview.chromium.org/1010893004/diff/60001/test/codegen/expect/dar...
File test/codegen/expect/dart/_internal.js (right):

https://codereview.chromium.org/1010893004/diff/60001/test/codegen/expect/dar...
test/codegen/expect/dart/_internal.js:448: return new
WhereIterator(this[_iterable].iterator, dart.closureWrap(this[_f], "(E) →
bool"));
hmmm. I'm a little confused, why did we closure wrap here? We're going from `E
-> bool` to `E -> bool`. The only thing I can think of is the typedef is messing
us up, and it doesn't understand that _ElementPredicate and `bool this._f(E
element)` are the same type, after doing the generic substitution. Maybe it
thinks it's calling `new WhereIterator<dynamic>`?

here's the input Dart code:

```dart
typedef bool _ElementPredicate<E>(E element);

class WhereIterable<E> extends IterableBase<E> {
  final Iterable<E> _iterable;
  final _ElementPredicate _f;

  WhereIterable(this._iterable, bool this._f(E element));

  Iterator<E> get iterator => new WhereIterator<E>(_iterable.iterator, _f);
}

class WhereIterator<E> extends Iterator<E> {
  final Iterator<E> _iterator;
  final _ElementPredicate _f;

  WhereIterator(this._iterator, bool this._f(E element));

  bool moveNext() {
    while (_iterator.moveNext()) {
      if (_f(_iterator.current)) {
        return true;
      }
    }
    return false;
  }

  E get current => _iterator.current;
}
```

https://codereview.chromium.org/1010893004/diff/60001/test/codegen/expect/dar...
test/codegen/expect/dart/_internal.js:463: if (dart.dinvokef(this[_f],
this[_iterator].current)) {
similar question to above: why is this a dynamic operation?

_f is an _ElementPredicate

_iterator is an Iterator<E> which would make _iterator.current an E

so we're calling a `E -> bool` function passing in an `E`.

[Jennifer Messerly, 2015-03-18 21:53:22]

https://codereview.chromium.org/1010893004/diff/60001/test/codegen/expect/dar...
File test/codegen/expect/dart/_internal.js (right):

https://codereview.chromium.org/1010893004/diff/60001/test/codegen/expect/dar...
test/codegen/expect/dart/_internal.js:448: return new
WhereIterator(this[_iterable].iterator, dart.closureWrap(this[_f], "(E) →
bool"));
On 2015/03/18 21:50:59, John Messerly wrote:
> hmmm. I'm a little confused, why did we closure wrap here? We're going from `E
> -> bool` to `E -> bool`. The only thing I can think of is the typedef is
messing
> us up, and it doesn't understand that _ElementPredicate and `bool this._f(E
> element)` are the same type, after doing the generic substitution. Maybe it
> thinks it's calling `new WhereIterator<dynamic>`?
> 
> here's the input Dart code:
> 
> ```dart
> typedef bool _ElementPredicate<E>(E element);
> 
> class WhereIterable<E> extends IterableBase<E> {
>   final Iterable<E> _iterable;
>   final _ElementPredicate _f;
> 
>   WhereIterable(this._iterable, bool this._f(E element));
> 
>   Iterator<E> get iterator => new WhereIterator<E>(_iterable.iterator, _f);
> }
> 
> class WhereIterator<E> extends Iterator<E> {
>   final Iterator<E> _iterator;
>   final _ElementPredicate _f;
> 
>   WhereIterator(this._iterator, bool this._f(E element));
> 
>   bool moveNext() {
>     while (_iterator.moveNext()) {
>       if (_f(_iterator.current)) {
>         return true;
>       }
>     }
>     return false;
>   }
> 
>   E get current => _iterator.current;
> }
> ```

aha, think I figured this out. It's a bug in the input code. Should be written
as _ElementPredicate<E> ... so my guess is it's being interpreted as
_ElementPredicate<dynamic>. Wow. Fix in the SDK source?

[vsm, 2015-03-18 22:34:38]

https://codereview.chromium.org/1010893004/diff/60001/test/codegen/expect/dar...
File test/codegen/expect/dart/_internal.js (right):

https://codereview.chromium.org/1010893004/diff/60001/test/codegen/expect/dar...
test/codegen/expect/dart/_internal.js:448: return new
WhereIterator(this[_iterable].iterator, dart.closureWrap(this[_f], "(E) →
bool"));
On 2015/03/18 21:53:22, John Messerly wrote:
> On 2015/03/18 21:50:59, John Messerly wrote:
> > hmmm. I'm a little confused, why did we closure wrap here? We're going from
`E
> > -> bool` to `E -> bool`. The only thing I can think of is the typedef is
> messing
> > us up, and it doesn't understand that _ElementPredicate and `bool this._f(E
> > element)` are the same type, after doing the generic substitution. Maybe it
> > thinks it's calling `new WhereIterator<dynamic>`?
> > 
> > here's the input Dart code:
> > 
> > ```dart
> > typedef bool _ElementPredicate<E>(E element);
> > 
> > class WhereIterable<E> extends IterableBase<E> {
> >   final Iterable<E> _iterable;
> >   final _ElementPredicate _f;
> > 
> >   WhereIterable(this._iterable, bool this._f(E element));
> > 
> >   Iterator<E> get iterator => new WhereIterator<E>(_iterable.iterator, _f);
> > }
> > 
> > class WhereIterator<E> extends Iterator<E> {
> >   final Iterator<E> _iterator;
> >   final _ElementPredicate _f;
> > 
> >   WhereIterator(this._iterator, bool this._f(E element));
> > 
> >   bool moveNext() {
> >     while (_iterator.moveNext()) {
> >       if (_f(_iterator.current)) {
> >         return true;
> >       }
> >     }
> >     return false;
> >   }
> > 
> >   E get current => _iterator.current;
> > }
> > ```
> 
> aha, think I figured this out. It's a bug in the input code. Should be written
> as _ElementPredicate<E> ... so my guess is it's being interpreted as
> _ElementPredicate<dynamic>. Wow. Fix in the SDK source?
> 

Right you are.  Fixed the source as suggested.

https://codereview.chromium.org/1010893004/diff/60001/test/codegen/expect/dar...
test/codegen/expect/dart/_internal.js:463: if (dart.dinvokef(this[_f],
this[_iterator].current)) {
On 2015/03/18 21:50:59, John Messerly wrote:
> similar question to above: why is this a dynamic operation?
> 
> _f is an _ElementPredicate
> 
> _iterator is an Iterator<E> which would make _iterator.current an E
> 
> so we're calling a `E -> bool` function passing in an `E`.

Ditto.

[Jennifer Messerly, 2015-03-18 22:54:30]

lgtm

[Leaf, 2015-03-18 22:57:26]

Couple small comments, but lgtm.

https://codereview.chromium.org/1010893004/diff/60001/lib/src/checker/rules.dart
File lib/src/checker/rules.dart (right):

https://codereview.chromium.org/1010893004/diff/60001/lib/src/checker/rules.d...
lib/src/checker/rules.dart:267: // In negative state, dynamic acts as bottom as
well as top, but in a
This comment I think is left over from the polarity version?

https://codereview.chromium.org/1010893004/diff/60001/lib/src/checker/rules.d...
lib/src/checker/rules.dart:508: if (call is SimpleIdentifier) {
This could probably be done for at least some prefixed identifiers as well.

https://codereview.chromium.org/1010893004/diff/60001/test/checker/checker_te...
File test/checker/checker_test.dart (right):

https://codereview.chromium.org/1010893004/diff/60001/test/checker/checker_te...
test/checker/checker_test.dart:488: // Note: because we allow A->B <:
dynamic->B, we have circularity
This comment is no longer true, I think?

https://codereview.chromium.org/1010893004/diff/60001/test/checker/checker_te...
test/checker/checker_test.dart:491: typedef dynamic Left(A x);         // Real
left
Nit: Sorting Top -> Left, Right -> Bottom makes this a little easier to read.

https://codereview.chromium.org/1010893004/diff/60001/test/checker/checker_te...
test/checker/checker_test.dart:2248:
/*severe:InvalidMethodOverride,severe:InvalidMethodOverride*/m(a) {}
We may want to treat this differently long term.  Semantically, we are treating
this as being m(Object a), which is a valid override.  Basically the same
"optimization" that applies to your isDynamicCall function applies here: m is a
concrete method which will not be reified as dynamic -> dynamic, but rather as
Object -> dynamic.

https://codereview.chromium.org/1010893004/diff/60001/test/codegen/expect/dar...
File test/codegen/expect/dart/collection.js (right):

https://codereview.chromium.org/1010893004/diff/60001/test/codegen/expect/dar...
test/codegen/expect/dart/collection.js:40: hashCode =
dart.closureWrap(_defaultHashCode, "(K) → int");
In this case, I believe that a cast will succeed, since _defaultHashCode will
get reified as Object -> int.  I wonder if this generalizes with this change...
that more of the ClosureWraps could usefully be made into casts?  On further
thought though - really this is another case where we should be generalizing
your "isDynamicCall" optimization further.  This shouldn't even be a cast - we
should treat _defaultHashCode as having type "Object -> int" not "dynamic ->
int", and so this subtyping should succeed.

[vsm, 2015-03-18 23:40:33]

https://codereview.chromium.org/1010893004/diff/60001/lib/src/checker/rules.dart
File lib/src/checker/rules.dart (right):

https://codereview.chromium.org/1010893004/diff/60001/lib/src/checker/rules.d...
lib/src/checker/rules.dart:267: // In negative state, dynamic acts as bottom as
well as top, but in a
On 2015/03/18 22:57:25, leafp wrote:
> This comment I think is left over from the polarity version?

Done.

https://codereview.chromium.org/1010893004/diff/60001/lib/src/checker/rules.d...
lib/src/checker/rules.dart:508: if (call is SimpleIdentifier) {
On 2015/03/18 22:57:25, leafp wrote:
> This could probably be done for at least some prefixed identifiers as well.

The "call" expression is the method name (not the actual call expression) the
way this is currently used.

https://codereview.chromium.org/1010893004/diff/60001/test/checker/checker_te...
File test/checker/checker_test.dart (right):

https://codereview.chromium.org/1010893004/diff/60001/test/checker/checker_te...
test/checker/checker_test.dart:488: // Note: because we allow A->B <:
dynamic->B, we have circularity
On 2015/03/18 22:57:26, leafp wrote:
> This comment is no longer true, I think?

Removed

https://codereview.chromium.org/1010893004/diff/60001/test/checker/checker_te...
test/checker/checker_test.dart:491: typedef dynamic Left(A x);         // Real
left
On 2015/03/18 22:57:26, leafp wrote:
> Nit: Sorting Top -> Left, Right -> Bottom makes this a little easier to read. 


Done.

https://codereview.chromium.org/1010893004/diff/60001/test/checker/checker_te...
test/checker/checker_test.dart:2248:
/*severe:InvalidMethodOverride,severe:InvalidMethodOverride*/m(a) {}
On 2015/03/18 22:57:26, leafp wrote:
> We may want to treat this differently long term.  Semantically, we are
treating
> this as being m(Object a), which is a valid override.  Basically the same
> "optimization" that applies to your isDynamicCall function applies here: m is
a
> concrete method which will not be reified as dynamic -> dynamic, but rather as
> Object -> dynamic.

Agree.  It seems as though we want the static type of m to be Object->dynamic,
but the static type of a to still be dynamic.

If we do this, a bunch of the DynamicInvokes in my test above also disappear.

https://codereview.chromium.org/1010893004/diff/60001/test/codegen/expect/dar...
File test/codegen/expect/dart/collection.js (right):

https://codereview.chromium.org/1010893004/diff/60001/test/codegen/expect/dar...
test/codegen/expect/dart/collection.js:40: hashCode =
dart.closureWrap(_defaultHashCode, "(K) → int");
On 2015/03/18 22:57:26, leafp wrote:
> In this case, I believe that a cast will succeed, since _defaultHashCode will
> get reified as Object -> int.  I wonder if this generalizes with this
change...
> that more of the ClosureWraps could usefully be made into casts?  On further
> thought though - really this is another case where we should be generalizing
> your "isDynamicCall" optimization further.  This shouldn't even be a cast - we
> should treat _defaultHashCode as having type "Object -> int" not "dynamic ->
> int", and so this subtyping should succeed.

Agreed.  I'll start taking a look at closure wrapping.

[vsm, 2015-03-18 23:40:51]

Committed patchset #6 (id:100001) manually as
8f41f42b26f2ee5f5d865e16b5c602460341096e (presubmit successful).