Simplify runtime type support.

Simplify runtime type support.

1. Store the class name on the JS constructor and only attach runtime type information to objects that need information about their type arguments.

2. Generate the runtime type string at runtime.

3. Change the structure of runtime type information to an array and access type arguments by index.

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

[ngeoffray, 2012-11-15 08:58:09]

I like the cleanup on generating the type info at runtime.

https://codereview.chromium.org/11360228/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js/nodes.dart (right):

https://codereview.chromium.org/11360228/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js/nodes.dart:717: : selector = new
LiteralString('$index');
Should it be a LiteralInt instead?

https://codereview.chromium.org/11360228/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/namer.dart (right):

https://codereview.chromium.org/11360228/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:209: String
_computeGuess(Element element, {allowUnsafeName: false}) {
bool and no need to make it optional

https://codereview.chromium.org/11360228/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:249: String
getName(Element element, {allowUnsafeName: false}) {
bool

https://codereview.chromium.org/11360228/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (left):

https://codereview.chromium.org/11360228/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1135: // allocating
a new object every single time.
Why removing this?

https://codereview.chromium.org/11360228/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/11360228/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1125: // We have to
check for null because factories may return null.
Assert that typeInfo is either null or a JsArray?

https://codereview.chromium.org/11360228/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1486: var typeInfo
= JS('Object', r'#.builtin$typeInfo', object);
'Object' -> 'var'. I know this is not documented, but it will conflict with
Stephen's work.

https://codereview.chromium.org/11360228/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1489: int length =
JS('int', r'#.length', typeInfo);
Looks to me that you don't really need all these JS. builtin$typeInfo is an
array.

https://codereview.chromium.org/11360228/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1494: var argument
= JS('String', r'#[#]', typeInfo, i);
typeInfo[i]

https://codereview.chromium.org/11360228/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1495: if
(JS('bool', r'# == (void 0)', argument)) {
argument == null

https://codereview.chromium.org/11360228/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1498:
arguments.add(argument != null ? argument : 'dynamic');
This is weird. builtin$typeInfo should have one kind of null in its entries.

https://codereview.chromium.org/11360228/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/runtime_types.dart (right):

https://codereview.chromium.org/11360228/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/runtime_types.dart:36: * using
[:dynamic:] for each type argument.
No need for ':'?

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

https://codereview.chromium.org/11360228/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:2483: arguments =
arguments.tail;
Was it not tested before?

https://codereview.chromium.org/11360228/diff/7001/tests/language/first_class...
File tests/language/first_class_types_literals_test.dart (right):

https://codereview.chromium.org/11360228/diff/7001/tests/language/first_class...
tests/language/first_class_types_literals_test.dart:11: isNotIdentical(a, b) {
Any reason for the prints? Otherwise why not use Expect.isFalse(identical....?

[karlklose, 2012-11-16 13:37:19]

PTAL.

https://codereview.chromium.org/11360228/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js/nodes.dart (right):

https://codereview.chromium.org/11360228/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js/nodes.dart:717: : selector = new
LiteralString('$index');
i used LiteralNumber.

https://codereview.chromium.org/11360228/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/namer.dart (right):

https://codereview.chromium.org/11360228/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:209: String
_computeGuess(Element element, {allowUnsafeName: false}) {
On 2012/11/15 08:58:09, ngeoffray wrote:
> bool and no need to make it optional

Done.

https://codereview.chromium.org/11360228/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:249: String
getName(Element element, {allowUnsafeName: false}) {
On 2012/11/15 08:58:09, ngeoffray wrote:
> bool

Done.

https://codereview.chromium.org/11360228/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (left):

https://codereview.chromium.org/11360228/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1135: // allocating
a new object every single time.
That was an accident. Restored.

https://codereview.chromium.org/11360228/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/11360228/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1125: // We have to
check for null because factories may return null.
On 2012/11/15 08:58:09, ngeoffray wrote:
> Assert that typeInfo is either null or a JsArray?

Done.

https://codereview.chromium.org/11360228/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1486: var typeInfo
= JS('Object', r'#.builtin$typeInfo', object);
On 2012/11/15 08:58:09, ngeoffray wrote:
> 'Object' -> 'var'. I know this is not documented, but it will conflict with
> Stephen's work.

Done.

https://codereview.chromium.org/11360228/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1489: int length =
JS('int', r'#.length', typeInfo);
On 2012/11/15 08:58:09, ngeoffray wrote:
> Looks to me that you don't really need all these JS. builtin$typeInfo is an
> array.

Done.

https://codereview.chromium.org/11360228/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1494: var argument
= JS('String', r'#[#]', typeInfo, i);
On 2012/11/15 08:58:09, ngeoffray wrote:
> typeInfo[i]

Done.

https://codereview.chromium.org/11360228/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1495: if
(JS('bool', r'# == (void 0)', argument)) {
On 2012/11/15 08:58:09, ngeoffray wrote:
> argument == null

Done.

https://codereview.chromium.org/11360228/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1498:
arguments.add(argument != null ? argument : 'dynamic');
This was a merge conflict, removed.

https://codereview.chromium.org/11360228/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/runtime_types.dart (right):

https://codereview.chromium.org/11360228/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/runtime_types.dart:36: * using
[:dynamic:] for each type argument.
On 2012/11/15 08:58:09, ngeoffray wrote:
> No need for ':'?

Done.

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

https://codereview.chromium.org/11360228/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:2483: arguments =
arguments.tail;
It was, but the tests were marked as failing.

[ngeoffray, 2012-11-16 13:47:34]

LGTM, but I'd like to know what others think about the builtin$cls added to the
constructor.

https://codereview.chromium.org/11360228/diff/15001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/11360228/diff/15001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:173:
constructor.builtin\$cls = cls;
You might want to check with Peter, Kasper or Florian about this. The
Primitives.objectTypeName in js_helper.dart is trying to do some clever things
that are probably not needed if the class name is attached to the constructor.

https://codereview.chromium.org/11360228/diff/15001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/interceptors.dart (right):

https://codereview.chromium.org/11360228/diff/15001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/interceptors.dart:583: return
createRuntimeType(getRuntimeTypeString(receiver));
Add a comment that this is because List might be parameterized.

https://codereview.chromium.org/11360228/diff/15001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/11360228/diff/15001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1493: String
getClassName(var object) {
Duplicating Primitives.objectTypeName?

[karlklose, 2012-11-20 10:35:26]

Thanks for the review, Nicolas.

https://codereview.chromium.org/11360228/diff/15001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/11360228/diff/15001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:173:
constructor.builtin\$cls = cls;
I talked to Florian and he sees no performance implications. I'll take a look at
constructorFallBack and simplify it in a follow up CL, if that is ok with you.

https://codereview.chromium.org/11360228/diff/15001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/interceptors.dart (right):

https://codereview.chromium.org/11360228/diff/15001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/interceptors.dart:583: return
createRuntimeType(getRuntimeTypeString(receiver));
On 2012/11/16 13:47:35, ngeoffray wrote:
> Add a comment that this is because List might be parameterized.

Done.

https://codereview.chromium.org/11360228/diff/15001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/11360228/diff/15001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1493: String
getClassName(var object) {
Only in some cases: if constructorNameFallback cannot find out the name, it will
return object. It also removes leading '$'s from the returned type names.

[ngeoffray, 2012-11-22 17:09:38]

https://codereview.chromium.org/11360228/diff/20013/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/interceptors.dart (right):

https://codereview.chromium.org/11360228/diff/20013/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/interceptors.dart:58:
get$runtimeType(receiver) {
This is dead code now.

https://codereview.chromium.org/11360228/diff/20013/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/js_array.dart (right):

https://codereview.chromium.org/11360228/diff/20013/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_array.dart:170: Type get
runtimeType => List;
You're missing the type parameters, and this is the method that will be called
when [].runtimeType is in Dart code. Are you missing tests on runtimeType with
parameterized lists?

[karlklose, 2012-11-23 08:47:45]

Good catch, I created https://codereview.chromium.org/11348207/ to fix it.

https://codereview.chromium.org/11360228/diff/20013/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/interceptors.dart (right):

https://codereview.chromium.org/11360228/diff/20013/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/interceptors.dart:58:
get$runtimeType(receiver) {
On 2012/11/22 17:09:39, ngeoffray wrote:
> This is dead code now.

Done.

https://codereview.chromium.org/11360228/diff/20013/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/js_array.dart (right):

https://codereview.chromium.org/11360228/diff/20013/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_array.dart:170: Type get
runtimeType => List;
On 2012/11/22 17:09:39, ngeoffray wrote:
> You're missing the type parameters, and this is the method that will be called
> when [].runtimeType is in Dart code. Are you missing tests on runtimeType with
> parameterized lists?

Done.