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

lib/compiler/implementation/ssa/codegen.dart

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 class SsaCodeGeneratorTask extends CompilerTask {
   final JavaScriptBackend backend;
   SsaCodeGeneratorTask(JavaScriptBackend backend)
       : this.backend = backend,
         super(backend.compiler);
   String get name() => 'SSA code generator';
@@ skipping 375 matching lines @@
     }
     generationState = oldState;
   }
 
   /**
    * Generate expressions from block information.
    */
   void generateExpression(HExpressionInformation expression) {
     // Currently we only handle sub-expression graphs.
     assert(expression is HSubExpressionBlockInformation);
+    // [visitSubGraph] will reset the [expectedPrecedence]. Make sure we don't
+    // need parenthesis.

[Lasse Reichstein Nielsen, 2012/06/27 08:53:58]

I.e., this only expects to be called for top-level expressions, not
sub-expressions.

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

Done.

+    assert(expectedPrecedence == JSPrecedence.STATEMENT_PRECEDENCE
+        || expectedPrecedence == JSPrecedence.EXPRESSION_PRECEDENCE);
+
     HSubExpressionBlockInformation expressionSubGraph = expression;
 
     int oldState = generationState;
     generationState = STATE_FIRST_EXPRESSION;
     visitSubGraph(expressionSubGraph.subExpression);
     generationState = oldState;
   }
 
   void generateDeclaration(HExpressionInformation expression) {
     // Currently we only handle sub-expression graphs.
@@ skipping 158 matching lines @@
     String inputName = variableNames.getName(instruction.checkedInput);
     if (name != inputName) return false;
     visit(instruction, JSPrecedence.STATEMENT_PRECEDENCE);
     return true;
   }
 
   void define(HInstruction instruction) {
     if (isGeneratingExpression()) {
       addExpressionSeparator();
     } else {
+      assert(expectedPrecedence == JSPrecedence.STATEMENT_PRECEDENCE);
       addIndentation();
     }
     if (!instruction.isControlFlow() && variableNames.hasName(instruction)) {
       var name = variableNames.getName(instruction);
       if (!handleSimpleUpdateDefinition(instruction, name)
           && !handleTypeConversion(instruction, name)) {
-        declareInstruction(instruction);
-        buffer.add(" = ");
-        visit(instruction, JSPrecedence.ASSIGNMENT_PRECEDENCE);
+        withPrecedence(JSPrecedence.ASSIGNMENT_PRECEDENCE, () {
+          declareInstruction(instruction);
+          buffer.add(" = ");
+          visit(instruction, JSPrecedence.ASSIGNMENT_PRECEDENCE);
+        });
       }
     } else {
-      visit(instruction, JSPrecedence.STATEMENT_PRECEDENCE);
+      visit(instruction, expectedPrecedence);
     }
     if (!isGeneratingExpression()) buffer.add(';\n');
   }
 
   void use(HInstruction argument, int expectedPrecedenceForArgument) {
     if (isGenerateAtUseSite(argument)) {
       visit(argument, expectedPrecedenceForArgument);
     } else if (argument is HCheck && argument.isControlFlow()) {
       // A [HCheck] that has control flow can never be used as an
       // expression and may not have a name. Therefore we just use the
@@ skipping 508 matching lines @@
       }
     }
   }
 
   void iterateBasicBlock(HBasicBlock node) {
     HInstruction instruction = node.first;
     while (instruction !== node.last) {
       if (instruction is HTypeGuard) {
         visit(instruction, JSPrecedence.STATEMENT_PRECEDENCE);
       } else if (!isGenerateAtUseSite(instruction)) {
+        expectedPrecedence = JSPrecedence.STATEMENT_PRECEDENCE;
         define(instruction);

[Lasse Reichstein Nielsen, 2012/06/27 08:53:58]

Won't a "define" always expect to be at the "statement" level? I.e., maybe move
the assignment into define(..).

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

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

       }
       instruction = instruction.next;
     }
     assignPhisOfSuccessors(node);
     if (instruction is HLoopBranch && isGeneratingExpression()) {
       addExpressionSeparator();
     }
     visit(instruction, JSPrecedence.STATEMENT_PRECEDENCE);
   }
 
@@ skipping 325 matching lines @@
     int thenKind = analyzeGraphForCodegen(thenGraph);
     int elseKind = analyzeGraphForCodegen(elseGraph);
 
     void visitWithoutIndent(HStatementInformation toVisit) {
       int oldIndent = indent;
       indent = 0;
       generateStatements(toVisit);
       indent = oldIndent;
     }
 
+    void visitExpression(HStatementInformation toVisit) {
+      // [generateExpression] only works if the [expectedPrecedence] is a
+      // statement or an expression. We therefore have to duplicate some
+      // work here.

[Lasse Reichstein Nielsen, 2012/06/27 08:53:58]

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.

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

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.

+      assert(toVisit.start == toVisit.end);
+      assert(toVisit.start.last is HGoto);
+      // Find the expression (there must only be one).
+      HInstruction expression = toVisit.start.first;
+      while (generateAtUseSite.contains(expression)) {
+        expression = expression.next;
+      }
+      assert(() {
+        HInstruction remaining = expression.next;
+        while (remaining is !HGoto) {
+          if (generateAtUseSite.contains(remaining)) return false;

[Lasse Reichstein Nielsen, 2012/06/27 08:53:58]

Shouldn't this test be negated?

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

Done.

+          remaining = remaining.next;
+        }
+        return true;
+      });
+
+      int oldState = generationState;
+      generationState = STATE_FIRST_EXPRESSION;

[Lasse Reichstein Nielsen, 2012/06/27 08:53:58]

x

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

Done.

+      define(expression);
+      generationState = oldState;
+    }
+
     void visitWithIndent(HStatementInformation toVisit) {
       buffer.add('{\n');
       indent++;
       generateStatements(toVisit);
       indent--;
       addIndented('}');
     }
 
     void emitIf() {
       addIndented('if (');
       use(node.inputs[0], JSPrecedence.EXPRESSION_PRECEDENCE);
       buffer.add(') ');
     }
 
     JSBinaryOperatorPrecedence operatorPrecedence = JSPrecedence.binary['&&'];
     void generateAnd(HStatementInformation toVisit, Function condition) {
       addIndentation();
       beginExpression(operatorPrecedence.precedence);
       condition();

[Lasse Reichstein Nielsen, 2012/06/27 08:53:58]

Shouldn't this be executed in a scope where expectedPrecedence is
operatorPrecedence.left?

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

Done.

       buffer.add(" && ");
       var oldPrecedence = expectedPrecedence;
       expectedPrecedence = operatorPrecedence.right;
-      visitWithoutIndent(toVisit);
+      visitExpression(toVisit);
       expectedPrecedence = oldPrecedence;
       endExpression(operatorPrecedence.precedence);
+      buffer.add(";\n");
     }
 
     List<HBasicBlock> thenSuccessors = thenGraph.end.successors;
     bool thenGraphHasSuccessor = thenSuccessors.length != 0
         && thenSuccessors[0] !== currentGraph.exit;
 
     switch (thenKind) {
       case EMPTY:
         switch (elseKind) {
           case EMPTY:
@@ skipping 1507 matching lines @@
       startBailoutSwitch();
     }
   }
 
   void endLabeledBlock(HLabeledBlockInformation labeledBlockInfo) {
     if (labeledBlockInfo.body.start.hasGuards()) {
       endBailoutSwitch();
     }
   }
 }