Naive handling of List#[] and List#[]=

Naive handling of List#[] and List#[]=

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

[polux, 2012-11-22 14:00:35]

Don't review that CL yet, I'll update it when eveything it depends on has been
submitted.

[polux, 2012-12-13 11:42:03]

The CL is now ready for review.

https://chromiumcodereview.appspot.com/11414130/diff/7001/sdk/lib/_internal/c...
File
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart
(right):

https://chromiumcodereview.appspot.com/11414130/diff/7001/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart:909:
listConstructor = compiler.listClass.constructors.head;
I would like to get rid of this line and get the commented code below to work.
Any idea how?

[polux, 2013-01-17 14:51:45]

Synced to head and defaulted Lists to JsArray. Ready to be reviewed.

[karlklose, 2013-01-23 13:16:55]

LGTM.

https://codereview.chromium.org/11414130/diff/7001/sdk/lib/_internal/compiler...
File
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart
(right):

https://codereview.chromium.org/11414130/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart:372:
* When true, the string litteral [:"__dynamic_for_test":] is inferred to
'litteral' -> 'literal'.

https://codereview.chromium.org/11414130/diff/14003/sdk/lib/_internal/compile...
File
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart
(right):

https://codereview.chromium.org/11414130/diff/14003/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart:285:
final ClassBaseType jsArrayBaseType;
I would still call this listBaseType and perhaps add a comment in the function
that looks it up, that in the JsBackend this is a special implementation class.

https://codereview.chromium.org/11414130/diff/14003/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart:290:
static _getJsArrayClass(Compiler compiler) {
You could call it 'getNativeListClass' and then switch on the backend type.
It could make sense to run the analysis with dart2dart as a pure analysis tool.
Or even have a separate backend that produces a serialization of the analysis'
results to be used with other tools.

https://codereview.chromium.org/11414130/diff/14003/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart:409:
* Constants representing builtin base types. Initialized in [initialize]
The explanation why these are not directly initialized could be shared for the
next four fields.

https://codereview.chromium.org/11414130/diff/14003/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart:420:
FunctionElement jsArrayBrackets;
How about calling this 'listIndex' ...

https://codereview.chromium.org/11414130/diff/14003/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart:427:
FunctionElement jsArrayBracketsEquals;
... and this one 'listIndexSet'.

https://codereview.chromium.org/11414130/diff/14003/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart:622:
/** Augment the inferred type of elements stored in Lists. */
You can use /// instead of /** ... */

https://codereview.chromium.org/11414130/diff/14003/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart:839:
/* Handles external methods that cannot be cached because they depend on some
Make this a valid dart-doc comment.

https://codereview.chromium.org/11414130/diff/14003/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart:885:
} else {  // external method
All external functions should be patched when we arrive here.

Could these be abstract methods, factory redirections or const constructors?

https://codereview.chromium.org/11414130/diff/14003/tests/compiler/dart2js/cp...
File tests/compiler/dart2js/cpa_inference_test.dart (right):

https://codereview.chromium.org/11414130/diff/14003/tests/compiler/dart2js/cp...
tests/compiler/dart2js/cpa_inference_test.dart:936: testListWithCapacity() {
Can you add a test with elements like in testLists AND capacity?

[polux, 2013-01-24 13:12:22]

https://chromiumcodereview.appspot.com/11414130/diff/7001/sdk/lib/_internal/c...
File
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart
(right):

https://chromiumcodereview.appspot.com/11414130/diff/7001/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart:372:
* When true, the string litteral [:"__dynamic_for_test":] is inferred to
On 2013/01/23 13:16:55, karlklose wrote:
> 'litteral' -> 'literal'.

Done.

https://chromiumcodereview.appspot.com/11414130/diff/14003/sdk/lib/_internal/...
File
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart
(right):

https://chromiumcodereview.appspot.com/11414130/diff/14003/sdk/lib/_internal/...
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart:285:
final ClassBaseType jsArrayBaseType;
On 2013/01/23 13:16:55, karlklose wrote:
> I would still call this listBaseType and perhaps add a comment in the function
> that looks it up, that in the JsBackend this is a special implementation
class.

Done.

https://chromiumcodereview.appspot.com/11414130/diff/14003/sdk/lib/_internal/...
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart:290:
static _getJsArrayClass(Compiler compiler) {
On 2013/01/23 13:16:55, karlklose wrote:
> You could call it 'getNativeListClass' and then switch on the backend type.
> It could make sense to run the analysis with dart2dart as a pure analysis
tool.
> Or even have a separate backend that produces a serialization of the analysis'
> results to be used with other tools.

Done.

https://chromiumcodereview.appspot.com/11414130/diff/14003/sdk/lib/_internal/...
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart:409:
* Constants representing builtin base types. Initialized in [initialize]
On 2013/01/23 13:16:55, karlklose wrote:
> The explanation why these are not directly initialized could be shared for the
> next four fields. 

Wasn't sure what you meant by "shared": I just erased the others. My concern was
that dartdoc doesn't preserve the declaration order but on the other hand, I'm
not sure we're ever gonna generate dartdoc for dart2js.

https://chromiumcodereview.appspot.com/11414130/diff/14003/sdk/lib/_internal/...
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart:420:
FunctionElement jsArrayBrackets;
On 2013/01/23 13:16:55, karlklose wrote:
> How about calling this 'listIndex' ...

Done.

https://chromiumcodereview.appspot.com/11414130/diff/14003/sdk/lib/_internal/...
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart:427:
FunctionElement jsArrayBracketsEquals;
On 2013/01/23 13:16:55, karlklose wrote:
> ... and this one 'listIndexSet'.

Done.

https://chromiumcodereview.appspot.com/11414130/diff/14003/sdk/lib/_internal/...
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart:622:
/** Augment the inferred type of elements stored in Lists. */
On 2013/01/23 13:16:55, karlklose wrote:
> You can use /// instead of /** ... */ 

Done.

https://chromiumcodereview.appspot.com/11414130/diff/14003/sdk/lib/_internal/...
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart:839:
/* Handles external methods that cannot be cached because they depend on some
On 2013/01/23 13:16:55, karlklose wrote:
> Make this a valid dart-doc comment.

Done.

https://chromiumcodereview.appspot.com/11414130/diff/14003/sdk/lib/_internal/...
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart:885:
} else {  // external method
On 2013/01/23 13:16:55, karlklose wrote:
> All external functions should be patched when we arrive here.
> 
> Could these be abstract methods, factory redirections or const constructors?

As discussed, I'll handle this in an upcoming CL.