Fix SkylarkCallable-annotation detection to appropriately handle methods with generic parameters
RELNOTES: None.
PiperOrigin-RevId: 197932265
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
index ab616e5..c02e014 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
@@ -1408,8 +1408,12 @@
repositoryName, outputDirName, directories, mainRepositoryName);
}
- /** Returns the bin directory for this build configuration. */
@Override
+ public ArtifactRoot getBinDir() {
+ return getBinDirectory(RepositoryName.MAIN);
+ }
+
+ /** Returns the bin directory for this build configuration. */
public ArtifactRoot getBinDirectory() {
return getBinDirectory(RepositoryName.MAIN);
}
@@ -1442,8 +1446,12 @@
repositoryName, outputDirName, directories, mainRepositoryName);
}
- /** Returns the genfiles directory for this build configuration. */
@Override
+ public ArtifactRoot getGenfilesDir() {
+ return getGenfilesDirectory(RepositoryName.MAIN);
+ }
+
+ /** Returns the genfiles directory for this build configuration. */
public ArtifactRoot getGenfilesDirectory() {
return getGenfilesDirectory(RepositoryName.MAIN);
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleContext.java
index df2b386..6d44019 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleContext.java
@@ -690,7 +690,7 @@
}
@Override
- public Artifact newFile(String filename) throws EvalException {
+ public Artifact newFileFromFilename(String filename) throws EvalException {
checkDeprecated("ctx.actions.declare_file", "ctx.new_file", null, skylarkSemantics);
checkMutable("new_file");
return actionFactory.declareFile(filename, Runtime.NONE);
@@ -698,13 +698,14 @@
// Kept for compatibility with old code.
@Override
- public Artifact newFile(FileRootApi root, String filename) throws EvalException {
+ public Artifact newFileFromRoot(FileRootApi root, String filename) throws EvalException {
checkMutable("new_file");
return ruleContext.getPackageRelativeArtifact(filename, (ArtifactRoot) root);
}
@Override
- public Artifact newFile(FileApi baseArtifact, String newBaseName) throws EvalException {
+ public Artifact newFileFromBaseFile(FileApi baseArtifact, String newBaseName)
+ throws EvalException {
checkDeprecated("ctx.actions.declare_file", "ctx.new_file", null, skylarkSemantics);
checkMutable("new_file");
return actionFactory.declareFile(newBaseName, baseArtifact);
@@ -712,7 +713,7 @@
// Kept for compatibility with old code.
@Override
- public Artifact newFile(FileRootApi root, FileApi baseArtifact, String suffix)
+ public Artifact newFileFromRootAndBase(FileRootApi root, FileApi baseArtifact, String suffix)
throws EvalException {
checkMutable("new_file");
PathFragment original = ((Artifact) baseArtifact).getRootRelativePath();
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/BuildConfigurationApi.java b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/BuildConfigurationApi.java
index ad73c80..c445eb9 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/BuildConfigurationApi.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/BuildConfigurationApi.java
@@ -31,11 +31,11 @@
@SkylarkCallable(name = "bin_dir", structField = true, documented = false)
@Deprecated
- public FileRootApi getBinDirectory();
+ public FileRootApi getBinDir();
@SkylarkCallable(name = "genfiles_dir", structField = true, documented = false)
@Deprecated
- public FileRootApi getGenfilesDirectory();
+ public FileRootApi getGenfilesDir();
@SkylarkCallable(name = "host_path_separator", structField = true,
doc = "Returns the separator for PATH environment variable, which is ':' on Unix.")
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkRuleContextApi.java b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkRuleContextApi.java
index 64b50eb..ca23f57 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkRuleContextApi.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkRuleContextApi.java
@@ -315,10 +315,10 @@
)
}
)
- public FileApi newFile(String filename) throws EvalException;
+ public FileApi newFileFromFilename(String filename) throws EvalException;
- @SkylarkCallable(documented = false)
- public FileApi newFile(FileRootApi root, String filename) throws EvalException;
+ @SkylarkCallable(name = "new_file", documented = false)
+ public FileApi newFileFromRoot(FileRootApi root, String filename) throws EvalException;
@SkylarkCallable(
name = "new_file",
@@ -338,10 +338,10 @@
)
}
)
- public FileApi newFile(FileApi baseArtifact, String newBaseName) throws EvalException;
+ public FileApi newFileFromBaseFile(FileApi baseArtifact, String newBaseName) throws EvalException;
- @SkylarkCallable(documented = false)
- public FileApi newFile(FileRootApi root, FileApi baseArtifact, String suffix)
+ @SkylarkCallable(name = "new_file", documented = false)
+ public FileApi newFileFromRootAndBase(FileRootApi root, FileApi baseArtifact, String suffix)
throws EvalException;
@SkylarkCallable(
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkInterfaceUtils.java b/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkInterfaceUtils.java
index 7788b47..81cd6ae 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkInterfaceUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkInterfaceUtils.java
@@ -93,24 +93,18 @@
/**
* Returns the {@link SkylarkCallable} annotation for the given method, if it exists, and
- * null otherwise. The first annotation of an overridden version of the method that is found
- * will be returned, starting with {@code classObj} and following its base classes and
- * interfaces recursively. This skips any method annotated inside a class that is not
- * marked {@link SkylarkModule} or is not a subclass of a class or interface marked
- * {@link SkylarkModule}.
+ * null otherwise.
+ *
+ * <p>Note that the annotation may be defined on a supermethod, rather than directly on the given
+ * method.
+ *
+ * <p>{@code classObj} is the class on which the given method is defined.
*/
@Nullable
public static SkylarkCallable getSkylarkCallable(Class<?> classObj, Method method) {
- try {
- Method superMethod = classObj.getMethod(method.getName(), method.getParameterTypes());
- boolean classAnnotatedForCallables = getParentWithSkylarkModule(classObj) != null
- || hasSkylarkGlobalLibrary(classObj);
- if (classAnnotatedForCallables
- && superMethod.isAnnotationPresent(SkylarkCallable.class)) {
- return superMethod.getAnnotation(SkylarkCallable.class);
- }
- } catch (NoSuchMethodException e) {
- // The class might not have the specified method, so an exception is OK.
+ SkylarkCallable callable = getCallableOnClassMatchingSignature(classObj, method);
+ if (callable != null) {
+ return callable;
}
if (classObj.getSuperclass() != null) {
SkylarkCallable annotation = getSkylarkCallable(classObj.getSuperclass(), method);
@@ -135,4 +129,56 @@
public static SkylarkCallable getSkylarkCallable(Method method) {
return getSkylarkCallable(method.getDeclaringClass(), method);
}
+
+ /**
+ * Returns the {@code SkylarkCallable} annotation corresponding to the given method of the given
+ * class, or null if there is no such annotation.
+ *
+ * <p>This method checks assignability instead of exact matches for purposes of generics. If
+ * Clazz has parameters BarT (extends BarInterface) and BazT (extends BazInterface), then
+ * foo(BarT, BazT) should match if the given method signature is foo(BarImpl, BazImpl). The
+ * signatures are in inexact match, but an "assignable" match.
+ */
+ @Nullable
+ private static SkylarkCallable getCallableOnClassMatchingSignature(
+ Class<?> classObj, Method signatureToMatch) {
+ // TODO(b/79877079): This method validates several invariants of @SkylarkCallable. These
+ // invariants should be verified in annotation processor or in test, and left out of this
+ // method.
+ Method[] methods = classObj.getDeclaredMethods();
+ Class<?>[] paramsToMatch = signatureToMatch.getParameterTypes();
+
+ SkylarkCallable callable = null;
+
+ for (Method method : methods) {
+ if (signatureToMatch.getName().equals(method.getName())
+ && method.isAnnotationPresent(SkylarkCallable.class)) {
+ Class<?>[] paramTypes = method.getParameterTypes();
+
+ if (paramTypes.length == paramsToMatch.length) {
+ for (int i = 0; i < paramTypes.length; i++) {
+ // This verifies assignability of the method signature to ensure this is not a
+ // coincidental overload. We verify assignability instead of matching exact parameter
+ // classes in order to match generic methods.
+ if (!paramTypes[i].isAssignableFrom(paramsToMatch[i])) {
+ throw new IllegalStateException(
+ String.format(
+ "Class %s has an incompatible overload of annotated method %s declared by %s",
+ classObj, signatureToMatch.getName(), signatureToMatch.getDeclaringClass()));
+ }
+ }
+ }
+ if (callable == null) {
+ callable = method.getAnnotation(SkylarkCallable.class);
+ } else {
+ throw new IllegalStateException(
+ String.format(
+ "Class %s has multiple overloaded methods named '%s' annotated "
+ + "with @SkylarkCallable",
+ classObj, signatureToMatch.getName()));
+ }
+ }
+ }
+ return callable;
+ }
}
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 6f5c2ef..9157de4 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
@@ -377,7 +377,7 @@
useLocation = true,
useEnvironment = true
)
- public Runtime.NoneType clear(
+ public Runtime.NoneType clearDict(
Location loc, Environment env)
throws EvalException {
clear(loc, env.mutability());
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 396e064..670c4d2 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
@@ -441,7 +441,7 @@
useLocation = true,
useEnvironment = true
)
- public Runtime.NoneType remove(Object x, Location loc, Environment env)
+ public Runtime.NoneType removeObject(Object x, Location loc, Environment env)
throws EvalException {
for (int i = 0; i < size(); i++) {
if (get(i).equals(x)) {
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 422ac03..adb8810 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
@@ -510,14 +510,35 @@
}
}
- @SkylarkModule(name = "MockMultipleMethodClass", doc = "")
- static final class MockMultipleMethodClass {
- @SuppressWarnings("unused")
- @SkylarkCallable(documented = false)
- public void method(Object o) {}
- @SuppressWarnings("unused")
- @SkylarkCallable(documented = false)
- public void method(String i) {}
+ @SkylarkModule(name = "ParamterizedMock", doc = "")
+ static interface ParameterizedApi<ObjectT> {
+ @SkylarkCallable(
+ name = "method",
+ documented = false,
+ parameters = {
+ @Param(name = "foo", named = true, positional = true, type = Object.class),
+ }
+ )
+ public ObjectT method(ObjectT o);
+ }
+
+ static final class ParameterizedMock implements ParameterizedApi<String> {
+ @Override
+ public String method(String o) {
+ return o;
+ }
+ }
+
+ // Verifies that a method implementation overriding a parameterized annotated interface method
+ // is still treated as skylark-callable. Concretely, method() below should be treated as
+ // callable even though its method signature isn't an *exact* match of the annotated method
+ // declaration, due to the interface's method declaration being generic.
+ @Test
+ public void testParameterizedMock() throws Exception {
+ new SkylarkTest()
+ .update("mock", new ParameterizedMock())
+ .setUp("result = mock.method('bar')")
+ .testLookup("result", "bar");
}
@Test
@@ -1000,15 +1021,6 @@
}
@Test
- public void testJavaCallsMultipleMethod() throws Exception {
- new SkylarkTest()
- .update("mock", new MockMultipleMethodClass())
- .testIfExactError(
- "type 'MockMultipleMethodClass' has multiple matches for function method(string)",
- "s = mock.method('string')");
- }
-
- @Test
public void testJavaCallWithKwargs() throws Exception {
new SkylarkTest()
.update("mock", new Mock())
