Update dart_runtime.dart rules

Update dart_runtime.dart rules

This should bring it in line with rules.dart.

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

Committed: https://github.com/dart-lang/dev_compiler/commit/2c91c53d1afa42c76de06fc203d47447c5ac1e5b

[vsm, 2015-03-31 23:37:18]

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

[vsm, 2015-04-01 17:07:26]

On 2015/03/31 23:37:24, vsm wrote:

Update to catch a bug on mixins noted by jmesserly.

[Jennifer Messerly, 2015-04-01 19:12:46]

this looks good, some questions about Null and type variables

https://codereview.chromium.org/1043323002/diff/20001/lib/runtime/dart_runtim...
File lib/runtime/dart_runtime.dart (right):

https://codereview.chromium.org/1043323002/diff/20001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.dart:227: if (t == _nullMirror &&
_typeOptions.nonnullableTypes.isEmpty) return true;
is this check needed? Normally isSubtype does not consider Null to be a subtype
of everything:

main() {
  print(null is String); // false
  print(null is Object); // true
  print(null is dynamic); // true
  print(null is Null); // true
}

... instead, it's just that null is explicitly allowed in "as" type cast:

> Evaluation of the cast expression e as T proceeds as follows:
> The expression e is evaluated to a value v. Then, if T is a malformed or
> deferred type (19.1), a dynamic error occurs. Otherwise, if the interface of
the
> class of v is a subtype of T, the cast expression evaluates to v. Otherwise,
if
> v is null, the cast expression evaluates to v. In all other cases, a CastError
is
> thrown.

this is also noted in the section on the `null` literal:

> The static type of null is ⊥.
> The decision to use ⊥ instead of Null allows null to be be assigned everywhere
> without complaint by the static checker.


Also the _typeOptions.nonnullableTypes.isEmpty seems odd -- just because we add
one nonnullableType to the system, why does that change the behavior of Null
uniformly?

https://codereview.chromium.org/1043323002/diff/20001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.dart:239: if (mirror is TypeVariableMirror) return
false;
Does this happen at runtime?

$ cat test.dart
import 'dart:mirrors';

class Foo<B> {
  foo() {
    print(B);
    print(reflectType(B));
    print(reflectType(new List<B>().runtimeType).typeArguments);
  }
}

class Bar<Z> {
  bar() {
    new Foo<Z>().foo();
  }
}

main() => new Bar<int>().bar();

$ dart test.dart
int
ClassMirror on 'int'
[ClassMirror on 'int']


In other words, can I possibly do "is E" where E is a TypeVariable instead of a
substituted value? This seems like probably dead code.

https://codereview.chromium.org/1043323002/diff/20001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.dart:299: if (t1 is TypeVariableMirror) {
similar question about TypeVariableMirror, do we actually get these?

[vsm, 2015-04-01 21:04:04]

thanks - PTAL

https://codereview.chromium.org/1043323002/diff/20001/lib/runtime/dart_runtim...
File lib/runtime/dart_runtime.dart (right):

https://codereview.chromium.org/1043323002/diff/20001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.dart:227: if (t == _nullMirror &&
_typeOptions.nonnullableTypes.isEmpty) return true;
On 2015/04/01 19:12:46, John Messerly wrote:
> is this check needed? Normally isSubtype does not consider Null to be a
subtype
> of everything:
> 
> main() {
>   print(null is String); // false
>   print(null is Object); // true
>   print(null is dynamic); // true
>   print(null is Null); // true
> }
> 
> ... instead, it's just that null is explicitly allowed in "as" type cast:
> 
> > Evaluation of the cast expression e as T proceeds as follows:
> > The expression e is evaluated to a value v. Then, if T is a malformed or
> > deferred type (19.1), a dynamic error occurs. Otherwise, if the interface of
> the
> > class of v is a subtype of T, the cast expression evaluates to v. Otherwise,
> if
> > v is null, the cast expression evaluates to v. In all other cases, a
CastError
> is
> > thrown.
> 
> this is also noted in the section on the `null` literal:
> 
> > The static type of null is ⊥.
> > The decision to use ⊥ instead of Null allows null to be be assigned
everywhere
> > without complaint by the static checker.
> 
> 
> Also the _typeOptions.nonnullableTypes.isEmpty seems odd -- just because we
add
> one nonnullableType to the system, why does that change the behavior of Null
> uniformly?

