Implemented class literals in the VM.

Implemented class literals in the VM.
Committed: https://code.google.com/p/dart/source/detail?r=16530

[regis, 2012-12-21 00:09:10]

https://codereview.chromium.org/11633054/diff/3001/runtime/vm/parser.cc
File runtime/vm/parser.cc (left):

https://codereview.chromium.org/11633054/diff/3001/runtime/vm/parser.cc#oldco...
runtime/vm/parser.cc:8346: }
Removing this code cannot be correct. The identifier still needs to get resolved
to the type parameter or it will wrongly get resolved in the library scope
below.

https://codereview.chromium.org/11633054/diff/3001/runtime/vm/parser.cc#oldco...
runtime/vm/parser.cc:9291: }
ditto

https://codereview.chromium.org/11633054/diff/3001/tests/language/class_liter...
File tests/language/class_literal_test.dart (right):

https://codereview.chromium.org/11633054/diff/3001/tests/language/class_liter...
tests/language/class_literal_test.dart:61: // This fails because toString() is
not static -- need spec decision
How about
vax x = Class;
Expect.equals(x.toString(), "Class");

https://codereview.chromium.org/11633054/diff/3001/tests/language/class_liter...
tests/language/class_literal_test.dart:63: Expect.throws(() { Class.toString();
}, (e) => e is NoSuchMethodError);
You do not have any tests for type parameters.

[Ivan Posva, 2012-12-21 00:14:27]

DBC

It would also be nice to add a test for this:

class D<A> {
  giveMeAnA() => A;
}

main() {
  Expect.identical((new D<int>()).runtimeType, int);
}

