starlark: avoid overloading of Dict.put and friends

These methods throw EvalException, so they do not match the abstract
methods defined in the Map and List interfaces. Currently, for historical
reasons, a final unused Location argument is required to resolve the
overload. This change uses new names for the methods to avoid overloading
altogether.

Also, improve documentation of type safety and Map/List interface compliance.

PiperOrigin-RevId: 339545597

src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java

@@ -46,7 +46,6 @@
 import net.starlark.java.eval.Mutability;
 import net.starlark.java.eval.NoneType;
 import net.starlark.java.eval.Starlark;
-import net.starlark.java.syntax.Location;
 
 /**
  * Utility class for common work done across {@link StarlarkAttributeTransitionProvider} and {@link
@@ -192,7 +191,7 @@ static Dict<String, Object> buildSettings(
           FragmentOptions options = buildOptions.get(optionInfo.getOptionClass());
           Object optionValue = field.get(options);
 
-          dict.put(optionKey, optionValue == null ? Starlark.NONE : optionValue, (Location) null);
+          dict.putEntry(optionKey, optionValue == null ? Starlark.NONE : optionValue);
         } catch (IllegalAccessException e) {
           // These exceptions should not happen, but if they do, throw a RuntimeException.
           throw new RuntimeException(e);
@@ -204,7 +203,7 @@ static Dict<String, Object> buildSettings(
         if (!remainingInputs.remove(starlarkOption.getKey().toString())) {
           continue;
         }
-        dict.put(starlarkOption.getKey().toString(), starlarkOption.getValue(), (Location) null);
+        dict.putEntry(starlarkOption.getKey().toString(), starlarkOption.getValue());
       }
 
       if (!remainingInputs.isEmpty()) {

src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java

@@ -160,7 +160,7 @@ public Dict<String, Dict<String, Object>> existingRules(StarlarkThread thread)
       if (t instanceof Rule) {
         Dict<String, Object> rule = targetDict(t, mu);
         Preconditions.checkNotNull(rule);
-        rules.put(t.getName(), rule, (Location) null);
+        rules.putEntry(t.getName(), rule);
       }
     }
 
@@ -305,16 +305,16 @@ private static Dict<String, Object> targetDict(Target target, Mutability mu)
         if (val == null) {
           continue;
         }
-        values.put(attr.getName(), val, (Location) null);
+        values.putEntry(attr.getName(), val);
       } catch (NotRepresentableException e) {
         throw new NotRepresentableException(
             String.format(
                 "target %s, attribute %s: %s", target.getName(), attr.getName(), e.getMessage()));
       }
     }
 
-    values.put("name", rule.getName(), (Location) null);
-    values.put("kind", rule.getRuleClass(), (Location) null);
+    values.putEntry("name", rule.getName());
+    values.putEntry("kind", rule.getRuleClass());
     return values;
   }
 

src/main/java/net/starlark/java/eval/Dict.java

@@ -29,15 +29,30 @@
 import net.starlark.java.annot.Param;
 import net.starlark.java.annot.StarlarkBuiltin;
 import net.starlark.java.annot.StarlarkMethod;
-import net.starlark.java.syntax.Location;
 
 /**
  * A Dict is a Starlark dictionary (dict), a mapping from keys to values.
  *
  * <p>Dicts are iterable in both Java and Starlark; the iterator yields successive keys.
  *
- * <p>Although this implements the {@link Map} interface, it is not mutable via that interface's
- * methods. Instead, use the mutators that take in a {@link Mutability} object.
+ * <p>Starlark operations on dicts, including element update {@code dict[k]=v} and the {@code
+ * update} and {@code setdefault} methods, may insert arbitrary Starlark values as dict keys/values,
+ * regardless of the type argument used to reference the dict from Java code. Therefore, as long as
+ * a dict is mutable, Java code should refer to it only through a type such as {@code Dict<Object,
+ * Object>} or {@code Dict<?, ?>} to avoid undermining the type-safety of the Java application. Once
+ * the dict becomes frozen, it is safe to {@link #cast} it to a more specific type that accurately
+ * reflects its entries, such as {@code Dict<String, StarlarkInt>}.
+ *
+ * <p>The following Dict methods, defined by the {@link Map} interface, are not supported. Use the
+ * corresponding methods with "entry" in their name; they may report mutation failure by throwing a
+ * checked exception:
+ *
+ * <pre>
+ * void clear()         -- use clearEntries
+ * V put(K, V)          -- use putEntry
+ * void putAll(Map)     -- use putEntries
+ * V remove(Object key) -- use removeEntry
+ * </pre>
  */
 @StarlarkBuiltin(
     name = "dict",
@@ -77,13 +92,6 @@
             + " order, positional arguments before named. As with comprehensions, duplicate keys"
             + " are permitted.\n"
             + "</ol>")
