Skip to content

Commit

Permalink
feat(ast): extend support for annotation named parameters (#1012)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
zhumin8 committed Sep 6, 2022
1 parent 66782d1 commit 5d3ff75
Show file tree
Hide file tree
Showing 5 changed files with 315 additions and 13 deletions.
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -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
Expand All @@ -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<Expr> descriptionExprs();

@Override
public void accept(AstNodeVisitor visitor) {
Expand All @@ -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<Expr> exprList) {
return AnnotationNode.builder().setType(type).setDescriptions(exprList).build();
}

public static AnnotationNode withSuppressWarnings(String description) {
return withTypeAndDescription(annotationType(SuppressWarnings.class), description);
}
Expand All @@ -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<Expr> 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<Expr> exprList) {
return setDescriptionExprs(exprList);
}

// this method is private, and called only by addDescription(AssignmentExpr expr)
private Builder addDescriptionToList(Expr expr) {
List<Expr> 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<Expr> descriptionExprs);

abstract AnnotationNode autoBuild();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -43,6 +45,9 @@ public abstract class VariableExpr implements Expr {

public abstract boolean isVolatile();

// Optional
public abstract ImmutableList<AnnotationNode> annotations();

// Please use this only in conjunction with methods.
// Supports only parameterized types like Map<K, V>.
// TODO(unsupported): Fully generic arguments, e.g. foobar(K key, V value).
Expand Down Expand Up @@ -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();
Expand All @@ -102,6 +108,10 @@ public abstract static class Builder {

public abstract Builder setIsVolatile(boolean isVolatile);

public abstract Builder setAnnotations(List<AnnotationNode> annotations);

abstract ImmutableList<AnnotationNode> annotations();

// This should be used only for method arguments.
public abstract Builder setTemplateObjects(List<Object> objects);

Expand Down Expand Up @@ -133,7 +143,20 @@ public VariableExpr build() {
})
.collect(Collectors.toList()));

// Remove duplicates while maintaining insertion order.
ImmutableList<AnnotationNode> 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),
Expand Down
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down Expand Up @@ -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());
}
}
Loading

0 comments on commit 5d3ff75

Please sign in to comment.