From 4baafaca73cee2d3237999048acfc0d5f5f59a5b Mon Sep 17 00:00:00 2001 From: cparsons Date: Wed, 11 Apr 2018 11:09:17 -0700 Subject: [PATCH] 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 --- .../build/lib/skylarkinterface/Param.java | 12 +++++ .../processor/SkylarkCallableProcessor.java | 10 ++-- .../build/lib/syntax/FuncallExpression.java | 9 +++- .../lib/syntax/SkylarkSignatureProcessor.java | 6 ++- .../lib/syntax/SkylarkEvaluationTest.java | 49 +++++++++++++++++++ 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 8f10c728503207..45ad6edc51bc68 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 @@ -97,6 +97,18 @@ */ boolean named() default false; + /** + * If this true, {@link #named} should be treated as true. + * + *

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 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 a39acba89ad073..cfc895876251a3 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 @@ private void verifyNotStructFieldWithParams( } } + 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 @@ private void verifyParamSemantics(ExecutableElement methodElement, SkylarkCallab 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 @@ private void verifyParamSemantics(ExecutableElement methodElement, SkylarkCallab + "non-positonal parameters", parameter.name())); } - if (!parameter.named() && !allowPositionalOnlyNext) { + if (!isParamNamed(parameter) && !allowPositionalOnlyNext) { throw new SkylarkCallableProcessorException( methodElement, String.format( @@ -205,7 +209,7 @@ private void verifyParamSemantics(ExecutableElement methodElement, SkylarkCallab // 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 c9f3aee01d256c..e19113c93ff4e7 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 @@ private static SkylarkType getType(Param param) { 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 @@ private ArgumentListConversionResult convertArgumentList( "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 cbc653a087f097..a4302b57edf7a5 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 static FunctionSignature.WithValues getSignatureForC annotation.extraKeywords(), defaultValues, paramDoc, enforcedTypesList); } + private static boolean isParamNamed(Param param) { + return param.named() || param.legacyNamed(); + } + private static FunctionSignature.WithValues getSignatureForCallable( String name, boolean documented, ImmutableList> mandatoryPositionals, @@ -133,7 +137,7 @@ private static FunctionSignature.WithValues getSignatureFor 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 ab2245bcae7103..7657ced3bb5c36 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 Map> stringListDict() { 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 void testProxyMethodsObject() throws Exception { .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 void testDirFindsJavaObjectStructFieldsAndMethods() throws Exception { "dir(mock)", "function", "is_empty", + "legacy_method", "nullfunc_failing", "nullfunc_working", "proxy_methods_object",