Enforce via annotation processor that a class may not have two @SkylarkCallable methods with the same name.
This greatly simplifies the code around method retrieval.
Fixes #6490.
RELNOTES: None.
PiperOrigin-RevId: 218568560
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 aaade02..0ef2b61 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
@@ -14,6 +14,8 @@
package com.google.devtools.build.lib.skylarkinterface.processor;
+import com.google.common.collect.LinkedHashMultimap;
+import com.google.common.collect.SetMultimap;
import com.google.devtools.build.lib.skylarkinterface.Param;
import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
import com.google.devtools.build.lib.syntax.SkylarkSemantics.FlagIdentifier;
@@ -68,7 +70,11 @@
public final class SkylarkCallableProcessor extends AbstractProcessor {
private Messager messager;
+ // A set containing the names of all classes which have a method with @SkylarkCallable.selfCall.
private Set<String> classesWithSelfcall;
+ // A multimap where keys are class names, and values are the callable method names identified in
+ // that class (where "method name" is @SkylarkCallable.name").
+ private SetMultimap<String, String> processedClassMethods;
private static final String SKYLARK_LIST = "com.google.devtools.build.lib.syntax.SkylarkList<?>";
private static final String SKYLARK_DICT =
@@ -89,6 +95,7 @@
super.init(processingEnv);
messager = processingEnv.getMessager();
classesWithSelfcall = new HashSet<>();
+ processedClassMethods = LinkedHashMultimap.create();
}
@Override
@@ -113,6 +120,7 @@
verifyExtraInterpreterParams(methodElement, annotation);
verifyIfSelfCall(methodElement, annotation);
verifyFlagToggles(methodElement, annotation);
+ verifyNoNameConflict(methodElement, annotation);
} catch (SkylarkCallableProcessorException exception) {
error(exception.methodElement, exception.errorMessage);
}
@@ -121,6 +129,20 @@
return true;
}
+ private void verifyNoNameConflict(ExecutableElement methodElement, SkylarkCallable annotation)
+ throws SkylarkCallableProcessorException {
+ boolean methodNameIsUniqueForClass =
+ processedClassMethods.put(
+ methodElement.getEnclosingElement().asType().toString(),
+ annotation.name());
+ if (!methodNameIsUniqueForClass) {
+ throw new SkylarkCallableProcessorException(
+ methodElement,
+ String.format("Containing class has more than one method with name '%s' defined.",
+ annotation.name()));
+ }
+ }
+
private void verifyFlagToggles(ExecutableElement methodElement, SkylarkCallable annotation)
throws SkylarkCallableProcessorException {
if (annotation.enableOnlyWithFlag() != FlagIdentifier.NONE
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java
index cee5ab2..0a31aaa 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java
@@ -13,11 +13,9 @@
// limitations under the License.
package com.google.devtools.build.lib.syntax;
-import com.google.common.collect.Streams;
import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.util.SpellChecker;
import java.io.IOException;
-import java.util.Optional;
/** Syntax node for a dot expression. e.g. obj.field, but not obj.method() */
public final class DotExpression extends Expression {
@@ -118,24 +116,19 @@
public static Object eval(Object objValue, String name,
Location loc, Environment env) throws EvalException, InterruptedException {
- Iterable<MethodDescriptor> methods =
+ MethodDescriptor method =
objValue instanceof Class<?>
- ? FuncallExpression.getMethods(env.getSemantics(), (Class<?>) objValue, name)
- : FuncallExpression.getMethods(env.getSemantics(), objValue.getClass(), name);
+ ? FuncallExpression.getMethod(env.getSemantics(), (Class<?>) objValue, name)
+ : FuncallExpression.getMethod(env.getSemantics(), objValue.getClass(), name);
- if (methods != null) {
- Optional<MethodDescriptor> method =
- Streams.stream(methods).filter(MethodDescriptor::isStructField).findFirst();
- if (method.isPresent() && method.get().isStructField()) {
- return method
- .get()
- .call(
- objValue,
- FuncallExpression.extraInterpreterArgs(method.get(), /* ast = */ null, loc, env)
- .toArray(),
- loc,
- env);
- }
+ if (method != null && method.isStructField()) {
+ return method
+ .call(
+ objValue,
+ FuncallExpression.extraInterpreterArgs(method, /* ast = */ null, loc, env)
+ .toArray(),
+ loc,
+ env);
}
if (objValue instanceof SkylarkClassObject) {
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 d906f8b..dcf038f 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
@@ -25,7 +25,6 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSortedMap;
import com.google.common.collect.Iterables;
-import com.google.common.collect.Lists;
import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
import com.google.devtools.build.lib.skylarkinterface.SkylarkInterfaceUtils;
@@ -39,7 +38,6 @@
import java.lang.reflect.Method;
import java.util.Collections;
import java.util.Comparator;
-import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
@@ -126,18 +124,19 @@
}
});
- private static final LoadingCache<MethodDescriptorKey, Map<String, List<MethodDescriptor>>>
+ private static final LoadingCache<MethodDescriptorKey, Map<String, MethodDescriptor>>
methodCache =
CacheBuilder.newBuilder()
.build(
- new CacheLoader<MethodDescriptorKey, Map<String, List<MethodDescriptor>>>() {
+ new CacheLoader<MethodDescriptorKey, Map<String, MethodDescriptor>>() {
@Override
- public Map<String, List<MethodDescriptor>> load(MethodDescriptorKey key)
+ public Map<String, MethodDescriptor> load(MethodDescriptorKey key)
throws Exception {
Class<?> keyClass = key.getClazz();
SkylarkSemantics semantics = key.getSemantics();
- Map<String, List<MethodDescriptor>> methodMap = new HashMap<>();
+ ImmutableMap.Builder<String, MethodDescriptor> methodMap =
+ ImmutableMap.builder();
for (Method method : keyClass.getMethods()) {
// Synthetic methods lead to false multiple matches
if (method.isSynthetic()) {
@@ -153,16 +152,10 @@
}
if (semantics.isFeatureEnabledBasedOnTogglingFlags(
callable.enableOnlyWithFlag(), callable.disableWithFlag())) {
- String name = callable.name();
- if (methodMap.containsKey(name)) {
- methodMap.get(name).add(MethodDescriptor.of(method, callable));
- } else {
- methodMap.put(
- name, Lists.newArrayList(MethodDescriptor.of(method, callable)));
- }
+ methodMap.put(callable.name(), MethodDescriptor.of(method, callable));
}
}
- return ImmutableMap.copyOf(methodMap);
+ return methodMap.build();
}
});
@@ -178,7 +171,6 @@
HashSet<String> fieldNamesForCollisions = new HashSet<>();
List<MethodDescriptor> fieldMethods =
methodCache.get(key).values().stream()
- .flatMap(List::stream)
.filter(MethodDescriptor::isStructField)
.collect(Collectors.toList());
@@ -385,12 +377,12 @@
* @deprecated use {@link #getMethods(SkylarkSemantics, Class, String)} instead
*/
@Deprecated
- public static List<MethodDescriptor> getMethods(Class<?> objClass, String methodName) {
- return getMethods(SkylarkSemantics.DEFAULT_SEMANTICS, objClass, methodName);
+ public static MethodDescriptor getMethod(Class<?> objClass, String methodName) {
+ return getMethod(SkylarkSemantics.DEFAULT_SEMANTICS, objClass, methodName);
}
/** Returns the list of Skylark callable Methods of objClass with the given name. */
- public static List<MethodDescriptor> getMethods(
+ public static MethodDescriptor getMethod(
SkylarkSemantics semantics, Class<?> objClass, String methodName) {
try {
return methodCache
@@ -465,13 +457,13 @@
*/
public static BuiltinCallable getBuiltinCallable(Object obj, String methodName) {
Class<?> objClass = obj.getClass();
- List<MethodDescriptor> methodDescriptors = getMethods(objClass, methodName);
- if (methodDescriptors.size() != 1) {
+ MethodDescriptor methodDescriptor = getMethod(objClass, methodName);
+ if (methodDescriptor == null) {
throw new IllegalStateException(String.format(
- "Expected exactly 1 method named '%s' in %s, but found %s",
- methodName, objClass, methodDescriptors.size()));
+ "Expected a method named '%s' in %s, but found none",
+ methodName, objClass));
}
- return new BuiltinCallable(methodName, obj, methodDescriptors.get(0));
+ return new BuiltinCallable(methodName, obj, methodDescriptor);
}
/**
@@ -499,10 +491,7 @@
return methodDescriptor.call(obj, new Object[0], Location.BUILTIN, null);
}
- // TODO(bazel-team): If there's exactly one usable method, this works. If there are multiple
- // matching methods, it still can be a problem. Figure out how the Java compiler does it
- // exactly and copy that behaviour.
- // Throws an EvalException when it cannot find a matching function.
+ // Throws an EvalException when there is no function matching the given name and arguments.
private Pair<MethodDescriptor, List<Object>> findJavaMethod(
Class<?> objClass,
String methodName,
@@ -510,57 +499,36 @@
Map<String, Object> kwargs,
Environment environment)
throws EvalException {
- Pair<MethodDescriptor, List<Object>> matchingMethod = null;
- List<MethodDescriptor> methods = getMethods(environment.getSemantics(), objClass, methodName);
+ MethodDescriptor method = getMethod(environment.getSemantics(), objClass, methodName);
ArgumentListConversionResult argumentListConversionResult = null;
- if (methods != null) {
- for (MethodDescriptor method : methods) {
- if (method.isStructField()) {
- // This indicates a built-in structField which returns a function which may have
- // one or more arguments itself. For example, foo.bar('baz'), where foo.bar is a
- // structField returning a function. Calling the "bar" callable of foo should
- // not have 'baz' propagated, though extra interpreter arguments should be supplied.
- return new Pair<>(method, extraInterpreterArgs(method, null, getLocation(), environment));
+ if (method != null) {
+ if (method.isStructField()) {
+ // This indicates a built-in structField which returns a function which may have
+ // one or more arguments itself. For example, foo.bar('baz'), where foo.bar is a
+ // structField returning a function. Calling the "bar" callable of foo should
+ // not have 'baz' propagated, though extra interpreter arguments should be supplied.
+ return new Pair<>(method, extraInterpreterArgs(method, null, getLocation(), environment));
+ } else {
+ argumentListConversionResult = convertArgumentList(args, kwargs, method, environment);
+
+ if (argumentListConversionResult.getArguments() != null) {
+ return new Pair<>(method, argumentListConversionResult.getArguments());
} else {
- argumentListConversionResult = convertArgumentList(args, kwargs, method, environment);
- if (argumentListConversionResult.getArguments() != null) {
- if (matchingMethod == null) {
- matchingMethod = new Pair<>(method, argumentListConversionResult.getArguments());
- } else {
- throw new EvalException(
- getLocation(),
- String.format(
- "type '%s' has multiple matches for function %s",
- EvalUtils.getDataTypeNameFromClass(objClass),
- formatMethod(objClass, methodName, args, kwargs)));
- }
- }
+ throw new EvalException(getLocation(),
+ String.format(
+ "%s, in method call %s of '%s'",
+ argumentListConversionResult.getError(),
+ formatMethod(objClass, methodName, args, kwargs),
+ EvalUtils.getDataTypeNameFromClass(objClass)));
}
}
+ } else { // method == null
+ throw new EvalException(getLocation(),
+ String.format(
+ "type '%s' has no method %s",
+ EvalUtils.getDataTypeNameFromClass(objClass),
+ formatMethod(objClass, methodName, args, kwargs)));
}
- if (matchingMethod == null) {
- String errorMessage;
- if (ClassObject.class.isAssignableFrom(objClass)) {
- errorMessage = String.format("struct has no method '%s'", methodName);
- } else if (argumentListConversionResult == null
- || argumentListConversionResult.getError() == null) {
- errorMessage =
- String.format(
- "type '%s' has no method %s",
- EvalUtils.getDataTypeNameFromClass(objClass),
- formatMethod(objClass, methodName, args, kwargs));
-
- } else {
- errorMessage =
- String.format(
- "%s, in method call %s of '%s'",
- argumentListConversionResult.getError(),
- formatMethod(objClass, methodName, args, kwargs),
- EvalUtils.getDataTypeNameFromClass(objClass));
- }
- throw new EvalException(getLocation(), errorMessage);
- }
- return matchingMethod;
}
/**
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
index c0374df..f904c42 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
@@ -2042,7 +2042,7 @@
checkError(
"test/skylark",
"r",
- "struct has no method 'output_group'",
+ "type 'Target' has no method output_group()",
"load('//test/skylark:extension.bzl', 'my_rule')",
"cc_binary(name = 'lib', data = ['a.txt'])",
"my_rule(name='r', dep = ':lib')");
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
index 0c12754..3337f07 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
@@ -1109,7 +1109,7 @@
@Test
public void testStructAccessingUnknownFieldWithArgs() throws Exception {
checkErrorContains(
- "struct has no method 'c'", "x = struct(a = 1, b = 2)", "y = x.c()");
+ "type 'struct' has no method c()", "x = struct(a = 1, b = 2)", "y = x.c()");
}
@Test
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 f42b695..0df865c 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
@@ -305,4 +305,14 @@
"Only one of @SkylarkCallable.enablingFlag and @SkylarkCallable.disablingFlag may be "
+ "specified.");
}
+
+ @Test
+ public void testConflictingMethodNames() throws Exception {
+ assertAbout(javaSource())
+ .that(getFile("ConflictingMethodNames.java"))
+ .processedWith(new SkylarkCallableProcessor())
+ .failsToCompile()
+ .withErrorContaining("Containing class has more than one method with name "
+ + "'conflicting_method' defined");
+ }
}
diff --git a/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/testsources/ConflictingMethodNames.java b/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/testsources/ConflictingMethodNames.java
new file mode 100644
index 0000000..e721402
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/testsources/ConflictingMethodNames.java
@@ -0,0 +1,46 @@
+// Copyright 2018 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 class which contains multiple SkylarkCallable methods with the same name.
+ * This should cause a compile failure -- overrides are not allowed.
+ */
+public class ConflictingMethodNames {
+
+ @SkylarkCallable(
+ name = "conflicting_method",
+ documented = false,
+ parameters = {
+ @Param(name = "one", type = String.class, named = true),
+ })
+ public String conflictingMethod(String one) {
+ return "foo";
+ }
+
+ @SkylarkCallable(
+ name = "conflicting_method",
+ documented = false,
+ parameters = {
+ @Param(name = "one", type = String.class, named = true),
+ @Param(name = "two", type = Integer.class, named = true),
+ })
+ public String conflictingMethodTwo(String one, Integer two) {
+ return "foo";
+ }
+}
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 071019e..d733836 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
@@ -2218,7 +2218,8 @@
.update("val", new SkylarkClassObjectWithSkylarkCallables())
.testIfExactError(
// TODO(bazel-team): This should probably match the error above better.
- "struct has no method 'nonexistent_method'", "v = val.nonexistent_method()");
+ "type 'SkylarkClassObjectWithSkylarkCallables' has no method nonexistent_method()",
+ "v = val.nonexistent_method()");
}
@Test
