Merge pull request #624 from Luro02/main

Fix some bugs

autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseModuloOperator.java

@@ -26,10 +26,6 @@
 @ExecutableCheck(reportedProblems = { ProblemType.USE_MODULO_OPERATOR })
 public class UseModuloOperator extends IntegratedCheck {
     private static final Set<BinaryOperatorKind> ALLOWED_OPERATORS = Set.of(
-        BinaryOperatorKind.LT,
-        BinaryOperatorKind.LE,
-        BinaryOperatorKind.GE,
-        BinaryOperatorKind.GT,
         BinaryOperatorKind.EQ
     );
 
@@ -53,6 +49,7 @@ private void checkModulo(CtIf ctIf) {
 
         CtVariableReference<?> assignedVariable = ctVariableWrite.getVariable();
 
+        // this swaps the condition operands, if the assigned variable is on the right side
         CtBinaryOperator<Boolean> condition = ExpressionUtil.normalizeBy(
             (left, right) -> right instanceof CtVariableAccess<?> ctVariableAccess && ctVariableAccess.getVariable().equals(assignedVariable),
             ctBinaryOperator
@@ -64,13 +61,6 @@ private void checkModulo(CtIf ctIf) {
             return;
         }
 
-        // if (variable >= 3) {
-        //    variable = 0;
-        // }
-        //
-        // is equal to
-        //
-        // variable %= 3;
         CtExpression<?> checkedValue = condition.getRightHandOperand();
 
         // for boxed types, one could check if the value is null,
@@ -79,7 +69,7 @@ private void checkModulo(CtIf ctIf) {
             return;
         }
 
-        if (Set.of(BinaryOperatorKind.GE, BinaryOperatorKind.EQ).contains(condition.getKind())) {
+        if (condition.getKind() == BinaryOperatorKind.EQ) {
             addLocalProblem(
                 ctIf,
                 new LocalizedMessage(

autograder-core/src/main/java/de/firemage/autograder/core/check/exceptions/ExceptionMessageCheck.java

@@ -6,6 +6,7 @@
 
 import de.firemage.autograder.core.integrated.ExpressionUtil;
 import de.firemage.autograder.core.integrated.IntegratedCheck;
+import de.firemage.autograder.core.integrated.MethodHierarchy;
 import de.firemage.autograder.core.integrated.StaticAnalysis;
 import de.firemage.autograder.core.integrated.TypeUtil;
 import spoon.processing.AbstractProcessor;
@@ -16,6 +17,8 @@
 import spoon.reflect.code.CtThrow;
 import spoon.reflect.declaration.CtConstructor;
 import spoon.reflect.declaration.CtElement;
+import spoon.reflect.declaration.CtMethod;
+import spoon.reflect.declaration.CtType;
 
 @ExecutableCheck(reportedProblems = ProblemType.EXCEPTION_WITHOUT_MESSAGE)
 public class ExceptionMessageCheck extends IntegratedCheck {
@@ -25,6 +28,13 @@ private static boolean isExceptionWithoutMessage(CtExpression<?> expression) {
             return false;
         }
 
+        // consider overriding getMessage() as having a message
+        CtType<?> exceptionType = expression.getType().getTypeDeclaration();
+        CtMethod<?> getMessageMethod = exceptionType.getMethod("getMessage");
+        if (getMessageMethod != null && MethodHierarchy.isOverridingMethod(getMessageMethod)) {
+            return false;
+        }
+
         // check if the invoked constructor passes a message to the parent exception like this:
         // class MyException extends Exception { MyException() { super("here is the message"); } }
         if (ctConstructorCall.getExecutable().getExecutableDeclaration() instanceof CtConstructor<?> ctConstructor

autograder-core/src/main/java/de/firemage/autograder/core/check/general/UseDifferentVisibility.java

@@ -85,6 +85,12 @@ private static Visibility getVisibility(CtTypeMember ctTypeMember) {
             && (ctType.equals(declaringType)
             // special case for inner classes
             || ctTypeMember.getTopLevelType().equals(ctType))) {
+            // You can not override private methods. They are still in the same class, so the next visibility is default.
+            // See the "testOverrideEnum" for an example of when this can happen.
+            if (ctTypeMember instanceof CtMethod<?> ctMethod && MethodHierarchy.isOverriddenMethod(ctMethod)) {
+                return Visibility.DEFAULT;
+            }
+
             return Visibility.PRIVATE;
         }
 

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,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);
                     }
                 }
             }

autograder-core/src/main/java/de/firemage/autograder/core/check/oop/MethodShouldBeStatic.java

@@ -3,9 +3,11 @@
 import de.firemage.autograder.core.LocalizedMessage;
 import de.firemage.autograder.core.ProblemType;
 import de.firemage.autograder.core.check.ExecutableCheck;
+import de.firemage.autograder.core.integrated.CoreUtil;
 import de.firemage.autograder.core.integrated.IntegratedCheck;
 import de.firemage.autograder.core.integrated.MethodHierarchy;
 import de.firemage.autograder.core.integrated.StaticAnalysis;
+import de.firemage.autograder.core.integrated.TypeUtil;
 import spoon.processing.AbstractProcessor;
 import spoon.reflect.code.CtExecutableReferenceExpression;
 import spoon.reflect.code.CtFieldAccess;
@@ -20,6 +22,7 @@
 import spoon.reflect.declaration.CtTypeMember;
 import spoon.reflect.visitor.Filter;
 
+import java.util.List;
 import java.util.Map;
 
 @ExecutableCheck(reportedProblems = { ProblemType.METHOD_SHOULD_BE_STATIC, ProblemType.METHOD_SHOULD_BE_STATIC_NOT_PUBLIC})
@@ -45,16 +48,31 @@ private boolean isThisTypeAccess(CtTargetedExpression<?, ?> ctTargetedExpression
                 && this.ctType.equals(ctTypeAccess.getAccessedType().getTypeDeclaration());
         }
 
+        private static final List<Class<?>> SUPPORTED_TYPES = List.of(
+            CtFieldAccess.class,
+            CtInvocation.class,
+            CtExecutableReferenceExpression.class
+        );
+
         @Override
         public boolean matches(CtElement element) {
-            if (element instanceof CtFieldAccess<?> ctFieldAccess) {
-                return this.isThisTypeAccess(ctFieldAccess);
-            } else if (element instanceof CtInvocation<?> ctInvocation) {
-                return this.isThisTypeAccess(ctInvocation);
-            } else if (element instanceof CtExecutableReferenceExpression<?, ?> ctExecutableReferenceExpression) {
-                return this.isThisTypeAccess(ctExecutableReferenceExpression);
+            // The following types can be a targeted expression:
+            // - CtArrayAccess
+            // - CtConstructorCall
+            // - CtExecutableReferenceExpression
+            // - CtFieldAccess
+            // - CtInvocation
+            // - CtNewClass
+            // - CtSuperAccess
+            // - CtThisAccess
+            if (element instanceof CtTargetedExpression<?,?> ctTargetedExpression && CoreUtil.isInstanceOfAny(ctTargetedExpression, SUPPORTED_TYPES)) {
+                return this.isThisTypeAccess(ctTargetedExpression);
             }
-            return false;
+
+            return element instanceof CtThisAccess<?> ctThisAccess
+                && ctThisAccess.getTarget() instanceof CtTypeAccess<?> ctTypeAccess
+                && this.ctType.equals(ctTypeAccess.getAccessedType().getTypeDeclaration())
+                && !CoreUtil.isInstanceOfAny(element.getParent(), SUPPORTED_TYPES);
         }
     }
 

autograder-core/src/main/java/de/firemage/autograder/core/integrated/CoreUtil.java

@@ -137,6 +137,10 @@ public static SourcePosition getNamePosition(CtNamedElement ctNamedElement) {
     }
 
     public static boolean isInstanceOfAny(Object object, Class<?>... classes) {
+        return isInstanceOfAny(object, Arrays.asList(classes));
+    }
+
+    public static boolean isInstanceOfAny(Object object, Iterable<Class<?>> classes) {
         for (Class<?> clazz : classes) {
             if (clazz.isInstance(object)) {
                 return true;

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<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();
+    }
 }

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.
 

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.
 

autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestUseModuloOperator.java

@@ -60,17 +60,7 @@ public static int adjust(int value, int limit) {
                 """
         ), PROBLEM_TYPES);
 
-        List<String> expectedSuggestions = List.of(
-            "result %= limit + 1",
-            "result %= limit",
-            "result %= limit"
-        );
-
-        for (String expectedSuggestion : expectedSuggestions) {
-            Problem problem = problems.next();
-
-            assertReimplementation(problem, expectedSuggestion);
-        }
+        assertReimplementation(problems.next(), "result %= limit");
         problems.assertExhausted();
     }
 

autograder-core/src/test/java/de/firemage/autograder/core/check/exceptions/TestExceptionMessageCheck.java

@@ -311,4 +311,66 @@ public static void main(String[] args) throws Exception {
 
         problems.assertExhausted();
     }
+
+
+    @Test
+    void testExceptionOverridingGetMessage() throws IOException, LinterException {
+        ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceStrings(
+            JavaVersion.JAVA_17,
+            Map.ofEntries(
+                Map.entry(
+                    "MyException",
+                    """
+                        public class MyException extends Exception {
+                            public String getMessage() {
+                                return "failed to start the application";
+                            }
+                        }
+                        """
+                ),
+                Map.entry(
+                    "Main",
+                    """
+                        public class Main {
+                            public static void main(String[] args) throws Exception {
+                                throw new MyException(); /*# ok #*/
+                            }
+                        }
+                    """
+                )
+            )
+        ), PROBLEM_TYPES);
+
+        problems.assertExhausted();
+    }
+
+    @Test
+    void testEmptyException() throws IOException, LinterException {
+        ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceStrings(
+            JavaVersion.JAVA_17,
+            Map.ofEntries(
+                Map.entry(
+                    "MyException",
+                    """
+                        public class MyException extends Exception {
+                        }
+                        """
+                ),
+                Map.entry(
+                    "Main",
+                    """
+                        public class Main {
+                            public static void main(String[] args) throws Exception {
+                                throw new MyException();
+                            }
+                        }
+                    """
+                )
+            )
+        ), PROBLEM_TYPES);
+
+        assertMissingMessage(problems.next());
+
+        problems.assertExhausted();
+    }
 }