Implement simple dynamic type check.

Implement simple dynamic type check.

The generated code for the test is:
(before)
 $.isTrue(typeof x === 'object' && !!x.is$Hest, (void 0));
 $.isFalse(typeof x === 'object' && !!x.is$Hest, (void 0));
(with patch)
 $.setRuntimeTypeInfo(x, ({X: 'int'}));
 var t0 = $.getRuntimeTypeInfo(x);
 $.isTrue(typeof x === 'object' && !!x.is$Hest && typeof t0 === 'object' && t0.X === 'int', (void 0));
 var t1 = $.getRuntimeTypeInfo(x);
 $.isFalse(typeof x === 'object' && !!x.is$Hest && typeof t1 === 'object' && t1.X === 'String', (void 0));

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

[kasperl, 2012-04-30 08:48:31]

LGTM, but somehow it feels silly to always have a call to
getRuntimeTypeInformation and then inline the actual checks. Maybe the right way
is to share the code and turn it into something ala:

   $.isTrue($.is$Hest$int(x));

You could even imagine adding caching extra information (on-demand) on the
object to make these checks cheaper if they are repeated.

https://chromiumcodereview.appspot.com/10232011/diff/6018/frog/tests/leg_only...
File frog/tests/leg_only/src/GenericsIsCheck1Test.dart (right):

https://chromiumcodereview.appspot.com/10232011/diff/6018/frog/tests/leg_only...
frog/tests/leg_only/src/GenericsIsCheck1Test.dart:1: // Copyright (c) 2012, the
Dart project authors.  Please see the AUTHORS file
Maybe rename this file? Didn't Siggi change the naming convention for these
files recently?

https://chromiumcodereview.appspot.com/10232011/diff/6018/frog/tests/leg_only...
frog/tests/leg_only/src/GenericsIsCheck1Test.dart:4: // Test that parameters
keep their names in the output.
Not sure I understand line 4 here. Get rid of it?

