commit 6ccff29c57c9795d93eec496fdc7cfc3c135d58a
author fzaiser <fzaiser@google.com> Mon Aug 28 17:50:51 2017 +0200
committer Vladimir Moskva <vladmos@google.com> Tue Aug 29 13:31:10 2017 +0200
tree 26c6fbcc9480c86862d0fecb00ff450e5c847090
parent c1309787cc4b55cb1d11e60a687f27b9b38ed09c

Visit subtrees of the AST in evaluation order

For instance, it makes more sense to visit the RHS of an assignment
first because this is evaluated first.

This also fixes a bug in the validator, which allowed definitions
like "a = a"

RELNOTES: None
PiperOrigin-RevId: 166709589

src/main/java/com/google/devtools/build/lib/syntax/SyntaxTreeVisitor.java

diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SyntaxTreeVisitor.java b/src/main/java/com/google/devtools/build/lib/syntax/SyntaxTreeVisitor.java
index 311b8c3..c7c0eb0 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SyntaxTreeVisitor.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SyntaxTreeVisitor.java
@@ -77,17 +77,17 @@
 
   public void visit(AbstractComprehension node) {
     for (ListComprehension.Clause clause : node.getClauses()) {
+      visit(clause.getExpression());
       if (clause.getLValue() != null) {
         visit(clause.getLValue());
       }
-      visit(clause.getExpression());
     }
     visitAll(node.getOutputExpressions());
   }
 
   public void visit(ForStatement node) {
-    visit(node.getVariable());
     visit(node.getCollection());
+    visit(node.getVariable());
     visitBlock(node.getBlock());
   }
 
@@ -108,13 +108,13 @@
   }
 
   public void visit(AssignmentStatement node) {
-    visit(node.getLValue());
     visit(node.getExpression());
+    visit(node.getLValue());
   }
 
   public void visit(AugmentedAssignmentStatement node) {
-    visit(node.getLValue());
     visit(node.getExpression());
+    visit(node.getLValue());
   }
 
   public void visit(ExpressionStatement node) {
@@ -183,8 +183,8 @@
   public void visit(@SuppressWarnings("unused") Comment node) {}
 
   public void visit(ConditionalExpression node) {
-    visit(node.getThenCase());
     visit(node.getCondition());
+    visit(node.getThenCase());
     if (node.getElseCase() != null) {
       visit(node.getElseCase());
     }

src/test/java/com/google/devtools/build/lib/syntax/SyntaxTreeVisitorTest.java

diff --git a/src/test/java/com/google/devtools/build/lib/syntax/SyntaxTreeVisitorTest.java b/src/test/java/com/google/devtools/build/lib/syntax/SyntaxTreeVisitorTest.java
index 79afdb9..39f46d0 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/SyntaxTreeVisitorTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/SyntaxTreeVisitorTest.java
@@ -62,7 +62,7 @@
             "  return h + i.j()");
     IdentVisitor visitor = new IdentVisitor();
     ast.accept(visitor);
-    assertThat(idents).containsExactly("a", "b", "c", "d", "e", "f", "g", "h", "i", "j").inOrder();
+    assertThat(idents).containsExactly("b", "a", "c", "e", "d", "f", "g", "h", "i", "j").inOrder();
     assertThat(params).containsExactly("p1", "p2=4", "**p3").inOrder();
   }
 }

src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java

diff --git a/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java
index 604c53f..49f5dbb 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java
@@ -105,6 +105,14 @@
   }
 
   @Test
+  public void testDefinitionByItself() throws Exception {
+    checkError("name 'a' is not defined", "a = a");
+    checkError("name 'a' is not defined", "a += a");
+    checkError("name 'a' is not defined", "[[] for a in a]");
+    checkError("name 'a' is not defined", "def f():", "  for a in a: pass");
+  }
+
+  @Test
   public void testLocalValidationEnvironmentsAreSeparated() throws Exception {
     parse("def func1():", "  a = 1", "def func2():", "  a = 'abc'\n");
   }