Correctly compile unresolved for-in loops.

Correctly compile unresolved for-in loops.

BUG=http://dartbug.com/6725

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

[ahe, 2013-04-09 14:02:53]

This fixes a crash was reported in November.

[ahe, 2013-04-09 14:29:22]

Added a unit test.

[Emily Fortuna, 2013-04-09 16:48:21]

glad to see this fixed!!

https://codereview.chromium.org/13866010/diff/3001/dart/sdk/lib/_internal/com...
File
dart/sdk/lib/_internal/compiler/implementation/types/simple_types_inferrer.dart
(right):

https://codereview.chromium.org/13866010/diff/3001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/types/simple_types_inferrer.dart:1536:
return handlePlainAssignment(node, element, setterSelector, receiverType,
rhsType, node.arguments.head);
80 char

https://codereview.chromium.org/13866010/diff/3001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/types/simple_types_inferrer.dart:1598:
} else if (element.isField()) {
not a show-stopper for this change, but it seems like isField and isFinal and
isSetter, etc might be good candidates as gettors instead of functions.

[kasperl, 2013-04-10 08:34:54]

STV!

https://codereview.chromium.org/13866010/diff/3001/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/13866010/diff/3001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/compiler.dart:388: final Selector
iteratorSelector =
Nice.

https://codereview.chromium.org/13866010/diff/3001/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/13866010/diff/3001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/ssa/builder.dart:4082:
addWithPosition(
Maybe compute the instruction before calling addWithPosition to make the code a
little bit easier to grok? You could share the addWithPosition call if you feel
like it.

https://codereview.chromium.org/13866010/diff/3001/dart/sdk/lib/_internal/com...
File
dart/sdk/lib/_internal/compiler/implementation/types/simple_types_inferrer.dart
(right):

https://codereview.chromium.org/13866010/diff/3001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/types/simple_types_inferrer.dart:1587:
TypeMask handlePlainAssignment(Send node,
Thank you for factoring this out!

https://codereview.chromium.org/13866010/diff/3001/dart/tests/compiler/dart2j...
File dart/tests/compiler/dart2js/bad_loop_test.dart (right):

https://codereview.chromium.org/13866010/diff/3001/dart/tests/compiler/dart2j...
dart/tests/compiler/dart2js/bad_loop_test.dart:11: main() {
This is a great way of writing this test.

https://codereview.chromium.org/13866010/diff/3001/dart/tests/compiler/dart2j...
dart/tests/compiler/dart2js/bad_loop_test.dart:23: if (kind ==
Diagnostic.VERBOSE_INFO) return;
Tiny nit: I might actually go for writing this with extra {} and else if -- if
nothing else then just to make the flow of this method more uniform.

[ahe, 2013-04-10 15:02:18]

PTAL, unfortunately, I had to merge with tip-of-tree to resolve a test failure,
so the difference between the last two patchsets may contain unrelated stuff.

https://codereview.chromium.org/13866010/diff/3001/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/13866010/diff/3001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/ssa/builder.dart:4082:
addWithPosition(
On 2013/04/10 08:34:54, kasperl wrote:
> Maybe compute the instruction before calling addWithPosition to make the code
a
> little bit easier to grok? You could share the addWithPosition call if you
feel
> like it.

I rewrote generateInstanceSetterWithCompiledReceiver so it can be used here as
well.

https://codereview.chromium.org/13866010/diff/3001/dart/sdk/lib/_internal/com...
File
dart/sdk/lib/_internal/compiler/implementation/types/simple_types_inferrer.dart
(right):

https://codereview.chromium.org/13866010/diff/3001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/types/simple_types_inferrer.dart:1536:
return handlePlainAssignment(node, element, setterSelector, receiverType,
rhsType, node.arguments.head);
On 2013/04/09 16:48:21, Emily Fortuna wrote:
> 80 char

Done.

https://codereview.chromium.org/13866010/diff/3001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/types/simple_types_inferrer.dart:1598:
} else if (element.isField()) {
On 2013/04/09 16:48:21, Emily Fortuna wrote:
> not a show-stopper for this change, but it seems like isField and isFinal and
> isSetter, etc might be good candidates as gettors instead of functions.

It is true that we should clean this up.  However, my longer term goal is to
have Element implement the Mirror API, and I'm deferring clean-up until then.

https://codereview.chromium.org/13866010/diff/3001/dart/tests/compiler/dart2j...
File dart/tests/compiler/dart2js/bad_loop_test.dart (right):

https://codereview.chromium.org/13866010/diff/3001/dart/tests/compiler/dart2j...
dart/tests/compiler/dart2js/bad_loop_test.dart:23: if (kind ==
Diagnostic.VERBOSE_INFO) return;
On 2013/04/10 08:34:54, kasperl wrote:
> Tiny nit: I might actually go for writing this with extra {} and else if -- if
> nothing else then just to make the flow of this method more uniform.

Done.

[ahe, 2013-04-12 07:29:43]

Ping

[kasperl, 2013-04-12 07:33:48]

Still LGTM.

https://codereview.chromium.org/13866010/diff/10001/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/13866010/diff/10001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/ssa/builder.dart:4075:
argumentValues: <HInstruction>[oldVariable]);
Long line.

[ahe, 2013-04-12 10:20:11]

Committed patchset #3 manually as r21342 (presubmit successful).