fix: prevent stackoverflow when visiting of cyclic annotation/package…

… structures (#4356)

src/main/java/spoon/support/visitor/java/JavaReflectionTreeBuilder.java

@@ -20,6 +20,7 @@
 import java.util.ArrayDeque;
 import java.util.Arrays;
 import java.util.Deque;
+import java.util.Iterator;
 import java.util.Set;
 import spoon.reflect.code.CtLiteral;
 import spoon.reflect.declaration.CtAnnotation;
@@ -132,15 +133,39 @@ public <T, R extends CtType<T>> R scan(Class<T> clazz) {
 
 	@Override
 	public void visitPackage(Package aPackage) {
-		final CtPackage ctPackage = factory.Package().getOrCreate(aPackage.getName());
-
-		enter(new PackageRuntimeBuilderContext(ctPackage));
-		super.visitPackage(aPackage);
-		exit();
+		CtPackage ctPackage = factory.Package().get(aPackage.getName());
+		// this is a dangerous section:
+		// we DON'T want to visit packages recursively if there are cyclic annotations
+		// => we only call the super method if:
+		//    - the package is not known by the factory (it wasn't visited before)
+		//    - the package is not in the current context stack
+		if (ctPackage == null || shouldVisitPackage(ctPackage)) {
+			ctPackage = factory.Package().getOrCreate(aPackage.getName());
+			enter(new PackageRuntimeBuilderContext(ctPackage));
+			super.visitPackage(aPackage);
+			exit();
+		}
 
 		contexts.peek().addPackage(ctPackage);
 	}
 
+	// Returns whether the given package is already in the context stack
+	private boolean shouldVisitPackage(CtPackage ctPackage) {
+		Iterator<RuntimeBuilderContext> iterator = contexts.iterator();
+		while (iterator.hasNext()) {
+			RuntimeBuilderContext next = iterator.next();
+			// we don't want to visit the context inserted first, as it's always
+			// a PackageRuntimeBuilderContext (see scan(...)) but it does not visit
+			// the package. So yes, the hasNext check is intended here
+			if (iterator.hasNext() && next instanceof PackageRuntimeBuilderContext) {
+				if (((PackageRuntimeBuilderContext) next).getPackage() == ctPackage) {
+					return false;
+				}
+			}
+		}
+		return true;
+	}
+
 	@Override
 	public <T> void visitClass(Class<T> clazz) {
 		final CtClass ctClass = factory.Core().createClass();

src/main/java/spoon/support/visitor/java/internal/PackageRuntimeBuilderContext.java

@@ -21,6 +21,15 @@ public PackageRuntimeBuilderContext(CtPackage ctPackage) {
 		this.ctPackage = ctPackage;
 	}
 
+	/**
+	 * Returns the package belonging to this context.
+	 *
+	 * @return the package of this context
+	 */
+	public CtPackage getPackage() {
+		return ctPackage;
+	}
+
 	@Override
 	public void addType(CtType<?> aType) {
 		ctPackage.addType(aType);

src/test/java/spoon/support/visitor/java/JavaReflectionTreeBuilderTest.java

@@ -16,6 +16,7 @@
  */
 package spoon.support.visitor.java;
 
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
@@ -93,6 +94,9 @@
 import spoon.support.visitor.equals.EqualsVisitor;
 import spoon.test.generics.testclasses3.ComparableComparatorBug;
 import spoon.test.pkg.PackageTest;
+import spoon.test.pkg.cyclic.Outside;
+import spoon.test.pkg.cyclic.direct.Cyclic;
+import spoon.test.pkg.cyclic.indirect.Indirect;
 
 public class JavaReflectionTreeBuilderTest {
 
@@ -739,4 +743,17 @@ void testShadowPackage() {
 		assertEquals(1, ctPackage.getAnnotations().size());
 		assertEquals(ctPackage.getAnnotations().get(0).getAnnotationType().getQualifiedName(), "java.lang.Deprecated");
 	}
+
+	@Test
+	void testCyclicAnnotationScanning() {
+		// contract: scanning annotations does not cause StackOverflowError
+		// due to recursive package -> annotation -> package -> annotation scanning
+		Factory factory = createFactory();
+		// a simple cycle: package a -> annotation a.A -> package a
+		assertDoesNotThrow(() -> new JavaReflectionTreeBuilder(factory).scan(Cyclic.class));
+		// an indirect cycle: package a -> annotation b.B -> package b -> annotation a.A -> package a
+		assertDoesNotThrow(() -> new JavaReflectionTreeBuilder(factory).scan(Indirect.class));
+		// an independent starting point, causing Cyclic and Indirect to be visited too
+		assertDoesNotThrow(() -> new JavaReflectionTreeBuilder(factory).scan(Outside.class));
+	}
 }

src/test/java/spoon/test/pkg/cyclic/Outside.java

@@ -0,0 +1,9 @@
+package spoon.test.pkg.cyclic;
+
+import spoon.test.pkg.cyclic.direct.Cyclic;
+import spoon.test.pkg.cyclic.indirect.Indirect;
+
+@Cyclic
+@Indirect
+public class Outside {
+}

src/test/java/spoon/test/pkg/cyclic/direct/Cyclic.java

@@ -0,0 +1,8 @@
+package spoon.test.pkg.cyclic.direct;
+
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+
+@Retention(RetentionPolicy.RUNTIME)
+public @interface Cyclic {
+}

src/test/java/spoon/test/pkg/cyclic/direct/package-info.java

@@ -0,0 +1,5 @@
+@Cyclic
+@Indirect
+package spoon.test.pkg.cyclic.direct;
+
+import spoon.test.pkg.cyclic.indirect.Indirect;

src/test/java/spoon/test/pkg/cyclic/indirect/Indirect.java

@@ -0,0 +1,8 @@
+package spoon.test.pkg.cyclic.indirect;
+
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+
+@Retention(RetentionPolicy.RUNTIME)
+public @interface Indirect {
+}

src/test/java/spoon/test/pkg/cyclic/indirect/package-info.java

@@ -0,0 +1,4 @@
+@Cyclic
+package spoon.test.pkg.cyclic.indirect;
+
+import spoon.test.pkg.cyclic.direct.Cyclic;