Use Identifiers instead of Strings

The high level summary of the changes:
- use `Identifier` instead of `name` in `Keyword` and `Parameter`.
- construct `Identifier` through a factory method in case future interning is desired.

These changes are in preparation for using `Identifier` instead of `name` for environment lookups.

Closes #5304.

PiperOrigin-RevId: 199869171

6 files changed, 71 insertions, 52 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Argument.java b/src/main/java/com/google/devtools/build/lib/syntax/Argument.java
index a4d782a44f..09f51202e1 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Argument.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Argument.java
@@ -58,11 +58,18 @@ public abstract class Argument extends ASTNode {
       return false;
     }
 
+    /** @deprecated Prefer {@link #getIdentifier()} instead. */
+    @Deprecated
     @Nullable
     public String getName() { // only for keyword arguments
       return null;
     }
 
+    @Nullable
+    public Identifier getIdentifier() {
+      return null;
+    }
+
     public Expression getValue() {
       return value;
     }
@@ -94,16 +101,21 @@ public abstract class Argument extends ASTNode {
   /** keyword argument: K = Expression */
   public static final class Keyword extends Passed {
 
-    final String name;
+    final Identifier identifier;
 
-    public Keyword(String name, Expression value) {
+    public Keyword(Identifier identifier, Expression value) {
       super(value);
-      this.name = name;
+      this.identifier = identifier;
     }
 
     @Override
     public String getName() {
-      return name;
+      return identifier.getName();
+    }
+
+    @Override
+    public Identifier getIdentifier() {
+      return identifier;
     }
 
     @Override
@@ -113,7 +125,7 @@ public abstract class Argument extends ASTNode {
 
     @Override
     public void prettyPrint(Appendable buffer) throws IOException {
-      buffer.append(name);
+      buffer.append(identifier.getName());
       buffer.append(" = ");
       value.prettyPrint(buffer);
     }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Eval.java b/src/main/java/com/google/devtools/build/lib/syntax/Eval.java
index 56c98a7140..a91ffae483 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Eval.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Eval.java
@@ -148,7 +148,7 @@ public class Eval {
     for (Map.Entry<Identifier, String> entry : node.getSymbolMap().entrySet()) {
       try {
         Identifier name = entry.getKey();
-        Identifier declared = new Identifier(entry.getValue());
+        Identifier declared = Identifier.of(entry.getValue());
 
         if (declared.isPrivate()) {
           throw new EvalException(
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java b/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java
index b18da59f8a..a4e7a9ecc0 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java
@@ -117,4 +117,9 @@ public final class Identifier extends Expression {
     String suggestion = SpellChecker.didYouMean(name, symbols);
     return new EvalException(getLocation(), "name '" + name + "' is not defined" + suggestion);
   }
+
+  /** @return The {@link Identifier} of the provided name. */
+  public static Identifier of(String name) {
+    return new Identifier(name);
+  }
 }
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 e5ffc1ca57..4f499d6d09 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
@@ -31,11 +31,11 @@ import javax.annotation.Nullable;
  */
 public abstract class Parameter<V, T> extends Argument {
 
-  @Nullable protected final String name;
+  @Nullable protected final Identifier identifier;
   @Nullable protected final T type;
 
-  private Parameter(@Nullable String name, @Nullable T type) {
-    this.name = name;
+  private Parameter(@Nullable Identifier identifier, @Nullable T type) {
+    this.identifier = identifier;
     this.type = type;
   }
 
@@ -59,7 +59,12 @@ public abstract class Parameter<V, T> extends Argument {
 
   @Nullable
   public String getName() {
-    return name;
+    return identifier != null ? identifier.getName() : null;
+  }
+
+  @Nullable
+  public Identifier getIdentifier() {
+    return identifier;
   }
 
   public boolean hasName() {
@@ -80,13 +85,13 @@ public abstract class Parameter<V, T> extends Argument {
   @AutoCodec
   public static final class Mandatory<V, T> extends Parameter<V, T> {
 
-    public Mandatory(String name) {
-      this(name, null);
+    public Mandatory(Identifier identifier) {
+      this(identifier, null);
     }
 
     @AutoCodec.Instantiator
-    public Mandatory(String name, @Nullable T type) {
-      super(name, type);
+    public Mandatory(Identifier identifier, @Nullable T type) {
+      super(identifier, type);
     }
 
     @Override
@@ -96,7 +101,7 @@ public abstract class Parameter<V, T> extends Argument {
 
     @Override
     public void prettyPrint(Appendable buffer) throws IOException {
-      buffer.append(name);
+      buffer.append(getName());
     }
   }
 
@@ -106,13 +111,13 @@ public abstract class Parameter<V, T> extends Argument {
 
     public final V defaultValue;
 
-    public Optional(String name, @Nullable V defaultValue) {
-      this(name, null, defaultValue);
+    public Optional(Identifier identifier, @Nullable V defaultValue) {
+      this(identifier, null, defaultValue);
     }
 
     @AutoCodec.Instantiator
-    public Optional(String name, @Nullable T type, @Nullable V defaultValue) {
-      super(name, type);
+    public Optional(Identifier identifier, @Nullable T type, @Nullable V defaultValue) {
+      super(identifier, type);
       this.defaultValue = defaultValue;
     }
 
@@ -129,7 +134,7 @@ public abstract class Parameter<V, T> extends Argument {
 
     @Override
     public void prettyPrint(Appendable buffer) throws IOException {
-      buffer.append(name);
+      buffer.append(getName());
       buffer.append('=');
       // This should only ever be used on a parameter representing static information, i.e. with V
       // and T instantiated as Expression.
@@ -140,7 +145,7 @@ public abstract class Parameter<V, T> extends Argument {
     // parameterized with.
     @Override
     public String toString() {
-      return name + "=" + defaultValue;
+      return getName() + "=" + defaultValue;
     }
   }
 
@@ -149,17 +154,17 @@ public abstract class Parameter<V, T> extends Argument {
   public static final class Star<V, T> extends Parameter<V, T> {
 
     @AutoCodec.Instantiator
-    public Star(@Nullable String name, @Nullable T type) {
-      super(name, type);
+    public Star(@Nullable Identifier identifier, @Nullable T type) {
+      super(identifier, type);
     }
 
-    public Star(@Nullable String name) {
-      this(name, null);
+    public Star(@Nullable Identifier identifier) {
+      this(identifier, null);
     }
 
     @Override
     public boolean hasName() {
-      return name != null;
+      return getName() != null;
     }
 
     @Override
@@ -170,8 +175,8 @@ public abstract class Parameter<V, T> extends Argument {
     @Override
     public void prettyPrint(Appendable buffer) throws IOException {
       buffer.append('*');
-      if (name != null) {
-        buffer.append(name);
+      if (getName() != null) {
+        buffer.append(getName());
       }
     }
   }
@@ -181,12 +186,12 @@ public abstract class Parameter<V, T> extends Argument {
   public static final class StarStar<V, T> extends Parameter<V, T> {
 
     @AutoCodec.Instantiator
-    public StarStar(String name, @Nullable T type) {
-      super(name, type);
+    public StarStar(Identifier identifier, @Nullable T type) {
+      super(identifier, type);
     }
 
-    public StarStar(String name) {
-      this(name, null);
+    public StarStar(Identifier identifier) {
+      this(identifier, null);
     }
 
     @Override
@@ -197,11 +202,12 @@ public abstract class Parameter<V, T> extends Argument {
     @Override
     public void prettyPrint(Appendable buffer) throws IOException {
       buffer.append("**");
-      buffer.append(name);
+      buffer.append(getName());
     }
   }
 
   @Override
+  @SuppressWarnings("unchecked")
   public void accept(SyntaxTreeVisitor visitor) {
     visitor.visit((Parameter<Expression, Expression>) this);
   }
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 ec6d323f1b..d2d0af16c9 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
@@ -422,7 +422,7 @@ public class Parser {
 
   // create an error expression
   private Identifier makeErrorExpression(int start, int end) {
-    return setLocation(new Identifier("$error$"), start, end);
+    return setLocation(Identifier.of("$error$"), start, end);
   }
 
   // Convenience wrapper method around ASTNode.setLocation
@@ -461,10 +461,9 @@ public class Parser {
     if (expr instanceof Identifier) {
       // parse a named argument
       if (token.kind == TokenKind.EQUALS) {
-        String name = ((Identifier) expr).getName();
         nextToken();
         Expression val = parseNonTupleExpression();
-        return setLocation(new Argument.Keyword(name, val), start, val);
+        return setLocation(new Argument.Keyword(((Identifier) expr), val), start, val);
       }
     }
 
@@ -480,28 +479,24 @@ public class Parser {
     if (token.kind == TokenKind.STAR_STAR) { // kwarg
       nextToken();
       Identifier ident = parseIdent();
-      return setLocation(new Parameter.StarStar<Expression, Expression>(
-          ident.getName()), start, ident);
+      return setLocation(new Parameter.StarStar<>(ident), start, ident);
     } else if (token.kind == TokenKind.STAR) { // stararg
       int end = token.right;
       nextToken();
       if (token.kind == TokenKind.IDENTIFIER) {
         Identifier ident = parseIdent();
-        return setLocation(new Parameter.Star<Expression, Expression>(ident.getName()),
-            start, ident);
+        return setLocation(new Parameter.Star<>(ident), start, ident);
       } else {
-        return setLocation(new Parameter.Star<Expression, Expression>(null), start, end);
+        return setLocation(new Parameter.Star<>(null), start, end);
       }
     } else {
       Identifier ident = parseIdent();
       if (token.kind == TokenKind.EQUALS) { // there's a default value
         nextToken();
         Expression expr = parseNonTupleExpression();
-        return setLocation(new Parameter.Optional<Expression, Expression>(
-            ident.getName(), expr), start, expr);
+        return setLocation(new Parameter.Optional<>(ident, expr), start, expr);
       } else {
-        return setLocation(new Parameter.Mandatory<Expression, Expression>(
-            ident.getName()), start, ident);
+        return setLocation(new Parameter.Mandatory<>(ident), start, ident);
       }
     }
   }
@@ -896,7 +891,7 @@ public class Parser {
       expect(TokenKind.IDENTIFIER);
       return makeErrorExpression(token.left, token.right);
     }
-    Identifier ident = new Identifier(((String) token.value));
+    Identifier ident = Identifier.of(((String) token.value));
     setLocation(ident, token.left, token.right);
     nextToken();
     return ident;
@@ -1075,7 +1070,7 @@ public class Parser {
     }
 
     String name = (String) token.value;
-    Identifier identifier = new Identifier(name);
+    Identifier identifier = Identifier.of(name);
     if (symbols.containsKey(identifier)) {
       syntaxError(
           String.format("Identifier '%s' is used more than once", identifier.getName()));
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSignatureProcessor.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSignatureProcessor.java
index a4302b57ed..5d5094117d 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSignatureProcessor.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSignatureProcessor.java
@@ -63,7 +63,8 @@ public class SkylarkSignatureProcessor {
 
     for (int paramIndex = 0; paramIndex < annotation.mandatoryPositionals(); paramIndex++) {
       Parameter<Object, SkylarkType> parameter =
-          new Parameter.Mandatory<Object, SkylarkType>("arg" + paramIndex,
+          new Parameter.Mandatory<Object, SkylarkType>(
+              Identifier.of("arg" + paramIndex),
               SkylarkType.of(javaMethodSignatureParams[paramIndex]));
       parameters.add(parameter);
     }
@@ -226,17 +227,17 @@ public class SkylarkSignatureProcessor {
       paramDoc.put(param.name(), param.doc());
     }
     if (starStar) {
-      return new Parameter.StarStar<>(param.name(), officialType);
+      return new Parameter.StarStar<>(Identifier.of(param.name()), officialType);
     } else if (star) {
-      return new Parameter.Star<>(param.name(), officialType);
+      return new Parameter.Star<>(Identifier.of(param.name()), officialType);
     } else if (mandatory) {
-      return new Parameter.Mandatory<>(param.name(), officialType);
+      return new Parameter.Mandatory<>(Identifier.of(param.name()), officialType);
     } else if (defaultValue != null && enforcedType != null) {
       Preconditions.checkArgument(enforcedType.contains(defaultValue),
           "In function '%s', parameter '%s' has default value %s that isn't of enforced type %s",
           name, param.name(), Printer.repr(defaultValue), enforcedType);
     }
-    return new Parameter.Optional<>(param.name(), officialType, defaultValue);
+    return new Parameter.Optional<>(Identifier.of(param.name()), officialType, defaultValue);
   }
 
   static Object getDefaultValue(Param param, Iterator<Object> iterator) {