Skip to content

Commit

Permalink
fix #611
Browse files Browse the repository at this point in the history
  • Loading branch information
Luro02 committed Oct 11, 2024
1 parent b9ae2f5 commit 4054640
Show file tree
Hide file tree
Showing 7 changed files with 260 additions and 109 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,11 @@
import de.firemage.autograder.core.ProblemType;
import de.firemage.autograder.core.check.ExecutableCheck;
import de.firemage.autograder.core.integrated.IntegratedCheck;
import de.firemage.autograder.core.integrated.MethodHierarchy;
import de.firemage.autograder.core.integrated.MethodUtil;
import de.firemage.autograder.core.integrated.StatementUtil;
import de.firemage.autograder.core.integrated.StaticAnalysis;
import de.firemage.autograder.core.integrated.TypeUtil;
import spoon.processing.AbstractProcessor;
import spoon.reflect.code.CtConstructorCall;
import spoon.reflect.code.CtLiteral;
Expand All @@ -21,7 +24,7 @@
@ExecutableCheck(reportedProblems = {ProblemType.METHOD_USES_PLACEHOLDER_IMPLEMENTATION})
public class MethodShouldBeAbstractCheck extends IntegratedCheck {
private static LocalizedMessage formatExplanation(CtMethod<?> method) {
return new LocalizedMessage("method-abstract-exp", Map.of(
return new LocalizedMessage("method-should-be-abstract", Map.of(
"type", method.getDeclaringType().getQualifiedName(),
"method", method.getSimpleName()
));
Expand All @@ -31,41 +34,45 @@ private static LocalizedMessage formatExplanation(CtMethod<?> method) {
protected void check(StaticAnalysis staticAnalysis) {
staticAnalysis.processWith(new AbstractProcessor<CtClass<?>>() {
@Override
public void process(CtClass<?> clazz) {
if (!clazz.isAbstract()) {
public void process(CtClass<?> ctClass) {
if (ctClass.isImplicit() || !ctClass.isAbstract()) {
return;
}

for (CtMethod<?> method : clazz.getMethods()) {
if (!method.isPublic() && !method.isProtected()) {
continue;
}

// False positives if the method overrides another method but is not correctly annotated
if (method.isStatic() || method.isAbstract() || method.hasAnnotation(Override.class)) {
for (CtMethod<?> method : ctClass.getMethods()) {
if (!method.isPublic() && !method.isProtected()
|| method.isStatic()
|| method.isAbstract()
// skip methods that override another method
|| MethodHierarchy.isOverridingMethod(method)
// skip methods that are never overridden (would never make sense to be abstract)
|| !MethodHierarchy.isOverriddenMethod(method)
|| MethodUtil.hasBeenInvoked(method)) {
continue;
}

List<CtStatement> statements = StatementUtil.getEffectiveStatements(method.getBody());
if (statements.isEmpty()) {
addLocalProblem(method, formatExplanation(method),
ProblemType.METHOD_USES_PLACEHOLDER_IMPLEMENTATION);
} else if (statements.size() == 1) {
CtStatement statement = statements.get(0);
if (statement instanceof CtReturn<?> ret
&& ret.getReturnedExpression() instanceof CtLiteral<?> literal
&& literal.getValue() == null) {
addLocalProblem(method, formatExplanation(method),
ProblemType.METHOD_USES_PLACEHOLDER_IMPLEMENTATION);
} else if (statement instanceof CtThrow ctThrow
&& ctThrow.getThrownExpression() instanceof CtConstructorCall<?> call) {
String type = call.getType().getQualifiedName();
if (type.equals("java.lang.UnsupportedOperationException") ||
type.equals("java.lang.IllegalStateException")) {
addLocalProblem(method, formatExplanation(method),
ProblemType.METHOD_USES_PLACEHOLDER_IMPLEMENTATION);
}
}
return;
}

if (statements.size() != 1) {
return;
}

CtStatement statement = statements.get(0);
if (statement instanceof CtReturn<?> ret
&& ret.getReturnedExpression() instanceof CtLiteral<?> literal
&& literal.getValue() == null) {
addLocalProblem(method, formatExplanation(method),
ProblemType.METHOD_USES_PLACEHOLDER_IMPLEMENTATION);
} else if (statement instanceof CtThrow ctThrow
&& ctThrow.getThrownExpression() instanceof CtConstructorCall<?> call
&& TypeUtil.isTypeEqualTo(call.getType(), java.lang.UnsupportedOperationException.class, java.lang.IllegalStateException.class)) {
addLocalProblem(method, formatExplanation(method),
ProblemType.METHOD_USES_PLACEHOLDER_IMPLEMENTATION);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import spoon.reflect.code.CtVariableAccess;
import spoon.reflect.code.CtVariableWrite;
import spoon.reflect.declaration.CtElement;
import spoon.reflect.declaration.CtExecutable;
import spoon.reflect.declaration.CtField;
import spoon.reflect.declaration.CtMethod;
import spoon.reflect.declaration.CtType;
Expand Down Expand Up @@ -309,4 +310,10 @@ private static Map<CtVariable<?>, List<CtVariableAccess<?>>> dependencies(
.filter(entry -> !codeSegmentVariables.contains(entry.getKey()) && isDependency.test(entry.getKey()))
.collect(Collectors.groupingBy(Map.Entry::getKey, IdentityHashMap::new, Collectors.mapping(Map.Entry::getValue, Collectors.toList())));
}


public static boolean hasBeenInvoked(CtExecutable<?> ctExecutable) {
// NOTE: at the moment, overrides are not considered uses -> every other use would be an invocation
return UsesFinder.getAllUses(ctExecutable).hasAny();
}
}
2 changes: 1 addition & 1 deletion autograder-core/src/main/resources/strings.de.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ leaked-collection-return = Die Methode '{$method}' gibt eine Referenz zu dem Fel
leaked-collection-constructor = Der Konstruktor '{$signature}' weißt dem Feld '{$field}' eine Referenz zu, dadurch ist es möglich das Feld von außerhalb zu verändern. Weise stattdessen eine Kopie dem Feld zu.
leaked-collection-assign = Die Methode '{$method}' weißt dem Feld '{$field}' eine Referenz zu, dadurch ist es möglich das Feld von außerhalb zu verändern. Weise stattdessen eine Kopie dem Feld zu.
method-abstract-exp = {$type}::{$method} sollte abstrakt sein, anstatt eine Platzhalter-Implementierung anzugeben
method-should-be-abstract = {$type}::{$method} sollte abstrakt sein, anstatt eine Platzhalter-Implementierung anzugeben
method-should-be-static = Die Methode '{$name}' sollte statisch sein, da sie auf keine Instanzattribute oder Methoden zugreift.
Expand Down
2 changes: 1 addition & 1 deletion autograder-core/src/main/resources/strings.en.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ leaked-collection-return = The method '{$method}' returns a reference to the fie
leaked-collection-constructor = The constructor '{$signature}' assigns a reference to the field '{$field}'. This allows the field to be modified from the outside. Instead, a copy should be made before setting the field.
leaked-collection-assign = The method '{$method}' assigns a reference to the field '{$field}'. This allows the field to be modified from the outside. Instead, a copy should be made before setting the field.
method-abstract-exp = {$type}::{$method} should be abstract and not provide a default implementation
method-should-be-abstract = {$type}::{$method} should be abstract and not provide a default implementation
method-should-be-static = The method '{$name}' should be static, because it does not access instance attributes or methods.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,218 @@
package de.firemage.autograder.core.check.oop;

import de.firemage.autograder.api.JavaVersion;
import de.firemage.autograder.api.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.file.StringSourceInfo;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.CsvSource;

import java.io.IOException;
import java.util.List;
import java.util.Map;

import static org.junit.jupiter.api.Assertions.assertEquals;

class TestMethodShouldBeAbstract extends AbstractCheckTest {
private static final List<ProblemType> PROBLEM_TYPES = List.of(ProblemType.METHOD_USES_PLACEHOLDER_IMPLEMENTATION);

void assertShouldBeAbstract(Problem problem, String type, String method) {
assertEquals(
this.linter.translateMessage(new LocalizedMessage(
"method-should-be-abstract",
Map.of(
"type", type,
"method", method
)
)),
this.linter.translateMessage(problem.getExplanation())
);
}

@Test
void testPrivateMethod() throws IOException, LinterException {
ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString(
JavaVersion.JAVA_17,
"Test",
"""
public abstract class Test {
private Object privateNull() { /*# ok; private #*/
return null;
}
private Object privateInstance() { /*# ok; private #*/
return new Object();
}
}
"""
), PROBLEM_TYPES);

problems.assertExhausted();
}


@Test
void testAbstractMethod() throws IOException, LinterException {
ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString(
JavaVersion.JAVA_17,
"Test",
"""
public abstract class Test {
public abstract Object abstractMethod(); /*# ok; abstract #*/
public static void main(String[] args) {
var test = new Test() {
@Override
public Object abstractMethod() {
return null;
}
};
test.abstractMethod();
}
}
"""
), PROBLEM_TYPES);

problems.assertExhausted();
}

@Test
void testStaticMethod() throws IOException, LinterException {
ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString(
JavaVersion.JAVA_17,
"Test",
"""
public abstract class Test {
public static Object staticNull() { /*# ok; static #*/
return null;
}
public static void main(String[] args) {
var test = new Test() {};
test.staticNull();
}
}
"""
), PROBLEM_TYPES);

problems.assertExhausted();
}


@Test
void testOverriddenMethod() throws IOException, LinterException {
ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString(
JavaVersion.JAVA_17,
"Test",
"""
public abstract class Test {
@Override
public String toString() { /*# ok; overrides #*/
return null;
}
public static void main(String[] args) {
var test = new Test() {
@Override
public String toString() {
return "abc";
}
};
System.out.println(test.toString());
}
}
"""
), PROBLEM_TYPES);

problems.assertExhausted();
}

@ParameterizedTest
@CsvSource(
delimiter = '|',
useHeadersInDisplayName = true,
value = {
" Return Type | Arguments | Body | Expected ",
" Object | | return null; | foo ",
" Object | int x | return null; | foo ",
" Object | | return new Object(); | ",
" void | | | foo ",
" void | int x | | foo ",
" void | | throw new IllegalStateException(); | foo ",
" void | | throw new UnsupportedOperationException(); | foo ",
" void | int x | throw new IllegalStateException(); | foo ",
" Object | | if (1 < 2) throw new IllegalStateException(); return null; | ",
}
)
void testShouldBeAbstract(String returnType, String arguments, String body, String expected) throws IOException, LinterException {
ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString(
JavaVersion.JAVA_17,
"Test",
"""
public abstract class Test {
public %s foo(%s) { %s }
public static void main(String[] args) {
var test = new Test() {
@Override
public %s foo(%s) {
%s
}
};
test.foo(%s);
}
}
""".formatted(
returnType, arguments == null ? "" : arguments, body == null ? "" : body,
returnType, arguments == null ? "" : arguments, body == null ? "" : body,
arguments == null ? "" : "1"
)
), PROBLEM_TYPES);

if (expected != null) {
assertShouldBeAbstract(problems.next(), "Test", expected);
}

problems.assertExhausted();
}

@Test
void testCalledInSubclass() throws IOException, LinterException {
ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString(
JavaVersion.JAVA_17,
"Command",
"""
public abstract class Command {
protected void policyError() throws IllegalStateException {
throw new IllegalStateException("Command was run against availability policy.");
}
public static class RollDiceCommand extends Command {
public void rollDice(String session) throws IllegalStateException {
if (session == null) {
super.policyError();
}
}
}
public static void main(String[] args) {
var test = new RollDiceCommand();
test.rollDice(null);
}
}
"""
), PROBLEM_TYPES);

problems.assertExhausted();
}

}
Loading

0 comments on commit 4054640

Please sign in to comment.