Misc cleanups of AST node API

- changed field names and a couple accessors to consistently use full words ("statement" instead of "stmt")

- applied several local analyzers (from IntelliJ) to remove redundant modifiers, unnecessary explicit types (yay Java 8), etc.

RELNOTES: None
PiperOrigin-RevId: 161551096

20 files changed, 185 insertions, 181 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ASTNode.java b/src/main/java/com/google/devtools/build/lib/syntax/ASTNode.java
index 18258a2d41..c2a1076c2a 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ASTNode.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ASTNode.java
@@ -25,7 +25,9 @@ import java.util.List;
  *
  * The standard {@link Object#equals} and {@link Object#hashCode} methods are not supported. This is
  * because their implementation would require traversing the entire tree in the worst case, and we
- * don't want this kind of cost to occur implicitly.
+ * don't want this kind of cost to occur implicitly. An incomplete way to compare for equality is to
+ * test whether two ASTs have the same string representation under {@link #prettyPrint()}. This
+ * might miss some metadata, but it's useful in test assertions.
  */
 public abstract class ASTNode implements Serializable {
 
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
index 12adf1a925..613deec104 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/AbstractComprehension.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/AbstractComprehension.java
@@ -48,7 +48,7 @@ public abstract class AbstractComprehension extends Expression {
   public interface Clause extends Serializable {
 
     /** Enum for distinguishing clause types. */
-    public enum Kind {
+    enum Kind {
       FOR,
       IF
     }
@@ -59,7 +59,7 @@ public abstract class AbstractComprehension extends Expression {
      * <p>This avoids having to rely on reflection, or on checking whether {@link #getLValue} is
      * null.
      */
-    public Kind getKind();
+    Kind getKind();
 
     /**
      * The evaluation of the comprehension is based on recursion. Each clause may
@@ -83,74 +83,74 @@ public abstract class AbstractComprehension extends Expression {
      * IfClause. This is needed for SyntaxTreeVisitor.
      */
     @Nullable  // for the IfClause
-    public LValue getLValue();
+    LValue getLValue();
 
     /**
      * The Expression defined in Clause, i.e. the collection for ForClause and the
      * condition for IfClause. This is needed for SyntaxTreeVisitor.
      */
-    public Expression getExpression();
+    Expression getExpression();
 
     /** Pretty print to a buffer. */
-    public void prettyPrint(Appendable buffer) throws IOException;
+    void prettyPrint(Appendable buffer) throws IOException;
   }
 
   /**
    * A for clause in a comprehension, e.g. "for a in b" in the example above.
    */
   public static final class ForClause implements Clause {
-    private final LValue variables;
-    private final Expression list;
+    private final LValue lvalue;
+    private final Expression iterable;
 
     @Override
     public Kind getKind() {
       return Kind.FOR;
     }
 
-    public ForClause(LValue variables, Expression list) {
-      this.variables = variables;
-      this.list = list;
+    public ForClause(LValue lvalue, Expression iterable) {
+      this.lvalue = lvalue;
+      this.iterable = iterable;
     }
 
     @Override
     public void eval(Environment env, OutputCollector collector, int step)
         throws EvalException, InterruptedException {
-      Object listValueObject = list.eval(env);
+      Object iterableObject = iterable.eval(env);
       Location loc = collector.getLocation();
-      Iterable<?> listValue = EvalUtils.toIterable(listValueObject, loc, env);
-      EvalUtils.lock(listValueObject, loc);
+      Iterable<?> listValue = EvalUtils.toIterable(iterableObject, loc, env);
+      EvalUtils.lock(iterableObject, loc);
       try {
         for (Object listElement : listValue) {
-          variables.assign(env, loc, listElement);
+          lvalue.assign(env, loc, listElement);
           evalStep(env, collector, step);
         }
       } finally {
-        EvalUtils.unlock(listValueObject, loc);
+        EvalUtils.unlock(iterableObject, loc);
       }
     }
 
     @Override
     public void validate(ValidationEnvironment env, Location loc) throws EvalException {
-      variables.validate(env, loc);
-      list.validate(env);
+      lvalue.validate(env, loc);
+      iterable.validate(env);
     }
 
     @Override
     public LValue getLValue() {
-      return variables;
+      return lvalue;
     }
 
     @Override
     public Expression getExpression() {
-      return list;
+      return iterable;
     }
 
     @Override
     public void prettyPrint(Appendable buffer) throws IOException {
       buffer.append("for ");
-      variables.prettyPrint(buffer);
+      lvalue.prettyPrint(buffer);
       buffer.append(" in ");
-      list.prettyPrint(buffer);
+      iterable.prettyPrint(buffer);
     }
 
     @Override
@@ -258,10 +258,10 @@ public abstract class AbstractComprehension extends Expression {
   /** Base class for comprehension builders. */
   public abstract static class AbstractBuilder {
 
-    protected List<Clause> clauses = new ArrayList<>();
+    protected final List<Clause> clauses = new ArrayList<>();
 
-    public void addFor(Expression loopVar, Expression listExpression) {
-      Clause forClause = new ForClause(new LValue(loopVar), listExpression);
+    public void addFor(LValue lvalue, Expression iterable) {
+      Clause forClause = new ForClause(lvalue, iterable);
       clauses.add(forClause);
     }
 
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java
index 6022768bee..e4e4b01970 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java
@@ -389,7 +389,7 @@ public final class BinaryOperatorExpression extends Expression {
   /**
    * Throws an exception signifying incorrect types for the given operator.
    */
-  private static final EvalException typeException(
+  private static EvalException typeException(
       Object lval, Object rval, Operator operator, Location location) {
     // NB: this message format is identical to that used by CPython 2.7.6 or 3.4.0,
     // though python raises a TypeError.
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BuildFileAST.java b/src/main/java/com/google/devtools/build/lib/syntax/BuildFileAST.java
index a0b7464d31..2d2b50ec60 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BuildFileAST.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BuildFileAST.java
@@ -41,7 +41,7 @@ import javax.annotation.Nullable;
 // does not itself extend ASTNode. This would help keep the AST minimalistic.
 public class BuildFileAST extends ASTNode {
 
-  private final ImmutableList<Statement> stmts;
+  private final ImmutableList<Statement> statements;
 
   private final ImmutableList<Comment> comments;
 
@@ -55,13 +55,13 @@ public class BuildFileAST extends ASTNode {
   @Nullable private final String contentHashCode;
 
   private BuildFileAST(
-      ImmutableList<Statement> stmts,
+      ImmutableList<Statement> statements,
       boolean containsErrors,
       String contentHashCode,
       Location location,
       ImmutableList<Comment> comments,
       @Nullable ImmutableList<SkylarkImport> imports) {
-    this.stmts = stmts;
+    this.statements = statements;
     this.containsErrors = containsErrors;
     this.contentHashCode = contentHashCode;
     this.comments = comments;
@@ -74,17 +74,18 @@ public class BuildFileAST extends ASTNode {
       ParseResult result,
       String contentHashCode,
       EventHandler eventHandler) {
-    ImmutableList<Statement> stmts =
+    ImmutableList<Statement> statements =
         ImmutableList.<Statement>builder()
             .addAll(preludeStatements)
             .addAll(result.statements)
             .build();
 
     boolean containsErrors = result.containsErrors;
-    Pair<Boolean, ImmutableList<SkylarkImport>> skylarkImports = fetchLoads(stmts, eventHandler);
+    Pair<Boolean, ImmutableList<SkylarkImport>> skylarkImports =
+        fetchLoads(statements, eventHandler);
     containsErrors |= skylarkImports.first;
     return new BuildFileAST(
-        stmts,
+        statements,
         containsErrors,
         contentHashCode,
         result.location,
@@ -97,9 +98,9 @@ public class BuildFileAST extends ASTNode {
    * {@code lastStatement} excluded.
    */
   public BuildFileAST subTree(int firstStatement, int lastStatement) {
-    ImmutableList<Statement> stmts = this.stmts.subList(firstStatement, lastStatement);
+    ImmutableList<Statement> statements = this.statements.subList(firstStatement, lastStatement);
     ImmutableList.Builder<SkylarkImport> imports = ImmutableList.builder();
-    for (Statement stmt : stmts) {
+    for (Statement stmt : statements) {
       if (stmt instanceof LoadStatement) {
         String str = ((LoadStatement) stmt).getImport().getValue();
         try {
@@ -111,11 +112,11 @@ public class BuildFileAST extends ASTNode {
       }
     }
     return new BuildFileAST(
-        stmts,
+        statements,
         containsErrors,
         null,
-        this.stmts.get(firstStatement).getLocation(),
-        ImmutableList.<Comment>of(),
+        this.statements.get(firstStatement).getLocation(),
+        ImmutableList.of(),
         imports.build());
   }
 
@@ -125,10 +126,10 @@ public class BuildFileAST extends ASTNode {
    */
   @VisibleForTesting
   static Pair<Boolean, ImmutableList<SkylarkImport>> fetchLoads(
-      List<Statement> stmts, EventHandler eventHandler) {
+      List<Statement> statements, EventHandler eventHandler) {
     ImmutableList.Builder<SkylarkImport> imports = ImmutableList.builder();
     boolean error = false;
-    for (Statement stmt : stmts) {
+    for (Statement stmt : statements) {
       if (stmt instanceof LoadStatement) {
         String importString = ((LoadStatement) stmt).getImport().getValue();
         try {
@@ -155,7 +156,7 @@ public class BuildFileAST extends ASTNode {
    * Returns an (immutable, ordered) list of statements in this BUILD file.
    */
   public ImmutableList<Statement> getStatements() {
-    return stmts;
+    return statements;
   }
 
   /**
@@ -174,7 +175,7 @@ public class BuildFileAST extends ASTNode {
   /** Returns a list of loads as strings in this BUILD file. */
   public ImmutableList<StringLiteral> getRawImports() {
     ImmutableList.Builder<StringLiteral> imports = ImmutableList.builder();
-    for (Statement stmt : stmts) {
+    for (Statement stmt : statements) {
       if (stmt instanceof LoadStatement) {
         imports.add(((LoadStatement) stmt).getImport());
       }
@@ -199,7 +200,7 @@ public class BuildFileAST extends ASTNode {
    */
   public boolean exec(Environment env, EventHandler eventHandler) throws InterruptedException {
     boolean ok = true;
-    for (Statement stmt : stmts) {
+    for (Statement stmt : statements) {
       if (!execTopLevelStatement(stmt, env, eventHandler)) {
         ok = false;
       }
@@ -249,14 +250,14 @@ public class BuildFileAST extends ASTNode {
   @Override
   public void prettyPrint(Appendable buffer, int indentLevel) throws IOException {
     // Only statements are printed, not comments and processed import data.
-    for (Statement stmt : stmts) {
+    for (Statement stmt : statements) {
       stmt.prettyPrint(buffer, indentLevel);
     }
   }
 
   @Override
   public String toString() {
-    return "<BuildFileAST with " + stmts.size() + " statements>";
+    return "<BuildFileAST with " + statements.size() + " statements>";
   }
 
   @Override
@@ -296,7 +297,7 @@ public class BuildFileAST extends ASTNode {
     ParserInputSource input = ParserInputSource.create(file, fileSize);
     Parser.ParseResult result = Parser.parseFile(input, eventHandler, SKYLARK);
     return create(
-        ImmutableList.<Statement>of(), result,
+        ImmutableList.of(), result,
         HashCode.fromBytes(file.getDigest()).toString(), eventHandler);
   }
 
@@ -328,11 +329,11 @@ public class BuildFileAST extends ASTNode {
    * @return a new AST (or the same), with the containsErrors flag updated.
    */
   public BuildFileAST validate(Environment env, EventHandler eventHandler) {
-    boolean valid = ValidationEnvironment.validateAst(env, stmts, eventHandler);
+    boolean valid = ValidationEnvironment.validateAst(env, statements, eventHandler);
     if (valid || containsErrors) {
       return this;
     }
-    return new BuildFileAST(stmts, true, contentHashCode, getLocation(), comments, imports);
+    return new BuildFileAST(statements, true, contentHashCode, getLocation(), comments, imports);
   }
 
   private static BuildFileAST parseString(
@@ -340,7 +341,7 @@ public class BuildFileAST extends ASTNode {
     String str = Joiner.on("\n").join(content);
     ParserInputSource input = ParserInputSource.create(str, PathFragment.EMPTY_FRAGMENT);
     Parser.ParseResult result = Parser.parseFile(input, eventHandler, dialect);
-    return create(ImmutableList.<Statement>of(), result, null, eventHandler);
+    return create(ImmutableList.of(), result, null, eventHandler);
   }
 
   public static BuildFileAST parseBuildString(EventHandler eventHandler, String... content) {
@@ -367,7 +368,7 @@ public class BuildFileAST extends ASTNode {
    */
   @Nullable public Object eval(Environment env) throws EvalException, InterruptedException {
     Object last = null;
-    for (Statement statement : stmts) {
+    for (Statement statement : statements) {
       if (statement instanceof ExpressionStatement) {
         last = ((ExpressionStatement) statement).getExpression().eval(env);
       } else {
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 fca62c3193..040485f814 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
@@ -98,7 +98,7 @@ public final class DictComprehension extends AbstractComprehension {
 
     DictOutputCollector(Environment env) {
       // We want to keep the iteration order
-      result = SkylarkDict.<Object, Object>of(env);
+      result = SkylarkDict.of(env);
     }
 
     @Override
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/DictionaryLiteral.java b/src/main/java/com/google/devtools/build/lib/syntax/DictionaryLiteral.java
index 56a62ac2de..8795fe0192 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/DictionaryLiteral.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/DictionaryLiteral.java
@@ -63,12 +63,12 @@ public final class DictionaryLiteral extends Expression {
 
   /** A new literal for an empty dictionary, onto which a new location can be specified */
   public static DictionaryLiteral emptyDict() {
-    return new DictionaryLiteral(ImmutableList.<DictionaryEntryLiteral>of());
+    return new DictionaryLiteral(ImmutableList.of());
   }
 
   @Override
   Object doEval(Environment env) throws EvalException, InterruptedException {
-    SkylarkDict<Object, Object> dict = SkylarkDict.<Object, Object>of(env);
+    SkylarkDict<Object, Object> dict = SkylarkDict.of(env);
     Location loc = getLocation();
     for (DictionaryEntryLiteral entry : entries) {
       Object key = entry.key.eval(env);
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java
index 9333e29f6a..e04bdf0538 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java
@@ -23,17 +23,17 @@ import java.util.Optional;
 /** Syntax node for a dot expression. e.g. obj.field, but not obj.method() */
 public final class DotExpression extends Expression {
 
-  private final Expression obj;
+  private final Expression object;
 
   private final Identifier field;
 
-  public DotExpression(Expression obj, Identifier field) {
-    this.obj = obj;
+  public DotExpression(Expression object, Identifier field) {
+    this.object = object;
     this.field = field;
   }
 
-  public Expression getObj() {
-    return obj;
+  public Expression getObject() {
+    return object;
   }
 
   public Identifier getField() {
@@ -42,14 +42,14 @@ public final class DotExpression extends Expression {
 
   @Override
   public void prettyPrint(Appendable buffer) throws IOException {
-    obj.prettyPrint(buffer);
+    object.prettyPrint(buffer);
     buffer.append('.');
     field.prettyPrint(buffer);
   }
 
   @Override
   Object doEval(Environment env) throws EvalException, InterruptedException {
-    Object objValue = obj.eval(env);
+    Object objValue = object.eval(env);
     String name = field.getName();
     Object result = eval(objValue, name, getLocation(), env);
     return checkResult(objValue, result, name, getLocation());
@@ -127,6 +127,6 @@ public final class DotExpression extends Expression {
 
   @Override
   void validate(ValidationEnvironment env) throws EvalException {
-    obj.validate(env);
+    object.validate(env);
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ExpressionStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/ExpressionStatement.java
index b6fc4c822b..c739dfaf1e 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ExpressionStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ExpressionStatement.java
@@ -21,26 +21,26 @@ import java.io.IOException;
  */
 public final class ExpressionStatement extends Statement {
 
-  private final Expression expr;
+  private final Expression expression;
 
-  public ExpressionStatement(Expression expr) {
-    this.expr = expr;
+  public ExpressionStatement(Expression expression) {
+    this.expression = expression;
   }
 
   public Expression getExpression() {
-    return expr;
+    return expression;
   }
 
   @Override
   public void prettyPrint(Appendable buffer, int indentLevel) throws IOException {
     printIndent(buffer, indentLevel);
-    expr.prettyPrint(buffer);
+    expression.prettyPrint(buffer);
     buffer.append('\n');
   }
 
   @Override
   void doExec(Environment env) throws EvalException, InterruptedException {
-    expr.eval(env);
+    expression.eval(env);
   }
 
   @Override
@@ -50,6 +50,6 @@ public final class ExpressionStatement extends Statement {
 
   @Override
   void validate(ValidationEnvironment env) throws EvalException {
-    expr.validate(env);
+    expression.validate(env);
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ForStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/ForStatement.java
index 8ac89bdd38..de87f9b2c4 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ForStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ForStatement.java
@@ -48,7 +48,7 @@ public final class ForStatement extends Statement {
     return collection;
   }
 
-  public ImmutableList<Statement> block() {
+  public ImmutableList<Statement> getBlock() {
     return block;
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
index 3ab4e4d71a..ab68d65b9e 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
@@ -186,24 +186,24 @@ public final class FuncallExpression extends Expression {
     }
   }
 
-  @Nullable private final Expression obj;
+  @Nullable private final Expression object;
 
-  private final Identifier func;
+  private final Identifier function;
 
-  private final List<Argument.Passed> args;
+  private final List<Argument.Passed> arguments;
 
   private final int numPositionalArgs;
 
-  public FuncallExpression(@Nullable Expression obj, Identifier func,
-                           List<Argument.Passed> args) {
-    this.obj = obj;
-    this.func = func;
-    this.args = args; // we assume the parser validated it with Argument#validateFuncallArguments()
+  public FuncallExpression(@Nullable Expression object, Identifier function,
+                           List<Argument.Passed> arguments) {
+    this.object = object;
+    this.function = function;
+    this.arguments = arguments;
     this.numPositionalArgs = countPositionalArguments();
   }
 
-  public FuncallExpression(Identifier func, List<Argument.Passed> args) {
-    this(null, func, args);
+  public FuncallExpression(Identifier function, List<Argument.Passed> arguments) {
+    this(null, function, arguments);
   }
 
   /**
@@ -211,7 +211,7 @@ public final class FuncallExpression extends Expression {
    */
   private int countPositionalArguments() {
     int num = 0;
-    for (Argument.Passed arg : args) {
+    for (Argument.Passed arg : arguments) {
       if (arg.isPositional()) {
         num++;
       }
@@ -223,15 +223,16 @@ public final class FuncallExpression extends Expression {
    * Returns the function expression.
    */
   public Identifier getFunction() {
-    return func;
+    return function;
   }
 
   /**
    * Returns the object the function called on.
    * It's null if the function is not called on an object.
    */
+  @Nullable
   public Expression getObject() {
-    return obj;
+    return object;
   }
 
   /**
@@ -240,7 +241,7 @@ public final class FuncallExpression extends Expression {
    * getNumPositionalArguments().
    */
   public List<Argument.Passed> getArguments() {
-    return Collections.unmodifiableList(args);
+    return Collections.unmodifiableList(arguments);
   }
 
   /**
@@ -253,14 +254,14 @@ public final class FuncallExpression extends Expression {
 
    @Override
    public void prettyPrint(Appendable buffer) throws IOException {
-     if (obj != null) {
-       obj.prettyPrint(buffer);
+     if (object != null) {
+       object.prettyPrint(buffer);
        buffer.append('.');
      }
-     func.prettyPrint(buffer);
+     function.prettyPrint(buffer);
      buffer.append('(');
      String sep = "";
-     for (Argument.Passed arg : args) {
+     for (Argument.Passed arg : arguments) {
        buffer.append(sep);
        arg.prettyPrint(buffer);
        sep = ", ";
@@ -271,11 +272,11 @@ public final class FuncallExpression extends Expression {
   @Override
   public String toString() {
     Printer.LengthLimitedPrinter printer = new Printer.LengthLimitedPrinter();
-    if (obj != null) {
-      printer.append(obj.toString()).append(".");
+    if (object != null) {
+      printer.append(object.toString()).append(".");
     }
-    printer.append(func.toString());
-    printer.printAbbreviatedList(args, "(", ", ", ")", null,
+    printer.append(function.toString());
+    printer.printAbbreviatedList(arguments, "(", ", ", ")", null,
         Printer.SUGGESTED_CRITICAL_LIST_ELEMENTS_COUNT,
         Printer.SUGGESTED_CRITICAL_LIST_ELEMENTS_STRING_LENGTH);
     return printer.toString();
@@ -374,9 +375,7 @@ public final class FuncallExpression extends Expression {
           argumentListConversionResult = convertArgumentList(args, kwargs, method);
           if (argumentListConversionResult.getArguments() != null) {
             if (matchingMethod == null) {
-              matchingMethod =
-                  new Pair<MethodDescriptor, List<Object>>(
-                      method, argumentListConversionResult.getArguments());
+              matchingMethod = new Pair<>(method, argumentListConversionResult.getArguments());
             } else {
               throw new EvalException(
                   getLocation(),
@@ -516,7 +515,7 @@ public final class FuncallExpression extends Expression {
 
   private String formatMethod(List<Object> args, Map<String, Object> kwargs) {
     StringBuilder sb = new StringBuilder();
-    sb.append(func.getName()).append("(");
+    sb.append(function.getName()).append("(");
     boolean first = true;
     for (Object obj : args) {
       if (!first) {
@@ -643,7 +642,7 @@ public final class FuncallExpression extends Expression {
         positionalArgs = positionals;
       }
       return function.call(
-          positionalArgs, ImmutableMap.<String, Object>copyOf(keyWordArgs), call, env);
+          positionalArgs, ImmutableMap.copyOf(keyWordArgs), call, env);
     } else if (fieldValue != null) {
       if (!(fieldValue instanceof BaseFunction)) {
         throw new EvalException(
@@ -651,10 +650,10 @@ public final class FuncallExpression extends Expression {
       }
       function = (BaseFunction) fieldValue;
       return function.call(
-          positionalArgs, ImmutableMap.<String, Object>copyOf(keyWordArgs), call, env);
+          positionalArgs, ImmutableMap.copyOf(keyWordArgs), call, env);
     } else {
       // When calling a Java method, the name is not in the Environment,
-      // so evaluating 'func' would fail.
+      // so evaluating 'function' would fail.
       Class<?> objClass;
       Object obj;
       if (value instanceof Class<?>) {
@@ -697,7 +696,7 @@ public final class FuncallExpression extends Expression {
     // or star arguments, because the argument list was already validated by
     // Argument#validateFuncallArguments, as called by the Parser,
     // which should be the only place that build FuncallExpression-s.
-    for (Argument.Passed arg : args) {
+    for (Argument.Passed arg : arguments) {
       Object value = arg.getValue().eval(env);
       if (arg.isPositional()) {
         posargs.add(value);
@@ -714,7 +713,7 @@ public final class FuncallExpression extends Expression {
         addKeywordArg(kwargs, arg.getName(), value, duplicates);
       }
     }
-    checkDuplicates(duplicates, func.getName(), getLocation());
+    checkDuplicates(duplicates, function.getName(), getLocation());
   }
 
   @VisibleForTesting
@@ -725,14 +724,14 @@ public final class FuncallExpression extends Expression {
 
   @Override
   Object doEval(Environment env) throws EvalException, InterruptedException {
-    return (obj != null) ? invokeObjectMethod(env) : invokeGlobalFunction(env);
+    return (object != null) ? invokeObjectMethod(env) : invokeGlobalFunction(env);
   }
 
   /**
-   * Invokes obj.func() and returns the result.
+   * Invokes object.function() and returns the result.
    */
   private Object invokeObjectMethod(Environment env) throws EvalException, InterruptedException {
-    Object objValue = obj.eval(env);
+    Object objValue = object.eval(env);
     ImmutableList.Builder<Object> posargs = new ImmutableList.Builder<>();
     posargs.add(objValue);
     // We copy this into an ImmutableMap in the end, but we can't use an ImmutableMap.Builder, or
@@ -740,14 +739,14 @@ public final class FuncallExpression extends Expression {
     Map<String, Object> kwargs = new LinkedHashMap<>();
     evalArguments(posargs, kwargs, env);
     return invokeObjectMethod(
-        func.getName(), posargs.build(), ImmutableMap.<String, Object>copyOf(kwargs), this, env);
+        function.getName(), posargs.build(), ImmutableMap.copyOf(kwargs), this, env);
   }
 
   /**
-   * Invokes func() and returns the result.
+   * Invokes function() and returns the result.
    */
   private Object invokeGlobalFunction(Environment env) throws EvalException, InterruptedException {
-    Object funcValue = func.eval(env);
+    Object funcValue = function.eval(env);
     return callFunction(funcValue, env);
   }
 
@@ -771,7 +770,7 @@ public final class FuncallExpression extends Expression {
    */
   @Nullable
   public String getNameArg() {
-    for (Argument.Passed arg : args) {
+    for (Argument.Passed arg : arguments) {
       if (arg != null) {
         String name = arg.getName();
         if (name != null && name.equals("name")) {
@@ -790,12 +789,12 @@ public final class FuncallExpression extends Expression {
 
   @Override
   void validate(ValidationEnvironment env) throws EvalException {
-    if (obj != null) {
-      obj.validate(env);
+    if (object != null) {
+      object.validate(env);
     } else {
-      func.validate(env);
+      function.validate(env);
     }
-    for (Argument.Passed arg : args) {
+    for (Argument.Passed arg : arguments) {
       arg.getValue().validate(env);
     }
   }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/FunctionDefStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/FunctionDefStatement.java
index 881368bf15..c8c388b231 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/FunctionDefStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/FunctionDefStatement.java
@@ -23,16 +23,16 @@ import java.util.List;
  */
 public final class FunctionDefStatement extends Statement {
 
-  private final Identifier ident;
+  private final Identifier identifier;
   private final FunctionSignature.WithValues<Expression, Expression> signature;
   private final ImmutableList<Statement> statements;
   private final ImmutableList<Parameter<Expression, Expression>> parameters;
 
-  public FunctionDefStatement(Identifier ident,
+  public FunctionDefStatement(Identifier identifier,
       Iterable<Parameter<Expression, Expression>> parameters,
       FunctionSignature.WithValues<Expression, Expression> signature,
       Iterable<Statement> statements) {
-    this.ident = ident;
+    this.identifier = identifier;
     this.parameters = ImmutableList.copyOf(parameters);
     this.signature = signature;
     this.statements = ImmutableList.copyOf(statements);
@@ -42,7 +42,6 @@ public final class FunctionDefStatement extends Statement {
   void doExec(Environment env) throws EvalException, InterruptedException {
     List<Expression> defaultExpressions = signature.getDefaultValues();
     ArrayList<Object> defaultValues = null;
-    ArrayList<SkylarkType> types = null;
 
     if (defaultExpressions != null) {
       defaultValues = new ArrayList<>(defaultExpressions.size());
@@ -61,10 +60,10 @@ public final class FunctionDefStatement extends Statement {
     }
 
     env.update(
-        ident.getName(),
+        identifier.getName(),
         new UserDefinedFunction(
-            ident,
-            FunctionSignature.WithValues.<Object, SkylarkType>create(sig, defaultValues, types),
+            identifier,
+            FunctionSignature.WithValues.create(sig, defaultValues, /*types=*/null),
             statements,
             env.getGlobals()));
   }
@@ -73,7 +72,7 @@ public final class FunctionDefStatement extends Statement {
   public void prettyPrint(Appendable buffer, int indentLevel) throws IOException {
     printIndent(buffer, indentLevel);
     buffer.append("def ");
-    ident.prettyPrint(buffer);
+    identifier.prettyPrint(buffer);
     buffer.append('(');
     String sep = "";
     for (Parameter<?, ?> param : parameters) {
@@ -87,11 +86,11 @@ public final class FunctionDefStatement extends Statement {
 
   @Override
   public String toString() {
-    return "def " + ident + "(" + signature + "): ...\n";
+    return "def " + identifier + "(" + signature + "): ...\n";
   }
 
-  public Identifier getIdent() {
-    return ident;
+  public Identifier getIdentifier() {
+    return identifier;
   }
 
   public ImmutableList<Statement> getStatements() {
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/IfStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/IfStatement.java
index 2c0915bffd..e6c8c23c16 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/IfStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/IfStatement.java
@@ -25,20 +25,23 @@ public final class IfStatement extends Statement {
 
   /**
    * Syntax node for an [el]if statement.
+   *
+   * <p>This extends Statement because it implements {@code doExec} and {@code validate}, but it
+   * is not actually an independent statement in the grammar.
    */
   public static final class ConditionalStatements extends Statement {
 
     private final Expression condition;
-    private final ImmutableList<Statement> stmts;
+    private final ImmutableList<Statement> statements;
 
-    public ConditionalStatements(Expression condition, List<Statement> stmts) {
+    public ConditionalStatements(Expression condition, List<Statement> statements) {
       this.condition = Preconditions.checkNotNull(condition);
-      this.stmts = ImmutableList.copyOf(stmts);
+      this.statements = ImmutableList.copyOf(statements);
     }
 
     @Override
     void doExec(Environment env) throws EvalException, InterruptedException {
-      for (Statement stmt : stmts) {
+      for (Statement stmt : statements) {
         stmt.exec(env);
       }
     }
@@ -51,7 +54,7 @@ public final class IfStatement extends Statement {
 
     @Override
     public String toString() {
-      return "[el]if " + condition + ": " + stmts + "\n";
+      return "[el]if " + condition + ": " + statements + "\n";
     }
 
     @Override
@@ -63,14 +66,14 @@ public final class IfStatement extends Statement {
       return condition;
     }
 
-    public ImmutableList<Statement> getStmts() {
-      return stmts;
+    public ImmutableList<Statement> getStatements() {
+      return statements;
     }
 
     @Override
     void validate(ValidationEnvironment env) throws EvalException {
       condition.validate(env);
-      validateStmts(env, stmts);
+      validateStmts(env, statements);
     }
   }
 
@@ -104,7 +107,7 @@ public final class IfStatement extends Statement {
       buffer.append(clauseWord);
       condStmt.getCondition().prettyPrint(buffer);
       buffer.append(":\n");
-      printSuite(buffer, condStmt.getStmts(), indentLevel);
+      printSuite(buffer, condStmt.getStatements(), indentLevel);
       clauseWord = "elif ";
     }
     if (!elseBlock.isEmpty()) {
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/IndexExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/IndexExpression.java
index 6d46e710d8..d77ccb9d9d 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/IndexExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/IndexExpression.java
@@ -19,17 +19,17 @@ import java.io.IOException;
 /** Syntax node for an index expression. e.g. obj[field], but not obj[from:to] */
 public final class IndexExpression extends Expression {
 
-  private final Expression obj;
+  private final Expression object;
 
   private final Expression key;
 
-  public IndexExpression(Expression obj, Expression key) {
-    this.obj = obj;
+  public IndexExpression(Expression object, Expression key) {
+    this.object = object;
     this.key = key;
   }
 
   public Expression getObject() {
-    return obj;
+    return object;
   }
 
   public Expression getKey() {
@@ -38,7 +38,7 @@ public final class IndexExpression extends Expression {
 
   @Override
   public void prettyPrint(Appendable buffer) throws IOException {
-    obj.prettyPrint(buffer);
+    object.prettyPrint(buffer);
     buffer.append('[');
     key.prettyPrint(buffer);
     buffer.append(']');
@@ -46,7 +46,7 @@ public final class IndexExpression extends Expression {
 
   @Override
   Object doEval(Environment env) throws EvalException, InterruptedException {
-    Object objValue = obj.eval(env);
+    Object objValue = object.eval(env);
     return eval(env, objValue);
   }
 
@@ -81,6 +81,6 @@ public final class IndexExpression extends Expression {
 
   @Override
   void validate(ValidationEnvironment env) throws EvalException {
-    obj.validate(env);
+    object.validate(env);
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ListLiteral.java b/src/main/java/com/google/devtools/build/lib/syntax/ListLiteral.java
index 47a6ea646d..6d5b9a16ce 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ListLiteral.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ListLiteral.java
@@ -32,28 +32,28 @@ public final class ListLiteral extends Expression {
   /**
    * Types of the ListLiteral.
    */
-  public static enum Kind {LIST, TUPLE}
+  public enum Kind {LIST, TUPLE}
 
   private final Kind kind;
 
-  private final List<Expression> exprs;
+  private final List<Expression> elements;
 
-  public ListLiteral(Kind kind, List<Expression> exprs) {
+  public ListLiteral(Kind kind, List<Expression> elements) {
     this.kind = kind;
-    this.exprs = exprs;
+    this.elements = elements;
   }
 
-  public static ListLiteral makeList(List<Expression> exprs) {
-    return new ListLiteral(Kind.LIST, exprs);
+  public static ListLiteral makeList(List<Expression> elements) {
+    return new ListLiteral(Kind.LIST, elements);
   }
 
-  public static ListLiteral makeTuple(List<Expression> exprs) {
-    return new ListLiteral(Kind.TUPLE, exprs);
+  public static ListLiteral makeTuple(List<Expression> elements) {
+    return new ListLiteral(Kind.TUPLE, elements);
   }
 
   /** A new literal for an empty list, onto which a new location can be specified */
   public static ListLiteral emptyList() {
-    return makeList(Collections.<Expression>emptyList());
+    return makeList(Collections.emptyList());
   }
 
   public Kind getKind() {
@@ -61,7 +61,7 @@ public final class ListLiteral extends Expression {
   }
 
   public List<Expression> getElements() {
-    return exprs;
+    return elements;
   }
 
   /**
@@ -75,12 +75,12 @@ public final class ListLiteral extends Expression {
   public void prettyPrint(Appendable buffer) throws IOException {
     buffer.append(isTuple() ? '(' : '[');
     String sep = "";
-    for (Expression e : exprs) {
+    for (Expression e : elements) {
       buffer.append(sep);
       e.prettyPrint(buffer);
       sep = ", ";
     }
-    if (isTuple() && exprs.size() == 1) {
+    if (isTuple() && elements.size() == 1) {
       buffer.append(',');
     }
     buffer.append(isTuple() ? ')' : ']');
@@ -89,7 +89,7 @@ public final class ListLiteral extends Expression {
   @Override
   public String toString() {
     return Printer.printAbbreviatedList(
-        exprs,
+        elements,
         isTuple(),
         Printer.SUGGESTED_CRITICAL_LIST_ELEMENTS_COUNT,
         Printer.SUGGESTED_CRITICAL_LIST_ELEMENTS_STRING_LENGTH);
@@ -97,15 +97,15 @@ public final class ListLiteral extends Expression {
 
   @Override
   Object doEval(Environment env) throws EvalException, InterruptedException {
-    List<Object> result = new ArrayList<>(exprs.size());
-    for (Expression expr : exprs) {
+    List<Object> result = new ArrayList<>(elements.size());
+    for (Expression element : elements) {
       // Convert NPEs to EvalExceptions.
-      if (expr == null) {
+      if (element == null) {
         throw new EvalException(getLocation(), "null expression in " + this);
       }
-      result.add(expr.eval(env));
+      result.add(element.eval(env));
     }
-    return isTuple() ? Tuple.copyOf(result) : new MutableList(result, env);
+    return isTuple() ? Tuple.copyOf(result) : new MutableList<>(result, env);
   }
 
   @Override
@@ -115,8 +115,8 @@ public final class ListLiteral extends Expression {
 
   @Override
   void validate(ValidationEnvironment env) throws EvalException {
-    for (Expression expr : exprs) {
-      expr.validate(env);
+    for (Expression element : elements) {
+      element.validate(env);
     }
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Parameter.java b/src/main/java/com/google/devtools/build/lib/syntax/Parameter.java
index 5e16ea3f4e..29e122c02d 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Parameter.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Parameter.java
@@ -30,7 +30,7 @@ import javax.annotation.Nullable;
  */
 public abstract class Parameter<V, T> extends Argument {
 
-  @Nullable protected String name;
+  @Nullable protected final String name;
   @Nullable protected final T type;
 
   private Parameter(@Nullable String name, @Nullable T type) {
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 7bc94c197b..eea376e3da 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
@@ -425,7 +425,7 @@ public class Parser {
 
   // Convenience wrapper around ASTNode.setLocation that returns the node.
   private <NODE extends ASTNode> NODE setLocation(NODE node, Location location) {
-    return ASTNode.<NODE>setLocation(location, node);
+    return ASTNode.setLocation(location, node);
   }
 
   // Another convenience wrapper method around ASTNode.setLocation
@@ -681,7 +681,7 @@ public class Parser {
           nextToken();
           // check for the empty tuple literal
           if (token.kind == TokenKind.RPAREN) {
-            ListLiteral literal = ListLiteral.makeTuple(Collections.<Expression>emptyList());
+            ListLiteral literal = ListLiteral.makeTuple(Collections.emptyList());
             setLocation(literal, start, token.right);
             nextToken();
             return literal;
@@ -811,12 +811,12 @@ public class Parser {
     while (true) {
       if (token.kind == TokenKind.FOR) {
         nextToken();
-        Expression loopVar = parseForLoopVariables();
+        Expression lhs = 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);
-        comprehensionBuilder.addFor(loopVar, listExpression);
+        comprehensionBuilder.addFor(new LValue(lhs), listExpression);
       } else if (token.kind == TokenKind.IF) {
         nextToken();
         // [x for x in li if 1, 2]  # parse error
@@ -1312,11 +1312,11 @@ public class Parser {
   private FunctionSignature.WithValues<Expression, Expression> functionSignature(
       List<Parameter<Expression, Expression>> parameters) {
     try {
-      return FunctionSignature.WithValues.<Expression, Expression>of(parameters);
+      return FunctionSignature.WithValues.of(parameters);
     } catch (FunctionSignature.SignatureException e) {
       reportError(e.getParameter().getLocation(), e.getMessage());
       // return bogus empty signature
-      return FunctionSignature.WithValues.<Expression, Expression>create(FunctionSignature.of());
+      return FunctionSignature.WithValues.create(FunctionSignature.of());
     }
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ReturnStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/ReturnStatement.java
index 3c8ac34d9b..ed5ffabc5a 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ReturnStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ReturnStatement.java
@@ -25,14 +25,14 @@ public final class ReturnStatement extends Statement {
    * Exception sent by the return statement, to be caught by the function body.
    */
   public static class ReturnException extends EvalException {
-    Object value;
+    private final Object value;
 
     public ReturnException(Location location, Object value) {
       super(
           location,
           "return statements must be inside a function",
-          /*dueToIncompleteAST=*/ false, /*fillInJavaStackTrace=*/
-          false);
+          /*dueToIncompleteAST=*/false,
+          /*fillInJavaStackTrace=*/false);
       this.value = value;
     }
 
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SliceExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/SliceExpression.java
index 92d51fe8b4..48768eab31 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SliceExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SliceExpression.java
@@ -17,23 +17,23 @@ import com.google.devtools.build.lib.events.Location;
 import java.io.IOException;
 import java.util.List;
 
-/** Syntax node for an index expression. e.g. obj[field], but not obj[from:to] */
+/** Syntax node for a slice expression, e.g. obj[:len(obj):2]. */
 public final class SliceExpression extends Expression {
 
-  private final Expression obj;
+  private final Expression object;
   private final Expression start;
   private final Expression end;
   private final Expression step;
 
-  public SliceExpression(Expression obj, Expression start, Expression end, Expression step) {
-    this.obj = obj;
+  public SliceExpression(Expression object, Expression start, Expression end, Expression step) {
+    this.object = object;
     this.start = start;
     this.end = end;
     this.step = step;
   }
 
   public Expression getObject() {
-    return obj;
+    return object;
   }
 
   public Expression getStart() {
@@ -57,7 +57,7 @@ public final class SliceExpression extends Expression {
     boolean stepIsDefault =
         (step instanceof IntegerLiteral) && ((IntegerLiteral) step).getValue().equals(1);
 
-    obj.prettyPrint(buffer);
+    object.prettyPrint(buffer);
     buffer.append('[');
     // Start and end are omitted if they are the literal identifier None, which is the default value
     // inserted by the parser if no bound is given. Likewise, step is omitted if it is the literal
@@ -81,14 +81,14 @@ public final class SliceExpression extends Expression {
 
   @Override
   Object doEval(Environment env) throws EvalException, InterruptedException {
-    Object objValue = obj.eval(env);
+    Object objValue = object.eval(env);
     Object startValue = start.eval(env);
     Object endValue = end.eval(env);
     Object stepValue = step.eval(env);
     Location loc = getLocation();
 
     if (objValue instanceof SkylarkList) {
-      SkylarkList<Object> list = (SkylarkList<Object>) objValue;
+      SkylarkList<?> list = (SkylarkList<?>) objValue;
       Object slice = list.getSlice(startValue, endValue, stepValue, loc);
       return SkylarkType.convertToSkylark(slice, env);
     } else if (objValue instanceof String) {
@@ -122,6 +122,6 @@ public final class SliceExpression extends Expression {
 
   @Override
   void validate(ValidationEnvironment env) throws EvalException {
-    obj.validate(env);
+    object.validate(env);
   }
 }
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 5ef0e177e3..8229e393e5 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,7 +77,7 @@ public class SyntaxTreeVisitor {
   public void visit(ForStatement node) {
     visit(node.getVariable().getExpression());
     visit(node.getCollection());
-    visitAll(node.block());
+    visitAll(node.getBlock());
   }
 
   public void visit(LoadStatement node) {
@@ -117,11 +117,11 @@ public class SyntaxTreeVisitor {
 
   public void visit(ConditionalStatements node) {
     visit(node.getCondition());
-    visitAll(node.getStmts());
+    visitAll(node.getStatements());
   }
 
   public void visit(FunctionDefStatement node) {
-    visit(node.getIdent());
+    visit(node.getIdentifier());
     visitAll(node.getParameters());
     visitAll(node.getStatements());
   }
@@ -144,7 +144,7 @@ public class SyntaxTreeVisitor {
   }
 
   public void visit(DotExpression node) {
-    visit(node.getObj());
+    visit(node.getObject());
     visit(node.getField());
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java b/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java
index bed21bcc29..46b47614d5 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java
@@ -197,7 +197,7 @@ public final class ValidationEnvironment {
     for (Statement statement : statements) {
       if (statement instanceof FunctionDefStatement) {
         FunctionDefStatement fct = (FunctionDefStatement) statement;
-        declare(fct.getIdent().getName(), fct.getLocation());
+        declare(fct.getIdentifier().getName(), fct.getLocation());
       }
     }