detect methods that should be static #402

autograder-core/src/main/java/de/firemage/autograder/core/ProblemType.java

@@ -73,7 +73,7 @@ public enum ProblemType {
     TOO_FEW_PACKAGES,
     TRY_CATCH_COMPLEXITY,
     AVOID_STATIC_BLOCKS,
-
+    METHOD_SHOULD_BE_STATIC,
     REASSIGNED_PARAMETER,
     DOUBLE_BRACE_INITIALIZATION,
     FOR_CAN_BE_FOREACH,

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

@@ -0,0 +1,108 @@
+package de.firemage.autograder.core.check.oop;
+
+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.IntegratedCheck;
+import de.firemage.autograder.core.integrated.SpoonUtil;
+import de.firemage.autograder.core.integrated.StaticAnalysis;
+import spoon.processing.AbstractProcessor;
+import spoon.reflect.code.CtExecutableReferenceExpression;
+import spoon.reflect.code.CtFieldAccess;
+import spoon.reflect.code.CtInvocation;
+import spoon.reflect.code.CtSuperAccess;
+import spoon.reflect.code.CtTargetedExpression;
+import spoon.reflect.code.CtThisAccess;
+import spoon.reflect.code.CtTypeAccess;
+import spoon.reflect.declaration.CtElement;
+import spoon.reflect.declaration.CtMethod;
+import spoon.reflect.declaration.CtType;
+import spoon.reflect.declaration.CtTypeMember;
+import spoon.reflect.visitor.Filter;
+
+import java.util.Map;
+
+
+@ExecutableCheck(reportedProblems = { ProblemType.METHOD_SHOULD_BE_STATIC })
+public class MethodShouldBeStatic extends IntegratedCheck {
+    /**
+     * Finds elements that access the given type through this. Accessing the same type through a different object is ok.
+     *
+     * @param ctType the type to be accessed
+     */
+    private record ThisAccessFilter(CtType<?> ctType) implements Filter<CtElement> {
+        private boolean isSuperTypeAccess(CtTargetedExpression<?, ?> ctTargetedExpression) {
+            return ctTargetedExpression.getTarget() instanceof CtSuperAccess<?> superAccess
+                && this.ctType.isSubtypeOf(superAccess.getType());
+        }
+
+        private boolean isThisTypeAccess(CtTargetedExpression<?, ?> ctTargetedExpression) {
+            if (this.isSuperTypeAccess(ctTargetedExpression)) {
+                return true;
+            }
+
+            return ctTargetedExpression.getTarget() instanceof CtThisAccess<?> thisAccess
+                && thisAccess.getTarget() instanceof CtTypeAccess<?> ctTypeAccess
+                && this.ctType.equals(ctTypeAccess.getAccessedType().getTypeDeclaration());
+        }
+
+        @Override
+        public boolean matches(CtElement element) {
+            return switch (element) {
+                case CtFieldAccess<?> ctFieldAccess -> this.isThisTypeAccess(ctFieldAccess);
+                case CtInvocation<?> ctInvocation -> this.isThisTypeAccess(ctInvocation);
+                case CtExecutableReferenceExpression<?, ?> ctExecutableReferenceExpression -> this.isThisTypeAccess(ctExecutableReferenceExpression);
+                default -> false;
+            };
+        }
+    }
+
+    private static boolean isEffectivelyStatic(CtTypeMember ctTypeMember) {
+        if (ctTypeMember.isStatic()) {
+            return true;
+        }
+
+        if (ctTypeMember.getDeclaringType().isInterface() || ctTypeMember.isAbstract()) {
+            return false;
+        }
+
+        if (ctTypeMember instanceof CtMethod<?> ctMethod) {
+            if (SpoonUtil.isOverriddenMethod(ctMethod) || SpoonUtil.isOverridden(ctMethod)) {
+                return false;
+            }
+
+            if (ctMethod.getBody() == null) {
+                return true;
+            }
+
+            return ctMethod.getBody().filterChildren(new ThisAccessFilter(ctTypeMember.getDeclaringType())).first() == null;
+        }
+
+        return false;
+    }
+
+    @Override
+    protected void check(StaticAnalysis staticAnalysis) {
+        staticAnalysis.processWith(new AbstractProcessor<CtMethod<?>>() {
+            @Override
+            public void process(CtMethod<?> ctMethod) {
+                if (ctMethod.isImplicit() || !ctMethod.getPosition().isValidPosition()) {
+                    return;
+                }
+
+
+                if (!ctMethod.isStatic() && isEffectivelyStatic(ctMethod)) {
+                    addLocalProblem(
+                        ctMethod,
+                        new LocalizedMessage(
+                            "method-should-be-static",
+                            Map.of("name", ctMethod.getSimpleName())
+                        ),
+                        ProblemType.METHOD_SHOULD_BE_STATIC
+                    );
+                }
+            }
+        });
+    }
+
+}

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

@@ -1096,6 +1096,16 @@ public static boolean isOverriddenMethod(CtMethod<?> ctMethod) {
         return !topDefinitions.isEmpty();
     }
 
+    /**
+     * Checks if the given method is overridden by another method.
+     *
+     * @param ctMethod the method to check, must not be null
+     * @return true if the given method is overridden by another method, false otherwise
+     */
+    public static boolean isOverridden(CtMethod<?> ctMethod) {
+        return ctMethod.getFactory().getModel().filterChildren(new OverridingMethodFilter(ctMethod)).first() != null;
+    }
+
     public static boolean isInOverriddenMethod(CtElement ctElement) {
         CtMethod<?> ctMethod = ctElement.getParent(CtMethod.class);
         if (ctMethod == null) {

autograder-core/src/main/resources/strings.de.ftl

@@ -231,6 +231,8 @@ leaked-collection-assign = Die Methode '{$method}' weißt dem Feld '{$field}' ei
 
 method-abstract-exp = {$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.
+
 utility-exp-final = Utility-Klasse ist nicht final
 utility-exp-constructor = Utility-Klassen müssen genau einen privaten und parameterlosen Konstruktor haben
 

autograder-core/src/main/resources/strings.en.ftl

@@ -230,6 +230,8 @@ leaked-collection-assign = The method '{$method}' assigns a reference to the fie
 
 method-abstract-exp = {$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.
+
 utility-exp-final = Utility class is not final
 utility-exp-constructor = Utility classes must have a single private no-arg constructor