-// TODO(b/64208606): eliminate these type parameters as they are wildly unsound.
-// Starlark code may update a Dict in ways incompatible with its Java
-// parameterized type. There is no realistic static or dynamic way to prevent
-// this, as Java parameterized types are not accessible at runtime.
-// Every cast to a parameterized type is a lie.
-// Unchecked warnings should be treated as errors.
-// Ditto Sequence.
 public final class Dict<K, V>
     implements Map<K, V>,
         StarlarkValue,
@@ -214,7 +222,7 @@ public Object get2(Object key, Object defaultValue, StarlarkThread thread) throw
   public Object pop(Object key, Object defaultValue, StarlarkThread thread) throws EvalException {
     Object value = get(key);
     if (value != null) {
-      remove(key, (Location) null);
+      removeEntry(key);
       return value;
     }
     if (defaultValue != Starlark.UNBOUND) {
@@ -239,7 +247,7 @@ public Tuple popitem(StarlarkThread thread) throws EvalException {
     }
     Object key = keySet().iterator().next();
     Object value = get(key);
-    remove(key, (Location) null);
+    removeEntry(key);
     return Tuple.pair(key, value);
   }
 
@@ -265,26 +273,24 @@ public Object setdefault(K key, Object defaultValue) throws EvalException {
     if (value != null) {
       return value;
     }
-    put(key, (V) defaultValue, (Location) null);
+    putEntry(key, (V) defaultValue);
     return defaultValue;
   }
 
   @StarlarkMethod(
       name = "update",
       doc =
-          "Update the dictionary with an optional positional argument <code>[pairs]</code> "
-              + " and an optional set of keyword arguments <code>[, name=value[, ...]</code>\n"
-              + "If the positional argument <code>pairs</code> is present, it must be "
-              + "<code>None</code>, another <code>dict</code>, or some other iterable. "
-              + "If it is another <code>dict</code>, then its key/value pairs are inserted. "
+          "Updates the dictionary first with the optional positional argument, <code>pairs</code>, "
+              + " then with the optional keyword arguments\n"
+              + "If the positional argument is present, it must be a dict, iterable, or None.\n"
+              + "If it is a dict, then its key/value pairs are inserted into this dict. "
               + "If it is an iterable, it must provide a sequence of pairs (or other iterables "
               + "of length 2), each of which is treated as a key/value pair to be inserted.\n"
-              + "For each <code>name=value</code> argument present, the name is converted to a "
-              + "string and used as the key for an insertion into D, with its corresponding "
-              + "value being <code>value</code>.",
+              + "Each keyword argument <code>name=value</code> causes the name/value "
+              + "pair to be inserted into this dict.",
       parameters = {
         @Param(
-            name = "args",
+            name = "pairs",
             defaultValue = "[]",
             doc =
                 "Either a dictionary or a list of entries. Entries must be tuples or lists with "
@@ -297,13 +303,13 @@ public NoneType update(Object args, Dict<String, Object> kwargs, StarlarkThread
       throws EvalException {
     // TODO(adonovan): opt: don't materialize dict; call put directly.
 
-    // All these types and casts are lies.
+    // These types and casts are unsafe; see class doc comment.
     Dict<K, V> dict =
         args instanceof Dict
             ? (Dict<K, V>) args
             : getDictFromArgs("update", args, thread.mutability());
     dict = Dict.plus(dict, (Dict<K, V>) kwargs, thread.mutability());
-    putAll(dict, (Location) null);
+    putEntries(dict);
     return Starlark.NONE;
   }
 
@@ -376,13 +382,17 @@ public static <K, V> Dict<K, V> of(@Nullable Mutability mu, K k1, V v1, K k2, V
   }
 
   /** Returns a new dict with the specified mutability containing the entries of {@code m}. */
-  @SuppressWarnings("unchecked")
   public static <K, V> Dict<K, V> copyOf(@Nullable Mutability mu, Map<? extends K, ? extends V> m) {
     // TODO(laurentlb): Move this method out of this file and rename it. It should go with
     // Starlark.fromJava; its main purpose is to convert a Java value to Starlark.
+    // TODO(adonovan): fromJava shouldn't be applied recursively.
+    // Instead, call checkValid(getValue()), fix all violations,
+    // and remove the fromJava call here. The function is otherwise sound API.
     Dict<K, V> dict = new Dict<>(mu);
-    for (Map.Entry<?, ?> e : m.entrySet()) {
-      dict.contents.put((K) e.getKey(), (V) Starlark.fromJava(e.getValue(), mu));
+    for (Map.Entry<? extends K, ? extends V> e : m.entrySet()) {
+      @SuppressWarnings("unchecked")
+      V v = (V) Starlark.fromJava(e.getValue(), mu);
+      dict.contents.put(e.getKey(), v);
     }
     return dict;
   }
@@ -409,10 +419,9 @@ public void unsafeShallowFreeze() {
    *
    * @param key the key of the added entry
    * @param value the value of the added entry
-   * @param unused a nonce value to select this overload, not Map.put
    * @throws EvalException if the key is invalid or the dict is frozen
    */
-  public void put(K key, V value, Location unused) throws EvalException {
+  public void putEntry(K key, V value) throws EvalException {
     Starlark.checkMutable(this);
     Starlark.checkHashable(key);
     contents.put(key, value);
@@ -422,14 +431,12 @@ public void put(K key, V value, Location unused) throws EvalException {
    * Puts all the entries from a given map into the dict, after validating that mutation is allowed.
    *
    * @param map the map whose entries are added
-   * @param unused a nonce value to select this overload, not Map.put
    * @throws EvalException if some key is invalid or the dict is frozen
    */
-  public <KK extends K, VV extends V> void putAll(Map<KK, VV> map, Location unused)
-      throws EvalException {
+  public <K2 extends K, V2 extends V> void putEntries(Map<K2, V2> map) throws EvalException {
     Starlark.checkMutable(this);
-    for (Map.Entry<KK, VV> e : map.entrySet()) {
-      KK k = e.getKey();
+    for (Map.Entry<K2, V2> e : map.entrySet()) {
+      K2 k = e.getKey();
       Starlark.checkHashable(k);
       contents.put(k, e.getValue());
     }
@@ -439,28 +446,21 @@ public <KK extends K, VV extends V> void putAll(Map<KK, VV> map, Location unused
    * Deletes the entry associated with the given key.
    *
    * @param key the key to delete
-   * @param unused a nonce value to select this overload, not Map.put
    * @return the value associated to the key, or {@code null} if not present
    * @throws EvalException if the dict is frozen
    */
-  V remove(Object key, Location unused) throws EvalException {
+  V removeEntry(Object key) throws EvalException {
     Starlark.checkMutable(this);
     return contents.remove(key);
   }
 
-  @StarlarkMethod(name = "clear", doc = "Remove all items from the dictionary.")
-  public NoneType clearDict() throws EvalException {
-    clear(null);
-    return Starlark.NONE;
-  }
-
   /**
    * Clears the dict.
    *
-   * @param unused a nonce value to select this overload, not Map.put
    * @throws EvalException if the dict is frozen
    */
-  private void clear(Location unused) throws EvalException {
+  @StarlarkMethod(name = "clear", doc = "Remove all items from the dictionary.")
+  public void clearEntries() throws EvalException {
     Starlark.checkMutable(this);
     contents.clear();
   }
@@ -569,7 +569,7 @@ static <K, V> Dict<K, V> getDictFromArgs(String funcname, Object args, @Nullable
             funcname, pos, pair.size());
       }
       // These casts are lies
-      result.put((K) pair.get(0), (V) pair.get(1), null);
+      result.putEntry((K) pair.get(0), (V) pair.get(1));
       pos++;
     }
     return result;
@@ -622,28 +622,26 @@ public Collection<V> values() {
   // TODO(adonovan): make mutability exception a subclass of (unchecked)
   // UnsupportedOperationException, allowing the primary Dict operations
   // to satisfy the Map operations below in the usual way (like ImmutableMap does).
-  // Add "ForStarlark" suffix to disambiguate StarlarkMethod-annotated methods.
-  // Same for StarlarkList.
 
-  @Deprecated
+  @Deprecated // use clearEntries
   @Override
   public void clear() {
     throw new UnsupportedOperationException();
   }
 
-  @Deprecated
+  @Deprecated // use putEntry
   @Override
   public V put(K key, V value) {
     throw new UnsupportedOperationException();
   }
 
-  @Deprecated
+  @Deprecated // use putEntries
   @Override
   public void putAll(Map<? extends K, ? extends V> map) {
     throw new UnsupportedOperationException();
   }
 
-  @Deprecated
+  @Deprecated // use removeEntry
   @Override
   public V remove(Object key) {
     throw new UnsupportedOperationException();

src/main/java/net/starlark/java/eval/Eval.java

@@ -523,7 +523,7 @@ private static Object evalDict(StarlarkThread.Frame fr, DictExpression dictexpr)
       Object v = eval(fr, entry.getValue());
       int before = dict.size();
       try {
-        dict.put(k, v, (Location) null);
+        dict.putEntry(k, v);
       } catch (EvalException ex) {
         fr.setErrorLocation(entry.getColonLocation());
         throw ex;
@@ -811,7 +811,7 @@ void execClauses(int index) throws EvalException, InterruptedException {
           try {
             Starlark.checkHashable(k);
             Object v = eval(fr, body.getValue());
-            dict.put(k, v, (Location) null);
+            dict.putEntry(k, v);
           } catch (EvalException ex) {
             fr.setErrorLocation(body.getColonLocation());
             throw ex;

src/main/java/net/starlark/java/eval/EvalUtils.java

@@ -15,7 +15,6 @@
 
 import com.google.common.base.Strings;
 import java.util.IllegalFormatException;
-import net.starlark.java.syntax.Location;
 import net.starlark.java.syntax.TokenKind;
 
 /** Internal declarations used by the evaluator. */
@@ -460,14 +459,14 @@ static void setIndex(Object object, Object key, Object value) throws EvalExcepti
     if (object instanceof Dict) {
       @SuppressWarnings("unchecked")
       Dict<Object, Object> dict = (Dict<Object, Object>) object;
-      dict.put(key, value, (Location) null);
+      dict.putEntry(key, value);
 
     } else if (object instanceof StarlarkList) {
       @SuppressWarnings("unchecked")
       StarlarkList<Object> list = (StarlarkList<Object>) object;
       int index = Starlark.toInt(key, "list index");
       index = EvalUtils.getSequenceIndex(index, list.size());
-      list.set(index, value, (Location) null);
+      list.setElementAt(index, value);
 
     } else {
       throw Starlark.errorf(

src/main/java/net/starlark/java/eval/StarlarkFunction.java

@@ -261,7 +261,7 @@ private Object[] processArgs(Mutability mu, Object[] positional, Object[] named)
       } else if (kwargs != null) {
         // residual keyword argument
         int sz = kwargs.size();
-        kwargs.put(keyword, value, null);
+        kwargs.putEntry(keyword, value);
         if (kwargs.size() == sz) {
           throw Starlark.errorf(
               "%s() got multiple values for keyword argument '%s'", getName(), keyword);