Yes, I'd been treating Null as bottom (as in the static types).  Updated.

https://codereview.chromium.org/1043323002/diff/20001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.dart:239: if (mirror is TypeVariableMirror) return
false;
On 2015/04/01 19:12:46, John Messerly wrote:
> Does this happen at runtime?
> 
> $ cat test.dart
> import 'dart:mirrors';
> 
> class Foo<B> {
>   foo() {
>     print(B);
>     print(reflectType(B));
>     print(reflectType(new List<B>().runtimeType).typeArguments);
>   }
> }
> 
> class Bar<Z> {
>   bar() {
>     new Foo<Z>().foo();
>   }
> }
> 
> main() => new Bar<int>().bar();
> 
> $ dart test.dart
> int
> ClassMirror on 'int'
> [ClassMirror on 'int']
> 
> 
> In other words, can I possibly do "is E" where E is a TypeVariable instead of
a
> substituted value? This seems like probably dead code.

I think you are right.  Changed this to an assert.

https://codereview.chromium.org/1043323002/diff/20001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.dart:299: if (t1 is TypeVariableMirror) {
On 2015/04/01 19:12:46, John Messerly wrote:
> similar question about TypeVariableMirror, do we actually get these?

I actually did hit this, in part because of how we canonicalize above.  The
above canonicalizes List<Object> to List.  Ideally, we'd canonicalize List to
List<Object>, but mirrors don't appear to let us construct the latter from the
former.

In JS, perhaps we should do it that way though.

[Jennifer Messerly, 2015-04-02 18:24:38]

lgtm

https://codereview.chromium.org/1043323002/diff/40001/lib/runtime/dart_runtim...
File lib/runtime/dart_runtime.dart (right):

https://codereview.chromium.org/1043323002/diff/40001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.dart:216: final _nullMirror = reflectType(Null);
fyi, I think this is unused now

[Leaf, 2015-04-03 00:34:02]

https://codereview.chromium.org/1043323002/diff/40001/lib/runtime/dart_runtim...
File lib/runtime/dart_runtime.dart (right):

https://codereview.chromium.org/1043323002/diff/40001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.dart:231: // Disallow generic type parameters.
More like // type parameters shouldn't happen here?

https://codereview.chromium.org/1043323002/diff/40001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.dart:267: if (!_isTop(typeArgument)) return false;
This treats List<Object> as a raw type.  This is maybe ok (at least for the
purpose of deciding whether to allow is checks) if we reify dynamic as Object? 
That is, we would always agree with normal dart on queries of the form (x is
List<dynamic>).  For queries of the form (x is List<Object>), we would would
disagree if x was reified as List<dynamic>, but if we rule that out on our side
by reifying x as List<Object>, then we will also accept.  So.. I think this is
ok for the specific uses we have in mind, but it's a little fragile.

https://codereview.chromium.org/1043323002/diff/40001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.dart:299: if (_isTop(t2, dynamicIsBottom:
dynamicIsBottom) ||
I think this makes dynamic <: Object, which I think we can no longer allow given
the fuzzy type rule.

[vsm, 2015-04-06 22:28:14]

https://codereview.chromium.org/1043323002/diff/40001/lib/runtime/dart_runtim...
File lib/runtime/dart_runtime.dart (right):

https://codereview.chromium.org/1043323002/diff/40001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.dart:216: final _nullMirror = reflectType(Null);
On 2015/04/02 18:24:38, John Messerly wrote:
> fyi, I think this is unused now

removed

https://codereview.chromium.org/1043323002/diff/40001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.dart:231: // Disallow generic type parameters.
On 2015/04/03 00:34:02, leafp wrote:
> More like // type parameters shouldn't happen here?

Done.

https://codereview.chromium.org/1043323002/diff/40001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.dart:267: if (!_isTop(typeArgument)) return false;
On 2015/04/03 00:34:02, leafp wrote:
> This treats List<Object> as a raw type.  This is maybe ok (at least for the
> purpose of deciding whether to allow is checks) if we reify dynamic as Object?

