From 6ab3a075f528ec759bebe9b14c9a16a021f0dc4a Mon Sep 17 00:00:00 2001
From: cparsons <cparsons@google.com>
Date: Thu, 21 Jun 2018 12:21:58 -0700
Subject: Remove several uses of @SkylarkCallable.mandatoryPositionals

mandatoryPositionals is going away soon. While it is less verbose than the alternative, it does not allow for documentation to be specified (nor type-checking), and is thus inferior to the alternative. Killing mandatoryPositionals will also make the interpreter code cleaner.

RELNOTES: None.
PiperOrigin-RevId: 201566377
---
 .../android/AndroidDataProcessingApi.java          | 195 ++++++++++++++++++---
 .../lib/skylarkbuildapi/java/JavaCommonApi.java    |  41 ++++-
 .../skylarkbuildapi/java/JavaProtoCommonApi.java   |  34 +++-
 .../lib/skylarkinterface/SkylarkCallable.java      |   3 +
 4 files changed, 231 insertions(+), 42 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/android/AndroidDataProcessingApi.java b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/android/AndroidDataProcessingApi.java
index 6cef1cfe3a..634069eb5f 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/android/AndroidDataProcessingApi.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/android/AndroidDataProcessingApi.java
@@ -54,7 +54,7 @@ public interface AndroidDataProcessingApi<
             generic1 = AndroidAssetsInfoApi.class,
             positional = false,
             named = true,