https://codereview.chromium.org/11633054/diff/3001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/11633054/diff/3001/runtime/vm/parser.cc#newco...
runtime/vm/parser.cc:7482: } else if
(left->AsPrimaryNode()->primary().IsClass()) {
left->AsPrimaryNode() can be short-circuited to primary.

https://codereview.chromium.org/11633054/diff/3001/runtime/vm/parser.cc#newco...
runtime/vm/parser.cc:7486: Type::New(type_class, TypeArguments::Handle(),
Instead of the acrobatics creating a type_class handle, you could do
Class::Cast(primary->primary()).

It would also be safer to be explicit about requesting an old space object here.

[hausner, 2012-12-21 00:31:42]

Definitely need tests that use type parameters.

https://codereview.chromium.org/11633054/diff/3001/tests/language/class_liter...
File tests/language/class_literal_test.dart (right):

https://codereview.chromium.org/11633054/diff/3001/tests/language/class_liter...
tests/language/class_literal_test.dart:43: Expect.throws(() { Class.method(); },
(e) => e is NoSuchMethodError);
This is probably still a static type warning, in addition to a runtime error.
Does the analyzer test pass without annotating the warning? Ditto for many of
the cases below.

[regis, 2012-12-21 02:23:46]

Tom,

As mentioned earlier, it may help to look at how type tests (is) and type casts
(as) are implemented. For both of them, the parser emits a TypeNode instead of a
LiteralNode as you currently do. The problem with a LiteralNode is that it is a
compile time constant, which works for classes, but not for type parameters,
since they are not instantiated.
A type parameter needs to be instantiated from the type arguments of the
instantiator at runtime, i.e. you need to emit code.

The parser should also make sure that the instantiator is captured when
necessary and a TypeError should be thrown if the type is malformed. However,
since you only support a raw type or a type parameter, I do not think the type
can be malformed.

Your code should look very similar to the code in Parser::ParseBinaryExpr that
creates the TypeNode.

Cheers,
Regis

[Tom Ball, 2012-12-21 22:24:11]

This CL now only adds support for class literals, not type parameter literals. 
I'll make that the next change, once sufficient tests for it are finished.

https://codereview.chromium.org/11633054/diff/3001/runtime/vm/parser.cc
File runtime/vm/parser.cc (left):

https://codereview.chromium.org/11633054/diff/3001/runtime/vm/parser.cc#oldco...
runtime/vm/parser.cc:8346: }
On 2012/12/21 00:09:10, regis wrote:
> Removing this code cannot be correct. The identifier still needs to get
resolved
> to the type parameter or it will wrongly get resolved in the library scope
> below.

I restored this block and the next, and restored the test failure lines for the
tests that failed.  That means this CL only addresses class literals now.

https://codereview.chromium.org/11633054/diff/3001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/11633054/diff/3001/runtime/vm/parser.cc#newco...
runtime/vm/parser.cc:7482: } else if
(left->AsPrimaryNode()->primary().IsClass()) {
On 2012/12/21 00:14:27, Ivan Posva wrote:
> left->AsPrimaryNode() can be short-circuited to primary.

Done.

https://codereview.chromium.org/11633054/diff/3001/runtime/vm/parser.cc#newco...
runtime/vm/parser.cc:7486: Type::New(type_class, TypeArguments::Handle(),
On 2012/12/21 00:14:27, Ivan Posva wrote:
> Instead of the acrobatics creating a type_class handle, you could do
> Class::Cast(primary->primary()).
> 
> It would also be safer to be explicit about requesting an old space object
here.

Done.

https://codereview.chromium.org/11633054/diff/3001/tests/language/class_liter...
File tests/language/class_literal_test.dart (right):

https://codereview.chromium.org/11633054/diff/3001/tests/language/class_liter...
tests/language/class_literal_test.dart:61: // This fails because toString() is
not static -- need spec decision
On 2012/12/21 00:09:10, regis wrote:
> How about
> vax x = Class;
> Expect.equals(x.toString(), "Class");

Added.  I also removed the comment about needing a spec decision, as it was
decided to tackle that issue when classes are made first-class types.

https://codereview.chromium.org/11633054/diff/3001/tests/language/class_liter...
tests/language/class_literal_test.dart:63: Expect.throws(() { Class.toString();
}, (e) => e is NoSuchMethodError);
On 2012/12/21 00:09:10, regis wrote:
> You do not have any tests for type parameters.

Right -- I took Ivan's example test and created
tests/language/type_parameter_literal_test.dart, and marked it as failing for
now.

[regis, 2012-12-21 22:39:01]

You need to upload your latest version for review.

I am fine with you only supporting classes in this cl, however, I do not see the
benefit, since you will have to revert most of the code added here to support
type parameters. As explained earlier, you will need to emit a TypeNode and not
a LiteralNode.

[Tom Ball, 2012-12-21 23:43:39]

On 2012/12/21 22:39:01, regis wrote:
> You need to upload your latest version for review.
> 
> I am fine with you only supporting classes in this cl, however, I do not see
the
> benefit, since you will have to revert most of the code added here to support
> type parameters. As explained earlier, you will need to emit a TypeNode and
not
> a LiteralNode.

Sorry, I thought your last feedback was specific to type parameter literals. The
latest emits a TypeNode as you suggest (though not necessarily as you envisioned
:-).  In addition to using a TypeNode, I had to add TypeNode::EvalConstExpr()
and add support in the flow graph builder for this new use of TypeNode.

[regis, 2012-12-22 00:04:50]

It's getting there. If you could make sure that the test passes, that would be
great.

https://codereview.chromium.org/11633054/diff/12001/runtime/vm/ast.h
File runtime/vm/ast.h (right):

https://codereview.chromium.org/11633054/diff/12001/runtime/vm/ast.h#newcode350
runtime/vm/ast.h:350: }
You need to check if the type is instantiated and not malformed before returning
it.

As explained, a TypeNode is only a compile time constant in the special case of
a non-generic class or raw type. Uninstantiated types can only be instantiated
at run time.

https://codereview.chromium.org/11633054/diff/12001/tests/language/language.s...
File tests/language/language.status (right):

https://codereview.chromium.org/11633054/diff/12001/tests/language/language.s...
tests/language/language.status:45: first_class_types_literals_test: Fail # Issue
7549
See my comment in issue 7549.
It would be nice to get this test working when submitting this cl.

[Tom Ball, 2012-12-27 22:13:44]

https://codereview.chromium.org/11633054/diff/12001/runtime/vm/ast.h
File runtime/vm/ast.h (right):

https://codereview.chromium.org/11633054/diff/12001/runtime/vm/ast.h#newcode350
runtime/vm/ast.h:350: }
On 2012/12/22 00:04:50, regis wrote:
> You need to check if the type is instantiated and not malformed before
returning
> it.
> 
> As explained, a TypeNode is only a compile time constant in the special case
of
> a non-generic class or raw type. Uninstantiated types can only be instantiated
> at run time. 

Done.

https://codereview.chromium.org/11633054/diff/12001/tests/language/language.s...
File tests/language/language.status (right):

https://codereview.chromium.org/11633054/diff/12001/tests/language/language.s...
tests/language/language.status:45: first_class_types_literals_test: Fail # Issue
7549
On 2012/12/22 00:04:50, regis wrote:
> See my comment in issue 7549.
> It would be nice to get this test working when submitting this cl.