> That is, we would always agree with normal dart on queries of the form (x is
> List<dynamic>).  For queries of the form (x is List<Object>), we would would
> disagree if x was reified as List<dynamic>, but if we rule that out on our
side
> by reifying x as List<Object>, then we will also accept.  So.. I think this is
> ok for the specific uses we have in mind, but it's a little fragile.

Hmm, both:
- List<Object> <: List<dynamic>
- List<dynamic> <: List<Object>
are true in both Dart and our current static rules (which treat both as
List<Top>), so I'm not seeing disagreement regardless of whether we reify
List<dynamic>.

Note: dart:mirrors only allows us to create List from List<Object>, not the
other way around.  Otherwise, I think I'd prefer to map both to List<Object> for
clarity.

https://codereview.chromium.org/1043323002/diff/40001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.dart:299: if (_isTop(t2, dynamicIsBottom:
dynamicIsBottom) ||
On 2015/04/03 00:34:01, leafp wrote:
> I think this makes dynamic <: Object, which I think we can no longer allow
given
> the fuzzy type rule.

Hmm, I thought we added this with the new rule.  :-)

[Leaf, 2015-04-06 23:42:52]

On 2015/04/06 22:28:14, vsm wrote:
>
https://codereview.chromium.org/1043323002/diff/40001/lib/runtime/dart_runtim...
> File lib/runtime/dart_runtime.dart (right):
> 
>
https://codereview.chromium.org/1043323002/diff/40001/lib/runtime/dart_runtim...
> lib/runtime/dart_runtime.dart:216: final _nullMirror = reflectType(Null);
> On 2015/04/02 18:24:38, John Messerly wrote:
> > fyi, I think this is unused now
> 
> removed
> 
>
https://codereview.chromium.org/1043323002/diff/40001/lib/runtime/dart_runtim...
> lib/runtime/dart_runtime.dart:231: // Disallow generic type parameters.
> On 2015/04/03 00:34:02, leafp wrote:
> > More like // type parameters shouldn't happen here?
> 
> Done.
> 
>
https://codereview.chromium.org/1043323002/diff/40001/lib/runtime/dart_runtim...
> lib/runtime/dart_runtime.dart:267: if (!_isTop(typeArgument)) return false;
> On 2015/04/03 00:34:02, leafp wrote:
> > This treats List<Object> as a raw type.  This is maybe ok (at least for the
> > purpose of deciding whether to allow is checks) if we reify dynamic as
Object?
> 
> > That is, we would always agree with normal dart on queries of the form (x is
> > List<dynamic>).  For queries of the form (x is List<Object>), we would would
> > disagree if x was reified as List<dynamic>, but if we rule that out on our
> side
> > by reifying x as List<Object>, then we will also accept.  So.. I think this
is
> > ok for the specific uses we have in mind, but it's a little fragile.
> 
> Hmm, both:
> - List<Object> <: List<dynamic>
> - List<dynamic> <: List<Object>
> are true in both Dart and our current static rules (which treat both as
> List<Top>), so I'm not seeing disagreement regardless of whether we reify
> List<dynamic>.
> 
> Note: dart:mirrors only allows us to create List from List<Object>, not the
> other way around.  Otherwise, I think I'd prefer to map both to List<Object>
for
> clarity.
> 
>
https://codereview.chromium.org/1043323002/diff/40001/lib/runtime/dart_runtim...
> lib/runtime/dart_runtime.dart:299: if (_isTop(t2, dynamicIsBottom:
> dynamicIsBottom) ||
> On 2015/04/03 00:34:01, leafp wrote:
> > I think this makes dynamic <: Object, which I think we can no longer allow
> given
> > the fuzzy type rule.
> 
> Hmm, I thought we added this with the new rule.  :-)

After some offline discussion, this lgtm.

[vsm, 2015-04-07 12:57:37]

Committed patchset #4 (id:60001) manually as
2c91c53d1afa42c76de06fc203d47447c5ac1e5b (presubmit successful).