Implement type argument access and reification of runtime types.

Implement type argument access and reification of runtime types.

R=asgerf@google.com

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

[asgerf, 2015-03-02 13:52:37]

https://codereview.chromium.org/968843003/diff/1/pkg/compiler/lib/src/cps_ir/...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart (right):

https://codereview.chromium.org/968843003/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:959: return
buildReifyTypeVariable(new ir.This(), type);
Use irBuilder.buildThis(), otherwise 'this' inside a nested function will be
translated to a JS 'this' instead of the captured receiver.

https://codereview.chromium.org/968843003/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1385: assert(target is
ir.This);
FYI I believe this assertion is still valid with the above change.

https://codereview.chromium.org/968843003/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1923: new
ir.ReadTypeVariable(variable, context, target);
This seems wrong. We are always passing variable.element.enclosingClass as the
context. Why do we have two fields when one can be derived from the other? (Also
see long comment in the ir_nodes file).

https://codereview.chromium.org/968843003/diff/1/pkg/compiler/lib/src/cps_ir/...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart (right):

https://codereview.chromium.org/968843003/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:863: class ReifyRuntimeType
extends Primitive implements JsSpecificNode {
Please clarify that [value] should be a "JS type" as produced by
ReadTypeVariable. My first intuition was that you could give it any object and
this would give you back the corresponding Type object.

Likewise clarify at ReadTypeVariable that the result is a "JS type" and not a
Type object.

https://codereview.chromium.org/968843003/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:874: final ClassElement context;
Please provide a doc comment for 'context' and for the class itself. 'context'
in particular is a fairly generic name it's not obvious what it means here.

From our discussion last week, I *believe* context is a subclass of the type
that declares [variable], AND a superclass of the runtime type of [target], and
this is the invariant that we use.

Is it correctly understood that any class satisfying the above will work? Will
it generate better code if using a more specific class rather than just the
class declaring [variable]?

From an operational semantics point-of-view, it seems to me that context is
redundant, and we just keep it because it happens to carry some useful static
type information about the other operands. This is fine, but please clarify.

https://codereview.chromium.org/968843003/diff/1/pkg/compiler/lib/src/cps_ir/...
File pkg/compiler/lib/src/cps_ir/type_propagation.dart (right):

https://codereview.chromium.org/968843003/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:838: setValue(node,
nonConst());
We could use nonConst(typeSystem.typeType) here, right?

https://codereview.chromium.org/968843003/diff/1/pkg/compiler/lib/src/js_back...
File pkg/compiler/lib/src/js_backend/codegen/codegen.dart (right):

https://codereview.chromium.org/968843003/diff/1/pkg/compiler/lib/src/js_back...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:526: giveup(node,
'substitution not implemented');
I am confused. This should never happen because the context is always set to the
variable's enclosing class, and other the hand, some kind of substitution does
seem to be implemented below.

https://codereview.chromium.org/968843003/diff/1/pkg/compiler/lib/src/tree_ir...
File pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart (right):

https://codereview.chromium.org/968843003/diff/1/pkg/compiler/lib/src/tree_ir...
pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart:492:
Expression visitReifyRuntimeType(ReifyRuntimeType node) => node;
Visit and update subexpressions so variables can be inlined.

https://codereview.chromium.org/968843003/diff/1/tests/compiler/dart2js/js_ba...
File tests/compiler/dart2js/js_backend_cps_ir_runtime_types_test.dart (right):

https://codereview.chromium.org/968843003/diff/1/tests/compiler/dart2js/js_ba...
tests/compiler/dart2js/js_backend_cps_ir_runtime_types_test.dart:29: v1 =
$getTypeArgument(v0, 0);
This output is clearly wrong since v0 is uninitialized at this point.

[karlklose, 2015-03-05 09:31:21]

Patchset #4 (id:60001) has been deleted

[karlklose, 2015-03-05 09:54:58]

PTAL.

https://codereview.chromium.org/968843003/diff/1/pkg/compiler/lib/src/cps_ir/...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart (right):

https://codereview.chromium.org/968843003/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:959: return
buildReifyTypeVariable(new ir.This(), type);
On 2015/03/02 13:52:36, asgerf wrote:
> Use irBuilder.buildThis(), otherwise 'this' inside a nested function will be
> translated to a JS 'this' instead of the captured receiver.

Done.

https://codereview.chromium.org/968843003/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1385: assert(target is
ir.This);
On 2015/03/02 13:52:36, asgerf wrote:
> FYI I believe this assertion is still valid with the above change.

Acknowledged.

https://codereview.chromium.org/968843003/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1923: new
ir.ReadTypeVariable(variable, context, target);
I removed the context. We do not need it until we inline or specialize and we
should introduce it, when we need it for these features.

