Handle switch with default case, and do/while loop better in the inferrer.

Handle switch with default case, and do/while loop better in the inferrer.

R=ahe@google.com

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

[ahe, 2013-08-13 16:26:45]

LGTM!

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

https://codereview.chromium.org/23050002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/types/inferrer_visitor.dart:227: void
forEachLocalUntil(Node node,
When I see a method like this, I automatically assume it keeps going until the
closure returns true (and that f has the wrong return type). So I suggest
renaming it to 'forEachLocalUntilNode'.

https://codereview.chromium.org/23050002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/types/inferrer_visitor.dart:423: * If
[keepIncomingLocals] is true, the types of locals in [:this:]
As discussed in person, 'keepIncomingLocals' is slightly confusing as it makes
me think of incoming edges to this locals handler, but it is really about wether
or not to merge this guy into the join point.

https://codereview.chromium.org/23050002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/types/inferrer_visitor.dart:457:
assert(index == handlers.lenghth);
length.

[sra1, 2013-08-14 03:59:54]

https://chromiumcodereview.appspot.com/23050002/diff/1/sdk/lib/_internal/comp...
File sdk/lib/_internal/compiler/implementation/types/inferrer_visitor.dart
(right):

https://chromiumcodereview.appspot.com/23050002/diff/1/sdk/lib/_internal/comp...
sdk/lib/_internal/compiler/implementation/types/inferrer_visitor.dart:410: T
otherType = elseBranch.locals[local];
Iterating over thenBranch and testing elseBranch is asymmetric.
This code infers a return type of [null]:

testIf1(a) {
  var c = null;
  if (a) {
  } else {
    c = 10;
  }
  return c;
}

https://chromiumcodereview.appspot.com/23050002/diff/1/sdk/lib/_internal/comp...
sdk/lib/_internal/compiler/implementation/types/inferrer_visitor.dart:414:
locals[local] = types.allocateDiamondPhi(type, otherType);
I find it confusing that 'locals' here could be an alias for thenBranch.locals

One way to make this clearer might be to split into
mergeIfThen
mergeIfThenElse

https://chromiumcodereview.appspot.com/23050002/diff/1/sdk/lib/_internal/comp...
sdk/lib/_internal/compiler/implementation/types/inferrer_visitor.dart:424: * are
being used in the merge.
What does it mean for the types of the locals in `this` to *not* be used?

[sra1, 2013-08-14 04:00:21]

There is still a code quality regression in swarm compared to the buggy
inference.
Swarm contains one function like this:

testSwitch5() {
  switch(topLevelGetter) {
    case 1: return 1;
    default: return 2;
  }
}

The old buggy inference had testSwitch5 return int, this CL returns int|null.

The code quality regression in swarm is:

      if (codeUnit === 32 || codeUnit === 13 ||
$.JSString__isWhitespace(codeUnit))
        ++startIndex;
--->
      if (codeUnit === 32 || codeUnit === 13 ||
$.JSString__isWhitespace(codeUnit) === true)
        ++startIndex;

[ngeoffray, 2013-08-14 07:53:46]

Thanks Peter and Stephen

Good catch for the code quality regression Stephen. It was actually a braino in
the mergeAll code, where the test to realize if there is a handler that does not
abort was a null check. It should have been a length check. I added the test you
wrote for it.

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

https://codereview.chromium.org/23050002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/types/inferrer_visitor.dart:227: void
forEachLocalUntil(Node node,
On 2013/08/13 16:26:45, ahe wrote:
> When I see a method like this, I automatically assume it keeps going until the
> closure returns true (and that f has the wrong return type). So I suggest
> renaming it to 'forEachLocalUntilNode'.

Done.

https://codereview.chromium.org/23050002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/types/inferrer_visitor.dart:410: T
otherType = elseBranch.locals[local];
On 2013/08/14 03:59:54, sra1 wrote:
> Iterating over thenBranch and testing elseBranch is asymmetric.
> This code infers a return type of [null]:
> 
> testIf1(a) {
>   var c = null;
>   if (a) {
>   } else {
>     c = 10;
>   }
>   return c;
> }

Good catch, I'll deal with that in another CL.

https://codereview.chromium.org/23050002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/types/inferrer_visitor.dart:423: * If
[keepIncomingLocals] is true, the types of locals in [:this:]
On 2013/08/13 16:26:45, ahe wrote:
> As discussed in person, 'keepIncomingLocals' is slightly confusing as it makes
> me think of incoming edges to this locals handler, but it is really about
wether
> or not to merge this guy into the join point.

Done.

https://codereview.chromium.org/23050002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/types/inferrer_visitor.dart:424: * are
being used in the merge.
On 2013/08/14 03:59:54, sra1 wrote:
> What does it mean for the types of the locals in `this` to *not* be used?

I added a comment. It means that 'this', which is the dominator of all handlers,
also flows directly to the join block.

https://codereview.chromium.org/23050002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/types/inferrer_visitor.dart:457:
assert(index == handlers.lenghth);
On 2013/08/13 16:26:45, ahe wrote:
> length.

Done.

[ngeoffray, 2013-08-14 08:10:14]

Committed patchset #2 manually as r26109 (presubmit successful).