bazel analysis/skylark: fix crash in attr.int(default=<function>)

The type of the default=... parameter of each of the attr.<type> functions
was inconsistent with the Param.type or Param.allowed_types annotations,
causing the annotation-based type-check to accept a StarlarkFunction value,
but the reflective call to then fail.

This CL removes StarlarkFunction from Param.allowed_types for these parameters:

   attr.int(default=function)
   attr.int_list(default=function)
   attr.string(default=function)
   attr.string_list(default=function)
   attr.bool(default=function)
   attr.string_dict(default=function)
   attr.string_list_dict(default=function)

Only these label-oriented attribute types accept a function:

  attr.label,
  attr.label_list
  attr.label_keyed_string_dict
  attr.output                     \ the default parameter
  attt.output_list                / is deprecated for these

CL 285849424 fixes the annotation processor to detect this and similar errors.
It catches other instances and does other checks, so it is split off as a separate change.

Also:
- move the nasty little optionMap function out of EvalUtils (where its
  generic type forces it to abuse unsafe casts) and into SkylarkAttr.
  (The only other use, in a test, was trivially replaced by simpler code.)
- use Map not Dict for the map produced by optionMap.

See github.com/bazelbuild/bazel/issues/9463.

PiperOrigin-RevId: 286037108
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkAttr.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkAttr.java
index 8b2cdd1..d43a8d4 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkAttr.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkAttr.java
@@ -71,7 +71,7 @@
 
   // Arguments
 