Fixed by commit 16516, status file updated.

[regis, 2012-12-27 22:51:19]

LGTM with a question.

https://codereview.chromium.org/11633054/diff/29001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/11633054/diff/29001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:6897: }
Why is this code here and not in TypeNode's MakeAssignmentNode?
Also, can you add a comment explaining why this code is necessary? Without it, I
guess that the parser would report a compile time error? Is that not acceptable
by the spec?

[Tom Ball, 2012-12-27 23:16:31]

https://codereview.chromium.org/11633054/diff/29001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/11633054/diff/29001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:6897: }
On 2012/12/27 22:51:24, regis wrote:
> Why is this code here and not in TypeNode's MakeAssignmentNode?

I was following existing code here for a test of original->IsStaticGetterNode(),
which was removed a few days ago.  That code was apparently in the parser rather
than ast, because there's no equivalent ThrowNoSuchMethodError function in
AstNode, nor the MakeStaticCall function it depends upon.  I can copy all that
code into TypeNode if necessary, but since there are several
ThrowNoSUchMethodError calls in the parser it seems this is the right place
(IMO).

> Also, can you add a comment explaining why this code is necessary? Without it,
I
> guess that the parser would report a compile time error? Is that not
acceptable
> by the spec?

This is one of several places where the VM catches errors at compile time, which
are spec'd to be caught at runtime.  I double-checked with Gilad before adding
this check.

[regis, 2012-12-27 23:37:45]

On 2012/12/27 23:16:31, Tom Ball wrote:
> https://codereview.chromium.org/11633054/diff/29001/runtime/vm/parser.cc
> File runtime/vm/parser.cc (right):
> 
>
https://codereview.chromium.org/11633054/diff/29001/runtime/vm/parser.cc#newc...
> runtime/vm/parser.cc:6897: }
> On 2012/12/27 22:51:24, regis wrote:
> > Why is this code here and not in TypeNode's MakeAssignmentNode?
> 
> I was following existing code here for a test of
original->IsStaticGetterNode(),
> which was removed a few days ago.  That code was apparently in the parser
rather
> than ast, because there's no equivalent ThrowNoSuchMethodError function in
> AstNode, nor the MakeStaticCall function it depends upon.  I can copy all that
> code into TypeNode if necessary, but since there are several
> ThrowNoSUchMethodError calls in the parser it seems this is the right place
> (IMO).
> 
> > Also, can you add a comment explaining why this code is necessary? Without
it,
> I
> > guess that the parser would report a compile time error? Is that not
> acceptable
> > by the spec?
> 
> This is one of several places where the VM catches errors at compile time,
which
> are spec'd to be caught at runtime.  I double-checked with Gilad before adding
> this check.

I see. ThrowNoSuchMethodError is not available in ast.cc. It's  fine to do it
here, then.

And are we sure that immediately throwing a NoSuchMethodError is the right thing
to do? In a dynamic context, I would imagine that trying to call a setter named
after the class would be more correct. Maybe you can add a TODO for now.

[Ivan Posva, 2012-12-28 01:22:39]

https://codereview.chromium.org/11633054/diff/36002/tests/language/type_param...
File tests/language/type_parameter_literal_test.dart (right):

https://codereview.chromium.org/11633054/diff/36002/tests/language/type_param...
tests/language/type_parameter_literal_test.dart:8: T getT() {
I am wondering why this is passing in checked mode. The returned value is of
type Type and not T.

[regis, 2012-12-28 01:35:00]

On 2012/12/28 01:22:39, Ivan Posva wrote:
>
https://codereview.chromium.org/11633054/diff/36002/tests/language/type_param...
> File tests/language/type_parameter_literal_test.dart (right):
> 
>
https://codereview.chromium.org/11633054/diff/36002/tests/language/type_param...
> tests/language/type_parameter_literal_test.dart:8: T getT() {
> I am wondering why this is passing in checked mode. The returned value is of
> type Type and not T.

Tom does not support type parameters yet, and this test is disabled. But that is
a good point. The result type should be Type.

[Tom Ball, 2012-12-28 04:49:28]

https://codereview.chromium.org/11633054/diff/36002/tests/language/type_param...
File tests/language/type_parameter_literal_test.dart (right):

https://codereview.chromium.org/11633054/diff/36002/tests/language/type_param...
tests/language/type_parameter_literal_test.dart:8: T getT() {
On 2012/12/28 01:22:39, Ivan Posva wrote:
> I am wondering why this is passing in checked mode. The returned value is of
> type Type and not T.

Fixed for next CL.