Don't lose the expected precedence during && compilation.

Don't lose the expected precedence during && compilation.

When transforming if (foo) bar; to foo && bar make sure that
bar is compiled with the correct precedence.
In particular avoid foo && toto = 499;
instead of
foo && (toto = 499).

Fixes issue 3862.

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

[floitsch, 2012-06-26 16:35:22]

The test is currently not triggering, but future CLs would make this test fail
without this patch.
I'm not sure if it is currently possible to exploit this bug.

[floitsch, 2012-06-26 17:12:40]

There was still a case in 'define' that overwrote the expected precedence. Fixed
now.

[Lasse Reichstein Nielsen, 2012-06-27 08:53:57]

LGTM

http://codereview.chromium.org/10669035/diff/4002/lib/compiler/implementation...
File lib/compiler/implementation/ssa/codegen.dart (right):

http://codereview.chromium.org/10669035/diff/4002/lib/compiler/implementation...
lib/compiler/implementation/ssa/codegen.dart:397: // need parenthesis.
I.e., this only expects to be called for top-level expressions, not
sub-expressions.

http://codereview.chromium.org/10669035/diff/4002/lib/compiler/implementation...
lib/compiler/implementation/ssa/codegen.dart:1123: define(instruction);
Won't a "define" always expect to be at the "statement" level? I.e., maybe move
the assignment into define(..).

http://codereview.chromium.org/10669035/diff/4002/lib/compiler/implementation...
lib/compiler/implementation/ssa/codegen.dart:1472: // work here.
Have you considered making a visitSubGraph that takes an expected precedence
with it. Perhaps make it optional and default to Statement?
Duplicating code because the other code almost does what you want smells like
you should just generalize the other code. It was originally written for a
specific use, but there is nothing wrong with making it more general.

http://codereview.chromium.org/10669035/diff/4002/lib/compiler/implementation...
lib/compiler/implementation/ssa/codegen.dart:1483: if
(generateAtUseSite.contains(remaining)) return false;
Shouldn't this test be negated?

http://codereview.chromium.org/10669035/diff/4002/lib/compiler/implementation...
lib/compiler/implementation/ssa/codegen.dart:1490: generationState =
STATE_FIRST_EXPRESSION;
x

http://codereview.chromium.org/10669035/diff/4002/lib/compiler/implementation...
lib/compiler/implementation/ssa/codegen.dart:1513: condition();
Shouldn't this be executed in a scope where expectedPrecedence is
operatorPrecedence.left?

[floitsch, 2012-06-27 11:18:24]

http://codereview.chromium.org/10669035/diff/4002/lib/compiler/implementation...
File lib/compiler/implementation/ssa/codegen.dart (right):

http://codereview.chromium.org/10669035/diff/4002/lib/compiler/implementation...
lib/compiler/implementation/ssa/codegen.dart:397: // need parenthesis.
On 2012/06/27 08:53:58, Lasse Reichstein Nielsen wrote:
> I.e., this only expects to be called for top-level expressions, not
> sub-expressions.

Done.

http://codereview.chromium.org/10669035/diff/4002/lib/compiler/implementation...
lib/compiler/implementation/ssa/codegen.dart:1123: define(instruction);
On 2012/06/27 08:53:58, Lasse Reichstein Nielsen wrote:
> Won't a "define" always expect to be at the "statement" level? I.e., maybe
move
> the assignment into define(..).

No. 'define' is called from visitExpression below. We have cases where we
produce: "foo && (x = bar)".

http://codereview.chromium.org/10669035/diff/4002/lib/compiler/implementation...
lib/compiler/implementation/ssa/codegen.dart:1472: // work here.
On 2012/06/27 08:53:58, Lasse Reichstein Nielsen wrote:
> Have you considered making a visitSubGraph that takes an expected precedence
> with it. Perhaps make it optional and default to Statement?
> Duplicating code because the other code almost does what you want smells like
> you should just generalize the other code. It was originally written for a
> specific use, but there is nothing wrong with making it more general.

Without the asserts we only have 8 lines here. They are fairly specialized, and
if we need more we should adapt generateExpressions. Until then I think it is
easier to keep it like it is now.

http://codereview.chromium.org/10669035/diff/4002/lib/compiler/implementation...
lib/compiler/implementation/ssa/codegen.dart:1483: if
(generateAtUseSite.contains(remaining)) return false;
On 2012/06/27 08:53:58, Lasse Reichstein Nielsen wrote:
> Shouldn't this test be negated?

Done.

http://codereview.chromium.org/10669035/diff/4002/lib/compiler/implementation...
lib/compiler/implementation/ssa/codegen.dart:1490: generationState =
STATE_FIRST_EXPRESSION;
On 2012/06/27 08:53:58, Lasse Reichstein Nielsen wrote:
> x

Done.

http://codereview.chromium.org/10669035/diff/4002/lib/compiler/implementation...
lib/compiler/implementation/ssa/codegen.dart:1513: condition();
On 2012/06/27 08:53:58, Lasse Reichstein Nielsen wrote:
> Shouldn't this be executed in a scope where expectedPrecedence is
> operatorPrecedence.left?

Done.

[sra1, 2012-06-29 02:03:57]

https://chromiumcodereview.appspot.com/10669035/diff/10001/lib/compiler/imple...
File lib/compiler/implementation/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/10669035/diff/10001/lib/compiler/imple...
lib/compiler/implementation/ssa/codegen.dart:587:
withPrecedence(JSPrecedence.ASSIGNMENT_PRECEDENCE, () {
It bothers me that there are >200 places in this file that say JSPrecedence.XXX
withPrecedence simplifies a few cases and I'm sure it could be used more widely.
 However, the closure passed to withPrecedence has an allocation cost and is
really just an ad-hoc temporary AST that knows only how to emit itself.

The visitor pattern seems error-prone and suffers from being excessively
state-sensitive.

Why not embrace the idea of a temporary AST and generate all expressions via a
lightweight AST for the specific task of emitting well formed expressions? 
There would be no mention of precedence except in the code that emits ASTs. 
Since about half of AST nodes are leaves, a String should be permitted as a
terminal AST, assumed to have a high (primary?) precedence.