fix: inconsistent parents for elements in model (#5870)

Co-authored-by: I-Al-Istannen <[email protected]>

src/main/java/spoon/reflect/factory/CodeFactory.java

@@ -369,7 +369,13 @@ public <T> CtCatchVariable<T> createCatchVariable(CtTypeReference<T> type, Strin
 	 * variable (strong referencing).
 	 */
 	public <T> CtCatchVariableReference<T> createCatchVariableReference(CtCatchVariable<T> catchVariable) {
-		return factory.Core().<T>createCatchVariableReference().setType(catchVariable.getType()).<CtCatchVariableReference<T>>setSimpleName(catchVariable.getSimpleName());
+		CtCatchVariableReference<T> ref = factory.Core().createCatchVariableReference();
+
+		ref.setType(catchVariable.getType() == null ? null : catchVariable.getType().clone());
+		ref.setSimpleName(catchVariable.getSimpleName());
+		ref.setParent(catchVariable);
+
+		return ref;
 	}
 
 	/**
@@ -429,7 +435,10 @@ public <T> CtVariableAccess<T> createVariableRead(CtVariableReference<T> variabl
 			va = factory.Core().createFieldRead();
 			// creates a this target for non-static fields to avoid name conflicts...
 			if (!isStatic) {
-				((CtFieldAccess<T>) va).setTarget(createThisAccess(((CtFieldReference<T>) variable).getDeclaringType()));
+				// We do not want to change the parent of the declaring type, so clone here
+				((CtFieldAccess<T>) va).setTarget(
+					createThisAccess(((CtFieldReference<T>) variable).getDeclaringType().clone())
+				);
 			}
 		} else {
 			va = factory.Core().createVariableRead();

src/main/java/spoon/reflect/visitor/ModelConsistencyChecker.java

@@ -10,10 +10,14 @@
 import spoon.compiler.Environment;
 import spoon.reflect.declaration.CtElement;
 import spoon.reflect.declaration.CtNamedElement;
+import spoon.support.Internal;
 import spoon.support.Level;
 
 import java.util.ArrayDeque;
+import java.util.ArrayList;
 import java.util.Deque;
+import java.util.List;
+import java.util.stream.Collectors;
 
 /**
  * This scanner checks that a program model is consistent with regards to the
@@ -30,11 +34,13 @@ public class ModelConsistencyChecker extends CtScanner {
 
 	Deque<CtElement> stack = new ArrayDeque<>();
 
+	private final List<InconsistentElements> inconsistentElements = new ArrayList<>();
+
 	/**
 	 * Creates a new model consistency checker.
 	 *
 	 * @param environment
-	 * 		the environment where to report errors
+	 * 		the environment where to report errors, if null, no errors are reported
 	 * @param fixInconsistencies
 	 * 		automatically fix the inconsistencies rather than reporting
 	 * 		warnings (to report warnings, set this to false)
@@ -48,22 +54,33 @@ public ModelConsistencyChecker(Environment environment, boolean fixInconsistenci
 		this.fixNullParents = fixNullParents;
 	}
 
+	/**
+	 * Lists the inconsistencies in the given element and its children.
+	 *
+	 * @param ctElement the element to check
+	 * @return a list of inconsistencies
+	 */
+	@Internal
+	public static List<InconsistentElements> listInconsistencies(CtElement ctElement) {
+		ModelConsistencyChecker checker = new ModelConsistencyChecker(null, false, false);
+		checker.scan(ctElement);
+		return checker.inconsistentElements();
+	}
+
 	/**
 	 * Enters an element.
 	 */
 	@Override
 	public void enter(CtElement element) {
-		if (!stack.isEmpty()) {
-			if (!element.isParentInitialized() || element.getParent() != stack.peek()) {
-				if ((!element.isParentInitialized() && fixNullParents) || (element.getParent() != stack.peek() && fixInconsistencies)) {
-					element.setParent(stack.peek());
-				} else {
-					final String name = element instanceof CtNamedElement ? " - " + ((CtNamedElement) element).getSimpleName() : "";
-					environment.report(null, Level.WARN,
-							(element.isParentInitialized() ? "inconsistent" : "null") + " parent for " + element.getClass() + name + " - " + element.getPosition() + " - " + stack.peek()
-									.getPosition());
-					dumpStack();
-				}
+		if (!stack.isEmpty() && (!element.isParentInitialized() || element.getParent() != stack.peek())) {
+			InconsistentElements inconsistentElements = new InconsistentElements(element, List.copyOf(stack));
+			this.inconsistentElements.add(inconsistentElements);
+
+			if ((!element.isParentInitialized() && fixNullParents) || (element.getParent() != stack.peek() && fixInconsistencies)) {
+				element.setParent(stack.peek());
+			} else if (environment != null) {
+				environment.report(null, Level.WARN, inconsistentElements.reason());
+				this.dumpStack();
 			}
 		}
 		stack.push(element);
@@ -77,11 +94,91 @@ protected void exit(CtElement e) {
 		stack.pop();
 	}
 
+	/**
+	 * Gets the list of elements that are considered inconsistent.
+	 * <p>
+	 * If {@link #fixInconsistencies} is set to true, this list will
+	 * contain all the elements that have been fixed.
+	 *
+	 * @return the invalid elements
+	 */
+	private List<InconsistentElements> inconsistentElements() {
+		return List.copyOf(inconsistentElements);
+	}
+
 	private void dumpStack() {
-		environment.debugMessage("model consistency checker stack:");
+		environment.debugMessage("model consistency checker expectedParents:");
 		for (CtElement e : stack) {
 			environment.debugMessage("    " + e.getClass().getSimpleName() + " " + (e.getPosition().isValidPosition() ? String.valueOf(e.getPosition()) : "(?)"));
 		}
 	}
 
+
+	/**
+	 * Represents an inconsistent element.
+	 *
+	 * @param element the element with the invalid parent
+	 * @param expectedParents the expected parents of the element
+	 */
+	@Internal
+	public record InconsistentElements(CtElement element, List<CtElement> expectedParents) {
+		/**
+		 * Creates a new inconsistent element.
+		 *
+		 * @param element the element with the invalid parent
+		 * @param expectedParents the expected parents of the element
+		 */
+		public InconsistentElements {
+			expectedParents = List.copyOf(expectedParents);
+		}
+
+		private String reason() {
+			CtElement expectedParent = this.expectedParents.isEmpty() ? null : this.expectedParents.get(0);
+			return "The element %s has the parent %s, but expected the parent %s".formatted(
+				formatElement(this.element),
+				this.element.isParentInitialized() ? formatElement(this.element.getParent()) : "null",
+				expectedParent != null ? formatElement(expectedParent) : "null"
+			);
+		}
+
+		private static String formatElement(CtElement ctElement) {
+			String name = ctElement instanceof CtNamedElement ctNamedElement ? " " + ctNamedElement.getSimpleName() : "";
+
+			return "%s%s".formatted(
+				ctElement.getClass().getSimpleName(),
+				name
+			);
+		}
+
+		private String dumpExpectedParents() {
+			return this.expectedParents.stream()
+				.map(ctElement -> "    %s %s".formatted(
+					ctElement.getClass().getSimpleName(),
+					ctElement.getPosition().isValidPosition() ? String.valueOf(ctElement.getPosition()) : "(?)"
+				))
+				.collect(Collectors.joining(System.lineSeparator()));
+		}
+
+		@Override
+		public String toString() {
+			return "%s%n%s".formatted(this.reason(), this.dumpExpectedParents());
+		}
+
+		@Override
+		public boolean equals(Object object) {
+			if (this == object) {
+				return true;
+			}
+			if (!(object instanceof InconsistentElements that)) {
+				return false;
+			}
+
+			return this.element == that.element();
+		}
+
+		@Override
+		public int hashCode() {
+			return System.identityHashCode(this.element);
+		}
+	}
 }

src/main/java/spoon/support/compiler/jdt/ParentExiter.java

@@ -1169,7 +1169,10 @@ public void visitCtRecord(CtRecord recordType) {
 	public void visitCtRecordPattern(CtRecordPattern pattern) {
 		CtElement child = adjustIfLocalVariableToTypePattern(this.child);
 		if (child instanceof CtTypeReference<?> typeReference) {
-			pattern.setRecordType(typeReference);
+			// JDTTreeBuilder#visit(SingleTypeReference wraps the child in a CtTypeAccess later on,
+			// replacing its parent. Therefore, we need to use a clone for this otherwise the typeReference
+			// has two different parents (one wins and the model is inconsistent).
+			pattern.setRecordType(typeReference.clone());
 		} else if (child instanceof CtPattern innerPattern) {
 			pattern.addPattern(innerPattern);
 		}

src/test/java/spoon/reflect/ast/AstCheckerTest.java

@@ -21,6 +21,7 @@
 import spoon.reflect.code.CtStatement;
 import spoon.reflect.declaration.CtClass;
 import spoon.reflect.reference.CtExecutableReference;
+import spoon.reflect.visitor.ModelConsistencyCheckerTestHelper;
 import spoon.support.modelobs.FineModelChangeListener;
 import spoon.reflect.CtModel;
 import spoon.reflect.code.CtBinaryOperator;
@@ -126,6 +127,8 @@ public void testAvoidSetCollectionSavedOnAST() {
 				factory.Type().createReference(Set.class),
 				factory.Type().createReference(Map.class));
 
+		ModelConsistencyCheckerTestHelper.assertModelIsConsistent(factory);
+
 		final List<CtInvocation<?>> invocations = Query.getElements(factory, new TypeFilter<CtInvocation<?>>(CtInvocation.class) {
 			@Override
 			public boolean matches(CtInvocation<?> element) {

src/test/java/spoon/reflect/visitor/ModelConsistencyCheckerTestHelper.java

@@ -0,0 +1,26 @@
+package spoon.reflect.visitor;
+
+import spoon.reflect.factory.Factory;
+
+import java.util.stream.Collectors;
+
+public class ModelConsistencyCheckerTestHelper {
+
+	public static void assertModelIsConsistent(Factory factory) {
+		// contract: each elements direct descendants should have the element as parent
+		factory.getModel().getAllModules().forEach(ctModule -> {
+			var invalidElements = ModelConsistencyChecker.listInconsistencies(ctModule);
+
+			if (!invalidElements.isEmpty()) {
+				throw new AssertionError("Model is inconsistent, %d elements have invalid parents:%n%s".formatted(
+					invalidElements.size(),
+					invalidElements.stream()
+						.map(ModelConsistencyChecker.InconsistentElements::toString)
+						.limit(5)
+						.collect(Collectors.joining(System.lineSeparator()))
+				));
+			}
+		});
+	}
+
+}

src/test/java/spoon/test/architecture/SpoonArchitectureEnforcerTest.java

@@ -78,7 +78,7 @@ public class SpoonArchitectureEnforcerTest {
 	@BeforeAll
 	static void beforeAll() {
 		Launcher launcher = new Launcher();
-		launcher.getEnvironment().setComplianceLevel(11);
+		launcher.getEnvironment().setComplianceLevel(17);
 		launcher.addInputResource("src/main/java/");
 		spoonSrcMainModel = launcher.buildModel();
 		spoonSrcMainFactory = launcher.getFactory();

src/test/java/spoon/testing/utils/ModelTestParameterResolver.java

@@ -7,6 +7,7 @@
 import spoon.Launcher;
 import spoon.reflect.CtModel;
 import spoon.reflect.factory.Factory;
+import spoon.reflect.visitor.ModelConsistencyCheckerTestHelper;
 
 import java.lang.reflect.Executable;
 
@@ -61,6 +62,9 @@ private Launcher createLauncher(Executable method) {
 			launcher.addInputResource(path);
 		}
 		launcher.buildModel();
+
+		ModelConsistencyCheckerTestHelper.assertModelIsConsistent(launcher.getFactory());
+
 		return launcher;
 	}
 }