Support type literals as compile-time constants.

Support type literals as compile-time constants.

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

[ngeoffray, 2012-11-15 16:07:24]

http://codereview.chromium.org/11299009/diff/1/sdk/lib/_internal/compiler/imp...
File sdk/lib/_internal/compiler/implementation/constants.dart (right):

http://codereview.chromium.org/11299009/diff/1/sdk/lib/_internal/compiler/imp...
sdk/lib/_internal/compiler/implementation/constants.dart:46: bool get isType =>
false;
bool isType() => false and move it to where the other predicates are.

http://codereview.chromium.org/11299009/diff/1/sdk/lib/_internal/compiler/imp...
sdk/lib/_internal/compiler/implementation/constants.dart:325: class TypeConstant
extends Constant {
extends ObjectConstant ?

http://codereview.chromium.org/11299009/diff/1/sdk/lib/_internal/compiler/imp...
sdk/lib/_internal/compiler/implementation/constants.dart:328: /// The type of
the constant (currently always [Type]).
Should it be something else? Please add a comment.

http://codereview.chromium.org/11299009/diff/1/sdk/lib/_internal/compiler/imp...
sdk/lib/_internal/compiler/implementation/constants.dart:337: return
compiler.typeClass.computeType(compiler);
Remove, it will be inherited with ObjectConstant.

http://codereview.chromium.org/11299009/diff/1/sdk/lib/_internal/compiler/imp...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

http://codereview.chromium.org/11299009/diff/1/sdk/lib/_internal/compiler/imp...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1542: // If we
use a class literal in a constant, the compile time constant
I think you should move that to the compile time constant handler.

http://codereview.chromium.org/11299009/diff/1/sdk/lib/_internal/compiler/imp...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

http://codereview.chromium.org/11299009/diff/1/sdk/lib/_internal/compiler/imp...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3029: if
(type.element == compiler.dynamicClass) {
This looks fishy, why can't rti.getName return 'dynamic'?

http://codereview.chromium.org/11299009/diff/1/sdk/lib/_internal/compiler/imp...
File sdk/lib/_internal/compiler/implementation/ssa/nodes.dart (right):

http://codereview.chromium.org/11299009/diff/1/sdk/lib/_internal/compiler/imp...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:172: if
(constant.isType) {
You can remove this once TypeConstant extends ObjectConstant.

[floitsch, 2012-11-16 13:53:31]

LGTM once Nicolas' comments are addressed.

https://codereview.chromium.org/11299009/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/constants.dart (right):

https://codereview.chromium.org/11299009/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/constants.dart:322: int get hashCode;
Remove the hashCode line too.

https://codereview.chromium.org/11299009/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/11299009/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:1841: } else if
(constant.isType) {
isn't this doing exactly the same thing as the 'else' branch?

[karlklose, 2012-11-19 15:08:58]

PTAL. I removed getting unsafeNames from the namer and added a special case for
'dynamic' to rti, which requires the compiler.

http://codereview.chromium.org/11299009/diff/1/sdk/lib/_internal/compiler/imp...
File sdk/lib/_internal/compiler/implementation/constants.dart (right):

http://codereview.chromium.org/11299009/diff/1/sdk/lib/_internal/compiler/imp...
sdk/lib/_internal/compiler/implementation/constants.dart:46: bool get isType =>
false;
On 2012/11/15 16:07:24, ngeoffray wrote:
> bool isType() => false and move it to where the other predicates are.

Done.

http://codereview.chromium.org/11299009/diff/1/sdk/lib/_internal/compiler/imp...
sdk/lib/_internal/compiler/implementation/constants.dart:322: int get hashCode;
On 2012/11/16 13:53:31, floitsch wrote:
> Remove the hashCode line too.

Done.

http://codereview.chromium.org/11299009/diff/1/sdk/lib/_internal/compiler/imp...
sdk/lib/_internal/compiler/implementation/constants.dart:325: class TypeConstant
extends Constant {
On 2012/11/15 16:07:24, ngeoffray wrote:
> extends ObjectConstant ?

Done.

http://codereview.chromium.org/11299009/diff/1/sdk/lib/_internal/compiler/imp...
sdk/lib/_internal/compiler/implementation/constants.dart:328: /// The type of
the constant (currently always [Type]).
It could be more specific in the future, but the comment is perhaps misleading.
After extending ObjectConstant, I removed this comment, since it is now clear
how this type is used.

http://codereview.chromium.org/11299009/diff/1/sdk/lib/_internal/compiler/imp...
sdk/lib/_internal/compiler/implementation/constants.dart:337: return
compiler.typeClass.computeType(compiler);
On 2012/11/15 16:07:24, ngeoffray wrote:
> Remove, it will be inherited with ObjectConstant.

Done.

http://codereview.chromium.org/11299009/diff/1/sdk/lib/_internal/compiler/imp...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

http://codereview.chromium.org/11299009/diff/1/sdk/lib/_internal/compiler/imp...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1542: // If we
use a class literal in a constant, the compile time constant
On 2012/11/15 16:07:24, ngeoffray wrote:
> I think you should move that to the compile time constant handler.

Done.

http://codereview.chromium.org/11299009/diff/1/sdk/lib/_internal/compiler/imp...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

http://codereview.chromium.org/11299009/diff/1/sdk/lib/_internal/compiler/imp...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3029: if
(type.element == compiler.dynamicClass) {
On 2012/11/15 16:07:24, ngeoffray wrote:
> This looks fishy, why can't rti.getName return 'dynamic'?

Done.

http://codereview.chromium.org/11299009/diff/1/sdk/lib/_internal/compiler/imp...
File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

http://codereview.chromium.org/11299009/diff/1/sdk/lib/_internal/compiler/imp...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:1841: } else if
(constant.isType) {
Yes. Removed.

http://codereview.chromium.org/11299009/diff/1/sdk/lib/_internal/compiler/imp...
File sdk/lib/_internal/compiler/implementation/ssa/nodes.dart (right):

http://codereview.chromium.org/11299009/diff/1/sdk/lib/_internal/compiler/imp...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:172: if
(constant.isType) {
On 2012/11/15 16:07:24, ngeoffray wrote:
> You can remove this once TypeConstant extends ObjectConstant.

Done.

[ngeoffray, 2012-11-19 16:54:09]

More comments

http://codereview.chromium.org/11299009/diff/5010/sdk/lib/_internal/compiler/...
File sdk/lib/_internal/compiler/implementation/compile_time_constants.dart
(right):

http://codereview.chromium.org/11299009/diff/5010/sdk/lib/_internal/compiler/...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:407: if
(!enabledRuntimeTypeSupport) {
Instead of having a helper, you can have an implementation Dart subclass with a
constructor. This will be consistent with how we handle map literals for
example. Also, it avoids making these mistakes of forgetting to put a helper
(the class of the CTC is always registered as being instantiated).

http://codereview.chromium.org/11299009/diff/5010/sdk/lib/_internal/compiler/...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:440: }
else if (Elements.isClass(element) || Elements.isTypedef(element)) {
Could you use element.impliesType instead?

http://codereview.chromium.org/11299009/diff/5010/sdk/lib/_internal/compiler/...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:451: }
else if (Elements.isClass(element) || Elements.isTypedef(element)) {
ditto

http://codereview.chromium.org/11299009/diff/5010/sdk/lib/_internal/compiler/...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

http://codereview.chromium.org/11299009/diff/5010/sdk/lib/_internal/compiler/...
sdk/lib/_internal/compiler/implementation/compiler.dart:544:
enqueuer.codegen.universe.rti = new RuntimeTypeInformation(this);
Setting this field here seems very arbitrary. The object should be created when
initializing the backend.

http://codereview.chromium.org/11299009/diff/5010/sdk/lib/_internal/compiler/...
File sdk/lib/_internal/compiler/implementation/js_backend/constant_emitter.dart
(right):

http://codereview.chromium.org/11299009/diff/5010/sdk/lib/_internal/compiler/...
sdk/lib/_internal/compiler/implementation/js_backend/constant_emitter.dart:204:
const SourceString('createRuntimeType');
See my comment in compile_time_constant.dart. I'd prefer if we did the same as
literal maps, by calling a Dart factory constructor of an implementation class
that implements/extends Type.

http://codereview.chromium.org/11299009/diff/5010/sdk/lib/_internal/compiler/...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

http://codereview.chromium.org/11299009/diff/5010/sdk/lib/_internal/compiler/...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3358: if
(element.isClass() || element.isTypedef()) {
element.impliesType

http://codereview.chromium.org/11299009/diff/5010/sdk/lib/_internal/compiler/...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3361: Node
constantNode = node.isCall ? node : node;
What is this line above?

[karlklose, 2012-11-20 14:04:38]

http://codereview.chromium.org/11299009/diff/5010/sdk/lib/_internal/compiler/...
File sdk/lib/_internal/compiler/implementation/compile_time_constants.dart
(right):

http://codereview.chromium.org/11299009/diff/5010/sdk/lib/_internal/compiler/...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:407: if
(!enabledRuntimeTypeSupport) {
As discussed offline, I'll leave that change for now, because rti with constants
is broken. I have filed http://code.google.com/p/dart/issues/detail?id=6827.

http://codereview.chromium.org/11299009/diff/5010/sdk/lib/_internal/compiler/...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:440: }
else if (Elements.isClass(element) || Elements.isTypedef(element)) {
That is not the same because type variables imply types.

http://codereview.chromium.org/11299009/diff/5010/sdk/lib/_internal/compiler/...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:451: }
else if (Elements.isClass(element) || Elements.isTypedef(element)) {
ditto

http://codereview.chromium.org/11299009/diff/5010/sdk/lib/_internal/compiler/...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

http://codereview.chromium.org/11299009/diff/5010/sdk/lib/_internal/compiler/...
sdk/lib/_internal/compiler/implementation/compiler.dart:544:
enqueuer.codegen.universe.rti = new RuntimeTypeInformation(this);
Done, moved rti to the backend.

http://codereview.chromium.org/11299009/diff/5010/sdk/lib/_internal/compiler/...
File sdk/lib/_internal/compiler/implementation/js_backend/constant_emitter.dart
(right):

http://codereview.chromium.org/11299009/diff/5010/sdk/lib/_internal/compiler/...
sdk/lib/_internal/compiler/implementation/js_backend/constant_emitter.dart:204:
const SourceString('createRuntimeType');
http://code.google.com/p/dart/issues/detail?id=6827

http://codereview.chromium.org/11299009/diff/5010/sdk/lib/_internal/compiler/...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

http://codereview.chromium.org/11299009/diff/5010/sdk/lib/_internal/compiler/...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3358: if
(element.isClass() || element.isTypedef()) {
Type variables imply types. The case for these is just below.

http://codereview.chromium.org/11299009/diff/5010/sdk/lib/_internal/compiler/...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3361: Node
constantNode = node.isCall ? node : node;
Debug code, removed.

[ngeoffray, 2012-11-20 14:13:19]

LGTM with one comment

http://codereview.chromium.org/11299009/diff/5024/sdk/lib/_internal/compiler/...
File sdk/lib/_internal/compiler/implementation/resolution/resolution.dart
(right):

http://codereview.chromium.org/11299009/diff/5024/sdk/lib/_internal/compiler/...
sdk/lib/_internal/compiler/implementation/resolution/resolution.dart:12: import
'../runtime_types.dart';
This doesn't look right.

http://codereview.chromium.org/11299009/diff/5024/sdk/lib/_internal/compiler/...
File sdk/lib/_internal/compiler/implementation/runtime_types.dart (right):

http://codereview.chromium.org/11299009/diff/5024/sdk/lib/_internal/compiler/...
sdk/lib/_internal/compiler/implementation/runtime_types.dart:1: // Copyright (c)
2012, the Dart project authors.  Please see the AUTHORS file
Please move this file to js_backend. It should not be used in non-js specific
compiler code.

[floitsch, 2012-11-20 16:25:43]

LGTM.

https://codereview.chromium.org/11299009/diff/9009/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/constant_emitter.dart
(right):

https://codereview.chromium.org/11299009/diff/9009/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/constant_emitter.dart:215:
typeName = element.name.slowToString();
Discuss with Erik if (and potentially how) this should be minified.
Not in this CL, though.