diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RedundantConstructorCheck.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RedundantConstructorCheck.java index 2dbebcce..061ff8d0 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RedundantConstructorCheck.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RedundantConstructorCheck.java @@ -2,14 +2,130 @@ import de.firemage.autograder.core.LocalizedMessage; import de.firemage.autograder.core.ProblemType; +import de.firemage.autograder.core.Translatable; import de.firemage.autograder.core.check.ExecutableCheck; -import de.firemage.autograder.core.pmd.PMDCheck; -import net.sourceforge.pmd.lang.java.rule.codestyle.UnnecessaryConstructorRule; +import de.firemage.autograder.core.dynamic.DynamicAnalysis; +import de.firemage.autograder.core.integrated.IntegratedCheck; +import de.firemage.autograder.core.integrated.StaticAnalysis; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import spoon.processing.AbstractProcessor; +import spoon.reflect.code.CtAssignment; +import spoon.reflect.code.CtBlock; +import spoon.reflect.code.CtFieldWrite; +import spoon.reflect.code.CtInvocation; +import spoon.reflect.code.CtVariableRead; +import spoon.reflect.declaration.CtClass; +import spoon.reflect.declaration.CtConstructor; +import spoon.reflect.declaration.CtElement; +import spoon.reflect.declaration.CtField; +import spoon.reflect.declaration.CtRecord; +import spoon.reflect.reference.CtParameterReference; +import spoon.reflect.reference.CtTypeReference; + +import java.util.function.Predicate; @ExecutableCheck(reportedProblems = {ProblemType.REDUNDANT_DEFAULT_CONSTRUCTOR}) -public class RedundantConstructorCheck extends PMDCheck { - public RedundantConstructorCheck() { - super(new LocalizedMessage("implicit-constructor-exp"), - new UnnecessaryConstructorRule(), ProblemType.REDUNDANT_DEFAULT_CONSTRUCTOR); +public class RedundantConstructorCheck extends IntegratedCheck { + private static final Logger LOG = LoggerFactory.getLogger(RedundantConstructorCheck.class); + private static final Translatable MESSAGE = new LocalizedMessage("implicit-constructor-exp"); + + @Override + protected void check(StaticAnalysis staticAnalysis, DynamicAnalysis dynamicAnalysis) { + staticAnalysis.processWith(new AbstractProcessor>() { + @Override + public void process(CtClass element) { + CtConstructor redundantCtor = null; + if (element instanceof CtRecord record) { + var types = record + .getFields() + .stream() + .map(CtField::getType) + .toArray(CtTypeReference[]::new); + var canonicalCtor = record.getConstructor(types); + + if (!canonicalCtor.isImplicit() + && hasEffectivelyDefaultVisibility(record, canonicalCtor) + && isDefaultBodyRecord(record, canonicalCtor)) { + redundantCtor = canonicalCtor; + } + } else { + var ctors = element.getConstructors(); + if (ctors.size() != 1) return; + var ctor = ctors.iterator().next(); + + if (!ctor.isImplicit() + && ctor.getParameters().isEmpty() + && hasEffectivelyDefaultVisibility(element, ctor) + && isDefaultBody(ctor.getBody()) + && ctor.getThrownTypes().isEmpty()) { + redundantCtor = ctor; + } + } + if (redundantCtor != null) { + addLocalProblem(redundantCtor, MESSAGE, ProblemType.REDUNDANT_DEFAULT_CONSTRUCTOR); + } + } + }); + } + + /** + * Checks if the constructor visibility is effectively the same as the class + * visibility. + *

+ * A constructor with higher visibility than the containing class is only ever useful + * with a protected (inner) class, because it can allow the class to be instantiated + * from a different package. + */ + private boolean hasEffectivelyDefaultVisibility(CtClass type, CtConstructor ctor) { + // enum constructors are always private + if (type.isEnum() || type.isPrivate()) return true; + + if (type.isPublic()) return ctor.isPublic(); + else if (type.isProtected()) return ctor.isProtected(); + // package-access class, only private is smaller + else return !ctor.isPrivate(); + } + + private boolean isDefaultBody(CtBlock block) { + return block + .getStatements() + .stream() + .filter(Predicate.not(CtElement::isImplicit)) + // A constructor invocation is either this or super. + // Because we know we are analyzing the body of a no-args constructor, it + // cannot be a recursive this() call, but has to be a redundant super() call. + // If the target is not null it is a qualified super invocation, which is + // required and not redundant. + .allMatch(statement -> statement instanceof CtInvocation invocation + && invocation.getExecutable().isConstructor() + && invocation.getArguments().isEmpty() + && invocation.getTarget() == null); + } + + private boolean isDefaultBodyRecord(CtRecord record, CtConstructor ctor) { + return ctor + .getBody() + .getStatements() + .stream() + .filter(Predicate.not(CtElement::isImplicit)) + // The canonical record constructor cannot contain an explicit constructor + // invocation. We check if all statements are standard field assignments. + // For a normal constructor this must mean it is the default constructor, + // because all fields have to be assigned. + // A compact constructor does not allow field assignments, so this checks + // for an empty block. + .allMatch(statement -> { + if (!(statement instanceof CtAssignment assignment)) return false; + if (!(assignment.getAssigned() instanceof CtFieldWrite fieldWrite)) return false; + if (!(assignment.getAssignment() instanceof CtVariableRead variableRead)) return false; + if (!(variableRead.getVariable() instanceof CtParameterReference parameter)) return false; + var index = ctor.getParameters().indexOf(parameter.getDeclaration()); + if (index < 0) { + LOG.error("encountered CtParameter not present in constructor parameters"); + return false; + } + return record.getFields().get(index) == fieldWrite.getVariable().getDeclaration(); + }); } } diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestRedundantConstructor.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestRedundantConstructor.java new file mode 100644 index 00000000..2ad3627a --- /dev/null +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestRedundantConstructor.java @@ -0,0 +1,372 @@ +package de.firemage.autograder.core.check.complexity; + +import de.firemage.autograder.core.LinterException; +import de.firemage.autograder.core.LocalizedMessage; +import de.firemage.autograder.core.Problem; +import de.firemage.autograder.core.ProblemType; +import de.firemage.autograder.core.check.AbstractCheckTest; +import de.firemage.autograder.core.compiler.JavaVersion; +import de.firemage.autograder.core.file.SourceInfo; +import de.firemage.autograder.core.file.StringSourceInfo; +import org.junit.jupiter.api.Test; + +import java.io.IOException; +import java.util.List; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +class TestRedundantConstructor extends AbstractCheckTest { + private static final List PROBLEM_TYPES = List.of(ProblemType.REDUNDANT_DEFAULT_CONSTRUCTOR); + + @Test + void testRedundantConstructor() throws IOException, LinterException { + assertRedundant( + "Test", + """ + public class Test { + public Test() { + + } + } + """ + ); + } + + @Test + void testRedundantHigherVisibility() throws IOException, LinterException { + assertRedundant( + "Test", + """ + class Test { + public Test() { + } + } + """ + ); + } + + @Test + void testRedundantHigherVisibilityInner() throws IOException, LinterException { + assertRedundant( + "Outer", + """ + public class Outer { + private class Inner { + Inner() {} + } + } + """ + ); + } + + @Test + void testRedundantExplicitSuper() throws IOException, LinterException { + assertRedundant( + "Test", + """ + public class Test { + public Test() { + super(); + } + } + """ + ); + } + + @Test + void testRedundantConstructorInnerClassSuper() throws IOException, LinterException { + assertRedundant( + "Test", + """ + public class Test { + public class Inner { + public Inner() { + super(/* test */); + } + } + } + """ + ); + } + + @Test + void testRedundantConstructorStaticInner() throws IOException, LinterException { + assertRedundant( + "Test", + """ + public class Test { + private static class Inner { + private Inner() { + } + } + } + """ + ); + } + + @Test + void testRedundantEnumConstructor() throws IOException, LinterException { + assertRedundant( + "TestEnum", + """ + public enum TestEnum { + ; + private TestEnum() { + } + } + """ + ); + } + + @Test + void testRedundantEnumConstructorNoModifier() throws IOException, LinterException { + assertRedundant( + "TestEnum", + """ + public enum TestEnum { + ; + TestEnum() { + } + } + """ + ); + } + + @Test + void testRedundantRecordCompactConstructor() throws IOException, LinterException { + assertRedundant( + "Test", + """ + record Test() { + Test { + } + } + """ + ); + } + + @Test + void testRedundantEmptyRecordNormalConstructor() throws IOException, LinterException { + assertRedundant( + "Test", + """ + public record Test() { + public Test() { + } + } + """ + ); + } + + @Test + void testRedundantRecordNormalConstructor() throws IOException, LinterException { + assertRedundant( + "Test", + """ + public record Test(int a, int b) { + public Test(int a, int b) { + this.b = b; + this.a = a; + } + } + """ + ); + } + + @Test + void testNotRedundantImplicitConstructor() throws IOException, LinterException { + assertNotRedundant( + "Test", + """ + public class Test { + } + """ + ); + } + + @Test + void testNotRedundantImplicitRecordConstructor() throws IOException, LinterException { + assertNotRedundant( + "TestRecord", + """ + record TestRecord() { + } + """ + ); + } + + @Test + void testNotRedundantRecordNormalConstructor() throws IOException, LinterException { + assertNotRedundant( + "Test", + """ + public record Test(int a, int b) { + public Test(int a, int b) { + this.a = b; + this.b = a; + } + } + """ + ); + } + + @Test + void testNotRedundantRecordNormalConstructor2() throws IOException, LinterException { + assertNotRedundant( + "Test", + """ + public record Test(int a, int b) { + public Test(int a, int b) { + this.a = a; + this.b = b; + System.out.println(); + } + } + """ + ); + } + + @Test + void testNotRedundantConstructorVisibility() throws IOException, LinterException { + assertNotRedundant( + "Test", + """ + public class Test { + protected Test() { + } + } + """ + ); + } + + @Test + void testNotRedundantBody() throws IOException, LinterException { + assertNotRedundant( + "Test", + """ + public class Test { + public Test() { + System.out.println(); + } + } + """ + ); + } + + @Test + void testNotRedundantThrows() throws IOException, LinterException { + assertNotRedundant( + "Test", + """ + public class Test { + public Test() throws java.io.IOException { + } + } + """ + ); + } + + @Test + void testNotRedundantConstructorSuperArgs() throws IOException, LinterException { + assertRedundant( + StringSourceInfo.fromSourceStrings(JavaVersion.JAVA_17, Map.of( + "Base", + """ + public class Base { + public Base(String foo) { + } + } + """, + "Child", + """ + public class Child extends Base { + public Child() { + super("foo"); + } + } + """ + )), + false + ); + } + + @Test + void testNotRedundantQualifiedSuper() throws IOException, LinterException { + // taken from JLS example 8.8.7.1-2 + assertRedundant( + StringSourceInfo.fromSourceStrings(JavaVersion.JAVA_17, Map.of( + "Outer", + """ + class Outer { + class Inner {} + } + """, + "Child", + """ + class Child extends Outer.Inner { + Child() { + (new Outer()).super(); + } + } + """ + )), + false + ); + } + + @Test + void testNotRedundantConstructorProtectedInnerClass() throws IOException, LinterException { + // Taken from JLS example 8.8.9-2 + assertRedundant( + StringSourceInfo.fromSourceStrings(JavaVersion.JAVA_17, Map.of( + "a.Outer", + """ + package a; + public class Outer { + protected class Inner { + public Inner() {} + } + } + """, + "b.Child", + """ + package b; + public class Child extends a.Outer { + void foo() { + new Inner(); + } + } + """ + )), + false + ); + } + + private void assertRedundant(String className, String source) throws LinterException, IOException { + assertRedundant(className, source, true); + } + + private void assertNotRedundant(String className, String source) throws LinterException, IOException { + assertRedundant(className, source, false); + } + + private void assertRedundant(String className, String source, boolean redundant) throws LinterException, IOException { + assertRedundant(StringSourceInfo.fromSourceString(JavaVersion.JAVA_17, className, source), redundant); + } + + private void assertRedundant(SourceInfo info, boolean redundant) throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(info, PROBLEM_TYPES); + if (redundant) { + assertEqualsRedundant(problems.next()); + } + problems.assertExhausted(); + } + + private void assertEqualsRedundant(Problem problem) { + assertEquals( + this.linter.translateMessage(new LocalizedMessage("implicit-constructor-exp")), + this.linter.translateMessage(problem.getExplanation()) + ); + } +}