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

review: fix issue related with overridden methods retrieved from getAllMethods #1375

Merged
merged 11 commits into from
Jun 22, 2017
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
import spoon.support.compiler.SnippetCompilationHelper;
import spoon.support.util.QualifiedNameBasedSortedSet;
import spoon.support.util.SignatureBasedSortedSet;
import spoon.support.visitor.ClassTypingContext;

import java.lang.annotation.Annotation;
import java.util.ArrayList;
Expand Down Expand Up @@ -891,9 +892,16 @@ public Collection<CtExecutableReference<?>> getAllExecutables() {
public Set<CtMethod<?>> getAllMethods() {
final Set<String> distinctSignatures = new HashSet<>();
final Set<CtMethod<?>> l = new SignatureBasedSortedSet<>();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This set should not be needed anymore.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You are right! Please remove it too

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually I may have found an issue which prevent me from removing the set, see #1407. I propose to keep it as it is (it works well now) and to remove it later if needed following the change made for #1407

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would first merge this, with a clean implementation, and maybe a temporary behavioral change highlighted in the test diff. And then, we can concentrate on #1407.

final ClassTypingContext ctc = new ClassTypingContext(this);
map(new AllTypeMembersFunction(CtMethod.class)).forEach(new CtConsumer<CtMethod<?>>() {
@Override
public void accept(CtMethod<?> method) {
for (CtMethod<?> methods : l) {
if (ctc.isSameSignature(methods, method)) {
return;
}
}

if (distinctSignatures.add(method.getSignature())) {
l.add(method);
}
Expand Down
5 changes: 4 additions & 1 deletion src/main/java/spoon/support/visitor/ClassTypingContext.java
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,9 @@ private CtTypeReference<?> getEnclosingType(CtTypeReference<?> typeRef) {
*/
@Override
protected CtTypeReference<?> adaptTypeParameter(CtTypeParameter typeParam) {
if (typeParam == null) {
return null;
}
CtFormalTypeDeclarer declarer = typeParam.getTypeParameterDeclarer();
if ((declarer instanceof CtType<?>) == false) {
return null;
Expand Down Expand Up @@ -643,7 +646,7 @@ private CtTypeReference<?> adaptTypeForNewMethod(CtTypeReference<?> typeRef) {
if (typeRef instanceof CtTypeParameterReference) {
CtTypeParameterReference typeParamRef = (CtTypeParameterReference) typeRef;
CtTypeParameter typeParam = typeParamRef.getDeclaration();
if (typeParam.getTypeParameterDeclarer() instanceof CtExecutable) {
if (typeParam != null && typeParam.getTypeParameterDeclarer() instanceof CtExecutable) {
//the parameter is declared in scope of Method or Constructor
Copy link
Collaborator

Choose a reason for hiding this comment

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

If typeParamRef.getDeclaration(); returns null then something is wrong ... may be it should throw SpoonException?

In what situtation it happens? Is it really correct to ignore that null?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was happening in LambdaTest.testCompileLambdaGeneratedBySpoon and MainTest.testTest. Apparently the problem is it tries to resolve R in public abstract R apply(T arg0); but it never reach the interface Function where it's defined.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So it sounds like another bug of CtTypeParameterReference#getDeclaration()
And then I do not like the idea to ignore null here. It should throw SpoonExpeception instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So it sounds like another bug of CtTypeParameterReference#getDeclaration()

Ok I'm working on it, it seems to be related with my recent changes with CtTypeParameter on method parameters.

And then I do not like the idea to ignore null here. It should throw SpoonExpeception instead.

I'll change that.

return typeRef.clone();
}
Expand Down
24 changes: 22 additions & 2 deletions src/test/java/spoon/reflect/declaration/CtTypeInformationTest.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package spoon.reflect.declaration;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static spoon.testing.utils.ModelUtils.build;

import java.lang.reflect.Field;
Expand Down Expand Up @@ -140,8 +142,8 @@ public void testGetSuperclass() throws Exception {
final CtType<?> subClass = this.factory.Type().get(Subclass.class);
assertEquals(2, subClass.getMethods().size());

// the abstract method from Comparable which is overridden is also present in the model
assertEquals(61+3, subClass.getAllMethods().size());
// the abstract method from Comparable which is overridden should not be present in the model
assertEquals(61+2, subClass.getAllMethods().size());

CtTypeReference<?> superclass = subClass.getSuperclass();
Assert.assertEquals(ExtendsObject.class.getName(), superclass.getQualifiedName());
Expand Down Expand Up @@ -172,4 +174,22 @@ public void testGetSuperclass() throws Exception {
type2.getMethodsByName("foo").get(0).getSignature());

}

@Test
public void testGetAllMethodsWontReturnOverriddenMethod() {
final CtType<?> subClass = this.factory.Type().get(Subclass.class);
Set<CtMethod<?>> listCtMethods = subClass.getAllMethods();

boolean detectedCompareTo = false;
for (CtMethod<?> ctMethod : listCtMethods) {
if (ctMethod.getSimpleName().equals("compareTo")) {
assertFalse(ctMethod.hasModifier(ModifierKind.ABSTRACT));
assertFalse(ctMethod.getParameters().get(0).getType() instanceof CtTypeParameter);
assertEquals("Object", ctMethod.getParameters().get(0).getType().getSimpleName());
detectedCompareTo = true;
}
}

assertTrue(detectedCompareTo);
}
}