Renamed Ident to Identifier, added some helper methods and refactored two methods.

--
MOS_MIGRATED_REVID=98922811

10 files changed, 81 insertions, 62 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
index 210c68bb11..76f0bc0d2d 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
@@ -42,7 +42,7 @@ import com.google.devtools.build.lib.syntax.Expression;
 import com.google.devtools.build.lib.syntax.FuncallExpression;
 import com.google.devtools.build.lib.syntax.FunctionSignature;
 import com.google.devtools.build.lib.syntax.GlobList;
-import com.google.devtools.build.lib.syntax.Ident;
+import com.google.devtools.build.lib.syntax.Identifier;
 import com.google.devtools.build.lib.syntax.Label;
 import com.google.devtools.build.lib.syntax.ParserInputSource;
 import com.google.devtools.build.lib.syntax.SkylarkEnvironment;
@@ -1340,10 +1340,10 @@ public final class PackageFactory {
     for (Statement stmt : ast.getStatements()) {
       if (stmt instanceof AssignmentStatement) {
         Expression lvalue = ((AssignmentStatement) stmt).getLValue().getExpression();
-        if (!(lvalue instanceof Ident)) {
+        if (!(lvalue instanceof Identifier)) {
           continue;
         }
-        String target = ((Ident) lvalue).getName();
+        String target = ((Identifier) lvalue).getName();
         if (pkgEnv.lookup(target, null) != null) {
           eventHandler.handle(Event.error(stmt.getLocation(), "Reassignment of builtin build "
               + "function '" + target + "' not permitted"));
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 e5b06a971d..bb0e8ed54b 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
@@ -27,9 +27,9 @@ public final class DotExpression extends Expression {
 
   private final Expression obj;
 
-  private final Ident field;
+  private final Identifier field;
 
-  public DotExpression(Expression obj, Ident field) {
+  public DotExpression(Expression obj, Identifier field) {
     this.obj = obj;
     this.field = field;
   }
@@ -38,7 +38,7 @@ public final class DotExpression extends Expression {
     return obj;
   }
 
-  public Ident getField() {
+  public Identifier getField() {
     return field;
   }
 
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 21df356d12..a44832537a 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
@@ -169,7 +169,7 @@ public final class FuncallExpression extends Expression {
 
   private final Expression obj;
 
-  private final Ident func;
+  private final Identifier func;
 
   private final List<Argument.Passed> args;
 
@@ -182,7 +182,7 @@ public final class FuncallExpression extends Expression {
    * arbitrary expressions. In any case, the "func" expression is always
    * evaluated, so functions and variables share a common namespace.
    */
-  public FuncallExpression(Expression obj, Ident func,
+  public FuncallExpression(Expression obj, Identifier func,
                            List<Argument.Passed> args) {
     this.obj = obj;
     this.func = func;
@@ -197,7 +197,7 @@ public final class FuncallExpression extends Expression {
    * arbitrary expressions. In any case, the "func" expression is always
    * evaluated, so functions and variables share a common namespace.
    */
-  public FuncallExpression(Ident func, List<Argument.Passed> args) {
+  public FuncallExpression(Identifier func, List<Argument.Passed> args) {
     this(null, func, args);
   }
 
@@ -217,7 +217,7 @@ public final class FuncallExpression extends Expression {
   /**
    * Returns the function expression.
    */
-  public Ident getFunction() {
+  public Identifier getFunction() {
     return func;
   }
 
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 dce9e9c4fc..f9e26edbf5 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
@@ -24,11 +24,11 @@ import java.util.List;
  */
 public class FunctionDefStatement extends Statement {
 
-  private final Ident ident;
+  private final Identifier ident;
   private final FunctionSignature.WithValues<Expression, Expression> args;
   private final ImmutableList<Statement> statements;
 
-  public FunctionDefStatement(Ident ident,
+  public FunctionDefStatement(Identifier ident,
       FunctionSignature.WithValues<Expression, Expression> args,
       Collection<Statement> statements) {
     this.ident = ident;
@@ -59,7 +59,7 @@ public class FunctionDefStatement extends Statement {
     return "def " + ident + "(" + args + "):\n";
   }
 
-  public Ident getIdent() {
+  public Identifier getIdent() {
     return ident;
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Ident.java b/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java
index 9bc58b18b5..26c5f1557f 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Ident.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java
@@ -14,30 +14,36 @@
 
 package com.google.devtools.build.lib.syntax;
 
+import javax.annotation.Nullable;
+
 // TODO(bazel-team): for extra performance:
 // (1) intern the strings, so we can use == to compare, and have .equals use the assumption.
-// Then have Argument and Parameter use Ident again instead of String as keys.
-// (2) Use Ident, not String, as keys in the Environment, which will be cleaner.
+// Then have Argument and Parameter use Identifier again instead of String as keys.
+// (2) Use Identifier, not String, as keys in the Environment, which will be cleaner.
 // (3) For performance, avoid doing HashMap lookups at runtime, and compile local variable access
 // into array reference with a constant index. Variable lookups are currently a speed bottleneck,
 // as previously measured in an experiment.
 /**
  *  Syntax node for an identifier.
  */
-public final class Ident extends Expression {
+public final class Identifier extends Expression {
 
   private final String name;
 
-  public Ident(String name) {
+  public Identifier(String name) {
     this.name = name;
   }
 
   /**
-   *  Returns the name of the Ident.
+   *  Returns the name of the Identifier.
    */
   public String getName() {
     return name;
   }
+  
+  public boolean isPrivate() {
+    return name.startsWith("_");
+  }
 
   @Override
   public String toString() {
@@ -49,12 +55,22 @@ public final class Ident extends Expression {
     try {
       return env.lookup(name);
     } catch (Environment.NoSuchVariableException e) {
-      if (name.equals("$error$")) {
-        throw new EvalException(getLocation(), "contains syntax error(s)", true);
-      } else {
-        throw new EvalException(getLocation(), "name '" + name + "' is not defined");
-      }
+      throw createInvalidIdentifierException();
+    }
+  }
+
+  @Override
+  public boolean equals(@Nullable Object object) {
+    if (object instanceof Identifier) {
+      Identifier that = (Identifier) object;
+      return this.name.equals(that.name);
     }
+    return false;
+  }
+  
+  @Override
+  public int hashCode() {
+    return name.hashCode();
   }
 
   @Override
@@ -65,11 +81,13 @@ public final class Ident extends Expression {
   @Override
   void validate(ValidationEnvironment env) throws EvalException {
     if (!env.hasSymbolInEnvironment(name)) {
-      if (name.equals("$error$")) {
-        throw new EvalException(getLocation(), "contains syntax error(s)", true);
-      } else {
-        throw new EvalException(getLocation(), "name '" + name + "' is not defined");
-      }
+      throw createInvalidIdentifierException();
     }
   }
+
+  private EvalException createInvalidIdentifierException() {
+    return name.equals("$error$")
+        ? new EvalException(getLocation(), "contains syntax error(s)", true)
+        : new EvalException(getLocation(), "name '" + name + "' is not defined");
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/LValue.java b/src/main/java/com/google/devtools/build/lib/syntax/LValue.java
index 2e9bf26e93..095a803df6 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/LValue.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/LValue.java
@@ -49,8 +49,8 @@ public class LValue implements Serializable {
 
   private static void assign(Environment env, Location loc, Expression lvalue, Object result)
       throws EvalException, InterruptedException {
-    if (lvalue instanceof Ident) {
-      assign(env, loc, (Ident) lvalue, result);
+    if (lvalue instanceof Identifier) {
+      assign(env, loc, (Identifier) lvalue, result);
       return;
     }
 
@@ -77,7 +77,7 @@ public class LValue implements Serializable {
   /**
    * Assign value to a single variable.
    */
-  private static void assign(Environment env, Location loc, Ident ident, Object result)
+  private static void assign(Environment env, Location loc, Identifier ident, Object result)
       throws EvalException, InterruptedException {
     Preconditions.checkNotNull(result, "trying to assign null to %s", ident);
 
@@ -103,8 +103,8 @@ public class LValue implements Serializable {
 
   private static void validate(ValidationEnvironment env, Location loc, Expression expr)
       throws EvalException {
-    if (expr instanceof Ident) {
-      Ident ident = (Ident) expr;
+    if (expr instanceof Identifier) {
+      Identifier ident = (Identifier) expr;
       env.declare(ident.getName(), loc);
       return;
     }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/LoadStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/LoadStatement.java
index 106b348980..0eae19bbc8 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/LoadStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/LoadStatement.java
@@ -26,20 +26,20 @@ public final class LoadStatement extends Statement {
 
   public static final String PATH_ERROR_MSG = "Path '%s' is not valid. "
       + "It should either start with a slash or refer to a file in the current directory.";
-  private final ImmutableList<Ident> symbols;
+  private final ImmutableList<Identifier> symbols;
   private final PathFragment importPath;
   private final String pathString;
 
   /**
    * Constructs an import statement.
    */
-  LoadStatement(String path, List<Ident> symbols) {
+  LoadStatement(String path, List<Identifier> symbols) {
     this.symbols = ImmutableList.copyOf(symbols);
     this.importPath = new PathFragment(path + ".bzl");
     this.pathString = path;
   }
 
-  public ImmutableList<Ident> getSymbols() {
+  public ImmutableList<Identifier> getSymbols() {
     return symbols;
   }
 
@@ -54,9 +54,9 @@ public final class LoadStatement extends Statement {
 
   @Override
   void exec(Environment env) throws EvalException, InterruptedException {
-    for (Ident i : symbols) {
+    for (Identifier i : symbols) {
       try {
-        if (i.getName().startsWith("_")) {
+        if (i.isPrivate()) {
           throw new EvalException(getLocation(), "symbol '" + i + "' is private and cannot "
               + "be imported");
         }
@@ -79,7 +79,7 @@ public final class LoadStatement extends Statement {
     if (!importPath.isAbsolute() && importPath.segmentCount() > 1) {
       throw new EvalException(getLocation(), String.format(PATH_ERROR_MSG, importPath));
     }
-    for (Ident symbol : symbols) {
+    for (Identifier symbol : symbols) {
       env.declare(symbol.getName(), getLocation());
     }
   }
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 efd40b4777..46e7ca74ce 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
@@ -388,8 +388,8 @@ class Parser {
   }
 
   // create an error expression
-  private Ident makeErrorExpression(int start, int end) {
-    return setLocation(new Ident("$error$"), start, end);
+  private Identifier makeErrorExpression(int start, int end) {
+    return setLocation(new Identifier("$error$"), start, end);
   }
 
   // Convenience wrapper around ASTNode.setLocation that returns the node.
@@ -410,7 +410,7 @@ class Parser {
   }
 
   // create a funcall expression
-  private Expression makeFuncallExpression(Expression receiver, Ident function,
+  private Expression makeFuncallExpression(Expression receiver, Identifier function,
                                            List<Argument.Passed> args,
                                            int start, int end) {
     if (function.getLocation() == null) {
@@ -474,21 +474,21 @@ class Parser {
     int start = token.left;
     if (token.kind == TokenKind.STAR_STAR) { // kwarg
       nextToken();
-      Ident ident = parseIdent();
+      Identifier ident = parseIdent();
       return setLocation(new Parameter.StarStar<Expression, Expression>(
           ident.getName()), start, ident);
     } else if (token.kind == TokenKind.STAR) { // stararg
       int end = token.right;
       nextToken();
       if (token.kind == TokenKind.IDENTIFIER) {
-        Ident ident = parseIdent();
+        Identifier ident = parseIdent();
         return setLocation(new Parameter.Star<Expression, Expression>(ident.getName()),
             start, ident);
       } else {
         return setLocation(new Parameter.Star<Expression, Expression>(null), start, end);
       }
     } else {
-      Ident ident = parseIdent();
+      Identifier ident = parseIdent();
       if (token.kind == TokenKind.EQUALS) { // there's a default value
         nextToken();
         Expression expr = parseNonTupleExpression();
@@ -502,7 +502,7 @@ class Parser {
   }
 
   // funcall_suffix ::= '(' arg_list? ')'
-  private Expression parseFuncallSuffix(int start, Expression receiver, Ident function) {
+  private Expression parseFuncallSuffix(int start, Expression receiver, Identifier function) {
     List<Argument.Passed> args = Collections.emptyList();
     expect(TokenKind.LPAREN);
     int end;
@@ -522,7 +522,7 @@ class Parser {
   private Expression parseSelectorSuffix(int start, Expression receiver) {
     expect(TokenKind.DOT);
     if (token.kind == TokenKind.IDENTIFIER) {
-      Ident ident = parseIdent();
+      Identifier ident = parseIdent();
       if (token.kind == TokenKind.LPAREN) {
         return parseFuncallSuffix(start, receiver, ident);
       } else {
@@ -599,7 +599,7 @@ class Parser {
         new StringLiteral(labelName, '"')), location));
     args.add(setLocation(new Argument.Positional(
         new StringLiteral(file, '"')), location));
-    Ident mockIdent = setLocation(new Ident("mocksubinclude"), location);
+    Identifier mockIdent = setLocation(new Identifier("mocksubinclude"), location);
     Expression funCall = new FuncallExpression(null, mockIdent, args);
     return setLocation(new ExpressionStatement(funCall), location);
   }
@@ -680,7 +680,7 @@ class Parser {
         return literal;
       }
       case IDENTIFIER: {
-        Ident ident = parseIdent();
+        Identifier ident = parseIdent();
         if (token.kind == TokenKind.LPAREN) { // it's a function application
           return parseFuncallSuffix(start, null, ident);
         } else {
@@ -720,7 +720,7 @@ class Parser {
         List<Argument.Passed> args = new ArrayList<>();
         Expression expr = parsePrimaryWithSuffix();
         args.add(setLocation(new Argument.Positional(expr), start, expr));
-        return makeFuncallExpression(null, new Ident("-"), args,
+        return makeFuncallExpression(null, new Identifier("-"), args,
                                      start, token.right);
       }
       default: {
@@ -765,7 +765,7 @@ class Parser {
     // This is a dictionary access
     if (token.kind == TokenKind.RBRACKET) {
       expect(TokenKind.RBRACKET);
-      return makeFuncallExpression(receiver, new Ident("$index"), args,
+      return makeFuncallExpression(receiver, new Identifier("$index"), args,
                                    start, token.right);
     }
     // This is a slice (or substring)
@@ -779,7 +779,7 @@ class Parser {
     expect(TokenKind.RBRACKET);
 
     args.add(setLocation(new Argument.Positional(endExpr), loc2, endExpr));
-    return makeFuncallExpression(receiver, new Ident("$slice"), args,
+    return makeFuncallExpression(receiver, new Identifier("$slice"), args,
                                  start, token.right);
   }
 
@@ -933,12 +933,12 @@ class Parser {
     return makeErrorExpression(start, end);
   }
 
-  private Ident parseIdent() {
+  private Identifier parseIdent() {
     if (token.kind != TokenKind.IDENTIFIER) {
       expect(TokenKind.IDENTIFIER);
       return makeErrorExpression(token.left, token.right);
     }
-    Ident ident = new Ident(((String) token.value));
+    Identifier ident = new Identifier(((String) token.value));
     setLocation(ident, token.left, token.right);
     nextToken();
     return ident;
@@ -1083,9 +1083,9 @@ class Parser {
     nextToken();
     expect(TokenKind.COMMA);
 
-    List<Ident> symbols = new ArrayList<>();
+    List<Identifier> symbols = new ArrayList<>();
     if (token.kind == TokenKind.STRING) {
-      symbols.add(new Ident((String) token.value));
+      symbols.add(new Identifier((String) token.value));
     }
     expect(TokenKind.STRING);
     while (token.kind != TokenKind.RPAREN && token.kind != TokenKind.EOF) {
@@ -1094,7 +1094,7 @@ class Parser {
         break;
       }
       if (token.kind == TokenKind.STRING) {
-        symbols.add(new Ident((String) token.value));
+        symbols.add(new Identifier((String) token.value));
       }
       expect(TokenKind.STRING);
     }
@@ -1122,7 +1122,7 @@ class Parser {
     // Check if there is an include
     if (token.kind == TokenKind.IDENTIFIER) {
       Token identToken = token;
-      Ident ident = parseIdent();
+      Identifier ident = parseIdent();
 
       if (ident.getName().equals("include")
           && token.kind == TokenKind.LPAREN
@@ -1200,7 +1200,8 @@ class Parser {
       Expression rvalue = parseExpression();
       if (expression instanceof FuncallExpression) {
         FuncallExpression func = (FuncallExpression) expression;
-        if (func.getFunction().getName().equals("$index") && func.getObject() instanceof Ident) {
+        if (func.getFunction().getName().equals("$index")
+            && func.getObject() instanceof Identifier) {
           // Special casing to translate 'ident[key] = value' to 'ident = ident + {key: value}'
           // Note that the locations of these extra expressions are fake.
           Preconditions.checkArgument(func.getArguments().size() == 1);
@@ -1274,7 +1275,7 @@ class Parser {
   private void parseFunctionDefStatement(List<Statement> list) {
     int start = token.left;
     expect(TokenKind.DEF);
-    Ident ident = parseIdent();
+    Identifier ident = parseIdent();
     expect(TokenKind.LPAREN);
     FunctionSignature.WithValues<Expression, Expression> args = parseFunctionSignature();
     expect(TokenKind.RPAREN);
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 3d699ffb23..67690617cd 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
@@ -55,7 +55,7 @@ public class SyntaxTreeVisitor {
     visitAll(node.getArguments());
   }
 
-  public void visit(Ident node) {
+  public void visit(Identifier node) {
   }
 
   public void visit(ListComprehension node) {
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/UserDefinedFunction.java b/src/main/java/com/google/devtools/build/lib/syntax/UserDefinedFunction.java
index 5bb9019f3c..4a5968cd36 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/UserDefinedFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/UserDefinedFunction.java
@@ -25,7 +25,7 @@ public class UserDefinedFunction extends BaseFunction {
   private final ImmutableList<Statement> statements;
   private final SkylarkEnvironment definitionEnv;
 
-  protected UserDefinedFunction(Ident function,
+  protected UserDefinedFunction(Identifier function,
       FunctionSignature.WithValues<Object, SkylarkType> signature,
       ImmutableList<Statement> statements, SkylarkEnvironment definitionEnv) {
     super(function.getName(), signature, function.getLocation());