From 5d3ff75db429d6dd43b0c3fc5dd4bfcb5ad06ae4 Mon Sep 17 00:00:00 2001 From: Min Zhu Date: Tue, 6 Sep 2022 09:46:08 -0400 Subject: [PATCH] feat(ast): extend support for annotation named parameters (#1012) This change is to extend annotation arguments support. **Before**: Only one argument that consist of a single `String` can be added to annotation description. **After**: For single unnamed parameter/description, add support for `ValueExpr` and `VariableExpr` in addition to `String`. Also allow adding multiple named parameters of type `VariableExpr`. So that annotations like below can be generated: ``` @ConditionalOnClass(VisionServiceClient.class) @ConditionalOnProperty(value = "spring.cloud.gcp.vision.enabled", matchIfMissing = true) ``` this improvement is an addition and does not affect existing usage of `setDescription(String description)`. Added tests in [JavaWriterVisitorTest.java](https://github.com/googleapis/gapic-generator-java/pull/1012/files#diff-60163f71e845b4cc97f9ccd7101646aa243fa5317a77cd326a7f61143618f62f) also shows the new feature usages. Adding a small enhancement to this PR since it's related. In addition to previous changes, I added an optional field to VariableExpr allowing Annotations. Prior to this, only annotations on ClassDefinition and MethodDefinition are supported. With this addition, I will be able to generate code like: ``` @NestedConfigurationProperty private final Credentials credentials = new Credentials(); // or @Autowired private LanguageServiceClient autoClient; ``` Field annotations are pretty common in Spring syntax, this enhancement is needed to generate Spring autoconfig code as I planned. Note: Added a todo note in `VariableExpr` only as a nice to have feature for future, it's not blocking any use cases for now. But it seems reasonable to have target info in `AnnotationNode` and apply checks on it when adding annotation nodes. --- .../generator/engine/ast/AnnotationNode.java | 86 ++++++++- .../generator/engine/ast/VariableExpr.java | 25 ++- .../engine/writer/JavaWriterVisitor.java | 15 +- .../engine/ast/VariableExprTest.java | 31 ++++ .../engine/writer/JavaWriterVisitorTest.java | 171 +++++++++++++++++- 5 files changed, 315 insertions(+), 13 deletions(-) diff --git a/src/main/java/com/google/api/generator/engine/ast/AnnotationNode.java b/src/main/java/com/google/api/generator/engine/ast/AnnotationNode.java index 1a3412aa6c..45464b333b 100644 --- a/src/main/java/com/google/api/generator/engine/ast/AnnotationNode.java +++ b/src/main/java/com/google/api/generator/engine/ast/AnnotationNode.java @@ -1,4 +1,4 @@ -// Copyright 2020 Google LLC +// Copyright 2022 Google LLC // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -16,6 +16,9 @@ import com.google.auto.value.AutoValue; import com.google.common.base.Preconditions; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; import javax.annotation.Nullable; @AutoValue @@ -31,10 +34,8 @@ private static TypeNode annotationType(Class clazz) { public abstract TypeNode type(); - // TODO(unsupported): Any args that do not consist of a single string. However, this can easily be - // extended to enable such support. @Nullable - public abstract Expr descriptionExpr(); + public abstract List descriptionExprs(); @Override public void accept(AstNodeVisitor visitor) { @@ -45,6 +46,10 @@ public static AnnotationNode withTypeAndDescription(TypeNode type, String descri return AnnotationNode.builder().setType(type).setDescription(description).build(); } + public static AnnotationNode withTypeAndDescription(TypeNode type, List exprList) { + return AnnotationNode.builder().setType(type).setDescriptions(exprList).build(); + } + public static AnnotationNode withSuppressWarnings(String description) { return withTypeAndDescription(annotationType(SuppressWarnings.class), description); } @@ -59,15 +64,80 @@ public static Builder builder() { @AutoValue.Builder public abstract static class Builder { + private static final String REPEAT_SINGLE_EXCEPTION_MESSAGE = + "Single parameter with no name cannot be set multiple times"; + + private static final String MULTIPLE_AFTER_SINGLE_EXCEPTION_MESSAGE = + "Multiple parameters must have names"; + + abstract List descriptionExprs(); + public abstract Builder setType(TypeNode type); + /** + * To set single String as description. + * + * @param description + * @return Builder + */ public Builder setDescription(String description) { - return setDescriptionExpr(ValueExpr.withValue(StringObjectValue.withValue(description))); + Preconditions.checkState(descriptionExprs() == null, REPEAT_SINGLE_EXCEPTION_MESSAGE); + return setDescriptionExprs( + Arrays.asList(ValueExpr.withValue(StringObjectValue.withValue(description)))); + } + + /** + * To set single ValueExpr as description. + * + * @param valueExpr + * @return Builder + */ + public Builder setDescription(ValueExpr valueExpr) { + Preconditions.checkState(descriptionExprs() == null, REPEAT_SINGLE_EXCEPTION_MESSAGE); + return setDescriptionExprs(Arrays.asList(valueExpr)); + } + + /** + * To set single VariableExpr as description. + * + * @param variableExpr + * @return Builder + */ + public Builder setDescription(VariableExpr variableExpr) { + Preconditions.checkState(descriptionExprs() == null, REPEAT_SINGLE_EXCEPTION_MESSAGE); + return setDescriptionExprs(Arrays.asList(variableExpr)); + } + + /** + * To add an AssignmentExpr as parameter. Can be used repeatedly to add multiple parameters. + * + * @param assignmentExpr + * @return Builder + */ + public Builder addDescription(AssignmentExpr assignmentExpr) { + return addDescriptionToList(assignmentExpr); + } + + private Builder setDescriptions(List exprList) { + return setDescriptionExprs(exprList); + } + + // this method is private, and called only by addDescription(AssignmentExpr expr) + private Builder addDescriptionToList(Expr expr) { + List exprList = descriptionExprs(); + // avoid when single parameter is already set. + Preconditions.checkState( + exprList == null || exprList instanceof ArrayList, + MULTIPLE_AFTER_SINGLE_EXCEPTION_MESSAGE); + if (exprList == null) { + exprList = new ArrayList<>(); + } + exprList.add(expr); + return setDescriptions(exprList); } - // This will never be anything other than a ValueExpr-wrapped StringObjectValue because - // this setter is private, and called only by setDescription above. - abstract Builder setDescriptionExpr(Expr descriptionExpr); + // this setter is private, and called only by setDescription() and setDescriptions() above. + abstract Builder setDescriptionExprs(List descriptionExprs); abstract AnnotationNode autoBuild(); diff --git a/src/main/java/com/google/api/generator/engine/ast/VariableExpr.java b/src/main/java/com/google/api/generator/engine/ast/VariableExpr.java index 404afd516f..f882c97e8d 100644 --- a/src/main/java/com/google/api/generator/engine/ast/VariableExpr.java +++ b/src/main/java/com/google/api/generator/engine/ast/VariableExpr.java @@ -18,6 +18,8 @@ import com.google.auto.value.AutoValue; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; +import java.util.Collections; +import java.util.LinkedHashSet; import java.util.List; import java.util.stream.Collectors; import javax.annotation.Nullable; @@ -43,6 +45,9 @@ public abstract class VariableExpr implements Expr { public abstract boolean isVolatile(); + // Optional + public abstract ImmutableList annotations(); + // Please use this only in conjunction with methods. // Supports only parameterized types like Map. // TODO(unsupported): Fully generic arguments, e.g. foobar(K key, V value). @@ -77,7 +82,8 @@ public static Builder builder() { .setIsStatic(false) .setIsVolatile(false) .setScope(ScopeNode.LOCAL) - .setTemplateObjects(ImmutableList.of()); + .setTemplateObjects(ImmutableList.of()) + .setAnnotations(Collections.emptyList()); } public abstract Builder toBuilder(); @@ -102,6 +108,10 @@ public abstract static class Builder { public abstract Builder setIsVolatile(boolean isVolatile); + public abstract Builder setAnnotations(List annotations); + + abstract ImmutableList annotations(); + // This should be used only for method arguments. public abstract Builder setTemplateObjects(List objects); @@ -133,7 +143,20 @@ public VariableExpr build() { }) .collect(Collectors.toList())); + // Remove duplicates while maintaining insertion order. + ImmutableList processedAnnotations = annotations(); + setAnnotations( + new LinkedHashSet<>(processedAnnotations).stream().collect(Collectors.toList())); + VariableExpr variableExpr = autoBuild(); + + // TODO: should match on AnnotationNode @Target of ElementType.FIELD + if (!variableExpr.isDecl()) { + Preconditions.checkState( + variableExpr.annotations().isEmpty(), + "Annotation can only be added to variable declaration."); + } + if (variableExpr.isDecl() || variableExpr.exprReferenceExpr() != null) { Preconditions.checkState( variableExpr.isDecl() ^ (variableExpr.exprReferenceExpr() != null), diff --git a/src/main/java/com/google/api/generator/engine/writer/JavaWriterVisitor.java b/src/main/java/com/google/api/generator/engine/writer/JavaWriterVisitor.java index 0a867b7cdb..731411bab6 100644 --- a/src/main/java/com/google/api/generator/engine/writer/JavaWriterVisitor.java +++ b/src/main/java/com/google/api/generator/engine/writer/JavaWriterVisitor.java @@ -1,4 +1,4 @@ -// Copyright 2020 Google LLC +// Copyright 2022 Google LLC // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -169,9 +169,15 @@ public void visit(ScopeNode scope) { public void visit(AnnotationNode annotation) { buffer.append(AT); annotation.type().accept(this); - if (annotation.descriptionExpr() != null) { + if (annotation.descriptionExprs() != null) { leftParen(); - annotation.descriptionExpr().accept(this); + for (int i = 0; i < annotation.descriptionExprs().size(); i++) { + annotation.descriptionExprs().get(i).accept(this); + if (i < annotation.descriptionExprs().size() - 1) { + buffer.append(COMMA); + buffer.append(SPACE); + } + } rightParen(); } newline(); @@ -253,6 +259,9 @@ public void visit(VariableExpr variableExpr) { // VariableExpr will handle isDecl and exprReferenceExpr edge cases. if (variableExpr.isDecl()) { + // Annotations, if any. + annotations(variableExpr.annotations()); + if (!scope.equals(ScopeNode.LOCAL)) { scope.accept(this); space(); diff --git a/src/test/java/com/google/api/generator/engine/ast/VariableExprTest.java b/src/test/java/com/google/api/generator/engine/ast/VariableExprTest.java index 9e8b87e353..43a5fd4f09 100644 --- a/src/test/java/com/google/api/generator/engine/ast/VariableExprTest.java +++ b/src/test/java/com/google/api/generator/engine/ast/VariableExprTest.java @@ -173,6 +173,26 @@ public void validVariableExpr_templatedArgNameAndTypeInMethod() { .containsExactly(IdentifierNode.withName("RequestT"), TypeNode.STRING); } + @Test + public void validVariableExpr_declarationWithAnnotations() { + Variable variable = Variable.builder().setName("x").setType(TypeNode.BOOLEAN).build(); + VariableExpr variableExpr = + VariableExpr.builder() + .setVariable(variable) + .setIsDecl(true) + .setAnnotations( + Arrays.asList( + AnnotationNode.withSuppressWarnings("all"), + AnnotationNode.DEPRECATED, + AnnotationNode.DEPRECATED)) + .build(); + assertThat(variableExpr.variable()).isEqualTo(variable); + assertThat(variableExpr.type()).isEqualTo(TypeNode.VOID); + assertThat(variableExpr.isDecl()).isTrue(); + assertThat(variableExpr.annotations()) + .containsExactly(AnnotationNode.withSuppressWarnings("all"), AnnotationNode.DEPRECATED); + } + @Test public void invalidVariableExpr_templatedArgInMethodHasNonStringNonTypeNodeObject() { Variable variable = @@ -288,4 +308,15 @@ public void invalidVariableExpr_classFieldOnPrimitiveType() { .build()) .build()); } + + @Test + public void invalidVariableExpr_annotationNoDeclaration() { + Variable variable = Variable.builder().setName("x").setType(TypeNode.BOOLEAN).build(); + VariableExpr.Builder variableExprBuilder = + VariableExpr.builder() + .setVariable(variable) + .setIsDecl(false) + .setAnnotations(Arrays.asList(AnnotationNode.DEPRECATED)); + assertThrows(IllegalStateException.class, () -> variableExprBuilder.build()); + } } diff --git a/src/test/java/com/google/api/generator/engine/writer/JavaWriterVisitorTest.java b/src/test/java/com/google/api/generator/engine/writer/JavaWriterVisitorTest.java index 75f25c5b94..713aa57939 100644 --- a/src/test/java/com/google/api/generator/engine/writer/JavaWriterVisitorTest.java +++ b/src/test/java/com/google/api/generator/engine/writer/JavaWriterVisitorTest.java @@ -16,6 +16,7 @@ import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThrows; import com.google.api.generator.engine.ast.AnnotationNode; import com.google.api.generator.engine.ast.AnonymousClassExpr; @@ -152,12 +153,161 @@ public void writeAnnotation_simple() { } @Test - public void writeAnnotation_withDescription() { + public void writeAnnotation_withStringDescription() { AnnotationNode annotation = AnnotationNode.withSuppressWarnings("all"); annotation.accept(writerVisitor); assertEquals("@SuppressWarnings(\"all\")\n", writerVisitor.write()); } + @Test + public void writeAnnotation_withValueDescription() { + TypeNode fakeAnnotationType = + TypeNode.withReference( + VaporReference.builder().setName("FakeAnnotation").setPakkage("com.foo.bar").build()); + AnnotationNode annotation = + AnnotationNode.builder() + .setType(fakeAnnotationType) + .setDescription( + ValueExpr.withValue( + PrimitiveValue.builder().setValue("12").setType(TypeNode.INT).build())) + .build(); + annotation.accept(writerVisitor); + assertEquals("@FakeAnnotation(12)\n", writerVisitor.write()); + } + + @Test + public void writeAnnotation_withVariableExprDescription() { + TypeNode conditionalOnPropertyType = + TypeNode.withReference( + VaporReference.builder() + .setName("ConditionalOnClass") + .setPakkage("org.springframework.boot.autoconfigure.condition") + .build()); + TypeNode clazzType = + TypeNode.withReference( + VaporReference.builder() + .setName("VisionServiceClient") + .setPakkage("com.foo.bar") + .build()); + + AnnotationNode annotation = + AnnotationNode.builder() + .setType(conditionalOnPropertyType) + .setDescription( + VariableExpr.builder() + .setVariable( + Variable.builder().setType(TypeNode.CLASS_OBJECT).setName("class").build()) + .setStaticReferenceType(clazzType) + .build()) + .build(); + annotation.accept(writerVisitor); + assertEquals("@ConditionalOnClass(VisionServiceClient.class)\n", writerVisitor.write()); + } + + @Test + public void writeAnnotation_withMultipleNamedDescriptions() { + TypeNode conditionalOnPropertyType = + TypeNode.withReference( + VaporReference.builder() + .setName("ConditionalOnProperty") + .setPakkage("org.springframework.boot.autoconfigure.condition") + .build()); + + AssignmentExpr matchIfMissing = + AssignmentExpr.builder() + .setVariableExpr( + VariableExpr.withVariable( + Variable.builder().setName("matchIfMissing").setType(TypeNode.BOOLEAN).build())) + .setValueExpr( + ValueExpr.withValue( + PrimitiveValue.builder().setValue("false").setType(TypeNode.BOOLEAN).build())) + .build(); + AssignmentExpr valueString = + AssignmentExpr.builder() + .setVariableExpr( + VariableExpr.withVariable( + Variable.builder().setName("value").setType(TypeNode.STRING).build())) + .setValueExpr( + ValueExpr.withValue( + StringObjectValue.withValue("spring.cloud.gcp.language.enabled"))) + .build(); + AnnotationNode annotation = + AnnotationNode.builder() + .setType(conditionalOnPropertyType) + .addDescription(valueString) + .addDescription(matchIfMissing) + .build(); + annotation.accept(writerVisitor); + assertEquals( + "@ConditionalOnProperty(value = \"spring.cloud.gcp.language.enabled\", matchIfMissing = false)\n", + writerVisitor.write()); + } + + @Test + public void writeAnnotation_withInvalidDescriptions() { + TypeNode fakeAnnotationType = + TypeNode.withReference( + VaporReference.builder().setName("FakeAnnotation").setPakkage("com.foo.bar").build()); + String stringA = "a string"; + String stringB = "another string"; + ValueExpr valueExpr = + ValueExpr.withValue(PrimitiveValue.builder().setValue("12").setType(TypeNode.INT).build()); + + VariableExpr clazzVariableExpr = + VariableExpr.builder() + .setVariable(Variable.builder().setType(TypeNode.CLASS_OBJECT).setName("class").build()) + .setExprReferenceExpr( + MethodInvocationExpr.builder() + .setMethodName("foobar") + .setReturnType(TypeNode.INT_OBJECT) + .build()) + .build(); + AssignmentExpr valueStringAssignmentExpr = + AssignmentExpr.builder() + .setVariableExpr( + VariableExpr.withVariable( + Variable.builder().setName("value").setType(TypeNode.STRING).build())) + .setValueExpr( + ValueExpr.withValue( + StringObjectValue.withValue("spring.cloud.gcp.language.enabled"))) + .build(); + + AnnotationNode.Builder annotationBuilder = + AnnotationNode.builder().setType(fakeAnnotationType).setDescription(stringA); + + IllegalStateException exceptionForSetDescription = + assertThrows( + IllegalStateException.class, + () -> { + annotationBuilder.setDescription(valueExpr); + }); + assertThat(exceptionForSetDescription) + .hasMessageThat() + .contains("Single parameter with no name cannot be set multiple times"); + + assertThrows( + IllegalStateException.class, + () -> { + annotationBuilder.setDescription(stringB); + }); + + assertThrows( + IllegalStateException.class, + () -> { + annotationBuilder.setDescription(clazzVariableExpr); + }); + + IllegalStateException exceptionForAddDescription = + assertThrows( + IllegalStateException.class, + () -> { + annotationBuilder.addDescription(valueStringAssignmentExpr); + }); + assertThat(exceptionForAddDescription) + .hasMessageThat() + .contains("Multiple parameters must have names"); + } + @Test public void writeNewObjectExpr_basic() { // isGeneric() is true, but generics() is empty. @@ -737,6 +887,25 @@ public void writeAssignmentExpr_stringObjectValue() { assertThat(writerVisitor.write()).isEqualTo("String x = \"Hi! World. \\n\""); } + @Test + public void writeAssignmentExpr_variableDeclarationWithAnnotation() { + Variable variable = Variable.builder().setName("x").setType(TypeNode.STRING).build(); + VariableExpr variableExpr = + VariableExpr.builder() + .setVariable(variable) + .setIsDecl(true) + .setAnnotations(Arrays.asList(AnnotationNode.DEPRECATED, AnnotationNode.DEPRECATED)) + .build(); + + Value value = StringObjectValue.withValue("Hi! World. \n"); + Expr valueExpr = ValueExpr.builder().setValue(value).build(); + AssignmentExpr assignExpr = + AssignmentExpr.builder().setVariableExpr(variableExpr).setValueExpr(valueExpr).build(); + + assignExpr.accept(writerVisitor); + assertThat(writerVisitor.write()).isEqualTo("@Deprecated\nString x = \"Hi! World. \\n\""); + } + @Test public void writeMethodInvocationExpr_basic() { MethodInvocationExpr methodExpr =