Introduce Param.legacyNamed() to handle migration from @SkylarkSignature to @SkylarkCallable.

@SkylarkSignature.parameters() treat named()=true for parameters regardless of whether named() was actually set to true, and thus for ease of migration we mark migrated parameters as legacyNamed() = true, which is currently semantically equivalent to named() = true.
We make a distinction so that it's easier to bulk fix these later.

RELNOTES: None.
PiperOrigin-RevId: 192477405

5 files changed, 80 insertions, 6 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skylarkinterface/Param.java b/src/main/java/com/google/devtools/build/lib/skylarkinterface/Param.java
index 8f10c72850..45ad6edc51 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkinterface/Param.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkinterface/Param.java
@@ -98,6 +98,18 @@ public @interface Param {
   boolean named() default false;
 
   /**
+   * If this true, {@link #named} should be treated as true.
+   *
+   * <p>This indicates this parameter is part of a {@link SkylarkCallable} method which
+   * was migrated from {@link SkylarkSignature}. Due to a pre-migration bug, all parameters were
+   * treated as if {@link #named} was true, even if it was false. To prevent breakages during
+   * migration, the interpreter can continue to treat these parameters as named. This is distinct
+   * from {@link #named}, however, so that a bulk fix/cleanup will be easier later.
+   */
+  // TODO(b/77902276): Remove this after a bulk cleanup/fix.
+  boolean legacyNamed() default false;
+
+  /**
    * If true, the parameter may be specified as a positional parameter. For example for an integer
    * positional parameter {@code foo} of a method {@code bar}, then the method call will look like
    * {@code bar(1)}. If {@link #named()} is {@code false}, then this will be the only way to call
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkinterface/processor/SkylarkCallableProcessor.java b/src/main/java/com/google/devtools/build/lib/skylarkinterface/processor/SkylarkCallableProcessor.java
index a39acba89a..cfc8958762 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkinterface/processor/SkylarkCallableProcessor.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkinterface/processor/SkylarkCallableProcessor.java
@@ -141,6 +141,10 @@ public final class SkylarkCallableProcessor extends AbstractProcessor {
     }
   }
 
+  private static boolean isParamNamed(Param param) {
+    return param.named() || param.legacyNamed();
+  }
+
   private void verifyParamSemantics(ExecutableElement methodElement, SkylarkCallable annotation)
       throws SkylarkCallableProcessorException {
     boolean allowPositionalNext = true;
@@ -148,7 +152,7 @@ public final class SkylarkCallableProcessor extends AbstractProcessor {
     boolean allowNonDefaultPositionalNext = true;
 
     for (Param parameter : annotation.parameters()) {
-      if ((!parameter.positional()) && (!parameter.named())) {
+      if ((!parameter.positional()) && (!isParamNamed(parameter))) {
         throw new SkylarkCallableProcessorException(
             methodElement,
             String.format("Parameter '%s' must be either positional or named",
@@ -180,7 +184,7 @@ public final class SkylarkCallableProcessor extends AbstractProcessor {
                       + "non-positonal parameters",
                   parameter.name()));
         }
-        if (!parameter.named() && !allowPositionalOnlyNext) {
+        if (!isParamNamed(parameter) && !allowPositionalOnlyNext) {
           throw new SkylarkCallableProcessorException(
               methodElement,
               String.format(
@@ -205,7 +209,7 @@ public final class SkylarkCallableProcessor extends AbstractProcessor {
         // No positional parameters can come after this parameter.
         allowPositionalNext = false;
       }
-      if (parameter.named()) {
+      if (isParamNamed(parameter)) {
         // No positional-only parameters can come after this parameter.
         allowPositionalOnlyNext = false;
       }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
index c9f3aee01d..e19113c93f 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
@@ -484,6 +484,10 @@ public final class FuncallExpression extends Expression {
     return result;
   }
 
+  private static boolean isParamNamed(Param param) {
+    return param.named() || param.legacyNamed();
+  }
+
   /**
    * Constructs the parameters list to actually pass to the method, filling with default values if
    * any. If there is a type or argument mismatch, returns a result containing an error message.
@@ -557,13 +561,14 @@ public final class FuncallExpression extends Expression {
                   "expected value of type '%s' for parameter '%s'",
                   type.toString(), param.name()));
         }
-        if (param.named() && keys.contains(param.name())) {
+        if (isParamNamed(param) && keys.contains(param.name())) {
           return ArgumentListConversionResult.fromError(
               String.format("got multiple values for keyword argument '%s'", param.name()));
         }
         argIndex++;
       } else { // No more positional arguments, or no more positional parameters.
-        if (param.named() && keys.remove(param.name())) { // Param specified by keyword argument.
+        if (isParamNamed(param) && keys.remove(param.name())) {
+          // Param specified by keyword argument.
           value = kwargs.get(param.name());
           if (!type.contains(value)) {
             return ArgumentListConversionResult.fromError(
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSignatureProcessor.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSignatureProcessor.java
index cbc653a087..a4302b57ed 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSignatureProcessor.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSignatureProcessor.java
@@ -112,6 +112,10 @@ public class SkylarkSignatureProcessor {
         annotation.extraKeywords(), defaultValues, paramDoc, enforcedTypesList);
   }
 
+  private static boolean isParamNamed(Param param) {
+    return param.named() || param.legacyNamed();
+  }
+
   private static FunctionSignature.WithValues<Object, SkylarkType> getSignatureForCallable(
       String name, boolean documented,
       ImmutableList<Parameter<Object, SkylarkType>> mandatoryPositionals,
@@ -133,7 +137,7 @@ public class SkylarkSignatureProcessor {
       for (Param param : parameters) {
         boolean mandatory = param.defaultValue() != null && param.defaultValue().isEmpty();
         Object defaultValue = mandatory ? null : getDefaultValue(param, defaultValuesIterator);
-        if (param.named() && !param.positional() && !named) {
+        if (isParamNamed(param) && !param.positional() && !named) {
           named = true;
           @Nullable Param starParam = null;
           if (extraPositionals != null && !extraPositionals.name().isEmpty()) {
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
index ab2245bcae..7657ced3bb 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
@@ -164,6 +164,24 @@ public class SkylarkEvaluationTest extends EvaluationTest {
       return ImmutableMap.of("a", ImmutableList.of("b", "c"));
     }
 
+    @SkylarkCallable(
+      name = "legacy_method",
+      documented = false,
+      parameters = {
+        @Param(name = "pos", positional = true, type = Boolean.class),
+        @Param(name = "legacyNamed", type = Boolean.class, positional = true, named = false,
+            legacyNamed = true),
+        @Param(name = "named", type = Boolean.class, positional = false, named = true),
+      })
+    public String legacyMethod(Boolean pos, Boolean legacyNamed, Boolean named) {
+      return "legacy_method("
+          + pos
+          + ", "
+          + legacyNamed
+          + ", "
+          + named
+          + ")";
+    }
 
     @SkylarkCallable(
       name = "with_params",
@@ -1020,6 +1038,36 @@ public class SkylarkEvaluationTest extends EvaluationTest {
         .testLookup("b", "with_params(1, true, false, true, false, a)");
   }
 
+  @Test
+  public void testLegacyNamed() throws Exception {
+    new SkylarkTest()
+        .update("mock", new Mock())
+        .setUp(
+            "b = mock.legacy_method(True, legacyNamed=True, named=True)")
+        .testLookup("b", "legacy_method(true, true, true)");
+
+    new SkylarkTest()
+        .update("mock", new Mock())
+        .setUp(
+            "b = mock.legacy_method(True, True, named=True)")
+        .testLookup("b", "legacy_method(true, true, true)");
+
+    // Verify legacyNamed also works with proxy method objects.
+    new SkylarkTest()
+        .update("mock", new Mock())
+        .setUp(
+            "m = mock.proxy_methods_object()",
+            "b = m.legacy_method(True, legacyNamed=True, named=True)")
+        .testLookup("b", "legacy_method(true, true, true)");
+
+    new SkylarkTest()
+        .update("mock", new Mock())
+        .setUp(
+            "m = mock.proxy_methods_object()",
+            "b = m.legacy_method(True, True, named=True)")
+        .testLookup("b", "legacy_method(true, true, true)");
+  }
+
   /**
    * This test verifies an error is raised when a method parameter is set both positionally and
    * by name.
@@ -1758,6 +1806,7 @@ public class SkylarkEvaluationTest extends EvaluationTest {
             "dir(mock)",
             "function",
             "is_empty",
+            "legacy_method",
             "nullfunc_failing",
             "nullfunc_working",
             "proxy_methods_object",