Fix compiler crash when we declare several (global) variables, but only define some of them.

Fix compiler crash when we declare several (global) variables, but only define some of them.

BUG=27553
R=johnniwinther@google.com, sigmund@google.com

Committed: https://github.com/dart-lang/sdk/commit/2206b43c3d59a0ad9de7951203aa8a53f903bc3f

[Emily Fortuna, 2016-11-02 00:04:13]

Patchset #1 (id:1) has been deleted

[Emily Fortuna, 2016-11-02 00:05:25]

efortuna@google.com changed reviewers:
+ sigmund@google.com

[Emily Fortuna, 2016-11-02 00:05:25]

https://codereview.chromium.org/2471543004/diff/20001/tests/compiler/dart2js/...
File tests/compiler/dart2js/constant_expression_test.dart (left):

https://codereview.chromium.org/2471543004/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/constant_expression_test.dart:150: //
TODO(johnniwinther): Enable this when the constructor evaluator doesn't
seemed like this was enabled given the below line was uncommented...?

[Johnni Winther, 2016-11-02 10:41:31]

johnniwinther@google.com changed reviewers:
+ johnniwinther@google.com

[Johnni Winther, 2016-11-02 10:41:31]

lgtm

https://codereview.chromium.org/2471543004/diff/20001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/builder.dart (right):

https://codereview.chromium.org/2471543004/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder.dart:753: iterator.moveNext();
To find the right node you need to check `if (iterator.current.arguments.first
== initializer)` otherwise you will just find the first initialized variable.
This is needed `var x = [], y, z = [];` when generating the lazy initalizer for
`z`.

A better strategy might be to use `initializer` as default source info position
if no more precise position can be found:

    ast.Node sourceInfoNode = initializer;
    for (var definition in node.definitions) {
      if (definition is SendSet &&
          definition.arguments.first == initializer) {
        sourceInfoNode = definition.assignmentOperator;
      }
    }

https://codereview.chromium.org/2471543004/diff/20001/tests/compiler/dart2js/...
File tests/compiler/dart2js/constant_expression_test.dart (left):

https://codereview.chromium.org/2471543004/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/constant_expression_test.dart:150: //
TODO(johnniwinther): Enable this when the constructor evaluator doesn't
On 2016/11/02 00:05:25, Emily Fortuna wrote:
> seemed like this was enabled given the below line was uncommented...?

Acknowledged.

[Emily Fortuna, 2016-11-02 18:09:21]

https://codereview.chromium.org/2471543004/diff/20001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/builder.dart (right):

https://codereview.chromium.org/2471543004/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder.dart:753: iterator.moveNext();
On 2016/11/02 10:41:31, Johnni Winther wrote:
> To find the right node you need to check `if (iterator.current.arguments.first
> == initializer)` otherwise you will just find the first initialized variable.
> This is needed `var x = [], y, z = [];` when generating the lazy initalizer
for
> `z`.
> 
> A better strategy might be to use `initializer` as default source info
position
> if no more precise position can be found:
> 
>     ast.Node sourceInfoNode = initializer;
>     for (var definition in node.definitions) {
>       if (definition is SendSet &&
>           definition.arguments.first == initializer) {
>         sourceInfoNode = definition.assignmentOperator;
>       }
>     }
> 

Thanks for helping me understand how this works. I updated the code slightly to
check the variable name instead of the value that is assigning to it for
clarity. Lmk if there are any issues with that.

[Siggi Cherem (dart-lang), 2016-11-02 18:17:20]

lgtm!

https://codereview.chromium.org/2471543004/diff/40001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/builder.dart (right):

https://codereview.chromium.org/2471543004/diff/40001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder.dart:756: definition.selector.toString() ==
variable.name) {
toString is mainly something we use for debugging (for complex nodes it can
recreate a string representation of the ast). In this case I think it's safe to
assume that the selctor is an identifier and do:

  definition.selector.asIdentifier().source == variable.name

https://codereview.chromium.org/2471543004/diff/40001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder.dart:757: sourceInfoNode =
definition.assignmentOperator;
let's add a break here as well (it doesn't change things, but we'd stop
iterating the rest of the definitions if there are more.

https://codereview.chromium.org/2471543004/diff/40001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder.dart:763: return closeFunction(); // token or
source
remove trailing comment?

[Emily Fortuna, 2016-11-02 18:24:58]

https://codereview.chromium.org/2471543004/diff/40001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/builder.dart (right):

https://codereview.chromium.org/2471543004/diff/40001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder.dart:756: definition.selector.toString() ==
variable.name) {
On 2016/11/02 18:17:20, Siggi Cherem (dart-lang) wrote:
> toString is mainly something we use for debugging (for complex nodes it can
> recreate a string representation of the ast). In this case I think it's safe
to
> assume that the selctor is an identifier and do:
> 
>   definition.selector.asIdentifier().source == variable.name

Done.

https://codereview.chromium.org/2471543004/diff/40001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder.dart:757: sourceInfoNode =
definition.assignmentOperator;
On 2016/11/02 18:17:20, Siggi Cherem (dart-lang) wrote:
> let's add a break here as well (it doesn't change things, but we'd stop
> iterating the rest of the definitions if there are more.

Done.

https://codereview.chromium.org/2471543004/diff/40001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder.dart:763: return closeFunction(); // token or
source
On 2016/11/02 18:17:20, Siggi Cherem (dart-lang) wrote:
> remove trailing comment?

whoops thanks.

[Emily Fortuna, 2016-11-02 18:28:23]

Description was changed from

==========
Fix compiler crash when we declare several (global) variables, but only define
some of them.

BUG=27553
==========

to

==========
Fix compiler crash when we declare several (global) variables, but only define
some of them.

BUG=27553
R=johnniwinther@google.com, sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/2206b43c3d59a0ad9de7951203aa8a53f903bc3f
==========

[Emily Fortuna, 2016-11-02 18:28:24]

Committed patchset #3 (id:60001) manually as
2206b43c3d59a0ad9de7951203aa8a53f903bc3f (presubmit successful).