Fix bad type-inferrence for logical expressions.

Fix bad type-inferrence for logical expressions.

BUG= http://dartbug.com/17149
R=ngeoffray@google.com, sra@google.com

Committed: https://code.google.com/p/dart/source/detail?r=33120
Reverted: https://code.google.com/p/dart/source/detail?r=33121

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

[floitsch, 2014-02-26 23:11:44]

Another bug is fixed in a follow-up CL
(https://codereview.chromium.org/181783006/)

I can merge the two, if you prefer.

[sra1, 2014-02-27 03:12:55]

lgtm.

I *think* it is correct, but this state-machine style of visitor programming
makes my head hurt.
Perhaps we could recode the conditional control flow to walk the expression with
true- and false- 'labels' that accumulate the conditional state.

https://codereview.chromium.org/182373002/diff/1/tests/language/logical_expre...
File tests/language/logical_expression2_test.dart (right):

https://codereview.chromium.org/182373002/diff/1/tests/language/logical_expre...
tests/language/logical_expression2_test.dart:9: // unconditionally propagated
is-checks of the logical and.
This past-tense sentence will be confusing in the future.

Rephrase to what must be done ("... would be incorrect to propagate ...")
and say this was a bug with dart2js, and reference the bug.

Since it is a bug in dart2js, maybe this test should live in dart2js_extra?

https://codereview.chromium.org/182373002/diff/1/tests/language/logical_expre...
tests/language/logical_expression2_test.dart:19: var o = [ "foo", 499 ][1];
With array tracing, dart2js can probably constant-fold this whole program (or
should be close to being able to.)

[ngeoffray, 2014-02-27 10:33:12]

LGTM

https://codereview.chromium.org/182373002/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/inferrer/inferrer_visitor.dart
(right):

https://codereview.chromium.org/182373002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/inferrer/inferrer_visitor.dart:867:
isChecks = null;
How about an abstraction:

bool stopAccumulatingIsChecks() {
  bool oldAccumulateIsChecks = accumulateIsChecks;
  accumulateIsChecks = false;
  isChecks = null;
  return oldAccumulateIsChecks.
}

And add a comment why.

https://codereview.chromium.org/182373002/diff/1/tests/language/logical_expre...
File tests/language/logical_expression2_test.dart (right):

https://codereview.chromium.org/182373002/diff/1/tests/language/logical_expre...
tests/language/logical_expression2_test.dart:3: // BSD-style license that can be
found in the LICENSE file.
New line.

https://codereview.chromium.org/182373002/diff/1/tests/language/logical_expre...
tests/language/logical_expression2_test.dart:4: // Dart test program for testing
if statement.
Rephrase comment.

[floitsch, 2014-02-27 16:07:08]

Found same bug in !(e1 && e2).

Rewrote (e1 && e2) so that this can't happen again.


@sra: I'm pretty sure that this CL improves the is-check handling, but I agree
that a rewrite could help.

https://codereview.chromium.org/182373002/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/inferrer/inferrer_visitor.dart
(right):

https://codereview.chromium.org/182373002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/inferrer/inferrer_visitor.dart:867:
isChecks = null;
On 2014/02/27 10:33:12, ngeoffray wrote:
> How about an abstraction:
> 
> bool stopAccumulatingIsChecks() {
>   bool oldAccumulateIsChecks = accumulateIsChecks;
>   accumulateIsChecks = false;
>   isChecks = null;
>   return oldAccumulateIsChecks.
> }
> 
> And add a comment why.

Added helper.

Changed the assert from the '&&' into code that sets the field always if the
oldAccumulateIsChecks was false. This avoids further errors.

https://codereview.chromium.org/182373002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/inferrer/inferrer_visitor.dart:875:
accumulateIsChecks = false;
Similar bug. (fixed in latest patch-set).

https://codereview.chromium.org/182373002/diff/1/tests/language/logical_expre...
File tests/language/logical_expression2_test.dart (right):

https://codereview.chromium.org/182373002/diff/1/tests/language/logical_expre...
tests/language/logical_expression2_test.dart:3: // BSD-style license that can be
found in the LICENSE file.
On 2014/02/27 10:33:12, ngeoffray wrote:
> New line.

Done.

https://codereview.chromium.org/182373002/diff/1/tests/language/logical_expre...
tests/language/logical_expression2_test.dart:4: // Dart test program for testing
if statement.
On 2014/02/27 10:33:12, ngeoffray wrote:
> Rephrase comment.

Removed. The description is below.

https://codereview.chromium.org/182373002/diff/1/tests/language/logical_expre...
tests/language/logical_expression2_test.dart:9: // unconditionally propagated
is-checks of the logical and.
On 2014/02/27 03:12:55, sra1 wrote:
> This past-tense sentence will be confusing in the future.
> 
> Rephrase to what must be done ("... would be incorrect to propagate ...")
> and say this was a bug with dart2js, and reference the bug.
> 
> Since it is a bug in dart2js, maybe this test should live in dart2js_extra?

Test should work with the VM too. I don't see any need to keep it dart2js
private.
Moved bug description to `if`, but referenced issue number here.

https://codereview.chromium.org/182373002/diff/1/tests/language/logical_expre...
tests/language/logical_expression2_test.dart:19: var o = [ "foo", 499 ][1];
On 2014/02/27 03:12:55, sra1 wrote:
> With array tracing, dart2js can probably constant-fold this whole program (or
> should be close to being able to.)

Done.

[floitsch, 2014-02-27 16:44:01]

I will commit now, but if you have more comments, I will address them in a
follow-up CL.

[floitsch, 2014-02-27 16:44:44]

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

[floitsch, 2014-02-27 18:26:07]

PTAL.

I had to revert.

[floitsch, 2014-02-27 18:27:03]

FYI: the version that was committed was Patch Set 3.

[herhut, 2014-02-28 08:24:45]

On 2014/02/27 18:27:03, floitsch wrote:
> FYI: the version that was committed was Patch Set 3.

The assumed invariant is that accumulateIsChecks => isChecks != null. A call to
stopAccumulatingIsChecks potentially breaks the invariant, as the old state of
accumulateIsChecks is saved (and later restored) but the isChecks will remain
null. This can be triggered by a conditional like below

main() {
  var a,b,c,d;
  if ((a is int || b is int) && (c is int)) 
    print("oha");
}

where the || will set isChecks to null but restore the accumulateIsChecks to
true before the rhs of the && is processed, using an invalid state.

More generally, does the code for || even do what it is supposed to? From (a is
T || a is U) we can derive that a is union(T,U), whereas (a is T || b is U) does
not give us anything. The current code seems to narrow the type of the receiver
of the is check no matter what the || looks like. This needs a general overhaul
but to just make it correct, I would propose to stop accumulating is checks for
operand expressions of || for now.

[floitsch, 2014-02-28 09:23:07]

We want to use the isChecks from the lhs.

[herhut, 2014-02-28 10:17:01]

On 2014/02/28 09:23:07, floitsch wrote:
> We want to use the isChecks from the lhs.

LGTM

[floitsch, 2014-02-28 10:19:21]

Committed patchset #5 manually as r33152 (presubmit successful).