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 b347e5c9eb7f40..4397ae74f96fc8 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 @@ -197,32 +197,6 @@ private static SkylarkCallable getAnnotationFromParentClass(Class classObj, M return null; } - private static class ArgumentListConversionResult { - private final ImmutableList arguments; - private final String error; - - private ArgumentListConversionResult(ImmutableList arguments, String error) { - this.arguments = arguments; - this.error = error; - } - - public static ArgumentListConversionResult fromArgumentList(ImmutableList arguments) { - return new ArgumentListConversionResult(arguments, null); - } - - public static ArgumentListConversionResult fromError(String error) { - return new ArgumentListConversionResult(null, error); - } - - public String getError() { - return error; - } - - public ImmutableList getArguments() { - return arguments; - } - } - /** * An exception class to handle exceptions in direct Java API calls. */ @@ -399,20 +373,18 @@ static Object callMethod(MethodDescriptor methodDescriptor, String methodName, O // 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. private Pair> findJavaMethod( Class objClass, String methodName, List args, Map kwargs) throws EvalException { Pair> matchingMethod = null; List methods = getMethods(objClass, methodName, getLocation()); - ArgumentListConversionResult argumentListConversionResult = null; if (methods != null) { for (MethodDescriptor method : methods) { if (!method.getAnnotation().structField()) { - argumentListConversionResult = convertArgumentList(args, kwargs, method); - if (argumentListConversionResult.getArguments() != null) { + List resultArgs = convertArgumentList(args, kwargs, method); + if (resultArgs != null) { if (matchingMethod == null) { - matchingMethod = new Pair<>(method, argumentListConversionResult.getArguments()); + matchingMethod = new Pair<>(method, resultArgs); } else { throw new EvalException( getLocation(), @@ -425,22 +397,11 @@ private Pair> findJavaMethod( } } if (matchingMethod == null) { - String errorMessage; - if (argumentListConversionResult == null || argumentListConversionResult.getError() == null) { - errorMessage = - String.format( - "Type %s has no %s", - EvalUtils.getDataTypeNameFromClass(objClass), formatMethod(args, kwargs)); - - } else { - errorMessage = - String.format( - "%s (in %s of %s).", - argumentListConversionResult.getError(), - formatMethod(args, kwargs), - EvalUtils.getDataTypeNameFromClass(objClass)); - } - throw new EvalException(getLocation(), errorMessage); + throw new EvalException( + getLocation(), + String.format( + "Type %s has no %s", + EvalUtils.getDataTypeNameFromClass(objClass), formatMethod(args, kwargs))); } return matchingMethod; } @@ -455,10 +416,10 @@ private static SkylarkType getType(Param param) { /** * Constructs the parameters list to actually pass to the method, filling with default values if - * any. If the type does not match, returns null and fills in methodTypeMatchError with the - * appropriate message. + * any. If the type does not match, returns null. */ - private ArgumentListConversionResult convertArgumentList( + @Nullable + private ImmutableList convertArgumentList( List args, Map kwargs, MethodDescriptor method) { ImmutableList.Builder builder = ImmutableList.builder(); Class[] params = method.getMethod().getParameterTypes(); @@ -473,17 +434,15 @@ private ArgumentListConversionResult convertArgumentList( } if (mandatoryPositionals > args.size() || args.size() > mandatoryPositionals + callable.parameters().length) { - return ArgumentListConversionResult.fromError("Too many arguments"); + return null; } - // First process the legacy positional parameters. + // First process the legacy positional parameters int i = 0; if (mandatoryPositionals > 0) { for (Class param : params) { Object value = args.get(i); if (!param.isAssignableFrom(value.getClass())) { - return ArgumentListConversionResult.fromError( - String.format( - "Cannot convert parameter at position %d to type %s", i, param.toString())); + return null; } builder.add(value); i++; @@ -493,7 +452,6 @@ private ArgumentListConversionResult convertArgumentList( } } } - // Then the parameters specified in callable.parameters() Set keys = new HashSet<>(kwargs.keySet()); for (Param param : callable.parameters()) { @@ -504,41 +462,32 @@ private ArgumentListConversionResult convertArgumentList( Object value = null; if (i < args.size()) { value = args.get(i); - if (!param.positional()) { - return ArgumentListConversionResult.fromError( - String.format("Parameter '%s' is not positional", param.name())); - } else if (!type.contains(value)) { - return ArgumentListConversionResult.fromError( - String.format( - "Cannot convert parameter '%s' to type %s", param.name(), type.toString())); + if (!param.positional() || !type.contains(value)) { + return null; // next positional argument is not of the good type } i++; } else if (param.named() && keys.remove(param.name())) { // Named parameters value = kwargs.get(param.name()); if (!type.contains(value)) { - return ArgumentListConversionResult.fromError( - String.format( - "Cannot convert parameter '%s' to type %s", param.name(), type.toString())); + return null; // type does not match } } else { // Use default value if (param.defaultValue().isEmpty()) { - return ArgumentListConversionResult.fromError( - String.format("Parameter '%s' has no default value", param.name())); + return null; // no default value } value = SkylarkSignatureProcessor.getDefaultValue(param, null); } builder.add(value); if (!param.noneable() && value instanceof NoneType) { - return ArgumentListConversionResult.fromError( - String.format("Parameter '%s' cannot be None", param.name())); + return null; // cannot be None } } if (i < args.size() || !keys.isEmpty()) { - return ArgumentListConversionResult.fromError("Too many arguments"); + return null; // too many arguments } - return ArgumentListConversionResult.fromArgumentList(builder.build()); + return builder.build(); } private String formatMethod(List args, Map kwargs) { 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 b9f58081bba2b9..b7650e0a4d3bba 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 @@ -675,14 +675,12 @@ public void testJavaCallWithPositionalAndKwargs() throws Exception { .update("mock", new Mock()) .setUp("") .testIfExactError( - "Parameter 'named' has no default value (in function with_params(int, bool) of Mock).", - "mock.with_params(1, True)"); + "Type Mock has no function with_params(int, bool)", "mock.with_params(1, True)"); new SkylarkTest() .update("mock", new Mock()) .setUp("") .testIfExactError( - "Parameter 'named' has no default value (in function with_params(int, bool, bool) " - + "of Mock).", + "Type Mock has no function with_params(int, bool, bool)", "mock.with_params(1, True, True)"); new SkylarkTest() .update("mock", new Mock()) @@ -700,15 +698,14 @@ public void testJavaCallWithPositionalAndKwargs() throws Exception { .update("mock", new Mock()) .setUp("") .testIfExactError( - "Too many arguments (in function with_params(int, bool, bool named, " - + "bool posOrNamed, int n) of Mock).", + "Type Mock has no function with_params(int, bool, bool named, bool posOrNamed, int n)", "mock.with_params(1, True, named=True, posOrNamed=True, n=2)"); new SkylarkTest() .update("mock", new Mock()) .setUp("") .testIfExactError( - "Parameter 'nonNoneable' cannot be None (in function with_params(int, bool, bool, " - + "bool named, bool optionalNamed, NoneType nonNoneable) of Mock).", + "Type Mock has no function with_params(int, bool, bool, bool named, bool optionalNamed," + + " NoneType nonNoneable)", "mock.with_params(1, True, True, named=True, optionalNamed=False, nonNoneable=None)"); }