https://chromiumcodereview.appspot.com/10232011/diff/6018/frog/tests/leg_only...
frog/tests/leg_only/src/GenericsIsCheck1Test.dart:10: Expect.isTrue(x is
Hest<int>);
So would 'x is Hest<num>', 'x is Hest<Object>', 'x is Hest<Dynamic>' or 'x is
Hest' work? Maybe add them if they already work.

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:1810:
<HInstruction>[typeInfoGetter, expression]);
Line too long.

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:1821: if
(compiler.universe.rti.hasTypeArguments(type)) {
Change this check to typeInfo != null?

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:2198: HType elementType =
HType.UNKNOWN;
Factor out the computation of elementType?

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:2216: if
(compiler.universe.rti.hasTypeArguments(type)) {
Factor out the code in this if?

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:2299: if
(send.selector.asTypeAnnotation() !== null) {
I wonder if it wouldn't be cleaner with is checks here?

   send.selector is TypeAnnotation

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:2307:
compiler.internalError("malformed send in new expression");
Do we usually start our error messages with a capital letter?

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
File lib/compiler/implementation/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:2092: final Type typeExpression;
It really isn't an expression but I guess it isn't a name either.

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:2096: HInstruction typeInfo, [nullOk
= false])
Can't we use this.nullOk in the parameter list?

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
File lib/compiler/implementation/ssa/ssa.dart (right):

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
lib/compiler/implementation/ssa/ssa.dart:42: return buffer.toString();
How about adding the { } as part of the return:

   return "{$buffer}";

That looks pretty nice to me.

[ngeoffray, 2012-04-30 08:51:12]

DBC

https://chromiumcodereview.appspot.com/10232011/diff/6018/frog/tests/leg_only...
File frog/tests/leg_only/src/GenericsIsCheck1Test.dart (right):

https://chromiumcodereview.appspot.com/10232011/diff/6018/frog/tests/leg_only...
frog/tests/leg_only/src/GenericsIsCheck1Test.dart:4: // Test that parameters
keep their names in the output.
Update comment.

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:1810:
<HInstruction>[typeInfoGetter, expression]);
line too long

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
File lib/compiler/implementation/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:1785:
cls.typeParameters.forEach((name, _) {
name -> Element typeParameter ?

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
File lib/compiler/implementation/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:2095:
HIs.withTypeInfo(this.typeExpression, HInstruction expression,
withTypeInfoCall?

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:2104: HInstruction get typeInfo() =>
inputs[1];
ditto

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
File lib/compiler/implementation/ssa/ssa.dart (right):

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
lib/compiler/implementation/ssa/ssa.dart:45: bool hasTypeArguments(Type type) {
Why is that not on the Type class?

[ahe, 2012-04-30 10:45:06]

On 2012/04/30 08:48:31, kasperl wrote:
>    $.isTrue($.is$Hest$int(x));

Unfortunately, that doesn't work in general. The "value" of the type argument
may not be known at compile time:

class Foo<T> {
  check(o) => o is Hest<T>;
}

Your suggest may be an optimization that should be tried out.

Cheers,
Peter

[karlklose, 2012-05-01 11:20:55]

Thanks for the reviews.

I will leave the inlined checks for now. I will implement checks involving type
variables next and optimize the type checks after that.

https://chromiumcodereview.appspot.com/10232011/diff/6018/frog/tests/leg_only...
File frog/tests/leg_only/src/GenericsIsCheck1Test.dart (right):

https://chromiumcodereview.appspot.com/10232011/diff/6018/frog/tests/leg_only...
frog/tests/leg_only/src/GenericsIsCheck1Test.dart:1: // Copyright (c) 2012, the
Dart project authors.  Please see the AUTHORS file
On 2012/04/30 08:48:32, kasperl wrote:
> Maybe rename this file? Didn't Siggi change the naming convention for these
> files recently?

Done.

https://chromiumcodereview.appspot.com/10232011/diff/6018/frog/tests/leg_only...
frog/tests/leg_only/src/GenericsIsCheck1Test.dart:4: // Test that parameters
keep their names in the output.
Done, removed.

https://chromiumcodereview.appspot.com/10232011/diff/6018/frog/tests/leg_only...
frog/tests/leg_only/src/GenericsIsCheck1Test.dart:10: Expect.isTrue(x is
Hest<int>);
It does not work currently.

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:1810:
<HInstruction>[typeInfoGetter, expression]);
On 2012/04/30 08:48:32, kasperl wrote:
> Line too long.

Done.

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:1810:
<HInstruction>[typeInfoGetter, expression]);
On 2012/04/30 08:51:12, ngeoffray wrote:
> line too long

Done.

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:1821: if
(compiler.universe.rti.hasTypeArguments(type)) {
On 2012/04/30 08:48:32, kasperl wrote:
> Change this check to typeInfo != null?

Done.

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:2198: HType elementType =
HType.UNKNOWN;
On 2012/04/30 08:48:32, kasperl wrote:
> Factor out the computation of elementType?

Done.

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:2216: if
(compiler.universe.rti.hasTypeArguments(type)) {
On 2012/04/30 08:48:32, kasperl wrote:
> Factor out the code in this if?

Done.

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:2299: if
(send.selector.asTypeAnnotation() !== null) {
Changed to is-checks.

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:2307:
compiler.internalError("malformed send in new expression");
We use both variants.

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
File lib/compiler/implementation/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:1785:
cls.typeParameters.forEach((name, _) {
typeParameters is a Map<SourceString, TypeVariableElement>, so the first
argument to the closure is actually a SourceString.

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
File lib/compiler/implementation/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:2092: final Type typeExpression;
This is actually reverting a renaming from an earlier CL.

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:2095:
HIs.withTypeInfo(this.typeExpression, HInstruction expression,
On 2012/04/30 08:51:12, ngeoffray wrote:
> withTypeInfoCall?

Done.

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:2096: HInstruction typeInfo, [nullOk
= false])
On 2012/04/30 08:48:32, kasperl wrote:
> Can't we use this.nullOk in the parameter list?

Done.

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:2104: HInstruction get typeInfo() =>
inputs[1];
On 2012/04/30 08:51:12, ngeoffray wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
File lib/compiler/implementation/ssa/ssa.dart (right):

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
lib/compiler/implementation/ssa/ssa.dart:42: return buffer.toString();
On 2012/04/30 08:48:32, kasperl wrote:
> How about adding the { } as part of the return:
> 
>    return "{$buffer}";
> 
> That looks pretty nice to me.

Done.

https://chromiumcodereview.appspot.com/10232011/diff/6018/lib/compiler/implem...
lib/compiler/implementation/ssa/ssa.dart:45: bool hasTypeArguments(Type type) {
On 2012/04/30 08:51:12, ngeoffray wrote:
> Why is that not on the Type class?

It isn't really used elsewhere, so I did not want to pollute Type's interface.