Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

review fix(annotation): bug when using an annotation targeting declaration and types #1774

Merged
merged 13 commits into from
Dec 12, 2017
Merged

review fix(annotation): bug when using an annotation targeting declaration and types #1774

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
0a684b2
Create new unit tests to display bug
surli Dec 5, 2017
af50c5b
Remove one test because of #520
surli Dec 5, 2017
e9aa2e9
Change the contract of the test
surli Dec 8, 2017
718443e
Fix issue and all tests
surli Dec 8, 2017
153f1e5
Fix checkstyle
surli Dec 8, 2017
855c505
Fix tests
surli Dec 8, 2017
19a26ba
Fix corner case
surli Dec 8, 2017
d49f763
Remove ugly try catch
surli Dec 8, 2017
28871ad
Fix checkstyle
surli Dec 8, 2017
74a9411
Try to understand the test error
surli Dec 8, 2017
b353b9f
Fix checkstyle and element printer helper
surli Dec 8, 2017
92eb3f0
Fix checkstyle!
surli Dec 8, 2017
87115d5
Fix comments
surli Dec 11, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions src/main/java/spoon/reflect/declaration/CtAnnotation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@
import spoon.reflect.code.CtExpression;
import spoon.reflect.code.CtFieldAccess;
import spoon.reflect.code.CtLiteral;
import spoon.reflect.code.CtLocalVariable;
import spoon.reflect.code.CtNewArray;
import spoon.reflect.reference.CtTypeParameterReference;
import spoon.reflect.reference.CtTypeReference;
import spoon.support.DerivedProperty;
import spoon.reflect.annotations.PropertyGetter;
Expand Down Expand Up @@ -165,4 +167,42 @@ public interface CtAnnotation<A extends Annotation> extends CtExpression<A>, CtS
@Override
@UnsettableProperty
<C extends CtExpression<A>> C setTypeCasts(List<CtTypeReference<?>> types);

static CtAnnotatedElementType getAnnotatedElementTypeForCtElement(CtElement element) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use an anonymous scanner to avoid multiple instanceof?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how to use easily an anonymous scanner here: I must return a variable and I cannot store it as a local variable if I use the anonymous scanner. And I don't have any method to call to setup the value.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, it's not straightforward.

I must return a variable and I cannot store it as a local variable if I use the anonymous scanner

Unless you create an inline class

class Result { CtVariable v }
final Result result = new Result();
// then the anonymous class

if (element == null) {
return null;
}

