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.

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

fix: consolidate a consistent behavior for CtElement#getParent #3793

Merged
merged 10 commits into from
Feb 24, 2021
8 changes: 5 additions & 3 deletions src/main/java/spoon/pattern/PatternBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -126,9 +126,11 @@ private CtTypeReference<?> getDeclaringTypeRef(List<? extends CtElement> templat
t = (CtType) ctElement;
type = mergeType(type, t);
}
t = ctElement.getParent(CtType.class);
if (t != null) {
type = mergeType(type, t);
if (ctElement.isParentInitialized()) {
t = ctElement.getParent(CtType.class);
if (t != null) {
type = mergeType(type, t);
}
}
}
return type == null ? null : type.getReference();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,7 @@ private void createPatternParameterForVariable(CtVariable<?> variable) {
}
searchScope.map(new VariableReferenceFunction(variable))
.forEach((CtVariableReference<?> varRef) -> {
CtFieldRead<?> fieldRead = varRef.getParent(CtFieldRead.class);
CtFieldRead<?> fieldRead = varRef.isParentInitialized() ? varRef.getParent(CtFieldRead.class) : null;
if (fieldRead != null) {
addSubstitutionRequest(
parameter(fieldRead.getVariable().getSimpleName()).getCurrentParameter(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ public <T> void visitCtThisAccess(CtThisAccess<T> thisAccess) {
}

protected boolean isRemovedParamOfRefactoredInvocation(CtParameterReference<?> paramRef) {
CtInvocation<?> invocation = paramRef.getParent(CtInvocation.class);
CtInvocation<?> invocation = paramRef.isParentInitialized() ? paramRef.getParent(CtInvocation.class) : null;
if (invocation == null) {
return false;
}
Expand Down
1 change: 0 additions & 1 deletion src/main/java/spoon/reflect/declaration/CtElement.java
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,6 @@ <E extends CtElement> List<E> getAnnotatedChildren(

/**
* Gets the first parent that matches the filter.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above.

* If the receiver (this) matches the filter, it is also returned
*/
<E extends CtElement> E getParent(Filter<E> filter) throws ParentNotInitializedException;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -860,7 +860,10 @@ public <T> void visitCtThisAccess(CtThisAccess<T> thisAccess) {

// the simplest case: we always print "this" if we're in the top-level class,
// this is shorter (no qualified this), explicit, and less fragile wrt transformation
if (targetType == null || (thisAccess.getParent(CtType.class) != null && thisAccess.getParent(CtType.class).isTopLevel())) {
if (targetType == null
|| (thisAccess.isParentInitialized()
&& thisAccess.getParent(CtType.class) != null
&& thisAccess.getParent(CtType.class).isTopLevel())) {
printer.writeKeyword("this");
return; // still go through finally block below
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,9 @@ protected void handleTypeReference(CtTypeReference<?> reference, LexicalScope na
return;
}

if (isTypeReferenceToEnclosingType(nameScope, reference) && reference.getParent(CtAnonymousExecutable.class) != null) {
if (isTypeReferenceToEnclosingType(nameScope, reference)
&& reference.isParentInitialized()
&& reference.getParent(CtAnonymousExecutable.class) != null) {
// for the java compiler, we must keep short version of field accesses in static blocks
return;
}
Expand All @@ -61,7 +63,7 @@ protected void handleTypeReference(CtTypeReference<?> reference, LexicalScope na
}

protected boolean isTypeReferenceToEnclosingType(LexicalScope nameScope, CtTypeReference<?> reference) {
CtType<?> enclosingType = reference.getParent(CtType.class);
CtType<?> enclosingType = reference.isParentInitialized() ? reference.getParent(CtType.class) : null;
if (enclosingType == null) {
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ protected void handleTypeReference(CtTypeReference<?> reference, LexicalScope na
} else {
//it is a reference to an child type
//if it is a reference in scope of parent type declaration then make it implicit, else keep it as it is
CtType<?> contextType = reference.getParent(CtType.class);
CtType<?> contextType = reference.isParentInitialized() ? reference.getParent(CtType.class) : null;
if (contextType != null) {
CtType<?> topLevelType = contextType.getTopLevelType();
CtTypeReference<?> referenceDeclaringType = reference.getDeclaringType();
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/spoon/reflect/visitor/ImportScannerImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,7 @@ public void computeImports(CtElement element) {
addClassImport(simpleType.getReference());
scan(simpleType);
} else {
CtType<?> type = element.getParent(CtType.class);
CtType<?> type = element.isParentInitialized() ? element.getParent(CtType.class) : null;
targetType = type == null ? null : type.getReference().getTopLevelType();
scan(element);
}
Expand Down Expand Up @@ -635,7 +635,7 @@ private boolean declaringTypeIsLocalOrImported(CtTypeReference declaringType) {
* @return
*/
private boolean isInCollisionWithLocalMethod(CtExecutableReference ref) {
CtType<?> typeDecl = ref.getParent(CtType.class);
CtType<?> typeDecl = ref.isParentInitialized() ? ref.getParent(CtType.class) : null;

if (typeDecl != null) {
String methodName = ref.getSimpleName();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ public ParameterScopeFunction(CtScannerListener queryListener) {

@Override
public void apply(CtParameter<?> parameter, CtConsumer<Object> outputConsumer) {
CtExecutable<?> exec = parameter.getParent(CtExecutable.class);
CtExecutable<?> exec = parameter.isParentInitialized() ? parameter.getParent(CtExecutable.class) : null;
if (exec == null) {
//cannot search for parameter references of parameter which has no executable
return;
Expand Down
34 changes: 16 additions & 18 deletions src/main/java/spoon/support/reflect/declaration/CtElementImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -383,34 +383,32 @@ public boolean isParentInitialized() {
@Override
@SuppressWarnings("unchecked")
public <P extends CtElement> P getParent(Class<P> parentType) throws ParentNotInitializedException {
if (parent == null) {
return null;
}
if (parentType.isAssignableFrom(getParent().getClass())) {
return (P) getParent();
}
return getParent().getParent(parentType);
CtElement current = this;
do {
current = current.getParent();
if (parentType.isAssignableFrom(current.getClass())) {
return (P) current;
}
} while (current.isParentInitialized());
Copy link
Collaborator

Choose a reason for hiding this comment

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

The same behavior can be achieved with a standard while loop. You can also get rid of the "unchecked" warning by casting with the parent type instead.

Suggested change
CtElement current = this;
do {
current = current.getParent();
if (parentType.isAssignableFrom(current.getClass())) {
return (P) current;
}
} while (current.isParentInitialized());
CtElement current = getParent();
while (current.isParentInitialized()) {
if (parentType.isAssignableFrom(current.getClass())) {
return parentType.cast(current);
}
current = current.getParent();
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

My suggestion here is incorrect:

The 'do-while' is actually required here. With your 'while' variants if the first parent matches but doesn't have a parent itself, it won't be returned.


return null;
}

@Override
@SuppressWarnings("unchecked")
public <E extends CtElement> E getParent(Filter<E> filter) throws ParentNotInitializedException {
E current = (E) getParent();
while (true) {
CtElement current = this;
do {
current = current.getParent();
try {
while (current != null && !filter.matches(current)) {
current = (E) current.getParent();
if (filter.matches((E) current)) {
return (E) current;
}
break;
} catch (ClassCastException e) {
} catch (ClassCastException ignored) {
// expected, some elements are not of type
current = (E) current.getParent();
}
}
} while (current.isParentInitialized());
Copy link
Collaborator

Choose a reason for hiding this comment

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

A standard while loop can do the same thing (couldn't do a suggestion here due to the fragmented diff):

		CtElement current = getParent();
		while (current.isParentInitialized()) {
			try {
				if (filter.matches((E) current)) {
					return (E) current;
				}
			} catch (ClassCastException ignored) {
				// expected, some elements are not of type
			}
			current = current.getParent();
		};

Copy link
Collaborator

Choose a reason for hiding this comment

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

My suggestion here is incorrect:

The 'do-while' is actually required here. With your 'while' variants if the first parent matches but doesn't have a parent itself, it won't be returned.


if (current != null && filter.matches(current)) {
return current;
}
return null;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -417,7 +417,7 @@ public <T> void visitCtInvocation(CtInvocation<T> invocation) {
}
if (constant) {
CtExecutable<?> executable = invocation.getExecutable().getDeclaration();
CtType<?> aType = invocation.getParent(CtType.class);
CtType<?> aType = invocation.isParentInitialized() ? invocation.getParent(CtType.class) : null;
CtTypeReference<?> execDeclaringType = invocation.getExecutable().getDeclaringType();
// try to inline partial evaluation results for local calls
// (including superclasses)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ public CtTypeParameter getDeclaration() {
return null;
}

CtElement e = this;
CtElement typeDeclarer = this;
CtElement parent = getParent();

if (parent instanceof CtTypeParameter && Objects.equals(getSimpleName(), ((CtTypeParameter) parent).getSimpleName())) {
Expand All @@ -119,8 +119,8 @@ public CtTypeParameter getDeclaration() {
// we might enter in that case because of a call
// of getSuperInterfaces() for example
CtTypeReference typeReference = (CtTypeReference) parent;
e = typeReference.getTypeDeclaration();
if (e == null) {
typeDeclarer = typeReference.getTypeDeclaration();
if (typeDeclarer == null) {
return null;
}
} else {
Expand All @@ -130,31 +130,31 @@ public CtTypeParameter getDeclaration() {

if (parent instanceof CtExecutableReference) {
CtExecutableReference parentExec = (CtExecutableReference) parent;
if (Objects.nonNull(parentExec.getDeclaringType()) && !parentExec.getDeclaringType().equals(e)) {
if (Objects.nonNull(parentExec.getDeclaringType()) && !parentExec.getDeclaringType().equals(typeDeclarer)) {
CtElement parent2 = parentExec.getExecutableDeclaration();
if (parent2 instanceof CtMethod) {
e = parent2;
} else {
e = e.getParent(CtFormalTypeDeclarer.class);
typeDeclarer = parent2;
}
} else {
e = e.getParent(CtFormalTypeDeclarer.class);
}
} else {
if (!(e instanceof CtFormalTypeDeclarer)) {
e = e.getParent(CtFormalTypeDeclarer.class);
}

if (!(typeDeclarer instanceof CtFormalTypeDeclarer)) {
if (typeDeclarer.isParentInitialized()) {
typeDeclarer = typeDeclarer.getParent(CtFormalTypeDeclarer.class);
} else {
typeDeclarer = null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not use the ternary operator here like you've done in the other places?

}
}

// case #1: we're a type of a method parameter, a local variable, ...
// the strategy is to look in the parents
// collecting all formal type declarers of the hierarchy
while (e != null) {
CtTypeParameter result = findTypeParamDeclaration((CtFormalTypeDeclarer) e, this.getSimpleName());
while (typeDeclarer != null) {
CtTypeParameter result = findTypeParamDeclaration((CtFormalTypeDeclarer) typeDeclarer, this.getSimpleName());
if (result != null) {
return result;
}
e = e.getParent(CtFormalTypeDeclarer.class);
typeDeclarer = typeDeclarer.isParentInitialized() ? typeDeclarer.getParent(CtFormalTypeDeclarer.class) : null;
}
return null;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -696,7 +696,7 @@ public CtTypeReference<?> getAccessType() {
if (declType == null) {
throw new SpoonException("The declaring type is expected, but " + getQualifiedName() + " is top level type");
}
CtType<?> contextType = getParent(CtType.class);
CtType<?> contextType = isParentInitialized() ? getParent(CtType.class) : null;
if (contextType == null) {
return declType;
}
Expand Down
27 changes: 27 additions & 0 deletions src/test/java/spoon/test/ctElement/ElementTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,23 @@

import org.junit.Test;
import spoon.Launcher;
import spoon.reflect.code.CtBlock;
import spoon.reflect.code.CtExpression;
import spoon.reflect.code.CtLocalVariable;
import spoon.reflect.code.CtStatement;
import spoon.reflect.declaration.CtClass;
import spoon.reflect.declaration.CtElement;
import spoon.reflect.declaration.ParentNotInitializedException;
import spoon.reflect.visitor.filter.TypeFilter;
import spoon.support.reflect.declaration.CtAnnotationImpl;
import spoon.support.reflect.declaration.CtMethodImpl;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertSame;
import static org.junit.Assert.assertThrows;
import static spoon.testing.utils.ModelUtils.createFactory;

/**
* Created by urli on 28/06/2017.
Expand Down Expand Up @@ -65,4 +74,22 @@ public void testGetChildren() {
CtClass cl = Launcher.parseClass("class A {int f; int g; public void m(int k){}}");
assertEquals(cl.getDirectChildren().size(),4);
}

@Test
public void testGetParentOverloadsInNoParentElements() {
CtStatement statement = createFactory().Code().createCodeSnippetStatement("String hello = \"t1\";").compile();

assertThrows(ParentNotInitializedException.class, () -> statement.getParent());
assertThrows(ParentNotInitializedException.class, () -> statement.getParent(CtBlock.class));
assertThrows(ParentNotInitializedException.class, () -> statement.getParent(new TypeFilter<>(CtBlock.class)));
}

@Test
public void testGetParentOverloadsWithNoMatchingElements() {
CtStatement statement = createFactory().Code().createCodeSnippetStatement("String hello = \"t1\";").compile();
CtExpression<?> expression = ((CtLocalVariable<?>) statement).getAssignment();

assertNull(expression.getParent(CtBlock.class));
assertNull(expression.getParent(new TypeFilter<>(CtBlock.class)));
}
}
7 changes: 1 addition & 6 deletions src/test/java/spoon/test/serializable/SerializableTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,8 @@ public void testSerialCtStatement() throws Exception {
String sigBef = sta2.getShortRepresentation();
String sigAf = deserializedSta2.getShortRepresentation();

CtType<?> typeBef = sta2.getParent(CtType.class);

// sta2 comes from a snippet, and snippets have no parent (#2318)
assertNull(typeBef);
assertFalse(sta2.isParentInitialized());

assertEquals(sigBef, sigAf);

Expand All @@ -63,10 +61,7 @@ public void testSerialCtStatement() throws Exception {

assertEquals(toSBef, toSgAf);

CtType<?> typeDes = deserializedSta2.getParent(CtType.class);

// typeDes comes from a serialized snippet, and snippets have no parent (#2318)
assertNull(typeDes);
assertFalse(deserializedSta2.isParentInitialized());
}

Expand Down