fix: Make CtFieldRead inherit from CtResource (#5860)

* fix: Make CtField inherit from CtResource Fields are allowed in try-with-resources statements now, so spoon should model this case. * fix: Make CtFieldRead extend CtResource and remove CtVariable supertype for CtResource This allows us to correctly model try-with-resources only taking a field, parameter, catch variable or local variable reference.
INRIA · Aug 2, 2024 · eb34399 · eb34399
1 parent 3bddf4d
commit eb34399
Show file tree

Hide file tree

Showing 7 changed files with 104 additions and 44 deletions.
diff --git a/src/main/java/spoon/reflect/code/CtResource.java b/src/main/java/spoon/reflect/code/CtResource.java
@@ -7,12 +7,12 @@
  */
 package spoon.reflect.code;
 
-import spoon.reflect.declaration.CtVariable;
+import spoon.reflect.declaration.CtElement;
 
 /**
  * This code element defines a resource used in the try-with-resource statement.
  * @param <T>
  *     The type of the resource.
  */
-public interface CtResource<T> extends CtVariable<T> {
+public interface CtResource<T> extends CtElement {
 }
diff --git a/src/main/java/spoon/reflect/code/CtVariableRead.java b/src/main/java/spoon/reflect/code/CtVariableRead.java
@@ -21,7 +21,7 @@
  * @param <T>
  * 		type of the variable
  */
-public interface CtVariableRead<T> extends CtVariableAccess<T> {
+public interface CtVariableRead<T> extends CtVariableAccess<T>, CtResource<T> {
 	@Override
 	CtVariableRead<T> clone();
 }
diff --git a/src/main/java/spoon/reflect/visitor/CtInheritanceScanner.java b/src/main/java/spoon/reflect/visitor/CtInheritanceScanner.java
@@ -949,6 +949,7 @@ public <T> void visitCtVariableRead(CtVariableRead<T> e) {
 		scanCtVariableAccess(e);
 		scanCtExpression(e);
 		scanCtCodeElement(e);
+		scanCtResource(e);
 		scanCtTypedElement(e);
 		scanCtElement(e);
 		scanCtVisitable(e);

diff --git a/src/main/java/spoon/support/compiler/jdt/ParentExiter.java b/src/main/java/spoon/support/compiler/jdt/ParentExiter.java
@@ -62,7 +62,6 @@
 import spoon.reflect.code.CtNewClass;
 import spoon.reflect.code.CtPattern;
 import spoon.reflect.code.CtRecordPattern;
-import spoon.reflect.code.CtResource;
 import spoon.reflect.code.CtReturn;
 import spoon.reflect.code.CtStatement;
 import spoon.reflect.code.CtSuperAccess;
@@ -108,11 +107,9 @@
 import spoon.reflect.reference.CtIntersectionTypeReference;
 import spoon.reflect.reference.CtTypeParameterReference;
 import spoon.reflect.reference.CtTypeReference;
-import spoon.reflect.reference.CtVariableReference;
 import spoon.reflect.reference.CtWildcardReference;
 import spoon.reflect.visitor.CtInheritanceScanner;
 import spoon.reflect.visitor.CtScanner;
-import spoon.reflect.visitor.filter.TypeFilter;
 
 import java.util.ArrayList;
 import java.util.Collections;
@@ -1103,31 +1100,11 @@ public void visitCtTry(CtTry tryBlock) {
 
 	@Override
 	public void visitCtTryWithResource(CtTryWithResource tryWithResource) {
-		if (child instanceof CtLocalVariable) {
+		if (child instanceof CtLocalVariable<?> var) {
 			// normal, happy path of declaring a new variable
-			tryWithResource.addResource((CtLocalVariable) child);
-		} else if (child instanceof CtVariableRead) {
-			// special case of the resource being declared before
-			final CtVariableReference<?> variableRef = ((CtVariableRead<?>) child).getVariable();
-			if (variableRef.getDeclaration() != null) {
-				// getDeclaration works
-				tryWithResource.addResource((CtResource<?>) variableRef.getDeclaration().clone().setImplicit(true));
-			} else {
-				// we have to find it manually
-				for (ASTPair pair: this.jdtTreeBuilder.getContextBuilder().getAllContexts()) {
-					final List<CtLocalVariable> variables = pair.element().getElements(new TypeFilter<>(CtLocalVariable.class));
-					for (CtLocalVariable v: variables) {
-						if (v.getSimpleName().equals(variableRef.getSimpleName())) {
-							// we found the resource
-							// we clone it in order to comply with the contract of being a tree
-							final CtLocalVariable clone = v.clone();
-							clone.setImplicit(true);
-							tryWithResource.addResource(clone);
-							break;
-						}
-					}
-				}
-			}
+			tryWithResource.addResource(var);
+		} else if (child instanceof CtVariableRead<?> read) {
+			tryWithResource.addResource(read);
 		}
 		super.visitCtTryWithResource(tryWithResource);
 	}

diff --git a/src/test/java/spoon/test/trycatch/TryCatchTest.java b/src/test/java/spoon/test/trycatch/TryCatchTest.java
@@ -34,29 +34,34 @@
 import spoon.reflect.CtModel;
 import spoon.reflect.code.CtCatch;
 import spoon.reflect.code.CtCatchVariable;
+import spoon.reflect.code.CtFieldRead;
+import spoon.reflect.code.CtLocalVariable;
 import spoon.reflect.code.CtResource;
 import spoon.reflect.code.CtTry;
 import spoon.reflect.code.CtTryWithResource;
+import spoon.reflect.code.CtVariableRead;
 import spoon.reflect.declaration.CtClass;
+import spoon.reflect.declaration.CtField;
 import spoon.reflect.declaration.CtMethod;
+import spoon.reflect.declaration.CtParameter;
 import spoon.reflect.declaration.CtVariable;
 import spoon.reflect.declaration.ModifierKind;
 import spoon.reflect.factory.Factory;
 import spoon.reflect.reference.CtCatchVariableReference;
 import spoon.reflect.reference.CtLocalVariableReference;
 import spoon.reflect.reference.CtTypeReference;
+import spoon.reflect.reference.CtVariableReference;
 import spoon.reflect.visitor.filter.AbstractFilter;
 import spoon.reflect.visitor.filter.TypeFilter;
+import spoon.support.compiler.VirtualFile;
 import spoon.support.reflect.CtExtendedModifier;
 import spoon.test.trycatch.testclasses.Foo;
 import spoon.test.trycatch.testclasses.Main;
 import spoon.testing.utils.ModelTest;
 import spoon.testing.utils.LineSeparatorExtension;
 
 
-import static org.hamcrest.CoreMatchers.equalTo;
-import static org.hamcrest.MatcherAssert.assertThat;
-import static org.hamcrest.collection.IsIterableContainingInOrder.contains;
+import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotEquals;
@@ -236,6 +241,7 @@ public void testCompileMultiTryCatchWithCustomExceptions() {
 			fail(e.getMessage());
 		}
 	}
+
 	@Test
 	public void testTryCatchVariableGetType() {
 		Factory factory = createFactory();
@@ -376,7 +382,7 @@ public void testCatchUnqualifiedReferenceMarkedSimplyQualifiedWhenMultipleTypesA
 
 		CtCatch targetCatch = catches.get(0);
 		List<CtTypeReference<?>> paramTypes = targetCatch.getParameter().getMultiTypes();
-		assertThat(paramTypes.size(), equalTo(2));
+		assertThat(paramTypes).hasSize(2);
 		assertTrue(paramTypes.get(0).isSimplyQualified(), "first type reference is fully qualified");
 		assertTrue(paramTypes.get(1).isSimplyQualified(), "second type reference is fully qualified");
 	}
@@ -393,7 +399,7 @@ public void testCatchWithQualifiedAndUnqualifiedTypeReferencesInSameCatcher() th
 
 		CtCatch targetCatch = catches.get(0);
 		List<CtTypeReference<?>> paramTypes = targetCatch.getParameter().getMultiTypes();
-		assertThat(paramTypes.size(), equalTo(2));
+		assertThat(paramTypes).hasSize(2);
 		assertTrue(paramTypes.get(0).isSimplyQualified(), "first type reference should be unqualified");
 		assertFalse(paramTypes.get(1).isSimplyQualified(), "second type reference should be qualified");
 	}
@@ -411,10 +417,10 @@ public void testNonCloseableGenericTypeInTryWithResources(CtModel model) {
 
 		CtLocalVariableReference<?> varRef = model.filterChildren(CtLocalVariableReference.class::isInstance).first();
 
-		assertThat(varRef.getType().getQualifiedName(), equalTo("NonClosableGenericInTryWithResources.GenericType"));
+		assertThat(varRef.getType().getQualifiedName()).isEqualTo("NonClosableGenericInTryWithResources.GenericType");
 
 		// We don't extract the type arguments
-		assertThat(varRef.getType().getActualTypeArguments().size(), equalTo(0));
+		assertThat(varRef.getType().getActualTypeArguments()).isEmpty();
 	}
 
 	@ExtendWith(LineSeparatorExtension.class)
@@ -431,15 +437,15 @@ public void testTryWithVariableAsResource() {
 		List<CtResource<?>> resources = tryStmt.getResources();
 		assertEquals(1, resources.size());
 		final CtResource<?> ctResource = resources.get(0);
-		assertTrue(ctResource instanceof CtVariable);
-		assertEquals("resource", ((CtVariable<?>) ctResource).getSimpleName());
+		assertThat(ctResource).isInstanceOf(CtVariableRead.class);
+		assertThat(((CtVariableRead<?>) ctResource).getVariable().getSimpleName()).isEqualTo("resource");
 
 		// contract: pretty-printing of existing resources works
-		assertEquals("try (resource) {\n}", tryStmt.toString());
+		assertThat(tryStmt).asString().isEqualTo("try (resource) {\n}");
 
 		// contract: removeResource does remove the resource
 		tryStmt.removeResource(ctResource);
-		assertEquals(0, tryStmt.getResources().size());
+		assertThat(tryStmt.getResources()).isEmpty();
 		// contract: removeResource of nothing is graceful and accepts it
 		tryStmt.removeResource(ctResource);
 
@@ -466,7 +472,7 @@ void addsCatcherAtTheSpecifiedPosition() {
 				.addCatcherAt(1, second);
 
 			// assert
-			assertThat(tryStatement.getCatchers(), contains(first, second, third));
+			assertThat(tryStatement.getCatchers()).containsExactlyInAnyOrder(first, second, third);
 		}
 
 		@Test
@@ -492,4 +498,80 @@ private <T extends Throwable> CtCatch createCatch(Factory factory, Class<T> type
 			);
 		}
 	}
+
+	@Test
+	void testFieldAsCtResource() {
+		// contract: Since java 9 normal variables, fields, parameters and catch variables are allowed
+		// in try-with-resources
+		CtModel model = createModelFromString("""
+			class ClosableException extends RuntimeException implements AutoCloseable {
+				@Override
+				public void close() throws Exception {}
+			}
+			class Foo {
+				final AutoCloseable field = null;
+				public void bar(AutoCloseable param) {
+					try { }
+					catch (ClosableException e) {
+						AutoCloseable localVar = param;
+						try (e; field; param; localVar; AutoCloseable inside = () -> {}) {
+						} catch (Exception ignored) {}
+					}
+				}
+			}
+			""");
+		CtClass<?> foo = (CtClass<?>) model.getAllTypes()
+			.stream()
+			.filter(it -> it.getSimpleName().equals("Foo"))
+			.findAny()
+			.orElseThrow();
+		CtMethod<?> bar = foo.getMethodsByName("bar").get(0);
+
+		CtCatchVariable<?> e = bar.getElements(new TypeFilter<>(CtCatchVariable.class)).get(0);
+		CtLocalVariable<?> localVar = bar.getElements(new TypeFilter<>(CtLocalVariable.class))
+			.stream()
+			.filter(it -> it.getSimpleName().equals("localVar"))
+			.findAny()
+			.orElseThrow();
+		CtField<?> field = foo.getField("field");
+		CtParameter<?> param = bar.getParameters().get(0);
+		CtLocalVariable<?> inside = bar.getElements(new TypeFilter<>(CtLocalVariable.class))
+			.stream()
+			.filter(it -> it.getSimpleName().equals("inside"))
+			.findAny()
+			.orElseThrow();
+
+		List<CtResource<?>> resources = bar.getElements(new TypeFilter<>(CtTryWithResource.class))
+			.get(0)
+			.getResources();
+		assertThat(resources).containsExactlyInAnyOrder(
+			getRead(field),
+			getRead(param.getReference()),
+			getRead(e.getReference()),
+			getRead(localVar.getReference()),
+			inside
+		);
+	}
+
+	private <T> CtVariableRead<T> getRead(CtVariableReference<T> ref) {
+		return ref.getFactory().<T>createVariableRead().<CtFieldRead<T>>setVariable(ref);
+	}
+
+	private <T> CtVariableRead<T> getRead(CtField<T> field) {
+		Factory factory = field.getFactory();
+		CtFieldRead<T> read = factory.createFieldRead();
+		read.setVariable(field.getReference());
+		read.setTarget(factory.createThisAccess(field.getDeclaringType().getReference()));
+
+		return read;
+	}
+
+	private static CtModel createModelFromString(String code) {
+		Launcher launcher = new Launcher();
+		launcher.getEnvironment().setComplianceLevel(21);
+		launcher.getEnvironment().setShouldCompile(true);
+		launcher.addInputResource(new VirtualFile(code));
+		return launcher.buildModel();
+	}
+
 }
diff --git a/src/test/java/spoon/testing/assertions/CtResourceAssertInterface.java b/src/test/java/spoon/testing/assertions/CtResourceAssertInterface.java
@@ -1,4 +1,4 @@
 package spoon.testing.assertions;
 import org.assertj.core.api.AbstractObjectAssert;
 import spoon.reflect.code.CtResource;
-public interface CtResourceAssertInterface<A extends AbstractObjectAssert<A, W>, W extends CtResource<?>> extends CtVariableAssertInterface<A, W> , SpoonAssert<A, W> {}
+public interface CtResourceAssertInterface<A extends AbstractObjectAssert<A, W>, W extends CtResource<?>> extends CtElementAssertInterface<A, W> , SpoonAssert<A, W> {}
diff --git a/src/test/java/spoon/testing/assertions/CtVariableReadAssertInterface.java b/src/test/java/spoon/testing/assertions/CtVariableReadAssertInterface.java
@@ -1,4 +1,4 @@
 package spoon.testing.assertions;
 import org.assertj.core.api.AbstractObjectAssert;
 import spoon.reflect.code.CtVariableRead;
-public interface CtVariableReadAssertInterface<A extends AbstractObjectAssert<A, W>, W extends CtVariableRead<?>> extends CtVariableAccessAssertInterface<A, W> , SpoonAssert<A, W> {}
+public interface CtVariableReadAssertInterface<A extends AbstractObjectAssert<A, W>, W extends CtVariableRead<?>> extends CtVariableAccessAssertInterface<A, W> , SpoonAssert<A, W> , CtResourceAssertInterface<A, W> {}