From 6517159b375f358ca2e9b890dbcbd207e6e7b305 Mon Sep 17 00:00:00 2001 From: Martin Monperrus Date: Sat, 26 Nov 2016 15:50:37 +0100 Subject: [PATCH] fix(ThisAccess): the type access associated to a this must be the type containing the this fixes #1006. Note that #1006 was neither in clone, nor in pretty-printer, but in the model. This is the problem when implicit=true, the model can be incorrect and one does not see it. --- .../spoon/reflect/factory/CodeFactory.java | 18 +++++----- .../support/compiler/jdt/ParentExiter.java | 3 -- .../java/spoon/test/delete/DeleteTest.java | 5 +-- .../test/delete/testclasses/Adobada.java | 4 +++ .../java/spoon/test/method/MethodTest.java | 6 +++- .../prettyprinter/QualifiedThisRefTest.java | 36 +++++++++++++++++++ .../test/targeted/TargetedExpressionTest.java | 4 +-- .../spoon/test/targeted/testclasses/Foo.java | 3 +- 8 files changed, 61 insertions(+), 18 deletions(-) diff --git a/src/main/java/spoon/reflect/factory/CodeFactory.java b/src/main/java/spoon/reflect/factory/CodeFactory.java index d66106a0d27..04bdb8ef608 100644 --- a/src/main/java/spoon/reflect/factory/CodeFactory.java +++ b/src/main/java/spoon/reflect/factory/CodeFactory.java @@ -107,7 +107,13 @@ public CtBinaryOperator createBinaryOperator(CtExpression left, CtExpr * @return a accessed type expression. */ public CtTypeAccess createTypeAccess(CtTypeReference accessedType) { - return createTypeAccessWithoutCloningReference(accessedType == null ? null : accessedType.clone()); + if (accessedType == null) { + return factory.Core().createTypeAccess(); + } + CtTypeReference access = accessedType.clone(); + // a type access doesn't contain actual type parameters + access.setActualTypeArguments(null); + return createTypeAccessWithoutCloningReference(access); } /** @@ -365,7 +371,7 @@ public CtStatementList createStatementList(CtBlock block) { } /** - * Creates an access to a this variable (of the form + * Creates an explicit access to a this variable (of the form * type.this). * * @param @@ -376,12 +382,7 @@ public CtStatementList createStatementList(CtBlock block) { * @return a type.this expression */ public CtThisAccess createThisAccess(CtTypeReference type) { - CtThisAccess thisAccess = factory.Core().createThisAccess(); - thisAccess.setType(type); - CtTypeAccess typeAccess = factory.Code().createTypeAccess(type); - typeAccess.setImplicit(true); - thisAccess.setTarget(typeAccess); - return thisAccess; + return createThisAccess(type, false); } /** @@ -402,7 +403,6 @@ public CtThisAccess createThisAccess(CtTypeReference type, boolean isI thisAccess.setImplicit(isImplicit); thisAccess.setType(type); CtTypeAccess typeAccess = factory.Code().createTypeAccess(type); - typeAccess.setImplicit(isImplicit); thisAccess.setTarget(typeAccess); return thisAccess; } diff --git a/src/main/java/spoon/support/compiler/jdt/ParentExiter.java b/src/main/java/spoon/support/compiler/jdt/ParentExiter.java index a0886cbc000..29215f6d0bb 100644 --- a/src/main/java/spoon/support/compiler/jdt/ParentExiter.java +++ b/src/main/java/spoon/support/compiler/jdt/ParentExiter.java @@ -610,9 +610,6 @@ public void visitCtInvocation(CtInvocation invocation) { final CtTypeReference declaringType = invocation.getExecutable().getDeclaringType(); if (declaringType != null && invocation.getExecutable().isStatic() && child.isImplicit()) { invocation.setTarget(jdtTreeBuilder.getFactory().Code().createTypeAccess(declaringType, declaringType.isAnonymous())); - } else if (declaringType != null && !invocation.getExecutable().isStatic() && child.isImplicit()) { - ((CtThisAccess) child).setTarget(jdtTreeBuilder.getFactory().Code().createTypeAccess(declaringType, true)); - invocation.setTarget((CtThisAccess) child); } else { invocation.setTarget((CtThisAccess) child); } diff --git a/src/test/java/spoon/test/delete/DeleteTest.java b/src/test/java/spoon/test/delete/DeleteTest.java index 91c04103afb..26ddd96fd96 100644 --- a/src/test/java/spoon/test/delete/DeleteTest.java +++ b/src/test/java/spoon/test/delete/DeleteTest.java @@ -150,11 +150,12 @@ public void testDeleteMethod() throws Exception { final CtMethod method = adobada.getMethod("m4", factory.Type().INTEGER_PRIMITIVE, factory.Type().FLOAT_PRIMITIVE, factory.Type().STRING); - assertEquals(4, adobada.getMethods().size()); + int n = adobada.getMethods().size(); + // deleting m4 method.delete(); - assertEquals(3, adobada.getMethods().size()); + assertEquals(n - 1, adobada.getMethods().size()); assertFalse(adobada.getMethods().contains(method)); } diff --git a/src/test/java/spoon/test/delete/testclasses/Adobada.java b/src/test/java/spoon/test/delete/testclasses/Adobada.java index efe6c84c2f5..f7f37361c4a 100644 --- a/src/test/java/spoon/test/delete/testclasses/Adobada.java +++ b/src/test/java/spoon/test/delete/testclasses/Adobada.java @@ -50,4 +50,8 @@ public void m4(int i, float j, String s) { int k; j = i = k = 3; } + + public void methodUsingjlObjectMethods() { + notify(); + } } diff --git a/src/test/java/spoon/test/method/MethodTest.java b/src/test/java/spoon/test/method/MethodTest.java index 57272fc2d96..86f5b899f3f 100644 --- a/src/test/java/spoon/test/method/MethodTest.java +++ b/src/test/java/spoon/test/method/MethodTest.java @@ -19,11 +19,15 @@ import org.junit.Test; import spoon.Launcher; +import spoon.reflect.code.CtStatement; +import spoon.reflect.code.CtThisAccess; +import spoon.reflect.code.CtTypeAccess; import spoon.reflect.declaration.CtClass; import spoon.reflect.declaration.CtMethod; import spoon.reflect.declaration.CtType; import spoon.reflect.declaration.ModifierKind; import spoon.reflect.factory.Factory; +import spoon.reflect.visitor.filter.TypeFilter; import spoon.test.delete.testclasses.Adobada; import spoon.test.method.testclasses.Tacos; @@ -33,6 +37,7 @@ import java.util.Set; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.fail; import static spoon.testing.utils.ModelUtils.build; import static spoon.testing.utils.ModelUtils.buildClass; @@ -84,5 +89,4 @@ public void testGetAllMethods() throws Exception { Set> methods = l.getFactory().Class().get("A3").getAllMethods(); assertEquals(1, methods.stream().filter(method -> "foo".equals(method.getSimpleName())).count()); } - } diff --git a/src/test/java/spoon/test/prettyprinter/QualifiedThisRefTest.java b/src/test/java/spoon/test/prettyprinter/QualifiedThisRefTest.java index b76e8377130..708f4315042 100644 --- a/src/test/java/spoon/test/prettyprinter/QualifiedThisRefTest.java +++ b/src/test/java/spoon/test/prettyprinter/QualifiedThisRefTest.java @@ -5,16 +5,28 @@ import org.junit.Test; import spoon.Launcher; import spoon.compiler.SpoonResourceHelper; +import spoon.reflect.code.CtInvocation; +import spoon.reflect.code.CtStatement; +import spoon.reflect.code.CtThisAccess; +import spoon.reflect.code.CtTypeAccess; +import spoon.reflect.declaration.CtClass; +import spoon.reflect.declaration.CtMethod; import spoon.reflect.declaration.CtType; import spoon.reflect.factory.Factory; import spoon.reflect.reference.CtTypeReference; import spoon.reflect.visitor.DefaultJavaPrettyPrinter; +import spoon.reflect.visitor.filter.TypeFilter; +import spoon.test.delete.testclasses.Adobada; import spoon.test.prettyprinter.testclasses.QualifiedThisRef; import java.util.ArrayList; import java.util.Collection; import java.util.List; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; +import static spoon.testing.utils.ModelUtils.build; + public class QualifiedThisRefTest { Factory factory; @@ -44,4 +56,28 @@ public void testQualifiedThisRef() { printer.scan(ctClass); Assert.assertTrue(printer.getResult().contains("Object o = QualifiedThisRef.Sub.this")); } + + @Test + public void testCloneThisAccess() throws Exception { + // contract: the target of "this" is correct and can be cloned + final Factory factory = build(Adobada.class); + final CtClass adobada = factory.Class().get(Adobada.class); + final CtMethod m2 = adobada.getMethod("methodUsingjlObjectMethods"); + + CtThisAccess th = (CtThisAccess) m2.getElements(new TypeFilter(CtThisAccess.class)).get(0); + assertEquals(true,th.isImplicit()); + assertEquals("notify()",th.getParent().toString()); + assertEquals("spoon.test.delete.testclasses.Adobada", ((CtTypeAccess)th.getTarget()).getAccessedType().toString()); + CtMethod clone = m2.clone(); + CtInvocation stat = clone.getBody().getStatement(0); + assertNotEquals("java.lang.Object.this.notify()", stat.toString()); // the original bug + assertEquals("spoon.test.delete.testclasses.Adobada.this.notify()", stat.toString()); + stat.getTarget().setImplicit(true); + assertEquals("notify()", stat.toString()); + + // note that this behavior means that you can only keep cloned "this" in the same class, + // and you cannot "transplant" a cloned "this" to another class + // it makes perfectly sense about the meaning of this. + // to "transplant" a this, you have to first set the target to null + } } diff --git a/src/test/java/spoon/test/targeted/TargetedExpressionTest.java b/src/test/java/spoon/test/targeted/TargetedExpressionTest.java index 565723574b5..f7417381557 100644 --- a/src/test/java/spoon/test/targeted/TargetedExpressionTest.java +++ b/src/test/java/spoon/test/targeted/TargetedExpressionTest.java @@ -103,7 +103,7 @@ public void testNotTargetedExpression() throws Exception { CtFieldAccess fieldAccess = factory.Core().createFieldRead(); fieldAccess.setVariable((CtFieldReference) iField.getReference()); fieldAccess.setTarget(factory.Code().createThisAccess(fooClass.getReference())); - assertEquals("this.i", fieldAccess.toString()); + assertEquals("spoon.test.targeted.testclasses.Foo.this.i", fieldAccess.toString()); // this test is made for this line. Check that we can setTarget(null) // without NPE fieldAccess.setTarget(null); @@ -293,7 +293,7 @@ public void testTargetsOfInv() throws Exception { assertEquals(fooTypeAccess.getType().getQualifiedName(), ((CtThisAccess) elements.get(2).getTarget()).getTarget().getType().getQualifiedName()); assertEquals(fooTypeAccess.getType().getQualifiedName(), ((CtThisAccess) elements.get(3).getTarget()).getTarget().getType().getQualifiedName()); - assertEquals(superClassTypeAccess, ((CtThisAccess) elements.get(6).getTarget()).getTarget()); + assertEquals(fooTypeAccess, ((CtThisAccess) elements.get(6).getTarget()).getTarget()); } @Test diff --git a/src/test/java/spoon/test/targeted/testclasses/Foo.java b/src/test/java/spoon/test/targeted/testclasses/Foo.java index da9f6d9fcbe..56cbc88a731 100644 --- a/src/test/java/spoon/test/targeted/testclasses/Foo.java +++ b/src/test/java/spoon/test/targeted/testclasses/Foo.java @@ -18,7 +18,8 @@ public class Foo extends SuperClass { public void m() { int x; - x= this.k; + // checking that this is correct Java and is correctly parsed + x= spoon.test.targeted.testclasses.Foo.this.k; x= Foo.k; x= k; this.k = x;