if (element instanceof CtMethod) {
return CtAnnotatedElementType.METHOD;
}
if (element instanceof CtAnnotation || element instanceof CtAnnotationType) {
return CtAnnotatedElementType.ANNOTATION_TYPE;
}
if (element instanceof CtType) {
return CtAnnotatedElementType.TYPE;
}
if (element instanceof CtField) {
return CtAnnotatedElementType.FIELD;
}
if (element instanceof CtConstructor) {
return CtAnnotatedElementType.CONSTRUCTOR;
}
if (element instanceof CtParameter) {
return CtAnnotatedElementType.PARAMETER;
}
if (element instanceof CtLocalVariable) {
return CtAnnotatedElementType.LOCAL_VARIABLE;
}
if (element instanceof CtPackage) {
return CtAnnotatedElementType.PACKAGE;
}
if (element instanceof CtTypeParameterReference) {
return CtAnnotatedElementType.TYPE_PARAMETER;
}
if (element instanceof CtTypeReference) {
return CtAnnotatedElementType.TYPE_USE;
}
return null;
}
}
5 changes: 5 additions & 0 deletions src/main/java/spoon/reflect/visitor/ElementPrinterHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import spoon.reflect.declaration.CtType;
import spoon.reflect.declaration.CtTypeMember;
import spoon.reflect.declaration.CtTypeParameter;
import spoon.reflect.declaration.CtTypedElement;
import spoon.reflect.declaration.ModifierKind;
import spoon.reflect.factory.Factory;
import spoon.reflect.reference.CtActualTypeContainer;
Expand Down Expand Up @@ -77,6 +78,10 @@ public ElementPrinterHelper(TokenWriter printerTokenWriter, DefaultJavaPrettyPri
*/
public void writeAnnotations(CtElement element) {
for (CtAnnotation<?> annotation : element.getAnnotations()) {
if (element.isParentInitialized() && element instanceof CtTypeReference && (element.getParent() instanceof CtTypedElement) && element.getParent().getAnnotations().contains(annotation)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

explain this complex condition with a fat comment?

continue;
}

prettyPrinter.scan(annotation);
printer.writeln();
}
Expand Down
27 changes: 21 additions & 6 deletions src/main/java/spoon/support/compiler/jdt/JDTTreeBuilderQuery.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,22 +169,37 @@ static boolean hasAnnotationWithType(Annotation a, CtAnnotatedElementType elemen
if (a.resolvedType == null) {
return false;
}

// JLS says:
// "If an annotation of type java.lang.annotation.Target is not present on the declaration of an annotation type T,
// then T is applicable in all declaration contexts except type parameter declarations, and in no type contexts."
boolean shouldTargetAnnotationExists = (elementType == CtAnnotatedElementType.TYPE_USE || elementType == CtAnnotatedElementType.TYPE_PARAMETER);
boolean targetAnnotationExists = false;

for (AnnotationBinding annotation : a.resolvedType.getAnnotations()) {
if (!"Target".equals(CharOperation.charToString(annotation.getAnnotationType().sourceName()))) {
continue;
}
targetAnnotationExists = true;
Object value = annotation.getElementValuePairs()[0].value;
if (value == null || !value.getClass().isArray()) {
if (value == null) {
continue;
}
Object[] fields = (Object[]) value;
for (Object field : fields) {
if (field instanceof FieldBinding && elementType.name().equals(CharOperation.charToString(((FieldBinding) field).name))) {
return true;
if (value instanceof FieldBinding && elementType.name().equals(CharOperation.charToString(((FieldBinding) value).name))) {
return true;
}
if (value.getClass().isArray()) {
Object[] fields = (Object[]) value;
for (Object field : fields) {
if (field instanceof FieldBinding && elementType.name().equals(CharOperation.charToString(((FieldBinding) field).name))) {
return true;
}
}
}
}
return false;

// true here means that the target annotation is not mandatory and we don't have found it
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't have -> have not

return !shouldTargetAnnotationExists && !targetAnnotationExists;
}

/**
Expand Down
36 changes: 25 additions & 11 deletions src/main/java/spoon/support/compiler/jdt/ParentExiter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ public class ParentExiter extends CtInheritanceScanner {

private CtElement child;
private ASTNode childJDT;
private Map<CtTypedElement<?>, List<CtTypeReference<? extends java.lang.annotation.Annotation>>> annotationsMap = new HashMap<>();
private Map<CtTypedElement<?>, List<CtAnnotation>> annotationsMap = new HashMap<>();

/**
* @param jdtTreeBuilder
Expand All @@ -133,29 +133,43 @@ public void setChild(ASTNode child) {
@Override
public void scanCtElement(CtElement e) {
if (child instanceof CtAnnotation && this.jdtTreeBuilder.getContextBuilder().annotationValueName.isEmpty()) {
e.addAnnotation((CtAnnotation<?>) child);
// we check if the current element can have the annotation attached
CtAnnotatedElementType annotatedElementType = CtAnnotation.getAnnotatedElementTypeForCtElement(e);
annotatedElementType = (e instanceof CtTypeParameter || e instanceof CtTypeParameterReference) ? CtAnnotatedElementType.TYPE_USE : annotatedElementType;

// in case of noclasspath, we can be 100% sure, so we guess it must be attached...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"can be sure" or "cannot be sure"?

if (this.jdtTreeBuilder.getFactory().getEnvironment().getNoClasspath() || (annotatedElementType != null && JDTTreeBuilderQuery.hasAnnotationWithType((Annotation) childJDT, annotatedElementType))) {
e.addAnnotation((CtAnnotation<?>) child);
}

// in this case the annotation should be (also) attached to the type
if (e instanceof CtTypedElement && JDTTreeBuilderQuery.hasAnnotationWithType((Annotation) childJDT, CtAnnotatedElementType.TYPE_USE)) {
List<CtTypeReference<? extends java.lang.annotation.Annotation>> annotations = new ArrayList<>();
List<CtAnnotation> annotations = new ArrayList<>();
if (!annotationsMap.containsKey(e)) {
annotationsMap.put((CtTypedElement<?>) e, annotations);
} else {
annotations = annotationsMap.get(e);
}
annotations.add(((CtAnnotation) child).getType());
annotations.add((CtAnnotation) child.clone());
annotationsMap.put((CtTypedElement<?>) e, annotations);
}
return;
}
}

private void substituteAnnotation(CtTypedElement ele) {
if (annotationsMap.containsKey(ele)) {
List<CtTypeReference<? extends java.lang.annotation.Annotation>> annotations = annotationsMap.get(ele);
for (CtTypeReference<? extends java.lang.annotation.Annotation> annotation : annotations) {
final CtAnnotation<? extends java.lang.annotation.Annotation> targetAnnotation = ele.getAnnotation(annotation);
ele.removeAnnotation(targetAnnotation);
if (!ele.getType().getAnnotations().contains(targetAnnotation)) {
ele.getType().addAnnotation(targetAnnotation);
List<CtAnnotation> annotations = annotationsMap.get(ele);
for (CtAnnotation annotation : annotations) {

// in case of noclasspath we attached previously the element:
// if we are here, we may have find an element for whom it's a better place
if (this.jdtTreeBuilder.getFactory().getEnvironment().getNoClasspath() && annotation.isParentInitialized()) {
CtElement parent = annotation.getParent();
parent.removeAnnotation(annotation);
}

if (!ele.getType().getAnnotations().contains(annotation)) {
ele.getType().addAnnotation(annotation.clone());
}
}
annotationsMap.remove(ele);
Expand Down
41 changes: 1 addition & 40 deletions src/main/java/spoon/support/reflect/declaration/CtAnnotationImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,25 +24,20 @@
import spoon.reflect.code.CtFieldAccess;
import spoon.reflect.code.CtFieldRead;
import spoon.reflect.code.CtLiteral;
import spoon.reflect.code.CtLocalVariable;
import spoon.reflect.code.CtNewArray;
import spoon.reflect.code.CtTypeAccess;
import spoon.reflect.declaration.CtAnnotatedElementType;
import spoon.reflect.declaration.CtAnnotation;
import spoon.reflect.declaration.CtAnnotationMethod;
import spoon.reflect.declaration.CtAnnotationType;
import spoon.reflect.declaration.CtConstructor;
import spoon.reflect.declaration.CtElement;
import spoon.reflect.declaration.CtField;
import spoon.reflect.declaration.CtMethod;
import spoon.reflect.declaration.CtPackage;
import spoon.reflect.declaration.CtParameter;
import spoon.reflect.declaration.CtShadowable;
import spoon.reflect.declaration.CtType;
import spoon.reflect.eval.PartialEvaluator;
import spoon.reflect.path.CtRole;
import spoon.reflect.reference.CtFieldReference;
import spoon.reflect.reference.CtTypeParameterReference;
import spoon.reflect.reference.CtTypeReference;
import spoon.reflect.visitor.CtVisitor;
import spoon.support.DerivedProperty;
Expand Down Expand Up @@ -425,41 +420,7 @@ public CtElement getAnnotatedElement() {
public CtAnnotatedElementType getAnnotatedElementType() {
CtElement annotatedElement = this.getAnnotatedElement();

if (annotatedElement == null) {
return null;
}

if (annotatedElement instanceof CtMethod) {
return CtAnnotatedElementType.METHOD;
}
if (annotatedElement instanceof CtAnnotation || annotatedElement instanceof CtAnnotationType) {
return CtAnnotatedElementType.ANNOTATION_TYPE;
}
if (annotatedElement instanceof CtType) {
return CtAnnotatedElementType.TYPE;
}
if (annotatedElement instanceof CtField) {
return CtAnnotatedElementType.FIELD;
}
if (annotatedElement instanceof CtConstructor) {
return CtAnnotatedElementType.CONSTRUCTOR;
}
if (annotatedElement instanceof CtParameter) {
return CtAnnotatedElementType.PARAMETER;
}
if (annotatedElement instanceof CtLocalVariable) {
return CtAnnotatedElementType.LOCAL_VARIABLE;
}
if (annotatedElement instanceof CtPackage) {
return CtAnnotatedElementType.PACKAGE;
}
if (annotatedElement instanceof CtTypeParameterReference) {
return CtAnnotatedElementType.TYPE_PARAMETER;
}
if (annotatedElement instanceof CtTypeReference) {
return CtAnnotatedElementType.TYPE_USE;
}
return null;
return CtAnnotation.getAnnotatedElementTypeForCtElement(annotatedElement);
}

@SuppressWarnings("unchecked")
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/spoon/testing/utils/ModelUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ public static void canBeBuilt(File outputDirectoryFile, int complianceLevel, boo
try {
compiler.build();
} catch (Exception e) {
final AssertionError error = new AssertionError("Can't compile " + outputDirectoryFile.getName());
final AssertionError error = new AssertionError("Can't compile " + outputDirectoryFile.getName() + " because " + e.getMessage());
error.initCause(e);
throw error;
}
Expand Down
Loading