diff --git a/src/main/java/com/google/api/generator/engine/ast/MethodDefinition.java b/src/main/java/com/google/api/generator/engine/ast/MethodDefinition.java index cc6a0a1b85..3067dfc49a 100644 --- a/src/main/java/com/google/api/generator/engine/ast/MethodDefinition.java +++ b/src/main/java/com/google/api/generator/engine/ast/MethodDefinition.java @@ -21,6 +21,7 @@ import java.util.Collections; import java.util.LinkedHashSet; import java.util.List; +import java.util.Set; import java.util.stream.Collectors; import javax.annotation.Nullable; @@ -169,6 +170,8 @@ public Builder setReturnExpr(Expr expr) { // Private accessors. abstract String name(); + abstract ImmutableList arguments(); + abstract ImmutableList headerCommentStatements(); abstract ImmutableList annotations(); @@ -314,28 +317,66 @@ public MethodDefinition build() { } } - for (VariableExpr varExpr : method.arguments()) { - Preconditions.checkState( - varExpr.isDecl(), - String.format( - "Argument %s must be a variable declaration", varExpr.variable().identifier())); - } - - for (TypeNode exceptionType : method.throwsExceptions()) { - Preconditions.checkState( - TypeNode.isExceptionType(exceptionType), - String.format("Type %s is not an exception type", exceptionType.reference())); - Preconditions.checkState( - !RUNTIME_EXCEPTION_REFERENCE.isAssignableFrom(exceptionType.reference()), - String.format( - "RuntimeException type %s does not need to be thrown", - exceptionType.reference().name())); - } + performArgumentChecks(); + performThrownExceptionChecks(); return method; } - void performNullChecks() { + private void performArgumentChecks() { + // Must be a declaration. + arguments().stream() + .forEach( + varExpr -> + Preconditions.checkState( + varExpr.isDecl(), + String.format( + "Argument %s must be a variable declaration", + varExpr.variable().identifier()))); + // No modifiers allowed. + arguments().stream() + .forEach( + varExpr -> + Preconditions.checkState( + varExpr.scope().equals(ScopeNode.LOCAL) + && !varExpr.isStatic() + && !varExpr.isVolatile(), + String.format( + "Argument %s must have local scope, and cannot have \"static\" or" + + " \"volatile\" modifiers", + varExpr.variable().identifier()))); + + // Check that there aren't any arguments with duplicate names. + List allArgNames = + arguments().stream() + .map(v -> v.variable().identifier().name()) + .collect(Collectors.toList()); + Set duplicateArgNames = + allArgNames.stream() + .filter(n -> Collections.frequency(allArgNames, n) > 1) + .collect(Collectors.toSet()); + Preconditions.checkState( + duplicateArgNames.isEmpty(), + String.format( + "Lambda arguments cannot have duplicate names: %s", duplicateArgNames.toString())); + } + + private void performThrownExceptionChecks() { + throwsExceptions().stream() + .forEach( + exceptionType -> { + Preconditions.checkState( + TypeNode.isExceptionType(exceptionType), + String.format("Type %s is not an exception type", exceptionType.reference())); + Preconditions.checkState( + !RUNTIME_EXCEPTION_REFERENCE.isAssignableFrom(exceptionType.reference()), + String.format( + "RuntimeException type %s does not need to be thrown", + exceptionType.reference().name())); + }); + } + + private void performNullChecks() { String contextInfo = String.format("method definition of %s", name()); NodeValidator.checkNoNullElements(headerCommentStatements(), "header comments", contextInfo); NodeValidator.checkNoNullElements(annotations(), "annotations", contextInfo); diff --git a/src/test/java/com/google/api/generator/engine/ast/MethodDefinitionTest.java b/src/test/java/com/google/api/generator/engine/ast/MethodDefinitionTest.java index 5cde3a0859..96e09352cc 100644 --- a/src/test/java/com/google/api/generator/engine/ast/MethodDefinitionTest.java +++ b/src/test/java/com/google/api/generator/engine/ast/MethodDefinitionTest.java @@ -36,6 +36,16 @@ public void validMethodDefinition_basicWithComments() { // No exception thrown, we're good. } + @Test + public void validMethodDefinition_emptyBody() { + MethodDefinition.builder() + .setHeaderCommentStatements(createCommentStatements()) + .setName("close") + .setScope(ScopeNode.PUBLIC) + .setReturnType(TypeNode.VOID) + .build(); + } + @Test public void validMethodDefinition_repeatedAnnotations() { MethodDefinition method = @@ -86,7 +96,6 @@ public void validMethodDefinition_throwInsteadOfReturnType() { ConcreteReference.withClazz(NullPointerException.class))) .build()))) .build(); - // No exception thrown, we're good. } @Test @@ -105,7 +114,6 @@ public void validMethodDefinition_voidThrowInsteadOfReturnType() { ConcreteReference.withClazz(NullPointerException.class))) .build()))) .build(); - // No exception thrown, we're good. } @Test @@ -645,6 +653,30 @@ public void invalidMethodDefinition_mismatchedPrimitiveToObjectReturnType() { }); } + @Test + public void invalidMethodDefinition_repeatedArgumentName() { + ValueExpr returnValueExpr = + ValueExpr.builder() + .setValue(PrimitiveValue.builder().setType(TypeNode.INT).setValue("3").build()) + .build(); + List arguments = + Arrays.asList( + VariableExpr.builder().setVariable(createVariable("x", TypeNode.INT)).build(), + VariableExpr.builder().setVariable(createVariable("x", TypeNode.STRING)).build()); + assertThrows( + IllegalStateException.class, + () -> { + MethodDefinition.builder() + .setName("close") + .setScope(ScopeNode.PUBLIC) + .setReturnType(TypeNode.INT) + .setArguments(arguments) + .setReturnExpr(returnValueExpr) + .setBody(Arrays.asList(ExprStatement.withExpr(createAssignmentExpr()))) + .build(); + }); + } + @Test public void invalidMethodDefinition_nonDeclArguments() { ValueExpr returnValueExpr = @@ -669,6 +701,59 @@ public void invalidMethodDefinition_nonDeclArguments() { }); } + @Test + public void invalidMethodDefinition_argumentsWithModifiers() { + ValueExpr returnValueExpr = + ValueExpr.builder() + .setValue(PrimitiveValue.builder().setType(TypeNode.INT).setValue("3").build()) + .build(); + List arguments = + Arrays.asList( + VariableExpr.builder() + .setVariable(createVariable("x", TypeNode.INT)) + .setIsDecl(true) + .setIsStatic(true) + .build(), + VariableExpr.builder().setVariable(createVariable("y", TypeNode.INT)).build()); + assertThrows( + IllegalStateException.class, + () -> { + MethodDefinition.builder() + .setName("close") + .setScope(ScopeNode.PUBLIC) + .setReturnType(TypeNode.INT) + .setArguments(arguments) + .setReturnExpr(returnValueExpr) + .build(); + }); + } + + @Test + public void invalidMethodDefinition_argumentsWithScope() { + ValueExpr returnValueExpr = + ValueExpr.builder() + .setValue(PrimitiveValue.builder().setType(TypeNode.INT).setValue("3").build()) + .build(); + List arguments = + Arrays.asList( + VariableExpr.builder().setVariable(createVariable("x", TypeNode.INT)).build(), + VariableExpr.builder() + .setVariable(createVariable("y", TypeNode.INT)) + .setScope(ScopeNode.PRIVATE) + .build()); + assertThrows( + IllegalStateException.class, + () -> { + MethodDefinition.builder() + .setName("close") + .setScope(ScopeNode.PUBLIC) + .setReturnType(TypeNode.INT) + .setArguments(arguments) + .setReturnExpr(returnValueExpr) + .build(); + }); + } + @Test public void invalidMethodDefinition_nullReturnType() { assertThrows(