Skip to content

Commit

Permalink
Detect redundant normal canonical constructors
Browse files Browse the repository at this point in the history
  • Loading branch information
@lunagl
lunagl committed Apr 10, 2024
1 parent f879011 commit a53f7e0
Show file tree
Hide file tree
Showing 2 changed files with 112 additions and 22 deletions.
86 changes: 65 additions & 21 deletions ...src/main/java/de/firemage/autograder/core/check/complexity/RedundantConstructorCheck.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,35 +7,55 @@
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.CtBlock;
import spoon.reflect.code.CtInvocation;
import spoon.reflect.declaration.CtClass;
import spoon.reflect.declaration.CtConstructor;
import spoon.reflect.declaration.CtElement;
import spoon.reflect.code.*;
import spoon.reflect.declaration.*;
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 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<CtClass<?>>() {
@Override
public void process(CtClass<?> element) {
var ctors = element.getConstructors();
if (ctors.size() != 1) return;

var ctor = ctors.iterator().next();
var isRedundant = !ctor.isImplicit()
&& ctor.getParameters().isEmpty()
&& hasDefaultVisibility(element, ctor)
&& isEmpty(ctor.getBody())
&& ctor.getThrownTypes().isEmpty();
if (isRedundant) {
addLocalProblem(ctor, MESSAGE, ProblemType.REDUNDANT_DEFAULT_CONSTRUCTOR);
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()
&& hasDefaultVisibility(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()
&& hasDefaultVisibility(element, ctor)
&& isDefaultBody(ctor.getBody())
&& ctor.getThrownTypes().isEmpty()) {
redundantCtor = ctor;
}
}
if (redundantCtor != null) {
addLocalProblem(redundantCtor, MESSAGE, ProblemType.REDUNDANT_DEFAULT_CONSTRUCTOR);
}
}
});
Expand All @@ -46,18 +66,42 @@ private boolean hasDefaultVisibility(CtClass<?> type, CtConstructor<?> ctor) {
return type.isEnum() || ctor.getVisibility() == type.getVisibility();
}

private boolean isEmpty(CtBlock<?> block) {
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.
.filter(statement -> !(statement instanceof CtInvocation<?> invocation
.allMatch(statement -> statement instanceof CtInvocation<?> invocation
&& invocation.getExecutable().isConstructor()
&& invocation.getArguments().isEmpty()))
.findAny()
.isEmpty();
&& invocation.getArguments().isEmpty());
}

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();
});
}
}
48 changes: 47 additions & 1 deletion .../src/test/java/de/firemage/autograder/core/check/complexity/TestRedundantConstructor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ record Test() {
}

@Test
void testRedundantRecordNormalConstructor() throws IOException, LinterException {
void testRedundantEmptyRecordNormalConstructor() throws IOException, LinterException {
assertRedundant(
"Test",
"""
Expand All @@ -117,6 +117,21 @@ 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(
Expand All @@ -139,6 +154,37 @@ 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(
Expand Down

0 comments on commit a53f7e0

Please sign in to comment.