Restrict noneable parameters to be of type Object
This fixes a number of crash bugs, as passing None to these "noneable" parameters had resulted in a crash.
Technically, this change *appears* to change the API. However, as these params were never truly successfully "noneable", this is not an incompatible change.
RELNOTES: None.
PiperOrigin-RevId: 277518828
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/WorkspaceGlobalsApi.java b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/WorkspaceGlobalsApi.java
index 97d36cb..5a00bde 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/WorkspaceGlobalsApi.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/WorkspaceGlobalsApi.java
@@ -68,7 +68,6 @@
name = "managed_directories",
type = SkylarkDict.class,
generic1 = String.class,
- noneable = true,
named = true,
positional = false,
defaultValue = "{}",
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/BazelCcModuleApi.java b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/BazelCcModuleApi.java
index cba68bd..08447d5 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/BazelCcModuleApi.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/BazelCcModuleApi.java
@@ -117,7 +117,6 @@
+ "Usually passed with -I. Propagated to dependents transitively.",
positional = false,
named = true,
- noneable = true,
defaultValue = "[]",
type = SkylarkList.class),
@Param(
@@ -129,7 +128,6 @@
+ "transitively.",
positional = false,
named = true,
- noneable = true,
defaultValue = "[]",
type = SkylarkList.class),
@Param(
@@ -141,7 +139,6 @@
+ "transitively.",
positional = false,
named = true,
- noneable = true,
defaultValue = "[]",
type = SkylarkList.class),
@Param(
@@ -152,7 +149,6 @@
+ "dependents transitively.",
positional = false,
named = true,
- noneable = true,
defaultValue = "[]",
type = SkylarkList.class),
@Param(
@@ -178,7 +174,6 @@
doc = "Additional list of compilation options.",
positional = false,
named = true,
- noneable = true,
defaultValue = "[]",
type = SkylarkList.class),
@Param(
@@ -282,7 +277,6 @@
positional = false,
named = true,
defaultValue = "[]",
- noneable = true,
type = SkylarkList.class),
@Param(
name = "linking_contexts",
@@ -291,7 +285,6 @@
+ "generated by this rule.",
positional = false,
named = true,
- noneable = true,
defaultValue = "[]",
type = SkylarkList.class),
@Param(
@@ -307,7 +300,6 @@
doc = "Only C++ supported for now. Do not use this parameter.",
positional = false,
named = true,
- noneable = true,
defaultValue = "'c++'",
type = String.class),
@Param(
@@ -315,7 +307,6 @@
doc = "Can be either 'executable' or 'dynamic_library'.",
positional = false,
named = true,
- noneable = true,
defaultValue = "'executable'",
type = String.class),
@Param(
@@ -323,7 +314,6 @@
doc = " True to link dependencies statically, False dynamically.",
positional = false,
named = true,
- noneable = true,
defaultValue = "True",
type = Boolean.class),
@Param(
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/CcModuleApi.java b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/CcModuleApi.java
index fcf8898..2240e74 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/CcModuleApi.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/CcModuleApi.java
@@ -958,7 +958,6 @@
positional = false,
named = true,
defaultValue = "[]",
- noneable = true,
type = SkylarkList.class),
@Param(
name = "linking_contexts",
@@ -967,7 +966,6 @@
+ "artifact of the link() call, be it a binary or a library.",
positional = false,
named = true,
- noneable = true,
defaultValue = "[]",
type = SkylarkList.class),
@Param(
@@ -983,7 +981,6 @@
doc = "Only C++ supported for now. Do not use this parameter.",
positional = false,
named = true,
- noneable = true,
defaultValue = "'c++'",
type = String.class),
@Param(
@@ -991,7 +988,6 @@
doc = "Whether this library should always be linked.",
positional = false,
named = true,
- noneable = true,
defaultValue = "False",
type = Boolean.class),
@Param(
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/GoWrapCcHelperApi.java b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/GoWrapCcHelperApi.java
index 84442f2..b4f919b 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/GoWrapCcHelperApi.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/GoWrapCcHelperApi.java
@@ -194,8 +194,6 @@
name = "gopkg",
positional = false,
named = true,
- defaultValue = "None",
- noneable = true,
allowedTypes = {@ParamType(type = NoneType.class), @ParamType(type = FileApi.class)}),
@Param(name = "export", positional = false, named = true, type = FileApi.class),
@Param(name = "swig_out_go", positional = false, named = true, type = FileApi.class),
@@ -213,8 +211,6 @@
name = "gopkg",
positional = false,
named = true,
- defaultValue = "None",
- noneable = true,
allowedTypes = {@ParamType(type = NoneType.class), @ParamType(type = FileApi.class)}),
})
public NestedSet<FileT> getGopackageFiles(
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/platform/ConstraintCollectionApi.java b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/platform/ConstraintCollectionApi.java
index ef00565..fe66df4 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/platform/ConstraintCollectionApi.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/platform/ConstraintCollectionApi.java
@@ -51,8 +51,6 @@
@Param(
name = "constraint",
type = ConstraintSettingInfoApi.class,
- defaultValue = "None",
- noneable = true,
named = true,
doc = "The constraint setting to fetch the value for.")
},
@@ -67,8 +65,6 @@
@Param(
name = "constraint",
type = ConstraintSettingInfoApi.class,
- defaultValue = "None",
- noneable = true,
named = true,
doc = "The constraint setting to check.")
},
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 2e6c37a..0a98d5c 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
@@ -59,6 +59,9 @@
* both.
* <li>Parameters may not specify their generic types (they must use the <code>?</code> wildcard
* exclusively.
+ * <li>Noneable parameters must have java parameter type Object (as the actual value may be either
+ * {@code None} or a value of a non-{@code None} type, which do not share a superclass other
+ * than Object (or SkylarkValue, which is typically no more descriptive than Object).
* <li>Each parameter must be positional or named (or both).
* <li>Positional-only parameters must be specified before any named parameters.
* <li>Positional parameters must be specified before any non-positional parameters.
@@ -224,21 +227,38 @@
boolean allowPositionalOnlyNext = true;
boolean allowNonDefaultPositionalNext = true;
+ int paramIndex = 0;
for (Param parameter : annotation.parameters()) {
+ if (parameter.noneable()) {
+ VariableElement methodParam = methodElement.getParameters().get(paramIndex);
+ if (!"java.lang.Object".equals(methodParam.asType().toString())) {
+ throw new SkylarkCallableProcessorException(
+ methodElement,
+ String.format(
+ "Expected type 'Object' but got type '%s' for noneable parameter '%s'. The "
+ + "argument for a noneable parameter may be None, so the java parameter "
+ + "must be compatible with the type of None as well as possible non-None "
+ + "values.",
+ methodParam.asType(), methodParam.getSimpleName()));
+ }
+ } else { // !parameter.noneable()
+ if ("None".equals(parameter.defaultValue())) {
+ throw new SkylarkCallableProcessorException(
+ methodElement,
+ String.format(
+ "Parameter '%s' has 'None' default value but is not noneable. "
+ + "(If this is intended as a mandatory parameter, leave the defaultValue "
+ + "field empty)",
+ parameter.name()));
+ }
+ }
+
if (!parameter.positional() && !parameter.named()) {
throw new SkylarkCallableProcessorException(
methodElement,
String.format("Parameter '%s' must be either positional or named",
parameter.name()));
}
- if ("None".equals(parameter.defaultValue()) && !parameter.noneable()) {
- throw new SkylarkCallableProcessorException(
- methodElement,
- String.format("Parameter '%s' has 'None' default value but is not noneable. "
- + "(If this is intended as a mandatory parameter, leave the defaultValue field "
- + "empty)",
- parameter.name()));
- }
if ((parameter.allowedTypes().length > 0)
&& (!"java.lang.Object".equals(paramTypeFieldCanonicalName(parameter)))) {
throw new SkylarkCallableProcessorException(
@@ -286,6 +306,7 @@
// No positional-only parameters can come after this parameter.
allowPositionalOnlyNext = false;
}
+ paramIndex++;
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java b/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java
index 04f6144..526d9b1 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java
@@ -189,8 +189,7 @@
doc = "Return results in descending order.",
named = true,
defaultValue = "False",
- positional = false,
- noneable = true)
+ positional = false)
},
useLocation = true,
useStarlarkThread = true)
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkDict.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkDict.java
index 3a7660a..90682ec 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkDict.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkDict.java
@@ -187,13 +187,14 @@
},
useLocation = true,
useStarlarkThread = true)
- public Object setdefault(K key, V defaultValue, Location loc, StarlarkThread thread)
+ @SuppressWarnings("unchecked") // Cast of value to V
+ public Object setdefault(K key, Object defaultValue, Location loc, StarlarkThread thread)
throws EvalException {
Object value = get(key);
if (value != null) {
return value;
}
- put(key, defaultValue, loc, thread);
+ put(key, (V) defaultValue, loc, thread);
return defaultValue;
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java
index 5dc7c41..479b08eb 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java
@@ -476,9 +476,10 @@
},
useLocation = true,
useStarlarkThread = true)
- public Runtime.NoneType append(E item, Location loc, StarlarkThread thread)
+ @SuppressWarnings("unchecked") // Cast of Object item to E
+ public Runtime.NoneType append(Object item, Location loc, StarlarkThread thread)
throws EvalException {
- add(item, loc, thread.mutability());
+ add((E) item, loc, thread.mutability());
return Runtime.NONE;
}
@@ -502,9 +503,10 @@
},
useLocation = true,
useStarlarkThread = true)
- public Runtime.NoneType insert(Integer index, E item, Location loc, StarlarkThread thread)
+ @SuppressWarnings("unchecked") // Cast of Object item to E
+ public Runtime.NoneType insert(Integer index, Object item, Location loc, StarlarkThread thread)
throws EvalException {
- add(EvalUtils.clampRangeEndpoint(index, size()), item, loc, thread.mutability());
+ add(EvalUtils.clampRangeEndpoint(index, size()), (E) item, loc, thread.mutability());
return Runtime.NONE;
}
diff --git a/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/SkylarkCallableProcessorTest.java b/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/SkylarkCallableProcessorTest.java
index c9d3895..1aad801 100644
--- a/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/SkylarkCallableProcessorTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/SkylarkCallableProcessorTest.java
@@ -376,4 +376,15 @@
"Parameter one has generic type "
+ "com.google.devtools.build.lib.syntax.SkylarkDict<?,java.lang.String>");
}
+
+ @Test
+ public void testInvalidNoneableParameter() throws Exception {
+ assertAbout(javaSource())
+ .that(getFile("InvalidNoneableParameter.java"))
+ .processedWith(new SkylarkCallableProcessor())
+ .failsToCompile()
+ .withErrorContaining(
+ "Expected type 'Object' but got type 'java.lang.String' "
+ + "for noneable parameter 'aParameter'.");
+ }
}
diff --git a/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/testsources/GoldenCase.java b/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/testsources/GoldenCase.java
index 2403fcd..de7f3f1 100644
--- a/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/testsources/GoldenCase.java
+++ b/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/testsources/GoldenCase.java
@@ -76,16 +76,21 @@
return 0;
}
- @SkylarkCallable(name = "three_arg_method_with_ast",
+ @SkylarkCallable(
+ name = "three_arg_method_with_ast",
documented = false,
parameters = {
- @Param(name = "one", type = String.class, named = true),
- @Param(name = "two", type = Integer.class, named = true),
- @Param(name = "three", type = String.class, named = true,
- defaultValue = "None", noneable = true),
+ @Param(name = "one", type = String.class, named = true),
+ @Param(name = "two", type = Integer.class, named = true),
+ @Param(
+ name = "three",
+ type = String.class,
+ named = true,
+ defaultValue = "None",
+ noneable = true),
},
useAst = true)
- public String threeArgMethod(String one, Integer two, String three, FuncallExpression ast) {
+ public String threeArgMethod(String one, Integer two, Object three, FuncallExpression ast) {
return "bar";
}
diff --git a/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/testsources/InvalidNoneableParameter.java b/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/testsources/InvalidNoneableParameter.java
new file mode 100644
index 0000000..69fbb36
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/testsources/InvalidNoneableParameter.java
@@ -0,0 +1,35 @@
+// Copyright 2019 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.lib.skylarkinterface.processor.testsources;
+
+import com.google.devtools.build.lib.skylarkinterface.Param;
+import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
+
+/**
+ * Test case for a SkylarkCallable method which has a parameter with both type and allowedTypes
+ * specified.
+ */
+public class InvalidNoneableParameter {
+
+ @SkylarkCallable(
+ name = "invalid_noneable_parameter",
+ documented = false,
+ parameters = {
+ @Param(name = "a_parameter", type = String.class, noneable = true, named = true)
+ })
+ public Integer invalidNoneableParameter(String aParameter) {
+ return 42;
+ }
+}
