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
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 a4d782a..09f5120 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 @@
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 @@
/** 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 @@
@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 56c98a7..a91ffae 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 @@
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 b18da59..a4e7a9e 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 @@
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 e5ffc1c..4f499d6 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 @@
*/
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 @@
@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 @@
@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 @@
@Override
public void prettyPrint(Appendable buffer) throws IOException {
- buffer.append(name);
+ buffer.append(getName());
}
}
@@ -106,13 +111,13 @@
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 @@
@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 @@
// parameterized with.
@Override
public String toString() {
- return name + "=" + defaultValue;
+ return getName() + "=" + defaultValue;
}
}
@@ -149,17 +154,17 @@
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 @@
@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 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 @@
@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 ec6d323..d2d0af1 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 @@
// 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 @@
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 @@
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 @@
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 @@
}
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 a4302b5..5d50941 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 @@
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 @@
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) {
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContextTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContextTest.java
index 29ed8aa..a4907ab 100644
--- a/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContextTest.java
+++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContextTest.java
@@ -82,7 +82,7 @@
Package.Builder packageBuilder = Package.newExternalPackageBuilder(
Package.Builder.DefaultHelper.INSTANCE, workspaceFile, "runfiles");
FuncallExpression ast =
- new FuncallExpression(new Identifier("test"), ImmutableList.<Passed>of());
+ new FuncallExpression(Identifier.of("test"), ImmutableList.<Passed>of());
ast.setLocation(Location.BUILTIN);
Rule rule =
WorkspaceFactoryHelper.createAndAddRepositoryRule(
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/ASTPrettyPrintTest.java b/src/test/java/com/google/devtools/build/lib/syntax/ASTPrettyPrintTest.java
index 8278345..ab2f23f 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/ASTPrettyPrintTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/ASTPrettyPrintTest.java
@@ -373,9 +373,10 @@
@Test
public void loadStatement() {
// load("foo.bzl", a="A", "B")
- ASTNode loadStatement = new LoadStatement(
- new StringLiteral("foo.bzl"),
- ImmutableMap.of(new Identifier("a"), "A", new Identifier("B"), "B"));
+ ASTNode loadStatement =
+ new LoadStatement(
+ new StringLiteral("foo.bzl"),
+ ImmutableMap.of(Identifier.of("a"), "A", Identifier.of("B"), "B"));
assertIndentedPrettyMatches(
loadStatement,
" load(\"foo.bzl\", a=\"A\", \"B\")\n");
@@ -393,8 +394,8 @@
new ReturnStatement(new StringLiteral("foo")),
"return \"foo\"\n");
- assertIndentedPrettyMatches(new ReturnStatement(new Identifier("None")), " return None\n");
- assertTostringMatches(new ReturnStatement(new Identifier("None")), "return None\n");
+ assertIndentedPrettyMatches(new ReturnStatement(Identifier.of("None")), " return None\n");
+ assertTostringMatches(new ReturnStatement(Identifier.of("None")), "return None\n");
assertIndentedPrettyMatches(new ReturnStatement(null), " return\n");
assertTostringMatches(new ReturnStatement(null), "return\n");
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/EnvironmentDebuggingTest.java b/src/test/java/com/google/devtools/build/lib/syntax/EnvironmentDebuggingTest.java
index 1e1add2..b79df9c 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/EnvironmentDebuggingTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/EnvironmentDebuggingTest.java
@@ -40,7 +40,7 @@
/** Enter a dummy function scope with the given name, and the current environment's globals. */
private static void enterFunctionScope(Environment env, String functionName, Location location) {
- FuncallExpression ast = new FuncallExpression(new Identifier("test"), ImmutableList.of());
+ FuncallExpression ast = new FuncallExpression(Identifier.of("test"), ImmutableList.of());
ast.setLocation(location);
env.enterScope(
new BaseFunction(functionName) {},
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/ExceptionTest.java b/src/test/java/com/google/devtools/build/lib/syntax/ExceptionTest.java
index 77a5a80..9dd835b 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/ExceptionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/ExceptionTest.java
@@ -25,7 +25,7 @@
*/
@RunWith(JUnit4.class)
public class ExceptionTest {
- private static final ASTNode DUMMY_NODE = new Identifier("DUMMY");
+ private static final ASTNode DUMMY_NODE = Identifier.of("DUMMY");
@Test
public void testEmptyMessage() throws Exception {
