Skylark: Unified ListComprehension and DictComprehension.

As a result, complex dict comprehensions (nested + with conditions) can be used.

--
MOS_MIGRATED_REVID=103374493

5 files changed, 372 insertions, 313 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/syntax/AbstractComprehension.java b/src/main/java/com/google/devtools/build/lib/syntax/AbstractComprehension.java
new file mode 100644
index 0000000000..2bbed64cd6
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/syntax/AbstractComprehension.java
@@ -0,0 +1,286 @@
+// Copyright 2014 Google Inc. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.lib.syntax;
+
+import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.events.Location;
+
+import java.io.Serializable;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+import javax.annotation.Nullable;
+
+/**
+ * Base class for list and dict comprehension expressions.
+ *
+ * <p> A comprehension contains one or more clauses, e.g.
+ *   [a+d for a in b if c for d in e]
+ * contains three clauses: "for a in b", "if c", "for d in e".
+ * For and If clauses can happen in any order, except that the first one has to be a For.
+ *
+ * <p> The code above can be expanded as:
+ * <pre>
+ *   for a in b:
+ *     if c:
+ *       for d in e:
+ *         result.append(a+d)
+ * </pre>
+ * result is initialized to [] (list) or {} (dict) and is the return value of the whole expression.
+ */
+public abstract class AbstractComprehension extends Expression {
+
+  /**
+   * The interface implemented by ForClause and (later) IfClause.
+   * A comprehension consists of one or many Clause.
+   */
+  public interface Clause extends Serializable {
+    /**
+     * The evaluation of the comprehension is based on recursion. Each clause may
+     * call recursively evalStep (ForClause will call it multiple times, IfClause will
+     * call it zero or one time) which will evaluate the next clause. To know which clause
+     * is the next one, we pass a step argument (it represents the index in the clauses
+     * list). Results are aggregated in the result argument, and are populated by
+     * evalStep.
+     *
+     * @param env environment in which we do the evaluation.
+     * @param collector the aggregated results of the comprehension.
+     * @param step the index of the next clause to evaluate.
+     */
+    abstract void eval(Environment env, OutputCollector collector, int step)
+        throws EvalException, InterruptedException;
+
+    abstract void validate(ValidationEnvironment env) throws EvalException;
+
+    /**
+     * The LValue defined in Clause, i.e. the loop variables for ForClause and null for
+     * IfClause. This is needed for SyntaxTreeVisitor.
+     */
+    @Nullable  // for the IfClause
+    public abstract LValue getLValue();
+
+    /**
+     * The Expression defined in Clause, i.e. the collection for ForClause and the
+     * condition for IfClause. This is needed for SyntaxTreeVisitor.
+     */
+    public abstract Expression getExpression();
+  }
+
+  /**
+   * A for clause in a comprehension, e.g. "for a in b" in the example above.
+   */
+  public final class ForClause implements Clause {
+    private final LValue variables;
+    private final Expression list;
+
+    public ForClause(LValue variables, Expression list) {
+      this.variables = variables;
+      this.list = list;
+    }
+
+    @Override
+    public void eval(Environment env, OutputCollector collector, int step)
+        throws EvalException, InterruptedException {
+      Object listValueObject = list.eval(env);
+      Location loc = getLocation();
+      Iterable<?> listValue = EvalUtils.toIterable(listValueObject, loc);
+      for (Object listElement : listValue) {
+        variables.assign(env, loc, listElement);
+        evalStep(env, collector, step);
+      }
+    }
+
+    @Override
+    public void validate(ValidationEnvironment env) throws EvalException {
+      variables.validate(env, getLocation());
+      list.validate(env);
+    }
+
+    @Override
+    public LValue getLValue() {
+      return variables;
+    }
+
+    @Override
+    public Expression getExpression() {
+      return list;
+    }
+
+    @Override
+    public String toString() {
+      return Printer.format("for %s in %r", variables.toString(), list);
+    }
+  }
+
+  /**
+   * A if clause in a comprehension, e.g. "if c" in the example above.
+   */
+  public final class IfClause implements Clause {
+    private final Expression condition;
+
+    public IfClause(Expression condition) {
+      this.condition = condition;
+    }
+
+    @Override
+    public void eval(Environment env, OutputCollector collector, int step)
+        throws EvalException, InterruptedException {
+      if (EvalUtils.toBoolean(condition.eval(env))) {
+        evalStep(env, collector, step);
+      }
+    }
+
+    @Override
+    public void validate(ValidationEnvironment env) throws EvalException {
+      condition.validate(env);
+    }
+
+    @Override
+    public LValue getLValue() {
+      return null;
+    }
+
+    @Override
+    public Expression getExpression() {
+      return condition;
+    }
+
+    @Override
+    public String toString() {
+      return String.format("if %s", condition);
+    }
+  }
+
+  /**
+   * The output expressions, e.g. "a+d" in the example above. This list has either one (list) or two
+   * (dict) items.
+   */
+  private final ImmutableList<Expression> outputExpressions;
+
+  private final List<Clause> clauses;
+  private final char openingBracket;
+  private final char closingBracket;
+
+  public AbstractComprehension(
+      char openingBracket, char closingBracket, Expression... outputExpressions) {
+    clauses = new ArrayList<>();
+    this.outputExpressions = ImmutableList.copyOf(outputExpressions);
+    this.openingBracket = openingBracket;
+    this.closingBracket = closingBracket;
+  }
+
+  public ImmutableList<Expression> getOutputExpressions() {
+    return outputExpressions;
+  }
+
+  @Override
+  public String toString() {
+    StringBuilder sb = new StringBuilder();
+    sb.append(openingBracket).append(printExpressions());
+    for (Clause clause : clauses) {
+      sb.append(' ').append(clause.toString());
+    }
+    sb.append(closingBracket);
+    return sb.toString();
+  }
+
+  /**
+   * Add a new ForClause to the comprehension. This is used only by the parser and must
+   * not be called once AST is complete.
+   * TODO(bazel-team): Remove this side-effect. Clauses should be passed to the constructor
+   * instead.
+   */
+  void addFor(Expression loopVar, Expression listExpression) {
+    Clause forClause = new ForClause(new LValue(loopVar), listExpression);
+    clauses.add(forClause);
+  }
+
+  /**
+   * Add a new ForClause to the comprehension.
+   * TODO(bazel-team): Remove this side-effect.
+   */
+  void addIf(Expression condition) {
+    clauses.add(new IfClause(condition));
+  }
+
+  public List<Clause> getClauses() {
+    return Collections.unmodifiableList(clauses);
+  }
+
+  @Override
+  public void accept(SyntaxTreeVisitor visitor) {
+    visitor.visit(this);
+  }
+
+  @Override
+  Object doEval(Environment env) throws EvalException, InterruptedException {
+    OutputCollector collector = createCollector();
+    evalStep(env, collector, 0);
+    return collector.getResult(env);
+  }
+
+  @Override
+  void validate(ValidationEnvironment env) throws EvalException {
+    for (Clause clause : clauses) {
+      clause.validate(env);
+    }
+    // Clauses have to be validated before expressions in order to introduce the variable names.
+    for (Expression expr : outputExpressions) {
+      expr.validate(env);
+    }
+  }
+
+  /**
+   * Evaluate the clause indexed by step, or elementExpression. When we evaluate the
+   * comprehension, step is 0 and we evaluate the first clause. Each clause may
+   * recursively call evalStep any number of times. After the last clause,
+   * the output expression(s) is/are evaluated and added to the results.
+   *
+   * <p> In the expanded example above, you can consider that evalStep is equivalent to
+   * evaluating the line number step.
+   */
+  private void evalStep(Environment env, OutputCollector collector, int step)
+      throws EvalException, InterruptedException {
+    if (step >= clauses.size()) {
+      collector.evaluateAndCollect(env);
+    } else {
+      clauses.get(step).eval(env, collector, step + 1);
+    }
+  }
+
+  /**
+   * Returns a {@link String} representation of the output expression(s).
+   */
+  abstract String printExpressions();
+
+  abstract OutputCollector createCollector();
+
+  /**
+   * Interface for collecting the intermediate output of an {@code AbstractComprehension} and for
+   * providing access to the final results.
+   */
+  interface OutputCollector {
+    /**
+     * Evaluates the output expression(s) of the comprehension and collects the result.
+     */
+    void evaluateAndCollect(Environment env) throws EvalException, InterruptedException;
+
+    /**
+     * Returns the final result of the comprehension.
+     */
+    Object getResult(Environment env) throws EvalException;
+  }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/DictComprehension.java b/src/main/java/com/google/devtools/build/lib/syntax/DictComprehension.java
index a8a6fdcf06..8befc0078b 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/DictComprehension.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/DictComprehension.java
@@ -16,74 +16,53 @@ package com.google.devtools.build.lib.syntax;
 import com.google.common.collect.ImmutableMap;
 
 import java.util.LinkedHashMap;
+import java.util.Map;
 
 /**
  * Syntax node for dictionary comprehension expressions.
  */
-public class DictComprehension extends Expression {
-  // TODO(bazel-team): Factor code with ListComprehension.java.
-
+public class DictComprehension extends AbstractComprehension {
   private final Expression keyExpression;
   private final Expression valueExpression;
-  private final LValue loopVar;
-  private final Expression listExpression;
 
-  public DictComprehension(Expression keyExpression, Expression valueExpression, Expression loopVar,
-      Expression listExpression) {
+  public DictComprehension(Expression keyExpression, Expression valueExpression) {
+    super('{', '}', keyExpression, valueExpression);
     this.keyExpression = keyExpression;
     this.valueExpression = valueExpression;
-    this.loopVar = new LValue(loopVar);
-    this.listExpression = listExpression;
   }
 
-  Expression getKeyExpression() {
-    return keyExpression;
+  @Override
+  String printExpressions() {
+    return String.format("%s: %s", keyExpression, valueExpression);
   }
 
-  Expression getValueExpression() {
-    return valueExpression;
+  @Override
+  OutputCollector createCollector() {
+    return new DictOutputCollector();
   }
 
-  LValue getLoopVar() {
-    return loopVar;
-  }
+  /**
+   * Helper class that collects the intermediate results of the {@link DictComprehension} and
+   * provides access to the resulting {@link Map}.
+   */
+  private final class DictOutputCollector implements OutputCollector {
+    private final Map<Object, Object> result;
 
-  Expression getListExpression() {
-    return listExpression;
-  }
+    DictOutputCollector() {
+      // We want to keep the iteration order
+      result = new LinkedHashMap<>();
+    }
 
-  @Override
-  Object doEval(Environment env) throws EvalException, InterruptedException {
-    // We want to keep the iteration order
-    LinkedHashMap<Object, Object> map = new LinkedHashMap<>();
-    Iterable<?> elements = EvalUtils.toIterable(listExpression.eval(env), getLocation());
-    for (Object element : elements) {
-      loopVar.assign(env, getLocation(), element);
+    @Override
+    public void evaluateAndCollect(Environment env) throws EvalException, InterruptedException {
       Object key = keyExpression.eval(env);
       EvalUtils.checkValidDictKey(key);
-      map.put(key, valueExpression.eval(env));
+      result.put(key, valueExpression.eval(env));
     }
-    return ImmutableMap.copyOf(map);
-  }
 
-  @Override
-  void validate(ValidationEnvironment env) throws EvalException {
-    listExpression.validate(env);
-    loopVar.validate(env, getLocation());
-    keyExpression.validate(env);
-  }
-
-  @Override
-  public String toString() {
-    StringBuilder sb = new StringBuilder();
-    sb.append('{').append(keyExpression).append(": ").append(valueExpression);
-    sb.append(" for ").append(loopVar).append(" in ").append(listExpression);
-    sb.append('}');
-    return sb.toString();
-  }
-
-  @Override
-  public void accept(SyntaxTreeVisitor visitor) {
-    visitor.accept(this);
+    @Override
+    public Object getResult(Environment env) throws EvalException {
+      return ImmutableMap.copyOf(result);
+    }
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ListComprehension.java b/src/main/java/com/google/devtools/build/lib/syntax/ListComprehension.java
index 6460bbca39..bfb2fbc10a 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ListComprehension.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ListComprehension.java
@@ -14,238 +14,52 @@
 
 package com.google.devtools.build.lib.syntax;
 
-import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.syntax.SkylarkList.MutableList;
 
-import java.io.Serializable;
 import java.util.ArrayList;
-import java.util.Collections;
 import java.util.List;
 
-import javax.annotation.Nullable;
-
 /**
  * Syntax node for lists comprehension expressions.
  *
- * <p> A list comprehension contains one or more clauses, e.g.
- *   [a+d for a in b if c for d in e]
- * contains three clauses: "for a in b", "if c", "for d in e".
- * For and If clauses can happen in any order, except that the first one has to be a For.
- *
- * <p> The code above can be expanded as:
- * <pre>
- *   for a in b:
- *     if c:
- *       for d in e:
- *         result.append(a+d)
- * </pre>
- * result is initialized to [] and is the return value of the whole expression.
  */
-public final class ListComprehension extends Expression {
-
-  /**
-   * The interface implemented by ForClause and (later) IfClause.
-   * A list comprehension consists of one or many Clause.
-   */
-  public interface Clause extends Serializable {
-    /**
-     * The evaluation of the list comprehension is based on recursion. Each clause may
-     * call recursively evalStep (ForClause will call it multiple times, IfClause will
-     * call it zero or one time) which will evaluate the next clause. To know which clause
-     * is the next one, we pass a step argument (it represents the index in the clauses
-     * list). Results are aggregated in the result argument, and are populated by
-     * evalStep.
-     *
-     * @param env environment in which we do the evaluation.
-     * @param result the agreggated results of the list comprehension.
-     * @param step the index of the next clause to evaluate.
-     */
-    abstract void eval(Environment env, List<Object> result, int step)
-        throws EvalException, InterruptedException;
+public final class ListComprehension extends AbstractComprehension {
+  private final Expression outputExpression;
 
-    abstract void validate(ValidationEnvironment env) throws EvalException;
-
-    /**
-     * The LValue defined in Clause, i.e. the loop variables for ForClause and null for
-     * IfClause. This is needed for SyntaxTreeVisitor.
-     */
-    @Nullable  // for the IfClause
-    public abstract LValue getLValue();
-
-    /**
-     * The Expression defined in Clause, i.e. the collection for ForClause and the
-     * condition for IfClause. This is needed for SyntaxTreeVisitor.
-     */
-    public abstract Expression getExpression();
+  public ListComprehension(Expression outputExpression) {
+    super('[', ']', outputExpression);
+    this.outputExpression = outputExpression;
   }
 
-  /**
-   * A for clause in a list comprehension, e.g. "for a in b" in the example above.
-   */
-  public final class ForClause implements Clause {
-    private final LValue variables;
-    private final Expression list;
-
-    public ForClause(LValue variables, Expression list) {
-      this.variables = variables;
-      this.list = list;
-    }
-
-    @Override
-    public void eval(Environment env, List<Object> result, int step)
-        throws EvalException, InterruptedException {
-      Object listValueObject = list.eval(env);
-      Location loc = getLocation();
-      Iterable<?> listValue = EvalUtils.toIterable(listValueObject, loc);
-      for (Object listElement : listValue) {
-        variables.assign(env, loc, listElement);
-        evalStep(env, result, step);
-      }
-    }
-
-    @Override
-    public void validate(ValidationEnvironment env) throws EvalException {
-      variables.validate(env, getLocation());
-      list.validate(env);
-    }
-
-    @Override
-    public LValue getLValue() {
-      return variables;
-    }
-
-    @Override
-    public Expression getExpression() {
-      return list;
-    }
+  @Override
+  String printExpressions() {
+    return outputExpression.toString();
+  }
 
-    @Override
-    public String toString() {
-      return Printer.format("for %s in %r", variables.toString(), list);
-    }
+  @Override
+  OutputCollector createCollector() {
+    return new ListOutputCollector();
   }
 
   /**
-   * A if clause in a list comprehension, e.g. "if c" in the example above.
+   * Helper class that collects the intermediate results of the {@code ListComprehension} and
+   * provides access to the resulting {@code List}.
    */
-  public final class IfClause implements Clause {
-    private final Expression condition;
+  private final class ListOutputCollector implements OutputCollector {
+    private final List<Object> result;
 
-    public IfClause(Expression condition) {
-      this.condition = condition;
+    ListOutputCollector() {
+      result = new ArrayList<>();
     }
 
     @Override
-    public void eval(Environment env, List<Object> result, int step)
-        throws EvalException, InterruptedException {
-      if (EvalUtils.toBoolean(condition.eval(env))) {
-        evalStep(env, result, step);
-      }
+    public void evaluateAndCollect(Environment env) throws EvalException, InterruptedException {
+      result.add(outputExpression.eval(env));
     }
 
     @Override
-    public void validate(ValidationEnvironment env) throws EvalException {
-      condition.validate(env);
-    }
-
-    @Override
-    public LValue getLValue() {
-      return null;
-    }
-
-    @Override
-    public Expression getExpression() {
-      return condition;
-    }
-
-    @Override
-    public String toString() {
-      return String.format("if %s", condition);
-    }
-  }
-
-  private List<Clause> clauses;
-  /** The return expression, e.g. "a+d" in the example above */
-  private final Expression elementExpression;
-
-  public ListComprehension(Expression elementExpression) {
-    this.elementExpression = elementExpression;
-    clauses = new ArrayList<>();
-  }
-
-  @Override
-  public String toString() {
-    StringBuilder sb = new StringBuilder();
-    sb.append('[').append(elementExpression);
-    for (Clause clause : clauses) {
-      sb.append(' ').append(clause.toString());
-    }
-    sb.append(']');
-    return sb.toString();
-  }
-
-  public Expression getElementExpression() {
-    return elementExpression;
-  }
-
-  /**
-   * Add a new ForClause to the list comprehension. This is used only by the parser and must
-   * not be called once AST is complete.
-   * TODO(bazel-team): Remove this side-effect. Clauses should be passed to the constructor
-   * instead.
-   */
-  void addFor(Expression loopVar, Expression listExpression) {
-    Clause forClause = new ForClause(new LValue(loopVar), listExpression);
-    clauses.add(forClause);
-  }
-
-  /**
-   * Add a new ForClause to the list comprehension.
-   * TODO(bazel-team): Remove this side-effect.
-   */
-  void addIf(Expression condition) {
-    clauses.add(new IfClause(condition));
-  }
-
-  public List<Clause> getClauses() {
-    return Collections.unmodifiableList(clauses);
-  }
-
-  @Override
-  public void accept(SyntaxTreeVisitor visitor) {
-    visitor.visit(this);
-  }
-
-  @Override
-  Object doEval(Environment env) throws EvalException, InterruptedException {
-    List<Object> result = new ArrayList<>();
-    evalStep(env, result, 0);
-    return env.isSkylark() ? new MutableList(result, env) : result;
-  }
-
-  @Override
-  void validate(ValidationEnvironment env) throws EvalException {
-    for (Clause clause : clauses) {
-      clause.validate(env);
-    }
-    elementExpression.validate(env);
-  }
-
-  /**
-   * Evaluate the clause indexed by step, or elementExpression. When we evaluate the list
-   * comprehension, step is 0 and we evaluate the first clause. Each clause may
-   * recursively call evalStep any number of times. After the last clause,
-   * elementExpression is evaluated and added to the results.
-   *
-   * <p> In the expanded example above, you can consider that evalStep is equivalent to
-   * evaluating the line number step.
-   */
-  private void evalStep(Environment env, List<Object> result, int step)
-      throws EvalException, InterruptedException {
-    if (step >= clauses.size()) {
-      result.add(elementExpression.eval(env));
-    } else {
-      clauses.get(step).eval(env, result, step + 1);
+    public Object getResult(Environment env) throws EvalException {
+      return env.isSkylark() ? new MutableList(result, env) : result;
     }
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Parser.java b/src/main/java/com/google/devtools/build/lib/syntax/Parser.java
index 078efce534..fb95228a65 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Parser.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Parser.java
@@ -767,32 +767,27 @@ class Parser {
   // comprehension_suffix ::= 'FOR' loop_variables 'IN' expr comprehension_suffix
   //                        | 'IF' expr comprehension_suffix
   //                        | ']'
-  private Expression parseComprehensionSuffix(ListComprehension listComprehension) {
+  private Expression parseComprehensionSuffix(
+      AbstractComprehension comprehension, TokenKind closingBracket) {
     while (true) {
-      switch (token.kind) {
-        case FOR:
-          nextToken();
-          Expression loopVar = parseForLoopVariables();
-          expect(TokenKind.IN);
-          // The expression cannot be a ternary expression ('x if y else z') due to
-          // conflicts in Python grammar ('if' is used by the comprehension).
-          Expression listExpression = parseNonTupleExpression(0);
-          listComprehension.addFor(loopVar, listExpression);
-          break;
-
-        case IF:
-          nextToken();
-          listComprehension.addIf(parseExpression());
-          break;
-
-        case RBRACKET:
-          nextToken();
-          return listComprehension;
-
-        default:
-          syntaxError(token, "expected ']', 'for' or 'if'");
-          syncPast(LIST_TERMINATOR_SET);
-          return makeErrorExpression(token.left, token.right);
+      if (token.kind == TokenKind.FOR) {
+        nextToken();
+        Expression loopVar = parseForLoopVariables();
+        expect(TokenKind.IN);
+        // The expression cannot be a ternary expression ('x if y else z') due to
+        // conflicts in Python grammar ('if' is used by the comprehension).
+        Expression listExpression = parseNonTupleExpression(0);
+        comprehension.addFor(loopVar, listExpression);
+      } else if (token.kind == TokenKind.IF) {
+        nextToken();
+        comprehension.addIf(parseExpression());
+      } else if (token.kind == closingBracket) {
+        nextToken();
+        return comprehension;
+      } else {
+        syntaxError(token, "expected '" + closingBracket.getPrettyName() + "', 'for' or 'if'");
+        syncPast(LIST_TERMINATOR_SET);
+        return makeErrorExpression(token.left, token.right);
       }
     }
   }
@@ -820,10 +815,12 @@ class Parser {
         nextToken();
         return literal;
       }
-      case FOR: { // list comprehension
-        Expression result = parseComprehensionSuffix(new ListComprehension(expression));
-        return setLocation(result, start, token.right);
-      }
+      case FOR:
+        { // list comprehension
+          Expression result =
+              parseComprehensionSuffix(new ListComprehension(expression), TokenKind.RBRACKET);
+          return setLocation(result, start, token.right);
+        }
       case COMMA: {
         List<Expression> list = parseExprList();
         Preconditions.checkState(!list.contains(null),
@@ -861,17 +858,10 @@ class Parser {
     }
     DictionaryEntryLiteral entry = parseDictEntry();
     if (token.kind == TokenKind.FOR) {
-      // TODO(bazel-team): Reuse parseComprehensionSuffix when dict
-      // comprehension is compatible with list comprehension.
-
       // Dict comprehension
-      nextToken();
-      Expression loopVar = parseForLoopVariables();
-      expect(TokenKind.IN);
-      Expression listExpression = parseExpression();
-      expect(TokenKind.RBRACE);
-      return setLocation(new DictComprehension(
-          entry.getKey(), entry.getValue(), loopVar, listExpression), start, token.right);
+      Expression result = parseComprehensionSuffix(
+          new DictComprehension(entry.getKey(), entry.getValue()), TokenKind.RBRACE);
+      return setLocation(result, start, token.right);
     }
     List<DictionaryEntryLiteral> entries = new ArrayList<>();
     entries.add(entry);
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 a19d5ca1c0..4b9e8b0334 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
@@ -40,7 +40,7 @@ public class SyntaxTreeVisitor {
     visit(node.getValue());
   }
 
-  public void visit(Parameter<?, ?> node) {
+  public void visit(@SuppressWarnings("unused") Parameter<?, ?> node) {
     // leaf node (we need the function for overrides)
   }
 
@@ -62,11 +62,11 @@ public class SyntaxTreeVisitor {
     visitAll(node.getArguments());
   }
 
-  public void visit(Identifier node) {
-  }
+  public void visit(@SuppressWarnings("unused") Identifier node) {}
+
+  public void visit(AbstractComprehension node) {
+    visitAll(node.getOutputExpressions());
 
-  public void visit(ListComprehension node) {
-    visit(node.getElementExpression());
     for (ListComprehension.Clause clause : node.getClauses()) {
       if (clause.getLValue() != null) {
         visit(clause.getLValue());
@@ -75,13 +75,6 @@ public class SyntaxTreeVisitor {
     }
   }
 
-  public void accept(DictComprehension node) {
-    visit(node.getKeyExpression());
-    visit(node.getValueExpression());
-    visit(node.getLoopVar().getExpression());
-    visit(node.getListExpression());
-  }
-
   public void visit(ForStatement node) {
     visit(node.getVariable().getExpression());
     visit(node.getCollection());
@@ -96,11 +89,9 @@ public class SyntaxTreeVisitor {
     visitAll(node.getElements());
   }
 
-  public void visit(IntegerLiteral node) {
-  }
+  public void visit(@SuppressWarnings("unused") IntegerLiteral node) {}
 
-  public void visit(StringLiteral node) {
-  }
+  public void visit(@SuppressWarnings("unused") StringLiteral node) {}
 
   public void visit(LValue node) {
     visit(node.getExpression());
@@ -153,8 +144,7 @@ public class SyntaxTreeVisitor {
     visit(node.getField());
   }
 
-  public void visit(Comment node) {
-  }
+  public void visit(@SuppressWarnings("unused") Comment node) {}
 
   public void visit(ConditionalExpression node) {
     visit(node.getThenCase());