fix: make CtRecordComponent#toMethod return a proper model (#5801)

Before, the method body was just a code snippet

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

Author: Luro02

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

@@ -27,6 +27,7 @@
 import spoon.reflect.code.CtLocalVariable;
 import spoon.reflect.code.CtNewArray;
 import spoon.reflect.code.CtNewClass;
+import spoon.reflect.code.CtReturn;
 import spoon.reflect.code.CtStatement;
 import spoon.reflect.code.CtStatementList;
 import spoon.reflect.code.CtTextBlock;
@@ -647,6 +648,19 @@ public <A extends Annotation> CtAnnotation<A> createAnnotation(CtTypeReference<A
 		return a;
 	}
 
+	/**
+	 * Creates a return statement.
+	 *
+	 * @param expression the expression to be returned.
+	 * @param <T> the type of the expression
+	 * @return a return.
+	 */
+	public <T> CtReturn<T> createCtReturn(CtExpression<T> expression) {
+		final CtReturn<T> result = factory.Core().createReturn();
+		result.setReturnedExpression(expression);
+		return result;
+	}
+
 	/**
 	 * Gets a list of references from a list of elements.
 	 *

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

@@ -321,6 +321,14 @@ public interface Factory {
 	 */
 	<T> CtLocalVariableReference<T> createLocalVariableReference(CtLocalVariable<T> localVariable);
 
+	/**
+	 * @param expression the expression to return
+	 * @param <T> the type of the expression
+	 * @return a return statement
+	 * @see CodeFactory#createCtReturn(CtExpression)
+	 */
+	<T> CtReturn<T> createCtReturn(CtExpression<T> expression);
+
 	/**
 	 *  @see CodeFactory#createLocalVariableReference(CtTypeReference,String)
 	 */

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

@@ -509,6 +509,11 @@ public <T> CtLocalVariable<T> createLocalVariable(CtTypeReference<T> type, Strin
 		return Code().createLocalVariable(type, name, defaultExpression);
 	}
 