https://codereview.chromium.org/968843003/diff/1/pkg/compiler/lib/src/cps_ir/...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart (right):

https://codereview.chromium.org/968843003/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:863: class ReifyRuntimeType
extends Primitive implements JsSpecificNode {
On 2015/03/02 13:52:36, asgerf wrote:
> Please clarify that [value] should be a "JS type" as produced by
> ReadTypeVariable. My first intuition was that you could give it any object and
> this would give you back the corresponding Type object.
> 
> Likewise clarify at ReadTypeVariable that the result is a "JS type" and not a
> Type object.

Done.

https://codereview.chromium.org/968843003/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:874: final ClassElement context;
On 2015/03/02 13:52:36, asgerf wrote:
> Please provide a doc comment for 'context' and for the class itself. 'context'
> in particular is a fairly generic name it's not obvious what it means here.
> 
> From our discussion last week, I *believe* context is a subclass of the type
> that declares [variable], AND a superclass of the runtime type of [target],
and
> this is the invariant that we use.
> 
> Is it correctly understood that any class satisfying the above will work? Will
> it generate better code if using a more specific class rather than just the
> class declaring [variable]?
> 
> From an operational semantics point-of-view, it seems to me that context is
> redundant, and we just keep it because it happens to carry some useful static
> type information about the other operands. This is fine, but please clarify.

Acknowledged.

https://codereview.chromium.org/968843003/diff/1/pkg/compiler/lib/src/cps_ir/...
File pkg/compiler/lib/src/cps_ir/type_propagation.dart (right):

https://codereview.chromium.org/968843003/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:838: setValue(node,
nonConst());
On 2015/03/02 13:52:36, asgerf wrote:
> We could use nonConst(typeSystem.typeType) here, right?

Done.

https://codereview.chromium.org/968843003/diff/1/pkg/compiler/lib/src/js_back...
File pkg/compiler/lib/src/js_backend/codegen/codegen.dart (right):

https://codereview.chromium.org/968843003/diff/1/pkg/compiler/lib/src/js_back...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:526: giveup(node,
'substitution not implemented');
On 2015/03/02 13:52:36, asgerf wrote:
> I am confused. This should never happen because the context is always set to
the
> variable's enclosing class, and other the hand, some kind of substitution does
> seem to be implemented below.

Acknowledged.

https://codereview.chromium.org/968843003/diff/1/pkg/compiler/lib/src/tree_ir...
File pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart (right):

https://codereview.chromium.org/968843003/diff/1/pkg/compiler/lib/src/tree_ir...
pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart:492:
Expression visitReifyRuntimeType(ReifyRuntimeType node) => node;
I lost this change when cherry picking from my feature branch, and that was the
reason the generated code was wrong. It would be nice to have to be able to
catch that kind of bug with a checked mode version of the visitor.

https://codereview.chromium.org/968843003/diff/1/tests/compiler/dart2js/js_ba...
File tests/compiler/dart2js/js_backend_cps_ir_runtime_types_test.dart (right):

https://codereview.chromium.org/968843003/diff/1/tests/compiler/dart2js/js_ba...
tests/compiler/dart2js/js_backend_cps_ir_runtime_types_test.dart:29: v1 =
$getTypeArgument(v0, 0);
On 2015/03/02 13:52:37, asgerf wrote:
> This output is clearly wrong since v0 is uninitialized at this point.

Acknowledged.

[asgerf, 2015-03-05 10:58:07]

LGTM.

https://codereview.chromium.org/968843003/diff/1/pkg/compiler/lib/src/cps_ir/...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart (right):

https://codereview.chromium.org/968843003/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:863: class ReifyRuntimeType
extends Primitive implements JsSpecificNode {
On 2015/03/05 09:54:58, karlklose wrote:
> On 2015/03/02 13:52:36, asgerf wrote:
> > Please clarify that [value] should be a "JS type" as produced by
> > ReadTypeVariable. My first intuition was that you could give it any object
and
> > this would give you back the corresponding Type object.
> > 
> > Likewise clarify at ReadTypeVariable that the result is a "JS type" and not
a
> > Type object.
> 
> Done.

Great!

https://codereview.chromium.org/968843003/diff/80001/tests/compiler/dart2js/j...
File tests/compiler/dart2js/js_backend_cps_ir_runtime_types_test.dart (right):

https://codereview.chromium.org/968843003/diff/80001/tests/compiler/dart2js/j...
tests/compiler/dart2js/js_backend_cps_ir_runtime_types_test.dart:45:
"""$getSubstitutedTypeArgument(this, "\$asC", 1))));
Personally I think we should bypass the "long line" restriction inside test
cases when it avoids this type of line break.

[karlklose, 2015-03-05 11:57:28]

Committed patchset #5 (id:100001) manually as 44259 (presubmit successful).