diff options
author | 2018-06-08 16:31:53 -0700 | |
---|---|---|
committer | 2018-06-08 16:33:35 -0700 | |
commit | 3694136dc22d9653c6b58a547be7957668b12818 (patch) | |
tree | 4e3960593c21cd37eb435b8a1d1fe209a39da2c7 | |
parent | d53d72e2841cd7e2c007751415679f9ff8a73062 (diff) |
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
10 files changed, 80 insertions, 60 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) { 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 29ed8aad2e..a4907abfac 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 @@ public class SkylarkRepositoryContextTest { 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 8278345aa3..ab2f23f6e1 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 @@ public class ASTPrettyPrintTest extends EvaluationTestCase { @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 @@ public class ASTPrettyPrintTest extends EvaluationTestCase { 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 1e1add2620..b79df9cad0 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 @@ public class EnvironmentDebuggingTest { /** 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 77a5a8068a..9dd835b6be 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 @@ import org.junit.runners.JUnit4; */ @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 { |