Support class and typedef literals as expressions.

Support class and typedef literals as expressions.

For a class C, the expression 'C' returns the canonicalized instance of Type for this class. If C has type variables, they are instantiated with 'dynamic'.

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

[ahe, 2012-10-08 09:09:36]

Initial comments.

http://codereview.chromium.org/10942028/diff/12001/lib/compiler/implementatio...
File lib/compiler/implementation/resolver.dart (right):

http://codereview.chromium.org/10942028/diff/12001/lib/compiler/implementatio...
lib/compiler/implementation/resolver.dart:1446: if (resolvedReceiver != null &&
node.isOperator
I don't understand why it is important to test for node.isOperator here?

http://codereview.chromium.org/10942028/diff/12001/lib/compiler/implementatio...
lib/compiler/implementation/resolver.dart:1609:
classElement.ensureResolved(compiler);
Why is this not done in resolveSend?

http://codereview.chromium.org/10942028/diff/12001/lib/compiler/implementatio...
lib/compiler/implementation/resolver.dart:1664:
classElement.ensureResolved(compiler);
Why is this not done in resolveSend?

[ahe, 2012-10-08 09:13:42]

http://codereview.chromium.org/10942028/diff/12001/lib/compiler/implementatio...
File lib/compiler/implementation/tree/nodes.dart (right):

http://codereview.chromium.org/10942028/diff/12001/lib/compiler/implementatio...
lib/compiler/implementation/tree/nodes.dart:303: bool get
isPropertyAccessOrTypeReference => argumentsNode === null;
I'm having a hard time seeing why this name is better than the original. To take
this approach to its logical conclusion, shouldn't it be:
isPropertyAccessOrMethodReferenceOrTypeReference?

In my mind, this is not an improvement.

Also, depending on your perspective, a type is a property on the current
library. This is a perspective that is very natural to me.

[ngeoffray, 2012-10-10 07:32:24]

http://codereview.chromium.org/10942028/diff/12001/lib/compiler/implementatio...
File lib/compiler/implementation/lib/js_helper.dart (right):

http://codereview.chromium.org/10942028/diff/12001/lib/compiler/implementatio...
lib/compiler/implementation/lib/js_helper.dart:1354: Type get runtimeType =>
getOrCreateCachedRuntimeType('Type');
Looks fishy that you need to specialize this class.

http://codereview.chromium.org/10942028/diff/12001/lib/compiler/implementatio...
File lib/compiler/implementation/ssa/builder.dart (left):

http://codereview.chromium.org/10942028/diff/12001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/builder.dart:1324: if
(compiler.world.needsRti(classElement)) {
I don't think you should remove this check.

http://codereview.chromium.org/10942028/diff/12001/lib/compiler/implementatio...
File lib/compiler/implementation/ssa/builder.dart (right):

http://codereview.chromium.org/10942028/diff/12001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/builder.dart:2884: if (!variables.isEmpty()) {
Please add a comment that you need to put runtime names to type variables.

http://codereview.chromium.org/10942028/diff/12001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/builder.dart:3117: visitClassLiteralSend(node);
What is this about? Object = 42?

http://codereview.chromium.org/10942028/diff/12001/lib/compiler/implementatio...
File lib/compiler/implementation/tree/nodes.dart (right):

http://codereview.chromium.org/10942028/diff/12001/lib/compiler/implementatio...
lib/compiler/implementation/tree/nodes.dart:303: bool get
isPropertyAccessOrTypeReference => argumentsNode === null;
On 2012/10/08 09:13:42, ahe wrote:
> I'm having a hard time seeing why this name is better than the original. To
take
> this approach to its logical conclusion, shouldn't it be:
> isPropertyAccessOrMethodReferenceOrTypeReference?
> 
> In my mind, this is not an improvement.
> 
> Also, depending on your perspective, a type is a property on the current
> library. This is a perspective that is very natural to me.

I also prefer the original name.

http://codereview.chromium.org/10942028/diff/12001/tests/language/first_class...
File tests/language/first_class_types_test.dart (right):

http://codereview.chromium.org/10942028/diff/12001/tests/language/first_class...
tests/language/first_class_types_test.dart:40: // Test type literals.
Maybe create a new test for this file, so that the VM does not fail it?

[karlklose, 2012-10-23 10:33:52]

PTAL.

https://codereview.chromium.org/10942028/diff/12001/lib/compiler/implementati...
File lib/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/10942028/diff/12001/lib/compiler/implementati...
lib/compiler/implementation/lib/js_helper.dart:1354: Type get runtimeType =>
getOrCreateCachedRuntimeType('Type');
I was trying to hide the implementation name, but maybe we don't want to do
that.

https://codereview.chromium.org/10942028/diff/12001/lib/compiler/implementati...
File lib/compiler/implementation/resolver.dart (right):

https://codereview.chromium.org/10942028/diff/12001/lib/compiler/implementati...
lib/compiler/implementation/resolver.dart:1446: if (resolvedReceiver != null &&
node.isOperator
For class literals that are targets for operators, we resolve the target class
element and ensure the element is resolved (because we need it resolved for
runtime type information).
But we do not associate the Send with the class element, because that would be
interpreted as a call to a static operator on the class. Since there are no
static operators, we should interpret it as a call of the operator on the Type
instance.

https://codereview.chromium.org/10942028/diff/12001/lib/compiler/implementati...
lib/compiler/implementation/resolver.dart:1609:
classElement.ensureResolved(compiler);
Moved.

https://codereview.chromium.org/10942028/diff/12001/lib/compiler/implementati...
lib/compiler/implementation/resolver.dart:1664:
classElement.ensureResolved(compiler);
On 2012/10/08 09:09:36, ahe wrote:
> Why is this not done in resolveSend?

Done.

https://codereview.chromium.org/10942028/diff/12001/lib/compiler/implementati...
File lib/compiler/implementation/ssa/builder.dart (left):

https://codereview.chromium.org/10942028/diff/12001/lib/compiler/implementati...
lib/compiler/implementation/ssa/builder.dart:1324: if
(compiler.world.needsRti(classElement)) {
On 2012/10/10 07:32:24, ngeoffray wrote:
> I don't think you should remove this check.

Done.

https://codereview.chromium.org/10942028/diff/12001/lib/compiler/implementati...
File lib/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/10942028/diff/12001/lib/compiler/implementati...
lib/compiler/implementation/ssa/builder.dart:2884: if (!variables.isEmpty()) {
Here I add 'dynamic' as type argument for type variables when the type
annotation uses raw syntax. I'll add a comment explaining this.

https://codereview.chromium.org/10942028/diff/12001/lib/compiler/implementati...
lib/compiler/implementation/ssa/builder.dart:3117: visitClassLiteralSend(node);
also for ++ and --.

https://codereview.chromium.org/10942028/diff/12001/lib/compiler/implementati...
File lib/compiler/implementation/tree/nodes.dart (right):

https://codereview.chromium.org/10942028/diff/12001/lib/compiler/implementati...
lib/compiler/implementation/tree/nodes.dart:303: bool get
isPropertyAccessOrTypeReference => argumentsNode === null;
I reverted that change.

[ahe, 2012-10-23 11:54:26]

https://codereview.chromium.org/10942028/diff/27001/lib/compiler/implementati...
File lib/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/10942028/diff/27001/lib/compiler/implementati...
lib/compiler/implementation/resolution/members.dart:1483: Element
resolveSend(Send node) {
Karl, this is getting too horrible. Let's take a look at a how this can be
simplified when we're both in the office.

[karlklose, 2012-10-31 09:02:12]

PTAL.

I have changed the allowedCategory to include IMPLIES_TYPE. It seems that we
only have to special case LIBRARY_PREFIX and SUPER now. Perhaps we can simplify
the whole handling of categories later.

[ngeoffray, 2012-10-31 10:33:03]

http://codereview.chromium.org/10942028/diff/35001/lib/compiler/implementatio...
File lib/compiler/implementation/compiler.dart (right):

http://codereview.chromium.org/10942028/diff/35001/lib/compiler/implementatio...
lib/compiler/implementation/compiler.dart:191: bool _enabledRuntimeType = false;
Why private? I'd prefer to have it consistent with the other flags.

http://codereview.chromium.org/10942028/diff/35001/lib/compiler/implementatio...
File lib/compiler/implementation/resolution/members.dart (right):

http://codereview.chromium.org/10942028/diff/35001/lib/compiler/implementatio...
lib/compiler/implementation/resolution/members.dart:1532: // instance of [Type]
that is the result of the expression 'Class'.
The comment is not helping me understand what's happening, please be more
verbose and compare it with non-operator calls (where we resolve to a static
method on the class).

http://codereview.chromium.org/10942028/diff/35001/lib/compiler/implementatio...
lib/compiler/implementation/resolution/members.dart:1592: if (target != null &&
target.kind == ElementKind.CLASS) {
Should that check be in the prefix check?

http://codereview.chromium.org/10942028/diff/35001/lib/compiler/implementatio...
lib/compiler/implementation/resolution/members.dart:1733: } else if (target is
FunctionElement
target.isFunction() ? Or have selector.applies do the filtering?

http://codereview.chromium.org/10942028/diff/35001/lib/compiler/implementatio...
File lib/compiler/implementation/ssa/builder.dart (right):

http://codereview.chromium.org/10942028/diff/35001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/builder.dart:3011: // provided, add [:dynamic:]
as argument for all type variables.
Can't we find that at runtime instead?

http://codereview.chromium.org/10942028/diff/35001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/builder.dart:3116: // reference.
Please add a comment that you're doing a regular closure call that you know will
throw at runtime, but we don't want to code generate the throw because otherwise
we would drag NoSuchMethod which may not be useful if there are no try/catch.

http://codereview.chromium.org/10942028/diff/35001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/builder.dart:3123: push(new
HInvokeDynamicMethod(selector, inputs));
Could you use HInvokeDynamicClosure? You should not need to create a selector.

http://codereview.chromium.org/10942028/diff/35001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/builder.dart:3126: // if we do not have
[runtimeType] in the program.
How can that happen?

http://codereview.chromium.org/10942028/diff/35001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/builder.dart:3320: Element selectorElement =
elements[node.selector];
Why is the class on the selector and not the receiver?

http://codereview.chromium.org/10942028/diff/35001/tests/language/first_class...
File tests/language/first_class_types_literals_test.dart (right):

http://codereview.chromium.org/10942028/diff/35001/tests/language/first_class...
tests/language/first_class_types_literals_test.dart:26: // Test that types from
runtimeTypes() and literals agree.
runtimeTypes() => runtimeType

http://codereview.chromium.org/10942028/diff/35001/tests/language/first_class...
tests/language/first_class_types_literals_test.dart:31:
Expect.identical((D).runtimeType, (D).runtimeType.runtimeType);
Also add Expect.identical(Type, D.runtimeType) ?

http://codereview.chromium.org/10942028/diff/35001/tests/language/first_class...
tests/language/first_class_types_literals_test.dart:31:
Expect.identical((D).runtimeType, (D).runtimeType.runtimeType);
Why parens around D?

http://codereview.chromium.org/10942028/diff/35001/tests/language/language.st...
File tests/language/language.status (right):

http://codereview.chromium.org/10942028/diff/35001/tests/language/language.st...
tests/language/language.status:19: constructor_negative_test: Pass # For the
wrong reason.
Please file a bug

http://codereview.chromium.org/10942028/diff/35001/tests/language/language.st...
tests/language/language.status:332: first_class_types_literals_test: Fail
ditto

http://codereview.chromium.org/10942028/diff/35001/tests/language/language_da...
File tests/language/language_dart2js.status (right):

http://codereview.chromium.org/10942028/diff/35001/tests/language/language_da...
tests/language/language_dart2js.status:6: class_literal_test: Fail, Pass # Class
literals are expression now; delete this test.
Why is it passing sometimes?

http://codereview.chromium.org/10942028/diff/35001/tests/language/language_da...
tests/language/language_dart2js.status:68: constructor_negative_test: Fail, Ok #
The expression 'C()' is valid with class literals.
Please move Fail, OK tests to one section.

[karlklose, 2012-10-31 13:01:18]

http://codereview.chromium.org/10942028/diff/35001/lib/compiler/implementatio...
File lib/compiler/implementation/compiler.dart (right):

http://codereview.chromium.org/10942028/diff/35001/lib/compiler/implementatio...
lib/compiler/implementation/compiler.dart:191: bool _enabledRuntimeType = false;
I have a change to implement the runtime cache to dart code so we do not need to
record if we have to emit it.

http://codereview.chromium.org/10942028/diff/35001/lib/compiler/implementatio...
File lib/compiler/implementation/resolution/members.dart (right):

http://codereview.chromium.org/10942028/diff/35001/lib/compiler/implementatio...
lib/compiler/implementation/resolution/members.dart:1532: // instance of [Type]
that is the result of the expression 'Class'.
Rephrased. Is it better to understand now?

http://codereview.chromium.org/10942028/diff/35001/lib/compiler/implementatio...
lib/compiler/implementation/resolution/members.dart:1592: if (target != null &&
target.kind == ElementKind.CLASS) {
Where exactly do you mean this should be?

http://codereview.chromium.org/10942028/diff/35001/lib/compiler/implementatio...
lib/compiler/implementation/resolution/members.dart:1733: } else if (target is
FunctionElement
I added a case for targets that imply a type.

http://codereview.chromium.org/10942028/diff/35001/lib/compiler/implementatio...
File lib/compiler/implementation/ssa/builder.dart (right):

http://codereview.chromium.org/10942028/diff/35001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/builder.dart:3011: // provided, add [:dynamic:]
as argument for all type variables.
We could by wrapping each type variable in a call to a helper method in the
factory. If you prefer that, I can do it, but I do not intend to keep the
current solution for long, so perhaps a TODO here would suffice?

http://codereview.chromium.org/10942028/diff/35001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/builder.dart:3116: // reference.
On 2012/10/31 10:33:03, ngeoffray wrote:
> Please add a comment that you're doing a regular closure call that you know
will
> throw at runtime, but we don't want to code generate the throw because
otherwise
> we would drag NoSuchMethod which may not be useful if there are no try/catch.

Done.

http://codereview.chromium.org/10942028/diff/35001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/builder.dart:3123: push(new
HInvokeDynamicMethod(selector, inputs));
Yes, it works!

http://codereview.chromium.org/10942028/diff/35001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/builder.dart:3126: // if we do not have
[runtimeType] in the program.
If we use a class literal, but not the runtimeType getter, we need the cache but
we do not need to enable runtime type information for all objects.

http://codereview.chromium.org/10942028/diff/35001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/builder.dart:3320: Element selectorElement =
elements[node.selector];
It is on both, but I changed it to read it from the node.

http://codereview.chromium.org/10942028/diff/35001/tests/language/first_class...
File tests/language/first_class_types_literals_test.dart (right):

http://codereview.chromium.org/10942028/diff/35001/tests/language/first_class...
tests/language/first_class_types_literals_test.dart:26: // Test that types from
runtimeTypes() and literals agree.
On 2012/10/31 10:33:03, ngeoffray wrote:
> runtimeTypes() => runtimeType

Done.

http://codereview.chromium.org/10942028/diff/35001/tests/language/first_class...
tests/language/first_class_types_literals_test.dart:31:
Expect.identical((D).runtimeType, (D).runtimeType.runtimeType);
That doesn't hold, did you mean
Expect.true(D.runtimeType is Type)
or
Expect.identical(Type, (D).runtimeType.runtimeType)?

http://codereview.chromium.org/10942028/diff/35001/tests/language/first_class...
tests/language/first_class_types_literals_test.dart:31:
Expect.identical((D).runtimeType, (D).runtimeType.runtimeType);
Because D.runtimeType is the static property 'runtimeType' of class 'D'.

http://codereview.chromium.org/10942028/diff/35001/tests/language/language.st...
File tests/language/language.status (right):

http://codereview.chromium.org/10942028/diff/35001/tests/language/language.st...
tests/language/language.status:19: constructor_negative_test: Pass # For the
wrong reason.
I do not know the reason, this is copied.

http://codereview.chromium.org/10942028/diff/35001/tests/language/language.st...
tests/language/language.status:332: first_class_types_literals_test: Fail
Added existing issue number.

http://codereview.chromium.org/10942028/diff/35001/tests/language/language_da...
File tests/language/language_dart2js.status (right):

http://codereview.chromium.org/10942028/diff/35001/tests/language/language_da...
tests/language/language_dart2js.status:6: class_literal_test: Fail, Pass # Class
literals are expression now; delete this test.
The case for type variables is not yet implemented, but eventually this test
will be obsolete. Do you prefer if I list the failing tests?

http://codereview.chromium.org/10942028/diff/35001/tests/language/language_da...
tests/language/language_dart2js.status:68: constructor_negative_test: Fail, Ok #
The expression 'C()' is valid with class literals.
On 2012/10/31 10:33:03, ngeoffray wrote:
> Please move Fail, OK tests to one section.

Done.

[ngeoffray, 2012-10-31 14:32:43]

http://codereview.chromium.org/10942028/diff/35001/lib/compiler/implementatio...
File lib/compiler/implementation/resolution/members.dart (right):

http://codereview.chromium.org/10942028/diff/35001/lib/compiler/implementatio...
lib/compiler/implementation/resolution/members.dart:1592: if (target != null &&
target.kind == ElementKind.CLASS) {
On 2012/10/31 13:01:18, karlklose wrote:
> Where exactly do you mean this should be?

line 1590

http://codereview.chromium.org/10942028/diff/35001/lib/compiler/implementatio...
File lib/compiler/implementation/ssa/builder.dart (right):

http://codereview.chromium.org/10942028/diff/35001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/builder.dart:3011: // provided, add [:dynamic:]
as argument for all type variables.
On 2012/10/31 13:01:18, karlklose wrote:
> We could by wrapping each type variable in a call to a helper method in the
> factory. If you prefer that, I can do it, but I do not intend to keep the
> current solution for long, so perhaps a TODO here would suffice?

What other solution are you thinking of? One that happens at runtime or
compile-time?

http://codereview.chromium.org/10942028/diff/35001/tests/language/first_class...
File tests/language/first_class_types_literals_test.dart (right):

http://codereview.chromium.org/10942028/diff/35001/tests/language/first_class...
tests/language/first_class_types_literals_test.dart:31:
Expect.identical((D).runtimeType, (D).runtimeType.runtimeType);
On 2012/10/31 13:01:18, karlklose wrote:
> That doesn't hold, did you mean
> Expect.true(D.runtimeType is Type)
> or
> Expect.identical(Type, (D).runtimeType.runtimeType)?

Ok, I see. It's Expect.true(D.runtimeType is Type) I had in mind. If I
understand correctly Expect.identical(Type (D).runtimeType.runtimeType) does not
have to hold, right?

http://codereview.chromium.org/10942028/diff/35001/tests/language/language_da...
File tests/language/language_dart2js.status (right):

http://codereview.chromium.org/10942028/diff/35001/tests/language/language_da...
tests/language/language_dart2js.status:6: class_literal_test: Fail, Pass # Class
literals are expression now; delete this test.
On 2012/10/31 13:01:18, karlklose wrote:
> The case for type variables is not yet implemented, but eventually this test
> will be obsolete. Do you prefer if I list the failing tests?

Ah it's a multi-test. Maybe add that you're disabling a multi-test.

[karlklose, 2012-10-31 16:32:17]

http://codereview.chromium.org/10942028/diff/35001/lib/compiler/implementatio...
File lib/compiler/implementation/resolution/members.dart (right):

http://codereview.chromium.org/10942028/diff/35001/lib/compiler/implementatio...
lib/compiler/implementation/resolution/members.dart:1592: if (target != null &&
target.kind == ElementKind.CLASS) {
Yes, makes sense.

[karlklose, 2012-11-01 07:57:13]

http://codereview.chromium.org/10942028/diff/35001/tests/language/language.st...
File tests/language/language.status (right):

http://codereview.chromium.org/10942028/diff/35001/tests/language/language.st...
tests/language/language.status:19: constructor_negative_test: Pass # For the
wrong reason.
Actually this does pass for the right reason, so I removed the line.

[ngeoffray, 2012-11-01 09:57:59]

LGTM

http://codereview.chromium.org/10942028/diff/44023/lib/compiler/implementatio...
File lib/compiler/implementation/resolution/members.dart (right):

http://codereview.chromium.org/10942028/diff/44023/lib/compiler/implementatio...
lib/compiler/implementation/resolution/members.dart:1590: if (target == null) {
Should you check isUnresolved here?

http://codereview.chromium.org/10942028/diff/44023/tests/language/first_class...
File tests/language/first_class_types_literals_test.dart (right):

http://codereview.chromium.org/10942028/diff/44023/tests/language/first_class...
tests/language/first_class_types_literals_test.dart:3: // BSD-style license that
can be found in the LICENSE file.
Please also check SendSet like:
C = 42;

[ahe, 2012-11-01 13:55:27]

http://codereview.chromium.org/10942028/diff/44023/lib/compiler/implementatio...
File lib/compiler/implementation/compiler.dart (right):

http://codereview.chromium.org/10942028/diff/44023/lib/compiler/implementatio...
lib/compiler/implementation/compiler.dart:199: void enableRuntimeTypeFunction()
{
Why do you need this complexity? What is wrong with a few booleans?

http://codereview.chromium.org/10942028/diff/44023/lib/compiler/implementatio...
File lib/compiler/implementation/resolution/members.dart (right):

http://codereview.chromium.org/10942028/diff/44023/lib/compiler/implementatio...
lib/compiler/implementation/resolution/members.dart:1492: Element
resolveSend(Send node) {
Mostly my own fault, but this method is crap. I really want to clean it up. It
hurts my eyes.

http://codereview.chromium.org/10942028/diff/44023/lib/compiler/implementatio...
lib/compiler/implementation/resolution/members.dart:1513: if (result.kind ==
ElementKind.CLASS) {
I think this belongs in visitIdentifier.

http://codereview.chromium.org/10942028/diff/44023/lib/compiler/implementatio...
lib/compiler/implementation/resolution/members.dart:1529:
classElement.ensureResolved(compiler);
Couldn't this code be merged with lines 1569-1571?

http://codereview.chromium.org/10942028/diff/40003/tests/language/language_da...
File tests/language/language_dart2js.status (right):

http://codereview.chromium.org/10942028/diff/40003/tests/language/language_da...
tests/language/language_dart2js.status:6: class_literal_test: Fail, Pass # Class
literals are expression now; delete this test.
Why is this test flaky?

[karlklose, 2012-11-01 14:34:20]

Thanks for the reviews!

http://codereview.chromium.org/10942028/diff/44023/lib/compiler/implementatio...
File lib/compiler/implementation/compiler.dart (right):

http://codereview.chromium.org/10942028/diff/44023/lib/compiler/implementatio...
lib/compiler/implementation/compiler.dart:199: void enableRuntimeTypeFunction()
{
It is not needed anymore. Before making the runtime cache a dart field, I wanted
to automatically include it, if the runtimeTypeFunction was found.

http://codereview.chromium.org/10942028/diff/44023/lib/compiler/implementatio...
File lib/compiler/implementation/resolution/members.dart (right):

http://codereview.chromium.org/10942028/diff/44023/lib/compiler/implementatio...
lib/compiler/implementation/resolution/members.dart:1492: Element
resolveSend(Send node) {
I agree.

http://codereview.chromium.org/10942028/diff/44023/lib/compiler/implementatio...
lib/compiler/implementation/resolution/members.dart:1513: if (result.kind ==
ElementKind.CLASS) {
On 2012/11/01 13:55:27, ahe wrote:
> I think this belongs in visitIdentifier.

Done.

http://codereview.chromium.org/10942028/diff/44023/lib/compiler/implementatio...
lib/compiler/implementation/resolution/members.dart:1529:
classElement.ensureResolved(compiler);
On 2012/11/01 13:55:27, ahe wrote:
> Couldn't this code be merged with lines 1569-1571?

Done.

http://codereview.chromium.org/10942028/diff/44023/lib/compiler/implementatio...
lib/compiler/implementation/resolution/members.dart:1590: if (target == null) {
On 2012/11/01 09:57:59, ngeoffray wrote:
> Should you check isUnresolved here?

Done.

http://codereview.chromium.org/10942028/diff/44023/tests/language/first_class...
File tests/language/first_class_types_literals_test.dart (right):

http://codereview.chromium.org/10942028/diff/44023/tests/language/first_class...
tests/language/first_class_types_literals_test.dart:3: // BSD-style license that
can be found in the LICENSE file.
On 2012/11/01 09:57:59, ngeoffray wrote:
> Please also check SendSet like:
> C = 42;

Done.

http://codereview.chromium.org/10942028/diff/40003/tests/language/language_da...
File tests/language/language_dart2js.status (right):

http://codereview.chromium.org/10942028/diff/40003/tests/language/language_da...
tests/language/language_dart2js.status:6: class_literal_test: Fail, Pass # Class
literals are expression now; delete this test.
It is a multi test. I removed this line and put the failing instances here
instead.

[ahe, 2012-11-05 09:47:05]

LGTM since you already submitted this CL.

http://codereview.chromium.org/10942028/diff/46003/lib/compiler/implementatio...
File lib/compiler/implementation/dart_backend/backend.dart (right):

http://codereview.chromium.org/10942028/diff/46003/lib/compiler/implementatio...
lib/compiler/implementation/dart_backend/backend.dart:319: if
(!shouldOutput(element) || treeElements == null) return;
Why would treeElements be null?

http://codereview.chromium.org/10942028/diff/46003/lib/compiler/implementatio...
File lib/compiler/implementation/runtime_types.dart (right):

http://codereview.chromium.org/10942028/diff/46003/lib/compiler/implementatio...
lib/compiler/implementation/runtime_types.dart:16: const
SourceString('getOrCreateCachedRuntimeType');
How about moving this to compiler.dart?

http://codereview.chromium.org/10942028/diff/46003/lib/compiler/implementatio...
File lib/compiler/implementation/ssa/builder.dart (right):

http://codereview.chromium.org/10942028/diff/46003/lib/compiler/implementatio...
lib/compiler/implementation/ssa/builder.dart:3346: Element selectorElement =
elements[node];
This is unused.

http://codereview.chromium.org/10942028/diff/46003/tests/language/first_class...
File tests/language/first_class_types_literals_test.dart (right):

http://codereview.chromium.org/10942028/diff/46003/tests/language/first_class...
tests/language/first_class_types_literals_test.dart:4: 
Please add a comment explaining what this test does.

http://codereview.chromium.org/10942028/diff/46003/tests/language/first_class...
tests/language/first_class_types_literals_test.dart:11: sameType(a, b) {
This method is not called.

http://codereview.chromium.org/10942028/diff/46003/tests/language/first_class...
tests/language/first_class_types_literals_test.dart:15: differentType(a, b) {
This method is not called.

http://codereview.chromium.org/10942028/diff/46003/tests/language/first_class...
tests/language/first_class_types_literals_test.dart:18:
Expect.isFalse(a.runtimeType === b.runtimeType);
The === operator is deprecated.

http://codereview.chromium.org/10942028/diff/46003/tests/language/first_class...
tests/language/first_class_types_literals_test.dart:24: Expect.isFalse(int ===
num);
Ditto

http://codereview.chromium.org/10942028/diff/46003/tests/language/language.st...
File tests/language/language.status (right):

http://codereview.chromium.org/10942028/diff/46003/tests/language/language.st...
tests/language/language.status:366: illegal_invocation_test/01: Fail, OK #
Typedef literals are expressions now.
Bug report number to delete the test?

http://codereview.chromium.org/10942028/diff/46003/tests/language/language_da...
File tests/language/language_dart2js.status (right):

http://codereview.chromium.org/10942028/diff/46003/tests/language/language_da...
tests/language/language_dart2js.status:6: class_literal_test/01: Fail # Class
literals are expression now; delete this test.
Bug report number to delete the test?
Shouldn't this be Fail, OK?

http://codereview.chromium.org/10942028/diff/46003/tests/language/language_da...
tests/language/language_dart2js.status:104: constructor_negative_test: Pass #
Wrong reason: the expression 'C()' is valid with class literals.
Bug number?

http://codereview.chromium.org/10942028/diff/46003/tests/language/language_da...
tests/language/language_dart2js.status:105: illegal_invocation_test/01: Fail, OK
# Typedef literals are expressions now.
Bug number?