From 4054640473d1b559abe2962131adbadab29592d5 Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Fri, 11 Oct 2024 10:41:52 +0200 Subject: [PATCH] fix #611 --- .../oop/MethodShouldBeAbstractCheck.java | 59 ++--- .../core/integrated/MethodUtil.java | 7 + .../src/main/resources/strings.de.ftl | 2 +- .../src/main/resources/strings.en.ftl | 2 +- .../check/oop/TestMethodShouldBeAbstract.java | 218 ++++++++++++++++++ .../MethodShouldBeAbstract/code/Test.java | 72 ------ .../MethodShouldBeAbstract/config.txt | 9 - 7 files changed, 260 insertions(+), 109 deletions(-) create mode 100644 autograder-core/src/test/java/de/firemage/autograder/core/check/oop/TestMethodShouldBeAbstract.java delete mode 100644 autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/MethodShouldBeAbstract/code/Test.java delete mode 100644 autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/MethodShouldBeAbstract/config.txt diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/oop/MethodShouldBeAbstractCheck.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/oop/MethodShouldBeAbstractCheck.java index d8be7d4d..2423df25 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/oop/MethodShouldBeAbstractCheck.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/oop/MethodShouldBeAbstractCheck.java @@ -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; @@ -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() )); @@ -31,18 +34,20 @@ private static LocalizedMessage formatExplanation(CtMethod method) { protected void check(StaticAnalysis staticAnalysis) { staticAnalysis.processWith(new AbstractProcessor>() { @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; } @@ -50,22 +55,24 @@ public void process(CtClass clazz) { 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); } } } diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/MethodUtil.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/MethodUtil.java index a897eb18..03e2d75d 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/MethodUtil.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/MethodUtil.java @@ -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; @@ -309,4 +310,10 @@ private static Map, List>> 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(); + } } diff --git a/autograder-core/src/main/resources/strings.de.ftl b/autograder-core/src/main/resources/strings.de.ftl index 6bef027e..838667a3 100644 --- a/autograder-core/src/main/resources/strings.de.ftl +++ b/autograder-core/src/main/resources/strings.de.ftl @@ -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. diff --git a/autograder-core/src/main/resources/strings.en.ftl b/autograder-core/src/main/resources/strings.en.ftl index 8ea5bdca..4e334258 100644 --- a/autograder-core/src/main/resources/strings.en.ftl +++ b/autograder-core/src/main/resources/strings.en.ftl @@ -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. diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/oop/TestMethodShouldBeAbstract.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/oop/TestMethodShouldBeAbstract.java new file mode 100644 index 00000000..581cf991 --- /dev/null +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/oop/TestMethodShouldBeAbstract.java @@ -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 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(); + } + +} diff --git a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/MethodShouldBeAbstract/code/Test.java b/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/MethodShouldBeAbstract/code/Test.java deleted file mode 100644 index 9dd5c96a..00000000 --- a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/MethodShouldBeAbstract/code/Test.java +++ /dev/null @@ -1,72 +0,0 @@ -package de.firemage.autograder.core.check_tests.MethodShouldBeAbstract.code; - -import java.util.ArrayDeque; -import java.util.ArrayList; -import java.util.HashMap; -import java.util.Hashtable; -import java.util.LinkedList; -import java.util.Stack; -import java.util.TreeMap; -import java.util.Vector; - -public abstract class Test { - private Object privateNull() { /*# ok; private #*/ - return null; - } - - public abstract Object abstractMethod(); /*# ok; abstract #*/ - - public static Object staticNull() { /*# ok; static #*/ - return null; - } - - @Override - public String toString() { /*# ok; overrides #*/ - return null; - } - - private Object privateInstance() { /*# ok; private #*/ - return new Object(); - } - - public Object publicNull() { /*# not ok; returns null #*/ - return null; - } - - public Object publicNullWithParameters(int x) { /*# not ok; returns null #*/ - return null; - } - - public Object publicInstance() { /*# ok; not null #*/ - return new Object(); - } - - public void publicDefaultEmpty() { /*# not ok; empty #*/ - - } - - public void publicDefaultEmptyWithParameters(int x) { /*# not ok; empty #*/ - - } - - public void publicThrows() { /*# not ok; only throws #*/ - throw new IllegalStateException(); - } - - public void publicThrows2() { /*# not ok; only throws #*/ - throw new UnsupportedOperationException(); - } - - public void publicThrowsWithParameters(int x) { /*# not ok; only throws #*/ - throw new IllegalStateException(); - } - - - public Object publicComplex() { /*# ok; does not look like a primitive default #*/ - if (1 < 2) { - throw new IllegalStateException(); - } else { - return null; - } - } -} diff --git a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/MethodShouldBeAbstract/config.txt b/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/MethodShouldBeAbstract/config.txt deleted file mode 100644 index a82a7d41..00000000 --- a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/MethodShouldBeAbstract/config.txt +++ /dev/null @@ -1,9 +0,0 @@ -oop.MethodShouldBeAbstractCheck -Method should be abstract instead of providing a dummy implementation -Test.java:32 -Test.java:36 -Test.java:44 -Test.java:48 -Test.java:52 -Test.java:56 -Test.java:60