-  private static boolean containsNonNoneKey(Dict<String, Object> arguments, String key) {
+  private static boolean containsNonNoneKey(Map<String, Object> arguments, String key) {
     return arguments.containsKey(key) && arguments.get(key) != Starlark.NONE;
   }
 
@@ -95,7 +95,7 @@
   private static ImmutableAttributeFactory createAttributeFactory(
       Type<?> type,
       String doc,
-      Dict<String, Object> arguments,
+      Map<String, Object> arguments,
       FuncallExpression ast,
       StarlarkThread thread)
       throws EvalException {
@@ -107,7 +107,7 @@
   private static ImmutableAttributeFactory createAttributeFactory(
       Type<?> type,
       String doc,
-      Dict<String, Object> arguments,
+      Map<String, Object> arguments,
       FuncallExpression ast,
       StarlarkThread thread,
       String name)
@@ -119,7 +119,7 @@
   private static Attribute.Builder<?> createAttribute(
       Type<?> type,
       String doc,
-      Dict<String, Object> arguments,
+      Map<String, Object> arguments,
       FuncallExpression ast,
       StarlarkThread thread,
       String name)
@@ -392,7 +392,7 @@
 
   private static Descriptor createAttrDescriptor(
       String name,
-      Dict<String, Object> kwargs,
+      Map<String, Object> kwargs,
       Type<?> type,
       FuncallExpression ast,
       StarlarkThread thread)
@@ -422,7 +422,7 @@
 
   private static Descriptor createNonconfigurableAttrDescriptor(
       String name,
-      Dict<String, Object> kwargs,
+      Map<String, Object> kwargs,
       Type<?> type,
       FuncallExpression ast,
       StarlarkThread thread)
@@ -449,7 +449,7 @@
 
   @Override
   public Descriptor intAttribute(
-      Integer defaultInt,
+      Integer defaultValue,
       String doc,
       Boolean mandatory,
       Sequence<?> values,
@@ -460,8 +460,7 @@
     BazelStarlarkContext.from(thread).checkLoadingOrWorkspacePhase("attr.int");
     return createAttrDescriptor(
         "int",
-        EvalUtils.<String, Object>optionMap(
-            thread, DEFAULT_ARG, defaultInt, MANDATORY_ARG, mandatory, VALUES_ARG, values),
+        optionMap(DEFAULT_ARG, defaultValue, MANDATORY_ARG, mandatory, VALUES_ARG, values),
         Type.INTEGER,
         ast,
         thread);
@@ -469,7 +468,7 @@
 
   @Override
   public Descriptor stringAttribute(
-      String defaultString,
+      String defaultValue,
       String doc,
       Boolean mandatory,
       Sequence<?> values,
@@ -479,8 +478,7 @@
     BazelStarlarkContext.from(thread).checkLoadingOrWorkspacePhase("attr.string");
     return createAttrDescriptor(
         "string",
-        EvalUtils.<String, Object>optionMap(
-            thread, DEFAULT_ARG, defaultString, MANDATORY_ARG, mandatory, VALUES_ARG, values),
+        optionMap(DEFAULT_ARG, defaultValue, MANDATORY_ARG, mandatory, VALUES_ARG, values),
         Type.STRING,
         ast,
         thread);
@@ -488,7 +486,7 @@
 
   @Override
   public Descriptor labelAttribute(
-      Object defaultO,
+      Object defaultValue, // Label | String | LateBoundDefaultApi | StarlarkFunction
       String doc,
       Boolean executable,
       Object allowFiles,
@@ -508,10 +506,9 @@
           createAttributeFactory(
               BuildType.LABEL,
               doc,
-              EvalUtils.<String, Object>optionMap(
-                  thread,
+              optionMap(
                   DEFAULT_ARG,
-                  defaultO,
+                  defaultValue,
                   EXECUTABLE_ARG,
                   executable,
                   ALLOW_FILES_ARG,
@@ -544,7 +541,7 @@
       Boolean mandatory,
       Boolean nonEmpty,
       Boolean allowEmpty,
-      Sequence<?> defaultList,
+      Sequence<?> defaultValue,
       String doc,
       FuncallExpression ast,
       StarlarkThread thread)
@@ -552,10 +549,9 @@
     BazelStarlarkContext.from(thread).checkLoadingOrWorkspacePhase("attr.string_list");
     return createAttrDescriptor(
         "string_list",
-        EvalUtils.<String, Object>optionMap(
-            thread,
+        optionMap(
             DEFAULT_ARG,
-            defaultList,
+            defaultValue,
             MANDATORY_ARG,
             mandatory,
             NON_EMPTY_ARG,
@@ -572,7 +568,7 @@
       Boolean mandatory,
       Boolean nonEmpty,
       Boolean allowEmpty,
-      Sequence<?> defaultList,
+      Sequence<?> defaultValue,
       String doc,
       FuncallExpression ast,
       StarlarkThread thread)
@@ -580,10 +576,9 @@
     BazelStarlarkContext.from(thread).checkLoadingOrWorkspacePhase("attr.int_list");
     return createAttrDescriptor(
         "int_list",
-        EvalUtils.<String, Object>optionMap(
-            thread,
+        optionMap(
             DEFAULT_ARG,
-            defaultList,
+            defaultValue,
             MANDATORY_ARG,
             mandatory,
             NON_EMPTY_ARG,
@@ -598,7 +593,7 @@
   @Override
   public Descriptor labelListAttribute(
       Boolean allowEmpty,
-      Object defaultList,
+      Object defaultValue, // Sequence | StarlarkFunction
       String doc,
       Object allowFiles,
       Object allowRules,
@@ -612,11 +607,10 @@
       StarlarkThread thread)
       throws EvalException {
     BazelStarlarkContext.from(thread).checkLoadingOrWorkspacePhase("attr.label_list");
-    Dict<String, Object> kwargs =
-        EvalUtils.<String, Object>optionMap(
-            thread,
+    Map<String, Object> kwargs =
+        optionMap(
             DEFAULT_ARG,
-            defaultList,
+            defaultValue,
             ALLOW_FILES_ARG,
             allowFiles,
             ALLOW_RULES_ARG,
@@ -647,7 +641,7 @@
   @Override
   public Descriptor labelKeyedStringDictAttribute(
       Boolean allowEmpty,
-      Object defaultList,
+      Object defaultValue, // Dict | StarlarkFunction
       String doc,
       Object allowFiles,
       Object allowRules,
@@ -661,11 +655,10 @@
       StarlarkThread thread)
       throws EvalException {
     BazelStarlarkContext.from(thread).checkLoadingOrWorkspacePhase("attr.label_keyed_string_dict");
-    Dict<String, Object> kwargs =
-        EvalUtils.<String, Object>optionMap(
-            thread,
+    Map<String, Object> kwargs =
+        optionMap(
             DEFAULT_ARG,
-            defaultList,
+            defaultValue,
             ALLOW_FILES_ARG,
             allowFiles,
             ALLOW_RULES_ARG,
@@ -701,13 +694,16 @@
 
   @Override
   public Descriptor boolAttribute(
-      Boolean defaultO, String doc, Boolean mandatory, FuncallExpression ast, StarlarkThread thread)
+      Boolean defaultValue,
+      String doc,
+      Boolean mandatory,
+      FuncallExpression ast,
+      StarlarkThread thread)
       throws EvalException {
     BazelStarlarkContext.from(thread).checkLoadingOrWorkspacePhase("attr.bool");
     return createAttrDescriptor(
         "bool",
-        EvalUtils.<String, Object>optionMap(
-            thread, DEFAULT_ARG, defaultO, MANDATORY_ARG, mandatory),
+        optionMap(DEFAULT_ARG, defaultValue, MANDATORY_ARG, mandatory),
         Type.BOOLEAN,
         ast,
         thread);
@@ -715,14 +711,17 @@
 
   @Override
   public Descriptor outputAttribute(
-      Object defaultO, String doc, Boolean mandatory, FuncallExpression ast, StarlarkThread thread)
+      Object defaultValue, // Label | StarlarkFunction
+      String doc,
+      Boolean mandatory,
+      FuncallExpression ast,
+      StarlarkThread thread)
       throws EvalException {
     BazelStarlarkContext.from(thread).checkLoadingOrWorkspacePhase("attr.output");
 
     return createNonconfigurableAttrDescriptor(
         "output",
-        EvalUtils.<String, Object>optionMap(
-            thread, DEFAULT_ARG, defaultO, MANDATORY_ARG, mandatory),
+        optionMap(DEFAULT_ARG, defaultValue, MANDATORY_ARG, mandatory),
         BuildType.OUTPUT,
         ast,
         thread);
@@ -731,7 +730,7 @@
   @Override
   public Descriptor outputListAttribute(
       Boolean allowEmpty,
-      Object defaultList,
+      Object defaultValue, // Sequence | StarlarkFunction
       String doc,
       Boolean mandatory,
       Boolean nonEmpty,
@@ -742,10 +741,9 @@
 
     return createAttrDescriptor(
         "output_list",
-        EvalUtils.<String, Object>optionMap(
-            thread,
+        optionMap(
             DEFAULT_ARG,
-            defaultList,
+            defaultValue,
             MANDATORY_ARG,
             mandatory,
             NON_EMPTY_ARG,
@@ -760,7 +758,7 @@
   @Override
   public Descriptor stringDictAttribute(
       Boolean allowEmpty,
-      Dict<?, ?> defaultO,
+      Dict<?, ?> defaultValue,
       String doc,
       Boolean mandatory,
       Boolean nonEmpty,
@@ -770,10 +768,9 @@
     BazelStarlarkContext.from(thread).checkLoadingOrWorkspacePhase("attr.string_dict");
     return createAttrDescriptor(
         "string_dict",
-        EvalUtils.<String, Object>optionMap(
-            thread,
+        optionMap(
             DEFAULT_ARG,
-            defaultO,
+            defaultValue,
             MANDATORY_ARG,
             mandatory,
             NON_EMPTY_ARG,
@@ -788,7 +785,7 @@
   @Override
   public Descriptor stringListDictAttribute(
       Boolean allowEmpty,
-      Dict<?, ?> defaultO,
+      Dict<?, ?> defaultValue,
       String doc,
       Boolean mandatory,
       Boolean nonEmpty,
@@ -798,10 +795,9 @@
     BazelStarlarkContext.from(thread).checkLoadingOrWorkspacePhase("attr.string_list_dict");
     return createAttrDescriptor(
         "string_list_dict",
-        EvalUtils.<String, Object>optionMap(
-            thread,
+        optionMap(
             DEFAULT_ARG,
-            defaultO,
+            defaultValue,
             MANDATORY_ARG,
             mandatory,
             NON_EMPTY_ARG,
@@ -815,13 +811,16 @@
 
   @Override
   public Descriptor licenseAttribute(
-      Object defaultO, String doc, Boolean mandatory, FuncallExpression ast, StarlarkThread thread)
+      Object defaultValue,
+      String doc,
+      Boolean mandatory,
+      FuncallExpression ast,
+      StarlarkThread thread)
       throws EvalException {
     BazelStarlarkContext.from(thread).checkLoadingOrWorkspacePhase("attr.license");
     return createNonconfigurableAttrDescriptor(
         "license",
-        EvalUtils.<String, Object>optionMap(
-            thread, DEFAULT_ARG, defaultO, MANDATORY_ARG, mandatory),
+        optionMap(DEFAULT_ARG, defaultValue, MANDATORY_ARG, mandatory),
         BuildType.LICENSE,
         ast,
         thread);
@@ -856,4 +855,19 @@
       printer.append("<attr." + name + ">");
     }
   }
+
+  // Returns an immutable map from a list of alternating name/value pairs,
+  // skipping values that are null or None. Keys must be unique.
+  private static Map<String, Object> optionMap(Object... pairs) {
+    Preconditions.checkArgument(pairs.length % 2 == 0);
+    ImmutableMap.Builder<String, Object> b = new ImmutableMap.Builder<>();
+    for (int i = 0; i < pairs.length; i += 2) {
+      String key = (String) Preconditions.checkNotNull(pairs[i]);
+      Object value = pairs[i + 1];
+      if (value != null && value != Starlark.NONE) {
+        b.put(key, value);
+      }
+    }
+    return b.build();
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkAttrApi.java b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkAttrApi.java
index 4bd7a11..350f4dc 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkAttrApi.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkAttrApi.java
@@ -149,10 +149,7 @@
       parameters = {
         @Param(
             name = DEFAULT_ARG,
-            allowedTypes = {
-              @ParamType(type = Integer.class),
-              @ParamType(type = StarlarkFunction.class)
-            },
+            type = Integer.class,
             defaultValue = "0",
             doc = DEFAULT_DOC,
             named = true,
@@ -183,7 +180,7 @@
       useAst = true,
       useStarlarkThread = true)
   Descriptor intAttribute(
-      Integer defaultInt,
+      Integer defaultValue,
       String doc,
       Boolean mandatory,
       Sequence<?> values,
@@ -197,9 +194,7 @@
       parameters = {
         @Param(
             name = DEFAULT_ARG,
-            allowedTypes = {
-              @ParamType(type = String.class),
-            },
+            type = String.class,
             defaultValue = "''",
             doc = DEFAULT_DOC,
             named = true,
@@ -230,7 +225,7 @@
       useAst = true,
       useStarlarkThread = true)
   Descriptor stringAttribute(
-      String defaultString,
+      String defaultValue,
       String doc,
       Boolean mandatory,
       Sequence<?> values,
@@ -259,6 +254,11 @@
               @ParamType(type = Label.class),
               @ParamType(type = String.class),
               @ParamType(type = LateBoundDefaultApi.class),
+              // TODO(adonovan): remove StarlarkFunction. It's undocumented,
+              // unused by Google's .bzl files, and likely unused in Bazel.
+              // I suspect it is a vestige of a "computed defaults" feature
+              // that was never fully exposed to Starlark (or was since
+              // withdrawn).
               @ParamType(type = StarlarkFunction.class)
             },
             callbackEnabled = true,
@@ -362,7 +362,7 @@
       useAst = true,
       useStarlarkThread = true)
   Descriptor labelAttribute(
-      Object defaultO,
+      Object defaultValue,
       String doc,
       Boolean executable,
       Object allowFiles,
@@ -401,10 +401,8 @@
             named = true),
         @Param(
             name = DEFAULT_ARG,
-            allowedTypes = {
-              @ParamType(type = Sequence.class, generic1 = String.class),
-              @ParamType(type = StarlarkFunction.class)
-            },
+            type = Sequence.class,
+            generic1 = String.class,
             defaultValue = "[]",
             doc = DEFAULT_DOC,
             named = true,
@@ -423,7 +421,7 @@
       Boolean mandatory,
       Boolean nonEmpty,
       Boolean allowEmpty,
-      Sequence<?> defaultList,
+      Sequence<?> defaultValue,
       String doc,
       FuncallExpression ast,
       StarlarkThread thread)
@@ -453,10 +451,8 @@
             named = true),
         @Param(
             name = DEFAULT_ARG,
-            allowedTypes = {
-              @ParamType(type = Sequence.class, generic1 = Integer.class),
-              @ParamType(type = StarlarkFunction.class)
-            },
+            type = Sequence.class,
+            generic1 = Integer.class,
             defaultValue = "[]",
             doc = DEFAULT_DOC,
             named = true,
@@ -475,7 +471,7 @@
       Boolean mandatory,
       Boolean nonEmpty,
       Boolean allowEmpty,
-      Sequence<?> defaultList,
+      Sequence<?> defaultValue,
       String doc,
       FuncallExpression ast,
       StarlarkThread thread)
@@ -581,7 +577,7 @@
       useStarlarkThread = true)
   Descriptor labelListAttribute(
       Boolean allowEmpty,
-      Object defaultList,
+      Object defaultValue,
       String doc,
       Object allowFiles,
       Object allowRules,
@@ -697,7 +693,7 @@
       useStarlarkThread = true)
   Descriptor labelKeyedStringDictAttribute(
       Boolean allowEmpty,
-      Object defaultList,
+      Object defaultValue,
       String doc,
       Object allowFiles,
       Object allowRules,
@@ -717,10 +713,7 @@
       parameters = {
         @Param(
             name = DEFAULT_ARG,
-            allowedTypes = {
-              @ParamType(type = Boolean.class),
-              @ParamType(type = StarlarkFunction.class)
-            },
+            type = Boolean.class,
             defaultValue = "False",
             named = true,
             positional = false,
@@ -743,7 +736,11 @@
       useAst = true,
       useStarlarkThread = true)
   Descriptor boolAttribute(
-      Boolean defaultO, String doc, Boolean mandatory, FuncallExpression ast, StarlarkThread thread)
+      Boolean defaultValue,
+      String doc,
+      Boolean mandatory,
+      FuncallExpression ast,
+      StarlarkThread thread)
       throws EvalException;
 
   @SkylarkCallable(
@@ -781,7 +778,11 @@
       useAst = true,
       useStarlarkThread = true)
   Descriptor outputAttribute(
-      Object defaultO, String doc, Boolean mandatory, FuncallExpression ast, StarlarkThread thread)
+      Object defaultValue,
+      String doc,
+      Boolean mandatory,
+      FuncallExpression ast,
+      StarlarkThread thread)
       throws EvalException;
 
   @SkylarkCallable(
@@ -833,7 +834,7 @@
       useStarlarkThread = true)
   Descriptor outputListAttribute(
       Boolean allowEmpty,
-      Object defaultList,
+      Object defaultValue,
       String doc,
       Boolean mandatory,
       Boolean nonEmpty,
@@ -855,10 +856,7 @@
             named = true),
         @Param(
             name = DEFAULT_ARG,
-            allowedTypes = {
-              @ParamType(type = Dict.class),
-              @ParamType(type = StarlarkFunction.class)
-            },
+            type = Dict.class,
             named = true,
             positional = false,
             defaultValue = "{}",
@@ -889,7 +887,7 @@
       useStarlarkThread = true)
   Descriptor stringDictAttribute(
       Boolean allowEmpty,
-      Dict<?, ?> defaultO,
+      Dict<?, ?> defaultValue,
       String doc,
       Boolean mandatory,
       Boolean nonEmpty,
@@ -911,10 +909,7 @@
             named = true),
         @Param(
             name = DEFAULT_ARG,
-            allowedTypes = {
-              @ParamType(type = Dict.class),
-              @ParamType(type = StarlarkFunction.class)
-            },
+            type = Dict.class,
             defaultValue = "{}",
             named = true,
             positional = false,
@@ -945,7 +940,7 @@
       useStarlarkThread = true)
   Descriptor stringListDictAttribute(
       Boolean allowEmpty,
-      Dict<?, ?> defaultO,
+      Dict<?, ?> defaultValue,
       String doc,
       Boolean mandatory,
       Boolean nonEmpty,
@@ -985,7 +980,11 @@
       useAst = true,
       useStarlarkThread = true)
   Descriptor licenseAttribute(
-      Object defaultO, String doc, Boolean mandatory, FuncallExpression ast, StarlarkThread thread)
+      Object defaultValue,
+      String doc,
+      Boolean mandatory,
+      FuncallExpression ast,
+      StarlarkThread thread)
       throws EvalException;
 
   /** An attribute descriptor. */
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java b/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java
index 69d6ae3..9dc203f 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java
@@ -16,7 +16,6 @@
 import com.google.common.base.Preconditions;
 import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Ordering;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.events.Location;
@@ -415,30 +414,6 @@
   }
 
   /**
-   * Build a Dict of kwarg arguments from a list, removing null-s or None-s.
-   *
-   * @param thread the StarlarkThread in which this map can be mutated.
-   * @param init a series of key, value pairs (as consecutive arguments) as in {@code optionMap(k1,
-   *     v1, k2, v2, k3, v3)} where each key is a String, each value is an arbitrary Objet.
-   * @return a {@code Map<String, Object>} that has all the specified entries, where key, value
-   *     pairs appearing earlier have precedence, i.e. {@code k1, v1} may override {@code k3, v3}.
-   *     <p>Ignore any entry where the value is null or None. Keys cannot be null.
-   */
-  @SuppressWarnings("unchecked")
-  public static <K, V> Dict<K, V> optionMap(StarlarkThread thread, Object... init) {
-    ImmutableMap.Builder<K, V> b = new ImmutableMap.Builder<>();
-    Preconditions.checkState(init.length % 2 == 0);
-    for (int i = init.length - 2; i >= 0; i -= 2) {
-      K key = (K) Preconditions.checkNotNull(init[i]);
-      V value = (V) init[i + 1];
-      if (!isNullOrNone(value)) {
-        b.put(key, value);
-      }
-    }
-    return Dict.copyOf(thread.mutability(), b.build());
-  }
-
-  /**
    * Installs a global hook that causes subsequently executed Starlark threads to notify the
    * debugger of important events. Closes any previously set debugger. Call {@code
    * setDebugger(null)} to disable debugging.
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
index 710d364..2301288 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
@@ -671,6 +671,13 @@
 
   @Test
   public void testLateBoundAttrWorksWithOnlyLabel() throws Exception {
+    // Late-bound attributes, which are computed during analysis as a function
+    // of the configuration, are only available for attributes involving labels:
+    //   attr.label
+    //   attr.label_list
+    //   attr.label_keyed_string_dict
+    //   attr.output,
+    //   attr.output_list
     checkEvalErrorContains(
         "expected value of type 'string' for parameter 'default', for call to method "
             + "string(default = '', doc = '', mandatory = False, values = []) "
@@ -679,6 +686,42 @@
         "attr.string(default=attr_value)");
   }
 
+  @Test
+  public void testNoComputedAttrDefaults() throws Exception {
+    // This is a regression test for github.com/bazelbuild/bazel/issues/9463.
+    // The loading-phase feature, computed attribute defaults, is not exposed
+    // to Starlark.
+    // (Not to be confused with "late-bound defaults", an analysis-phase
+    // mechanism only for attributes involving labels; see test above.)
+    //
+    // Most attributes like attr.string should not accept a function as
+    // their default value. (The bug was that the @SkylarkCallable
+    // annotation was more permissive than the method declaration.)
+    exec("def f(): pass");
+    checkEvalErrorContains(
+        "expected value of type 'string' for parameter 'default'", "attr.string(default=f)");
+    checkEvalErrorContains(
+        "expected value of type 'sequence of strings' for parameter 'default'",
+        "attr.string_list(default=f)");
+    checkEvalErrorContains(
+        "expected value of type 'int' for parameter 'default'", "attr.int(default=f)");
+    checkEvalErrorContains(
+        "expected value of type 'sequence of ints' for parameter 'default'",
+        "attr.int_list(default=f)");
+    checkEvalErrorContains(
+        "expected value of type 'bool' for parameter 'default'", "attr.bool(default=f)");
+    checkEvalErrorContains(
+        "expected value of type 'dict' for parameter 'default'", "attr.string_dict(default=f)");
+    checkEvalErrorContains(
+        "expected value of type 'dict' for parameter 'default'",
+        "attr.string_list_dict(default=f)");
+    // Also:
+    // - The attr.output(default=...) parameter is deprecated
+    //   (see --incompatible_no_output_attr_default)
+    // - attr.license appears to be disabled already.
+    //   (see --incompatible_no_attr_license)
+  }
+
   private static final Label FAKE_LABEL = Label.parseAbsoluteUnchecked("//fake/label.bzl");
 
   @Test
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java
index e01dd6b..d881afa 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java
@@ -60,7 +60,6 @@
 import com.google.devtools.build.lib.skylarkinterface.SkylarkGlobalLibrary;
 import com.google.devtools.build.lib.syntax.Depset;
 import com.google.devtools.build.lib.syntax.EvalException;
-import com.google.devtools.build.lib.syntax.EvalUtils;
 import com.google.devtools.build.lib.syntax.Printer;
 import com.google.devtools.build.lib.syntax.Sequence;
 import com.google.devtools.build.lib.syntax.Starlark;
@@ -112,16 +111,12 @@
       Object mandatoryKey,
       Object optionalKey,
       StarlarkThread thread) {
-    return EvalUtils.optionMap(
-        thread,
-        "mandatory",
-        mandatory,
-        "optional",
-        optional,
-        "mandatory_key",
-        mandatoryKey,
-        "optional_key",
-        optionalKey);
+    Map<String, Object> m = new HashMap<>();
+    m.put("mandatory", mandatory);
+    m.put("optional", optional);
+    m.put("mandatory_key", mandatoryKey);
+    m.put("optional_key", optionalKey);
+    return m;
   }
 
   @Before
@@ -2792,8 +2787,7 @@
     assertThat(expected)
         .hasMessageThat()
         .contains(
-            "expected value of type 'int or function' for parameter 'default', "
-                + "for call to method int(");
+            "expected value of type 'int' for parameter 'default', " + "for call to method int(");
   }
 
   @Test