+	@Override
+	public <T> CtReturn<T> createCtReturn(CtExpression<T> expression) {
+		return Code().createCtReturn(expression);
+	}
+
 	@SuppressWarnings(value = "unchecked")
 	@Override
 	public <T> CtNewArray<T[]> createLiteralArray(T[] value) {

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

@@ -251,8 +251,13 @@ public <T> void scanCtType(CtType<T> type) {
 			return;
 		} else if (child instanceof CtEnumValue && type instanceof CtEnum) {
 			((CtEnum) type).addEnumValue((CtEnumValue) child);
-		} else if (child instanceof CtField) {
-			type.addField((CtField<?>) child);
+		} else if (child instanceof CtField<?> field) {
+			// We add the field in addRecordComponent. Afterward, however, JDT visits the Field itself -> Duplication.
+			// To combat this, we delete the existing field and trust JDTs version.
+			if (type instanceof CtRecord record) {
+				record.removeField(record.getField(field.getSimpleName()));
+			}
+			type.addField(field);
 			return;
 		} else if (child instanceof CtConstructor) {
 			return;

src/main/java/spoon/support/reflect/declaration/CtRecordComponentImpl.java

@@ -10,17 +10,24 @@
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.Set;
+
+import org.jspecify.annotations.Nullable;
 import spoon.JLSViolation;
 import spoon.reflect.annotations.MetamodelPropertyField;
+import spoon.reflect.code.CtFieldAccess;
+import spoon.reflect.declaration.CtElement;
 import spoon.reflect.declaration.CtField;
 import spoon.reflect.declaration.CtMethod;
 import spoon.reflect.declaration.CtNamedElement;
+import spoon.reflect.declaration.CtRecord;
 import spoon.reflect.declaration.CtRecordComponent;
 import spoon.reflect.declaration.CtShadowable;
 import spoon.reflect.declaration.CtTypedElement;
 import spoon.reflect.declaration.ModifierKind;
 import spoon.reflect.path.CtRole;
+import spoon.reflect.reference.CtFieldReference;
 import spoon.reflect.reference.CtTypeReference;
+import spoon.reflect.visitor.CtScanner;
 import spoon.reflect.visitor.CtVisitor;
 import spoon.support.reflect.CtExtendedModifier;
 
@@ -36,31 +43,58 @@ public class CtRecordComponentImpl extends CtNamedElementImpl implements CtRecor
 	public CtMethod<?> toMethod() {
 		CtMethod<?> method = this.getFactory().createMethod();
 		method.setSimpleName(getSimpleName());
-		method.setType((CtTypeReference) getType());
+		method.setType(getClonedType());
 		method.setExtendedModifiers(Collections.singleton(new CtExtendedModifier(ModifierKind.PUBLIC, true)));
-		method.setImplicit(true);
-		method.setBody(getFactory().createCodeSnippetStatement("return " + getSimpleName()));
-		return method;
+
+		CtFieldAccess<?> ctVariableAccess = (CtFieldAccess<?>) getFactory().Code()
+			.createVariableRead(getRecordFieldReference(), false);
+
+		method.setBody(getFactory().Code().createCtReturn(ctVariableAccess));
+
+		return makeTreeImplicit(method);
+	}
+
+	private CtFieldReference<?> getRecordFieldReference() {
+		CtRecord parent = isParentInitialized() ? (CtRecord) getParent() : null;
+
+		// Reference the field we think should exist. It might be added to the record later on, so do not directly
+		// query for it.
+		CtFieldReference<?> reference = getFactory().createFieldReference()
+			.setFinal(true)
+			.setStatic(false)
+			.setType(getClonedType())
+			.setSimpleName(getSimpleName());
+
+		// We have a parent record, make the field refer to it. Ideally we could do this all the time, but if we
+		// do not yet have a parent that doesn't work.
+		if (parent != null) {
+			reference.setDeclaringType(parent.getReference());
+		}
+
+		return reference;
 	}
 
 	@Override
 	public CtField<?> toField() {
 		CtField<?> field = this.getFactory().createField();
 		field.setSimpleName(getSimpleName());
-		field.setType((CtTypeReference) getType());
+		field.setType(getClonedType());
 		Set<CtExtendedModifier> modifiers = new HashSet<>();
 		modifiers.add(new CtExtendedModifier(ModifierKind.PRIVATE, true));
 		modifiers.add(new CtExtendedModifier(ModifierKind.FINAL, true));
 		field.setExtendedModifiers(modifiers);
-		field.setImplicit(true);
-		return field;
+		return makeTreeImplicit(field);
 	}
 
 	@Override
 	public boolean isImplicit() {
 		return true;
 	}
 
+	private @Nullable CtTypeReference<?> getClonedType() {
+		return getType() != null ? getType().clone() : null;
+	}
+
 	@Override
 	public CtTypeReference<Object> getType() {
 		return type;
@@ -92,17 +126,15 @@ private void checkName(String simpleName) {
 			JLSViolation.throwIfSyntaxErrorsAreNotIgnored(this, "The name '" + simpleName + "' is not allowed as record component name.");
 		}
 	}
+
 	private static Set<String> createForbiddenNames() {
 		return Set.of("clone", "finalize", "getClass", "notify", "notifyAll", "equals", "hashCode", "toString", "wait");
 	}
-
 	@Override
 	public CtRecordComponent clone() {
 		return (CtRecordComponent) super.clone();
 	}
 
-
-
 	@Override
 	public boolean isShadow() {
 		return isShadow;
@@ -114,5 +146,14 @@ public <E extends CtShadowable> E setShadow(boolean isShadow) {
 		this.isShadow = isShadow;
 		return (E) this;
 	}
-}
 
+	private static <T extends CtElement> T makeTreeImplicit(T element) {
+		element.accept(new CtScanner() {
+			@Override
+			protected void enter(CtElement e) {
+				e.setImplicit(true);
+			}
+		});
+		return element;
+	}
+}

src/main/java/spoon/support/reflect/declaration/CtRecordImpl.java

@@ -66,6 +66,10 @@ public CtRecord addRecordComponent(CtRecordComponent component) {
 		component.setParent(this);
 		getFactory().getEnvironment().getModelChangeListener().onSetAdd(this, CtRole.RECORD_COMPONENT, components, component);
 		components.add(component);
+
+		if (getField(component.getSimpleName()) == null) {
+			addField(component.toField());
+		}
 		if (!hasMethodWithSameNameAndNoParameter(component)) {
 			addMethod(component.toMethod());
 		}
@@ -100,12 +104,14 @@ public void accept(CtVisitor v) {
 	public <C extends CtType<Object>> C addTypeMemberAt(int position, CtTypeMember member) {
 		// a record can have only implicit instance fields and this is the best point to preserve the invariant
 		// because there are multiple ways to add a field to a record
+		String memberName = member.getSimpleName();
+
 		if (member instanceof CtField && !member.isStatic()) {
 			member.setImplicit(true);
-			getAnnotationsWithName(member.getSimpleName(), ElementType.FIELD).forEach(member::addAnnotation);
+			getAnnotationsWithName(memberName, ElementType.FIELD).forEach(member::addAnnotation);
 		}
 		if (member instanceof CtMethod && member.isImplicit()) {
-			getAnnotationsWithName(member.getSimpleName(), ElementType.METHOD).forEach(member::addAnnotation);
+			getAnnotationsWithName(memberName, ElementType.METHOD).forEach(member::addAnnotation);
 		}
 		if (member instanceof CtConstructor && member.isImplicit()) {
 			for (CtParameter<?> parameter : ((CtConstructor<?>) member).getParameters()) {
@@ -115,7 +121,7 @@ public <C extends CtType<Object>> C addTypeMemberAt(int position, CtTypeMember m
 		}
 		if (member instanceof CtMethod && (member.isAbstract() || member.isNative())) {
 			JLSViolation.throwIfSyntaxErrorsAreNotIgnored(this, String.format("%s method is native or abstract, both is not allowed",
-					member.getSimpleName()));
+				memberName));
 		}
 		if (member instanceof CtAnonymousExecutable && !member.isStatic()) {
 			JLSViolation.throwIfSyntaxErrorsAreNotIgnored(this, "Instance initializer is not allowed in a record (JLS 17 $8.10.2)");

src/test/java/spoon/test/record/CtRecordTest.java

@@ -1,12 +1,13 @@
 package spoon.test.record;
 
 import static java.lang.System.lineSeparator;
-import static org.hamcrest.CoreMatchers.equalTo;
-import static org.hamcrest.MatcherAssert.assertThat;
 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.assertTrue;
+import static spoon.testing.assertions.SpoonAssertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThat;
+
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Comparator;
@@ -15,17 +16,25 @@
 import java.util.stream.Collectors;
 import javax.validation.constraints.NotNull;
 
+import org.assertj.core.api.InstanceOfAssertFactory;
 import org.junit.jupiter.api.Test;
 import spoon.Launcher;
 import spoon.reflect.CtModel;
+import spoon.reflect.code.CtFieldRead;
+import spoon.reflect.code.CtReturn;
+import spoon.reflect.code.CtStatement;
 import spoon.reflect.declaration.CtAnonymousExecutable;
 import spoon.reflect.declaration.CtConstructor;
+import spoon.reflect.declaration.CtElement;
 import spoon.reflect.declaration.CtField;
 import spoon.reflect.declaration.CtMethod;
 import spoon.reflect.declaration.CtRecord;
+import spoon.reflect.declaration.CtRecordComponent;
 import spoon.reflect.declaration.CtType;
 import spoon.reflect.factory.Factory;
+import spoon.reflect.visitor.CtScanner;
 import spoon.reflect.visitor.filter.TypeFilter;
+import spoon.testing.assertions.SpoonAssertions;
 import spoon.testing.utils.ModelTest;
 
 public class CtRecordTest {
@@ -76,24 +85,33 @@ public void testMultipleParameterRecord() {
 
 		assertEquals(1, records.size());
 		assertEquals("public record MultiParameter(int first, float second) {}", head(records).toString());
-		
+
 		// test fields
 		assertEquals(
 				Arrays.asList(
-						"private final int first;", 
+						"private final int first;",
 						"private final float second;"
-				), 
+				),
 				head(records).getFields().stream().map(String::valueOf).collect(Collectors.toList())
 		);
-		
+
+		// Make them explicit so we can print them (but assert they were implicit initially)
+		assertThat(head(records)).getMethods().allSatisfy(CtElement::isImplicit);
+		head(records).getMethods().forEach(it -> it.accept(new CtScanner() {
+			@Override
+			protected void enter(CtElement e) {
+				e.setImplicit(false);
+			}
+		}));
+
 		// test methods
 		assertEquals(
 				Arrays.asList(
 						"int first() {\n" +
-					    "    return first;\n" +
-					    "}", 
+					    "    return this.first;\n" +
+					    "}",
 						"float second() {\n" +
-					    "    return second;\n" +
+					    "    return this.second;\n" +
 					    "}"
 				),
 				head(records).getMethods().stream()
@@ -211,7 +229,7 @@ private CtModel createModelFromPath(String code) {
 
 	@Test
 	void testGenericTypeParametersArePrintedBeforeTheFunctionParameters() {
-		// contract: a record with generic type arguments should be printed correctly 
+		// contract: a record with generic type arguments should be printed correctly
 		String code = "src/test/resources/records/GenericRecord.java";
 		CtModel model = createModelFromPath(code);
 		Collection<CtRecord> records = model.getElements(new TypeFilter<>(CtRecord.class));
@@ -225,7 +243,7 @@ void testBuildRecordModelWithStaticInitializer() {
 		String code = "src/test/resources/records/WithStaticInitializer.java";
 		CtModel model = assertDoesNotThrow(() -> createModelFromPath(code));
 		List<CtAnonymousExecutable> execs = model.getElements(new TypeFilter<>(CtAnonymousExecutable.class));
-		assertThat(execs.size(), equalTo(2));
+		assertThat(execs).hasSize(2);
 	}
 
 	@ModelTest(value = "./src/test/resources/records/MultipleConstructors.java", complianceLevel = 16)
@@ -275,8 +293,29 @@ void testNonCompactCanonicalConstructor(Factory factory) {
 		assertEquals(constructor.getParameters().get(0).getSimpleName(), "x");
 	}
 
+	@ModelTest(value = "./src/test/resources/records/GenericRecord.java", complianceLevel = 16)
+	void testProperReturnInRecordAccessor(Factory factory) {
+		// contract: the return statement in the accessor method should return a field read expression to the correct
+		// field
+		CtRecord record = head(factory.getModel().getElements(new TypeFilter<>(CtRecord.class)));
+
+		assertThat(record.getRecordComponents()).isNotEmpty();
+		for (CtRecordComponent component : record.getRecordComponents()) {
+			CtMethod<?> method = component.toMethod();
+
+			assertThat(method.getBody().<CtStatement>getLastStatement())
+				.asInstanceOf(new InstanceOfAssertFactory<>(CtReturn.class, SpoonAssertions::assertThat))
+				.getReturnedExpression()
+				.self()
+				.asInstanceOf(new InstanceOfAssertFactory<>(CtFieldRead.class, SpoonAssertions::assertThat))
+				.getVariable()
+				.getDeclaringType()
+				.getSimpleName()
+				.isEqualTo(record.getSimpleName());
+		}
+	}
+
 	private <T> T head(Collection<T> collection) {
 		return collection.iterator().next();
 	}
-
 }