Minor cleanups in Skylark

--
MOS_MIGRATED_REVID=87535290

6 files changed, 36 insertions, 46 deletions

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 fe0ae3cc9f..43dc269257 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.ImmutableList;
 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.SkylarkNestedSet;
 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 @@ public class MethodLibrary {
       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 @@ public class MethodLibrary {
             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 @@ public class MethodLibrary {
     @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 @@ public class MethodLibrary {
     @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 @@ public class MethodLibrary {
         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 @@ public class MethodLibrary {
       .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 48ba948090..fb6a04feb0 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.FilesetEntry;
 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 @@ public abstract class Type<T> {
         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 @@ public abstract class Type<T> {
         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 @@ public abstract class Type<T> {
 
     /**
      * 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 @@ public abstract class Type<T> {
     @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 @@ public abstract class Type<T> {
 
   /**
    * 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 953840b3f1..0b926f6ae1 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 @@ public class DictionaryLiteral extends Expression {
 
   @Override
   public String toString() {
-    StringBuffer sb = new StringBuffer();
+    StringBuilder sb = new StringBuilder();
     sb.append("{");
     String sep = "";
     for (DictionaryEntryLiteral e : entries) {
@@ -116,6 +116,6 @@ public class DictionaryLiteral extends Expression {
       }
       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 c317c7d0a3..8b0f5b0751 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 @@ public abstract class FunctionSignature implements Serializable {
           || 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 @@ public abstract class FunctionSignature implements Serializable {
 
   // 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 b8fb203351..97a4bbd015 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 @@ class Parser {
     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 @@ class Parser {
         "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 c30824f1af..4e25b771b0 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 @@ public abstract class SkylarkList implements Iterable<Object> {
   /**
    * @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 @@ public abstract class SkylarkList implements Iterable<Object> {
   /**
    * @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 @@ public abstract class SkylarkList implements Iterable<Object> {
   /**
    * @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) {