Minor cleanups in Skylark
--
MOS_MIGRATED_REVID=87535290
diff --git a/src/main/java/com/google/devtools/build/lib/packages/MethodLibrary.java b/src/main/java/com/google/devtools/build/lib/packages/MethodLibrary.java
index fe0ae3c..43dc269 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/MethodLibrary.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/MethodLibrary.java
@@ -21,6 +21,7 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
+import com.google.common.collect.Ordering;
import com.google.devtools.build.lib.collect.nestedset.Order;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.Location;
@@ -46,10 +47,8 @@
import com.google.devtools.build.lib.syntax.SkylarkType;
import com.google.devtools.build.lib.syntax.SkylarkType.SkylarkFunctionType;
-import java.util.ArrayList;
import java.util.Collection;
import java.util.HashMap;
-import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Set;
@@ -113,18 +112,10 @@
ImmutableList.of("this", "elements"), 2, false) {
@Override
public Object call(Object[] args, FuncallExpression ast) throws ConversionException {
- String thiz = Type.STRING.convert(args[0], "'join' operand");
+ String thisString = Type.STRING.convert(args[0], "'join' operand");
List<?> seq = Type.OBJECT_LIST.convert(args[1], "'join' argument");
- StringBuilder sb = new StringBuilder();
- for (Iterator<?> i = seq.iterator(); i.hasNext();) {
- sb.append(i.next().toString());
- if (i.hasNext()) {
- sb.append(thiz);
- }
- }
- return sb.toString();
- }
- };
+ return Joiner.on(thisString).join(seq);
+ }};
@SkylarkBuiltin(name = "lower", objectType = StringModule.class, returnType = String.class,
doc = "Returns the lower case version of this string.")
@@ -295,7 +286,7 @@
end = Type.INTEGER.convert(args[3], "'count' argument");
}
String str = getPythonSubstring(thiz, start, end);
- if (sub.equals("")) {
+ if (sub.isEmpty()) {
return str.length() + 1;
}
int count = 0;
@@ -591,7 +582,7 @@
@Override
public Object call(List<Object> args, Map<String, Object> kwargs, FuncallExpression ast,
Environment env) throws EvalException, InterruptedException {
- if (args.size() > 0) {
+ if (!args.isEmpty()) {
throw new EvalException(ast.getLocation(), "struct only supports keyword arguments");
}
return new SkylarkClassObject(kwargs, ast.getLocation());
@@ -836,7 +827,7 @@
@Override
public Object call(Object[] args, FuncallExpression ast) throws EvalException {
// There is no 'type' type in Skylark, so we return a string with the type name.
- return EvalUtils.getDataTypeName(args[0]);
+ return EvalUtils.getDataTypeName(args[0], false);
}
};
@@ -888,12 +879,10 @@
count = 1;
}
if (kwargs.size() > count) {
- kwargs = new HashMap<String, Object>(kwargs);
+ kwargs = new HashMap<>(kwargs);
kwargs.remove("sep");
- List<String> bad = new ArrayList<String>(kwargs.keySet());
- java.util.Collections.sort(bad);
- throw new EvalException(ast.getLocation(),
- "unexpected keywords: '" + bad + "'");
+ List<String> bad = Ordering.natural().sortedCopy(kwargs.keySet());
+ throw new EvalException(ast.getLocation(), "unexpected keywords: '" + bad + "'");
}
String msg = Joiner.on(sep).join(Iterables.transform(args,
new com.google.common.base.Function<Object, String>() {
@@ -962,11 +951,7 @@
.put(count, SkylarkType.INT)
.build();
- public static final List<Function> listFunctions = ImmutableList
- .<Function>builder()
- .add(append)
- .add(extend)
- .build();
+ public static final List<Function> listFunctions = ImmutableList.of(append, extend);
public static final Map<Function, SkylarkType> dictFunctions = ImmutableMap
.<Function, SkylarkType>builder()
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Type.java b/src/main/java/com/google/devtools/build/lib/packages/Type.java
index 48ba948..fb6a04f 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Type.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Type.java
@@ -27,6 +27,7 @@
import com.google.devtools.build.lib.syntax.GlobList;
import com.google.devtools.build.lib.syntax.Label;
import com.google.devtools.build.lib.syntax.SelectorValue;
+import com.google.devtools.build.lib.syntax.SkylarkList;
import com.google.devtools.build.lib.util.LoggingUtil;
import com.google.devtools.build.lib.util.StringCanonicalizer;
@@ -444,9 +445,7 @@
throw new IllegalStateException(msg);
}
String tag = (Boolean) value ? name : "no" + name;
- return new ImmutableSet.Builder<String>()
- .add(tag)
- .build();
+ return ImmutableSet.of(tag);
}
}
@@ -540,9 +539,7 @@
String msg = "Illegal tag conversion from null on Attribute " + name + ".";
throw new IllegalStateException(msg);
}
- return new ImmutableSet.Builder<String>()
- .add((String) value)
- .build();
+ return ImmutableSet.of((String) value);
}
}
@@ -888,7 +885,7 @@
/**
* A list is representable as a tag set as the contents of itself expressed
- * as Strings. So a List<String> is effectively converted to a Set<String>.
+ * as Strings. So a {@code List<String>} is effectively converted to a {@code Set<String>}.
*/
@Override
public Set<String> toTagSet(Object items, String name) {
@@ -919,7 +916,9 @@
@SuppressWarnings("unchecked")
public List<Object> convert(Object x, String what, Label currentRule)
throws ConversionException {
- if (x instanceof List) {
+ if (x instanceof SkylarkList) {
+ return ((SkylarkList) x).toList();
+ } else if (x instanceof List) {
return (List<Object>) x;
} else if (x instanceof Iterable) {
return ImmutableList.copyOf((Iterable<?>) x);
@@ -945,7 +944,7 @@
/**
* Special Type that represents a selector expression for configurable attributes. Holds a
- * mapping of <Label, T> entries, where keys are configurability patterns and values are
+ * mapping of {@code <Label, T>} entries, where keys are configurability patterns and values are
* objects of the attribute's native Type.
*/
public static final class Selector<T> {
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 953840b..0b926f6 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
@@ -83,7 +83,7 @@
@Override
public String toString() {
- StringBuffer sb = new StringBuffer();
+ StringBuilder sb = new StringBuilder();
sb.append("{");
String sep = "";
for (DictionaryEntryLiteral e : entries) {
@@ -116,6 +116,6 @@
}
type = type.infer(nextType, "dict literal", entry.getLocation(), getLocation());
}
- return SkylarkType.of(Map.class, type.getType());
+ return SkylarkType.of(SkylarkType.MAP, type);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/FunctionSignature.java b/src/main/java/com/google/devtools/build/lib/syntax/FunctionSignature.java
index c317c7d..8b0f5b0 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/FunctionSignature.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/FunctionSignature.java
@@ -191,7 +191,7 @@
|| defaultValues.size() == shape.getOptionals());
Preconditions.checkArgument(types == null
|| types.size() == shape.getArguments());
- return new AutoValueFunctionSignatureWithValues<V, T>(signature, defaultValues, types);
+ return new AutoValueFunctionSignatureWithValues<>(signature, defaultValues, types);
}
public static <V, T> WithValues<V, T> create(FunctionSignature signature,
@@ -500,7 +500,12 @@
// Minimal boilerplate to get things running in absence of AutoValue
// TODO(bazel-team): actually migrate to AutoValue when possible,
- // which importantly for future plans will define .equals() and .hashValue() (also toString())
+ // which importantly for future plans will define .equals() and .hashCode() (also toString()).
+ // Then, intern Shape, name list, FunctionSignature and WithDefaults, so that
+ // comparison can be done with == and more memory and caches can be shared between functions.
+ // Later, this can lead to further optimizations of function call by using tables matching
+ // a FunctionSignature and a CallerSignature. (struct access can be similarly sped up
+ // by interning the list of names.)
private static class AutoValueFunctionSignatureShape extends Shape {
private int mandatoryPositionals;
private int optionalPositionals;
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 b8fb203..97a4bbd 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
@@ -148,9 +148,9 @@
this.eventHandler = eventHandler;
this.parsePython = parsePython;
this.tokens = lexer.getTokens().iterator();
- this.comments = new ArrayList<Comment>();
+ this.comments = new ArrayList<>();
this.locator = locator;
- this.includedFiles = new ArrayList<Path>();
+ this.includedFiles = new ArrayList<>();
this.includedFiles.add(lexer.getFilename());
nextToken();
}
@@ -762,8 +762,7 @@
"null element in list in AST at %s:%s", token.left, token.right);
switch (token.kind) {
case RBRACKET: { // singleton List
- ListLiteral literal =
- ListLiteral.makeList(Collections.singletonList(expression));
+ ListLiteral literal = ListLiteral.makeList(Collections.singletonList(expression));
setLocation(literal, start, token.right);
nextToken();
return literal;
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java
index c30824f..4e25b77 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java
@@ -312,7 +312,7 @@
/**
* @param elements the contents of the list
* @param contentType a Java class for the contents of the list
- * @return a Skylark list containing elements without a type check.
+ * @return a Skylark list containing elements without a type check
* Only use if you already know for sure all elements are of the specified type.
*/
@SuppressWarnings("unchecked")
@@ -323,11 +323,12 @@
/**
* @param elements the contents of the list
* @param contentType a SkylarkType for the contents of the list
- * @return a Skylark list without a type check and without creating an immutable copy.
+ * @return a Skylark list without a type check and without creating an immutable copy
* Therefore the iterable containing elements must be immutable
* (which is not checked here so callers must be extra careful).
* This way it's possibly to create a SkylarkList without requesting the original iterator.
* This can be useful for nested set - list conversions.
+ * Only use if you already know for sure all elements are of the specified type.
*/
@SuppressWarnings("unchecked")
public static SkylarkList lazyList(Iterable<?> elements, SkylarkType contentType) {
@@ -337,11 +338,12 @@
/**
* @param elements the contents of the list
* @param contentType a Java class for the contents of the list
- * @return a Skylark list without a type check and without creating an immutable copy.
+ * @return a Skylark list without a type check and without creating an immutable copy
* Therefore the iterable containing elements must be immutable
* (which is not checked here so callers must be extra careful).
* This way it's possibly to create a SkylarkList without requesting the original iterator.
* This can be useful for nested set - list conversions.
+ * Only use if you already know for sure all elements are of the specified type.
*/
@SuppressWarnings("unchecked")
public static SkylarkList lazyList(Iterable<?> elements, Class<?> contentType) {
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/BuildFileASTTest.java b/src/test/java/com/google/devtools/build/lib/syntax/BuildFileASTTest.java
index 3d463f3..1cc8deb 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/BuildFileASTTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/BuildFileASTTest.java
@@ -13,6 +13,7 @@
// limitations under the License.
package com.google.devtools.build.lib.syntax;
+import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNull;
@@ -107,7 +108,7 @@
+ "bar()\n"
+ "something = baz()\n"
+ "bar()");
- assertEquals(4, buildFileAST.getStatements().size());
+ assertThat(buildFileAST.getStatements()).hasSize(4);
}
@Test
@@ -205,7 +206,7 @@
locator, false);
assertFalse(buildFileAST.containsErrors());
- assertEquals(5, buildFileAST.getStatements().size());
+ assertThat(buildFileAST.getStatements()).hasSize(5);
}
@Test
@@ -220,7 +221,7 @@
locator, false);
assertFalse(buildFileAST.containsErrors());
- assertEquals(3, buildFileAST.getStatements().size());
+ assertThat(buildFileAST.getStatements()).hasSize(3);
Environment env = new Environment();
Reporter reporter = new Reporter();
@@ -244,7 +245,7 @@
BuildFileAST buildFileAST = parseBuildFile(fileA);
assertFalse(buildFileAST.containsErrors());
- assertEquals(8, buildFileAST.getStatements().size());
+ assertThat(buildFileAST.getStatements()).hasSize(8);
Environment env = new Environment();
Reporter reporter = new Reporter();
@@ -257,7 +258,7 @@
public void testFailInclude() throws Exception {
events.setFailFast(false);
BuildFileAST buildFileAST = parseBuildFile("include(\"//nonexistent\")");
- assertEquals(1, buildFileAST.getStatements().size());
+ assertThat(buildFileAST.getStatements()).hasSize(1);
events.assertContainsEvent("Include of '//nonexistent' failed");
}
@@ -278,7 +279,7 @@
"include(\"//nonexistent:foo\")\n");
BuildFileAST buildFileAST = BuildFileAST.parseBuildFile(buildFile, events.reporter(),
emptyLocator, false);
- assertEquals(1, buildFileAST.getStatements().size());
+ assertThat(buildFileAST.getStatements()).hasSize(1);
events.assertContainsEvent("Package 'nonexistent' not found");
}
@@ -286,7 +287,7 @@
public void testInvalidInclude() throws Exception {
events.setFailFast(false);
BuildFileAST buildFileAST = parseBuildFile("include(2)");
- assertEquals(0, buildFileAST.getStatements().size());
+ assertThat(buildFileAST.getStatements()).isEmpty();
events.assertContainsEvent("syntax error at '2'");
}
@@ -319,6 +320,6 @@
public void testNonExistentIncludeReported() throws Exception {
events.setFailFast(false);
BuildFileAST buildFileAST = parseBuildFile("include('//foo:bar')");
- assertEquals(1, buildFileAST.getStatements().size());
+ assertThat(buildFileAST.getStatements()).hasSize(1);
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/EvalUtilsTest.java b/src/test/java/com/google/devtools/build/lib/syntax/EvalUtilsTest.java
index 9da4ed2..0753b0a 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/EvalUtilsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/EvalUtilsTest.java
@@ -14,6 +14,7 @@
package com.google.devtools.build.lib.syntax;
+import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
@@ -137,7 +138,7 @@
// Note: formatString doesn't perform scalar x -> (x) conversion;
// The %-operator is responsible for that.
- assertEquals("", EvalUtils.formatString("", makeTuple()));
+ assertThat(EvalUtils.formatString("", makeTuple())).isEmpty();
assertEquals("foo", EvalUtils.formatString("%s", makeTuple("foo")));
assertEquals("3.14159", EvalUtils.formatString("%s", makeTuple(3.14159)));
checkFormatPositionalFails("%s", makeTuple(1, 2, 3),
@@ -205,8 +206,8 @@
String prettyWithout = EvalUtils.prettyPrintValue(withoutStripPrefix);
String prettyWith = EvalUtils.prettyPrintValue(withStripPrefix);
- assertTrue(prettyWithout.contains("strip_prefix = \".\""));
- assertTrue(prettyWith.contains("strip_prefix = \"orange\""));
+ assertThat(prettyWithout).contains("strip_prefix = \".\"");
+ assertThat(prettyWith).contains("strip_prefix = \"orange\"");
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
index c31b17b..932ea3b 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
@@ -23,6 +23,7 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
+import com.google.common.collect.Ordering;
import com.google.devtools.build.lib.packages.PackageFactory;
import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
@@ -253,8 +254,7 @@
final Map<String, Object> kwargs,
FuncallExpression ast,
Environment env) {
- ArrayList<String> keys = new ArrayList<>(kwargs.keySet());
- Collections.sort(keys);
+ List<String> keys = Ordering.natural().sortedCopy(new ArrayList<String>(kwargs.keySet()));
if ((Integer) args.get(0) == 0) {
return keys;
} else {
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java b/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java
index a4fa5d1..9a407d2 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java
@@ -292,7 +292,7 @@
String expr = "f(1 % 2)";
FuncallExpression call = (FuncallExpression) parseExpr(expr);
Argument.Passed arg = call.getArguments().get(0);
- assertTrue(arg.getLocation().getEndOffset() < call.getLocation().getEndOffset());
+ assertThat(arg.getLocation().getEndOffset()).isLessThan(call.getLocation().getEndOffset());
}
@Test
@@ -300,7 +300,7 @@
String expr = "f(1 + 2)";
FuncallExpression call = (FuncallExpression) parseExpr(expr);
Argument.Passed arg = call.getArguments().get(0);
- assertTrue(arg.getLocation().getEndOffset() < call.getLocation().getEndOffset());
+ assertThat(arg.getLocation().getEndOffset()).isLessThan(call.getLocation().getEndOffset());
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkListTest.java b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkListTest.java
index e3ee1e1..aea87b7 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkListTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkListTest.java
@@ -13,8 +13,8 @@
// limitations under the License.
package com.google.devtools.build.lib.syntax;
+import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import com.google.common.base.Joiner;
@@ -79,7 +79,7 @@
@Test
public void testLazyListConcat() throws Exception {
exec("v = [1, 2] + lazy");
- assertTrue(env.lookup("v") instanceof SkylarkList);
+ assertThat(env.lookup("v")).isInstanceOf(SkylarkList.class);
}
@Test