-            doc = "Dependencies to inherit assets from"),
+            doc = "Dependencies to inherit assets from."),
         @Param(
             name = "neverlink",
             defaultValue = "False",
@@ -75,8 +75,14 @@ public interface AndroidDataProcessingApi<
 
   @SkylarkCallable(
       name = "resources_from_deps",
-      mandatoryPositionals = 1, // context
       parameters = {
+        @Param(
+            name = "ctx",
+            positional = true,
+            named = false,
+            type = AndroidDataContextApi.class,
+            doc = "The Android data context object for this target."
+        ),
         @Param(
             name = "deps",
             defaultValue = "[]",
@@ -84,7 +90,7 @@ public interface AndroidDataProcessingApi<
             generic1 = AndroidResourcesInfoApi.class,
             positional = false,
             named = true,
-            doc = "Dependencies to inherit resources from"),
+            doc = "Dependencies to inherit resources from."),
         @Param(
             name = "neverlink",
             defaultValue = "False",
@@ -126,8 +132,14 @@ public interface AndroidDataProcessingApi<
 
   @SkylarkCallable(
       name = "stamp_manifest",
-      mandatoryPositionals = 1, // AndroidDataContextApi ctx is mandatory
       parameters = {
+        @Param(
+            name = "ctx",
+            positional = true,
+            named = false,
+            type = AndroidDataContextApi.class,
+            doc = "The Android data context object for this target."
+        ),
         @Param(
             name = "manifest",
             positional = false,
@@ -135,7 +147,7 @@ public interface AndroidDataProcessingApi<
             type = FileApi.class,
             noneable = true,
             named = true,
-            doc = "The manifest to stamp. If not passed, a dummy manifest will be generated"),
+            doc = "The manifest to stamp. If not passed, a dummy manifest will be generated."),
         @Param(
             name = "custom_package",
             positional = false,
@@ -173,8 +185,14 @@ public interface AndroidDataProcessingApi<
 
   @SkylarkCallable(
       name = "merge_assets",
-      mandatoryPositionals = 1, // context
       parameters = {
+        @Param(
+            name = "ctx",
+            positional = true,
+            named = false,
+            type = AndroidDataContextApi.class,
+            doc = "The Android data context object for this target."
+        ),
         @Param(
             name = "assets",
             positional = false,
@@ -235,8 +253,22 @@ public interface AndroidDataProcessingApi<
 
   @SkylarkCallable(
       name = "merge_resources",
-      mandatoryPositionals = 2, // context, manifest
       parameters = {
+        @Param(
+            name = "ctx",
+            positional = true,
+            named = false,
+            type = AndroidDataContextApi.class,
+            doc = "The Android data context object for this target."
+        ),
+        @Param(
+            name = "manifest",
+            positional = true,
+            named = false,
+            type = AndroidManifestInfoApi.class,
+            doc = "The provider of this target's manifest. This provider is produced by, "
+                + "for example, stamp_android_manifest."
+        ),
         @Param(
             name = "resources",
             positional = false,
@@ -244,7 +276,7 @@ public interface AndroidDataProcessingApi<
             type = SkylarkList.class,
             generic1 = FileProviderApi.class,
             named = true,
-            doc = "Providers of this target's resources"),
+            doc = "Providers of this target's resources."),
         @Param(
             name = "deps",
             positional = false,
@@ -298,8 +330,37 @@ public interface AndroidDataProcessingApi<
 
   @SkylarkCallable(
       name = "make_aar",
-      mandatoryPositionals = 4, // context, resource info, asset info, and library class jar
       parameters = {
+        @Param(
+            name = "ctx",
+            positional = true,
+            named = false,
+            type = AndroidDataContextApi.class,
+            doc = "The Android data context object for this target."
+        ),
+        @Param(
+            name = "resource_info",
+            positional = true,
+            named = false,
+            type = AndroidResourcesInfoApi.class,
+            doc = "The provider containing processed resources for this target, produced, "
+                + "for example, by merge_resources."
+        ),
+        @Param(
+            name = "asset_info",
+            positional = true,
+            named = false,
+            type = AndroidAssetsInfoApi.class,
+            doc = "The provider containing processed assets for this target, produced, "
+                + "for example, by merge_assets."
+        ),
+        @Param(
+            name = "library_class_jar",
+            positional = true,
+            named = false,
+            type = FileApi.class,
+            doc = "The library class jar."
+        ),
         @Param(
             name = "proguard_specs",
             type = SkylarkList.class,
@@ -309,7 +370,7 @@ public interface AndroidDataProcessingApi<
             named = true,
             doc =
                 "Files to be used as Proguard specification for this target, which will be"
-                    + " inherited in the top-level target"),
+                    + " inherited in the top-level target."),
         @Param(
             name = "deps",
             type = SkylarkList.class,
@@ -344,8 +405,21 @@ public interface AndroidDataProcessingApi<
 
   @SkylarkCallable(
       name = "process_library_data",
-      mandatoryPositionals = 2, // ctx and libraryClassJar are required
       parameters = {
+        @Param(
+            name = "ctx",
+            positional = true,
+            named = false,
+            type = AndroidDataContextApi.class,
+            doc = "The Android data context object for this target."
+        ),
+        @Param(
+            name = "library_class_jar",
+            positional = true,
+            named = false,
+            type = FileApi.class,
+            doc = "The library class jar."
+        ),
         @Param(
             name = "manifest",
             positional = false,
@@ -364,7 +438,7 @@ public interface AndroidDataProcessingApi<
             generic1 = FileProviderApi.class,
             named = true,
             noneable = true,
-            doc = "Providers of this target's resources"),
+            doc = "Providers of this target's resources."),
         @Param(
             name = "assets",
             positional = false,
@@ -436,7 +510,7 @@ public interface AndroidDataProcessingApi<
             named = true,
             doc =
                 "Files to be used as Proguard specification for this target, which will be"
-                    + " inherited in the top-level target"),
+                    + " inherited in the top-level target."),
         @Param(
             name = "deps",
             positional = false,
@@ -472,9 +546,35 @@ public interface AndroidDataProcessingApi<
 
   @SkylarkCallable(
       name = "process_aar_import_data",
-      // context, resource, asset, and manifest are all mandatory
-      mandatoryPositionals = 4,
       parameters = {
+        @Param(
+            name = "ctx",
+            positional = true,
+            named = false,
+            type = AndroidDataContextApi.class,
+            doc = "The Android data context object for this target."
+        ),
+        @Param(
+            name = "resource",
+            positional = true,
+            named = false,
+            type = FileApi.class,
+            doc = "The resouce file."
+        ),
+        @Param(
+            name = "assets",
+            positional = true,
+            named = false,
+            type = FileApi.class,
+            doc = "The assets file."
+        ),
+        @Param(
+            name = "manifest",
+            positional = true,
+            named = false,
+            type = FileApi.class,
+            doc = "The manifest file."
+        ),
         @Param(
             name = "deps",
             type = SkylarkList.class,
@@ -495,8 +595,14 @@ public interface AndroidDataProcessingApi<
 
   @SkylarkCallable(
       name = "process_local_test_data",
-      mandatoryPositionals = 1, // context is mandatory
       parameters = {
+        @Param(
+            name = "ctx",
+            positional = true,
+            named = false,
+            type = AndroidDataContextApi.class,
+            doc = "The Android data context object for this target."
+        ),
         @Param(
             name = "manifest",
             positional = false,
@@ -514,7 +620,7 @@ public interface AndroidDataProcessingApi<
             type = SkylarkList.class,
             generic1 = FileProviderApi.class,
             named = true,
-            doc = "Providers of this target's resources"),
+            doc = "Providers of this target's resources."),
         @Param(
             name = "assets",
             positional = false,
@@ -599,8 +705,14 @@ public interface AndroidDataProcessingApi<
 
   @SkylarkCallable(
       name = "make_binary_settings",
-      mandatoryPositionals = 1, // AndroidDataContextApi is mandatory
       parameters = {
+        @Param(
+            name = "ctx",
+            positional = true,
+            named = false,
+            type = AndroidDataContextApi.class,
+            doc = "The Android data context object for this target."
+        ),
         @Param(
             name = "shrink_resources",
             positional = false,
@@ -670,8 +782,14 @@ public interface AndroidDataProcessingApi<
 
   @SkylarkCallable(
       name = "process_binary_data",
-      mandatoryPositionals = 1, // AndroidDataContextApi is mandatory
       parameters = {
+        @Param(
+            name = "ctx",
+            positional = true,
+            named = false,
+            type = AndroidDataContextApi.class,
+            doc = "The Android data context object for this target."
+        ),
         @Param(
             name = "resources",
             positional = false,
@@ -679,7 +797,7 @@ public interface AndroidDataProcessingApi<
             type = SkylarkList.class,
             generic1 = FileProviderApi.class,
             named = true,
-            doc = "Providers of this target's resources"),
+            doc = "Providers of this target's resources."),
         @Param(
             name = "assets",
             positional = false,
@@ -761,7 +879,7 @@ public interface AndroidDataProcessingApi<
             named = true,
             doc =
                 "Settings common to various binary processing methods, created with"
-                    + " make_binary_data_settings"),
+                    + " make_binary_data_settings."),
         @Param(
             name = "crunch_png",
             positional = false,
@@ -803,10 +921,31 @@ public interface AndroidDataProcessingApi<
 
   @SkylarkCallable(
       name = "shrink_data_apk",
-      // Required: AndroidDataContextApi, AndroidBinaryDataInfoApi to shrink, and two proguard
-      // outputs
-      mandatoryPositionals = 4,
       parameters = {
+        @Param(
+            name = "ctx",
+            positional = true,
+            named = false,
+            type = AndroidDataContextApi.class,
+            doc = "The Android data context object for this target."),
+        @Param(
+            name = "binary_data_info",
+            positional = true,
+            named = false,
+            type = AndroidBinaryDataInfoApi.class,
+            doc = "The Info about the binary to shrink, as produced by process_binary_data."),
+        @Param(
+            name = "proguard_output_jar",
+            positional = true,
+            named = false,
+            type = FileApi.class,
+            doc = "The proguard jar output file."),
+        @Param(
+            name = "proguard_mapping",
+            positional = true,
+            named = false,
+            type = FileApi.class,
+            doc = "The proguard mapping output file."),
         @Param(
             name = "binary_settings",
             type = AndroidBinaryDataSettingsApi.class,
@@ -816,7 +955,7 @@ public interface AndroidDataProcessingApi<
             named = true,
             doc =
                 "Settings common to various binary processing methods, created with"
-                    + " make_binary_data_settings"),
+                    + " make_binary_data_settings."),
         @Param(
             name = "deps",
             positional = false,
@@ -836,7 +975,7 @@ public interface AndroidDataProcessingApi<
             named = true,
             doc =
                 "Files to be used as Proguard specification for this target, which will be"
-                    + " inherited in the top-level target"),
+                    + " inherited in the top-level target."),
         @Param(
             name = "extra_proguard_specs,",
             type = SkylarkList.class,
@@ -846,8 +985,8 @@ public interface AndroidDataProcessingApi<
             named = true,
             doc =
                 "Additional proguard specs that should be added for top-level targets. This  value"
-                    + " is controlled by Java configuration."),
-      },
+                    + " is controlled by Java configuration.")
+        },
       useLocation = true,
       useEnvironment = true,
       doc =
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/java/JavaCommonApi.java b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/java/JavaCommonApi.java
index 64c90c68c0..3f8a1dd073 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/java/JavaCommonApi.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/java/JavaCommonApi.java
@@ -181,9 +181,14 @@ public interface JavaCommonApi<FileT extends FileApi, JavaInfoT extends JavaInfo
     doc = "Compiles Java source files/jars from the implementation of a Skylark rule and returns a "
       + "provider that represents the results of the compilation and can be added to the set of "
       + "providers emitted by this rule.",
-    // There is one mandatory positional: the Skylark rule context.
-    mandatoryPositionals = 1,
     parameters = {
+      @Param(
+          name = "ctx",
+          positional = true,
+          named = false,
+          type = SkylarkRuleContextApi.class,
+          doc = "The rule context."
+      ),
       @Param(
           name = "source_jars",
           positional = false,
@@ -473,9 +478,14 @@ public interface JavaCommonApi<FileT extends FileApi, JavaInfoT extends JavaInfo
       name = "default_javac_opts",
       // This function is experimental for now.
       documented = false,
-      // There's only one mandatory positional,the Skylark context
-      mandatoryPositionals = 1,
       parameters = {
+        @Param(
+            name = "ctx",
+            positional = true,
+            named = false,
+            type = SkylarkRuleContextApi.class,
+            doc = "The rule context."
+        ),
         @Param(name = "java_toolchain_attr", positional = false, named = true, type = String.class)
       })
   // TODO(b/78512644): migrate callers to passing explicit javacopts or using custom toolchains, and
@@ -486,8 +496,16 @@ public interface JavaCommonApi<FileT extends FileApi, JavaInfoT extends JavaInfo
   @SkylarkCallable(
     name = "merge",
     doc = "Merges the given providers into a single JavaInfo.",
-    // We have one positional argument: the list of providers to merge.
-    mandatoryPositionals = 1
+    parameters = {
+      @Param(
+          name = "providers",
+          positional = true,
+          named = false,
+          type = SkylarkList.class,
+          generic1 = JavaInfoApi.class,
+          doc = "The list of providers to merge."
+      ),
+    }
   )
   public JavaInfoT mergeJavaProviders(SkylarkList<JavaInfoT> providers);
 
@@ -496,8 +514,15 @@ public interface JavaCommonApi<FileT extends FileApi, JavaInfoT extends JavaInfo
     doc =
         "Returns a new Java provider whose direct-jars part is the union of both the direct and"
             + " indirect jars of the given Java provider.",
-    // There's only one mandatory positional, the Java provider.
-    mandatoryPositionals = 1
+    parameters = {
+      @Param(
+          name = "java_info",
+          positional = true,
+          named = false,
+          type = JavaInfoApi.class,
+          doc = "The java info."
+      ),
+    }
   )
   public JavaInfoT makeNonStrict(JavaInfoT javaInfo);
 
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/java/JavaProtoCommonApi.java b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/java/JavaProtoCommonApi.java
index 3afc99ec0c..8cab6bd8e3 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/java/JavaProtoCommonApi.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/java/JavaProtoCommonApi.java
@@ -35,9 +35,21 @@ public interface JavaProtoCommonApi<FileT extends FileApi,
       name = "create_java_lite_proto_compile_action",
       // This function is experimental for now.
       documented = false,
-      // There's 2 mandatory positional arguments, the Skylark context and the ConfiguredTarget.
-      mandatoryPositionals = 2,
       parameters = {
+          @Param(
+              name = "ctx",
+              positional = true,
+              named = false,
+              type = SkylarkRuleContextApi.class,
+              doc = "The rule context."
+          ),
+          @Param(
+              name = "target",
+              positional = true,
+              named = false,
+              type = TransitiveInfoCollectionApi.class,
+              doc = "The target."
+          ),
           @Param(
               name = "src_jar",
               positional = false,
@@ -78,9 +90,14 @@ public interface JavaProtoCommonApi<FileT extends FileApi,
     name = "toolchain_deps",
     // This function is experimental for now.
     documented = false,
-    // There's only one mandatory positional,the Skylark context
-    mandatoryPositionals = 1,
     parameters = {
+      @Param(
+          name = "ctx",
+          positional = true,
+          named = false,
+          type = SkylarkRuleContextApi.class,
+          doc = "The rule context."
+      ),
       @Param(name = "proto_toolchain_attr", positional = false, named = true, type = String.class)
     }
   )
@@ -91,9 +108,14 @@ public interface JavaProtoCommonApi<FileT extends FileApi,
     name = "javac_opts",
     // This function is experimental for now.
     documented = false,
-    // There's only one mandatory positional,the Skylark context
-    mandatoryPositionals = 1,
     parameters = {
+      @Param(
+          name = "ctx",
+          positional = true,
+          named = false,
+          type = SkylarkRuleContextApi.class,
+          doc = "The rule context."
+      ),
       @Param(name = "java_toolchain_attr", positional = false, named = true, type = String.class)
     }
   )
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkCallable.java b/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkCallable.java
index 4856601b59..9b5720fcf5 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkCallable.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkCallable.java
@@ -89,7 +89,10 @@ public @interface SkylarkCallable {
    * value (default is {@code -1}), means that all arguments are mandatory positionals if {@link
    * #parameters()} remains empty. If {@link #parameters()} is non empty, then a negative value for
    * {@link #mandatoryPositionals()} is taken as 0.
+   *
+   * @deprecated specify parameters via the {@link #parameters} field instead
    */
+  @Deprecated
   int mandatoryPositionals() default -1;
 
   /**
-- 
cgit v1.2.3