Merge pull request #334 from Luro02/main

Implement some things

autograder-cmd/src/main/java/de/firemage/autograder/cmd/Application.java

@@ -37,10 +37,13 @@
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.util.ArrayList;
 import java.util.Collection;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Locale;
 import java.util.Optional;
+import java.util.Set;
 import java.util.concurrent.Callable;
 import java.util.function.Consumer;
 import java.util.stream.Stream;
@@ -92,9 +95,11 @@ public class Application implements Callable<Integer> {
     private CommandSpec spec;
 
     private final TempLocation tempLocation;
+    private final Set<String> exludedClasses;
 
     public Application(TempLocation tempLocation) {
         this.tempLocation = tempLocation;
+        this.exludedClasses = new HashSet<>();
     }
 
     private static Charset getConsoleCharset() {
@@ -188,6 +193,33 @@ private void execute(
         CmdUtil.endSection();
     }
 
+    private List<ProblemType> getChecks() throws IOException {
+        List<String> checks;
+        if (passConfig) {
+            checks = List.of(new ObjectMapper(new YAMLFactory()).readValue(checkConfig, String[].class));
+        } else {
+            checks = List.of(new ObjectMapper(new YAMLFactory()).readValue(new File(checkConfig), String[].class));
+        }
+
+        List<ProblemType> result = new ArrayList<>();
+
+        // HACK: EXCLUDE is used to ignore some classes, blame the config format for the hacky solution
+        for (String check : checks) {
+            if (check.startsWith("EXCLUDE")) {
+                this.exludedClasses.addAll(List.of(check.substring("EXCLUDE".length() + 1).split(" ")));
+                continue;
+            }
+
+            try {
+                result.add(ProblemType.valueOf(check));
+            } catch (IllegalArgumentException e) {
+                throw new IllegalArgumentException("Unknown check '%s'".formatted(check), e);
+            }
+        }
+
+        return result;
+    }
+
     @Override
     public Integer call() {
         if (!JavaVersion.isValidJavaVersion(javaVersion)) {
@@ -220,11 +252,7 @@ public Integer call() {
 
         List<ProblemType> checks;
         try {
-            if (passConfig) {
-                checks = List.of(new ObjectMapper(new YAMLFactory()).readValue(checkConfig, ProblemType[].class));
-            } else {
-                checks = List.of(new ObjectMapper(new YAMLFactory()).readValue(new File(checkConfig), ProblemType[].class));
-            }
+            checks = this.getChecks();
         } catch (IOException e) {
             e.printStackTrace();
             return IO_EXIT_CODE;
@@ -235,6 +263,7 @@ public Integer call() {
             .tempLocation(this.tempLocation)
             .enableDynamicAnalysis(isDynamicAnalysisEnabled)
             .maxProblemsPerCheck(this.maxProblemsPerCheck)
+            .exclude(problem -> this.exludedClasses.contains(problem.getPosition().file().getName().replace(".java", "")))
             .build();
 
         Consumer<LinterStatus> statusConsumer = status ->

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

@@ -4,6 +4,7 @@
 import de.firemage.autograder.core.file.SourcePath;
 import spoon.reflect.cu.SourcePosition;
 import spoon.reflect.declaration.CtElement;
+import spoon.reflect.declaration.CtMethod;
 import spoon.reflect.declaration.CtType;
 
 import java.io.File;
@@ -28,7 +29,10 @@ public static CodePosition fromSourcePosition(
 
         SourcePath relativePath = sourceInfo.getCompilationUnit(file.toPath()).path();
 
-        if (ctElement instanceof CtType<?> && ctElement.getPosition().equals(sourcePosition)) {
+        // Instead of highlighting all lines of a class or method, only highlight the first line.
+        //
+        // Someone might explicitly specify a source position, in which case it will differ from the element's position.
+        if ((ctElement instanceof CtType<?> || ctElement instanceof CtMethod<?>) && ctElement.getPosition().equals(sourcePosition)) {
             return new CodePosition(
                 sourceInfo,
                 relativePath,

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

@@ -35,6 +35,7 @@
 import java.util.Locale;
 import java.util.Map;
 import java.util.function.Consumer;
+import java.util.function.Predicate;
 import java.util.stream.Collectors;
 
 public final class Linter {
@@ -44,14 +45,16 @@ public final class Linter {
     private final boolean disableDynamicAnalysis;
     private final ClassLoader classLoader;
     private final int maxProblemsPerCheck;
+    private final Predicate<Problem> isExcluded;
 
     private Linter(
         Locale locale,
         TempLocation tempLocation,
         int threads,
         boolean disableDynamicAnalysis,
         ClassLoader classLoader,
-        int maxProblemsPerCheck
+        int maxProblemsPerCheck,
+        Predicate<Problem> isExcluded
     ) {
         String filename = switch (locale.getLanguage()) {
             case "de" -> "/strings.de.ftl";
@@ -72,6 +75,7 @@ private Linter(
         this.disableDynamicAnalysis = disableDynamicAnalysis;
         this.classLoader = classLoader;
         this.maxProblemsPerCheck = maxProblemsPerCheck;
+        this.isExcluded = isExcluded;
     }
 
     public static class Builder {
@@ -81,9 +85,11 @@ public static class Builder {
         private boolean disableDynamicAnalysis = true;
         private ClassLoader classLoader;
         private int maxProblemsPerCheck = -1;
+        private Predicate<Problem> isExcluded;
 
         private Builder(Locale locale) {
             this.locale = locale;
+            this.isExcluded = problem -> false;
         }
 
         public Builder tempLocation(TempLocation tempLocation) {
@@ -115,6 +121,11 @@ public Builder classLoader(ClassLoader classLoader) {
             return this;
         }
 
+        public Builder exclude(Predicate<Problem> isExcluded) {
+            this.isExcluded = isExcluded;
+            return this;
+        }
+
         public Linter build() {
             TempLocation tempLocation = this.tempLocation;
 
@@ -128,7 +139,8 @@ public Linter build() {
                 this.threads,
                 this.disableDynamicAnalysis,
                 this.classLoader,
-                this.maxProblemsPerCheck
+                this.maxProblemsPerCheck,
+                this.isExcluded
             );
         }
     }
@@ -249,10 +261,15 @@ public List<Problem> checkFile(
             unreducedProblems = result
                 .problems()
                 .stream()
-                .filter(p -> problemsToReport.contains(p.getProblemType()))
+                .filter(problem -> problemsToReport.contains(problem.getProblemType()))
                 .toList();
         }
 
+        // filter out excluded problems:
+        unreducedProblems = unreducedProblems.stream()
+            .filter(this.isExcluded.negate())
+            .toList();
+
         return this.mergeProblems(unreducedProblems);
     }
 

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

@@ -3,6 +3,7 @@
 import de.firemage.autograder.core.LocalizedMessage;
 import de.firemage.autograder.core.ProblemType;
 import de.firemage.autograder.core.check.ExecutableCheck;
+import de.firemage.autograder.core.check.general.ForToForEachLoop;
 import de.firemage.autograder.core.dynamic.DynamicAnalysis;
 import de.firemage.autograder.core.integrated.ForLoopRange;
 import de.firemage.autograder.core.integrated.IntegratedCheck;
@@ -578,22 +579,11 @@ private void checkSubList(CtFor ctFor) {
         }
 
         // ensure that the variable is only used to access the list elements via get
-        CtVariable<?> ctListVariable = null;
-        CtTypeReference<?> listType = ctFor.getFactory().Type().get(java.util.List.class).getReference();
-        for (CtElement use : SpoonUtil.findUsesIn(forLoopRange.loopVariable().getDeclaration(), ctFor.getBody())) {
-            if (!(use instanceof CtVariableAccess<?> variableAccess)
-                || !(variableAccess.getParent() instanceof CtInvocation<?> ctInvocation)
-                || !(SpoonUtil.isSignatureEqualTo(ctInvocation.getExecutable(), Object.class, "get", int.class))
-                || !(ctInvocation.getTarget() instanceof CtVariableAccess<?> ctVariableAccess)
-                || !ctVariableAccess.getType().isSubtypeOf(listType)
-                || (ctListVariable != null && !ctListVariable.getReference().equals(ctVariableAccess.getVariable()))) {
-                return;
-            }
-
-            if (ctListVariable == null) {
-                ctListVariable = ctVariableAccess.getVariable().getDeclaration();
-            }
-        }
+        CtVariable<?> ctListVariable = ForToForEachLoop.getForEachLoopVariable(
+            ctFor,
+            forLoopRange,
+            ForToForEachLoop.LOOP_VARIABLE_ACCESS_LIST
+        ).orElse(null);
 
         if (ctListVariable == null) {
             return;
@@ -605,6 +595,13 @@ private void checkSubList(CtFor ctFor) {
             listElementType = ctListVariable.getType().getActualTypeArguments().get(0);
         }
 
+        // check if the loop iterates over the whole list (then it is covered by the foreach loop check)
+        if (SpoonUtil.resolveConstant(forLoopRange.start()) instanceof CtLiteral<Integer> ctLiteral
+            && ctLiteral.getValue() == 0
+            && ForToForEachLoop.findIterable(forLoopRange).isPresent()) {
+            return;
+        }
+
         this.addLocalProblem(
             ctFor,
             new LocalizedMessage(

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

@@ -30,6 +30,7 @@
 @ExecutableCheck(reportedProblems = { ProblemType.USE_FORMAT_STRING })
 public class UseFormatString extends IntegratedCheck {
     private static final int MIN_NUMBER_CONCATENATIONS = 3;
+    private static final int MIN_NUMBER_LITERALS = 2;
 
     private List<CtExpression<?>> getFormatArgs(CtBinaryOperator<?> ctBinaryOperator) {
         List<CtExpression<?>> result = new ArrayList<>();
@@ -73,6 +74,12 @@ private String buildFormattedString(Iterable<? extends CtExpression<?>> ctExpres
         // true if the formatString has a %n placeholder
         boolean hasInlineNewline = false;
         for (CtExpression<?> ctExpression : ctExpressions) {
+            if (SpoonUtil.resolveConstant(ctExpression) instanceof CtLiteral<?> literal
+                && literal.getValue() != null
+                && SpoonUtil.isTypeEqualTo(literal.getType(), java.lang.String.class)) {
+                ctExpression = literal;
+            }
+
             if (ctExpression instanceof CtLiteral<?> ctLiteral) {
                 CtTypeInformation ctTypeInformation = ctLiteral.getType();
                 if (ctLiteral.getValue() instanceof String value) {
@@ -175,7 +182,14 @@ private void checkCtBinaryOperator(CtBinaryOperator<?> ctBinaryOperator) {
             return;
         }
 
-        this.checkArgs(ctBinaryOperator, this.getFormatArgs(ctBinaryOperator), suggestion -> suggestion);
+        List<CtExpression<?>> formatArgs = this.getFormatArgs(ctBinaryOperator);
+
+        int numberOfLiterals = (int) formatArgs.stream().filter(ctExpression -> SpoonUtil.resolveConstant(ctExpression) instanceof CtLiteral<?> literal && literal.getValue() != null).count();
+        if (numberOfLiterals < MIN_NUMBER_LITERALS) {
+            return;
+        }
+
+        this.checkArgs(ctBinaryOperator, formatArgs, suggestion -> suggestion);
     }
 
     private void checkCtInvocation(CtInvocation<?> ctInvocation) {

autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RedundantIfForBooleanCheck.java

@@ -61,7 +61,7 @@ private void checkIfElse(CtExpression<?> condition, CtStatement thenStmt, CtStat
         Boolean elseLiteral = SpoonUtil.tryGetBooleanLiteral(elseValue).orElse(null);
 
         // skip non-sense like if (a) return true else return true
-        if (thenLiteral != null && elseLiteral != null && thenLiteral == elseLiteral) {
+        if (thenLiteral != null && thenLiteral.equals(elseLiteral)) {
             return;
         }
 

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

@@ -8,7 +8,10 @@
 import de.firemage.autograder.core.integrated.SpoonUtil;
 import de.firemage.autograder.core.integrated.StaticAnalysis;
 import spoon.processing.AbstractProcessor;
+import spoon.reflect.code.CtLocalVariable;
+import spoon.reflect.code.CtVariableRead;
 import spoon.reflect.declaration.CtConstructor;
+import spoon.reflect.declaration.CtElement;
 import spoon.reflect.declaration.CtMethod;
 import spoon.reflect.declaration.CtType;
 import spoon.reflect.declaration.CtTypeInformation;
@@ -22,7 +25,7 @@
 import java.util.Map;
 
 
-@ExecutableCheck(reportedProblems = { ProblemType.AVOID_SHADOWING })
+@ExecutableCheck(reportedProblems = {ProblemType.AVOID_SHADOWING})
 public class AvoidShadowing extends IntegratedCheck {
     private static final List<String> ALLOWED_FIELDS = List.of("serialVersionUID");
 
@@ -46,6 +49,18 @@ private static Collection<CtFieldReference<?>> getAllVisibleFields(CtTypeInforma
         return result;
     }
 
+    /**
+     * Searches for a variable read of the given variable in the given element.
+     *
+     * @param ctVariable the variable that should be read
+     * @param in the element to search in
+     * @return true if a variable read was found, false otherwise
+     * @param <T> the type of the variable
+     */
+    private static <T> boolean hasVariableReadIn(CtVariable<T> ctVariable, CtElement in) {
+        return SpoonUtil.findUsesIn(ctVariable, in).stream().anyMatch(ctElement -> ctElement instanceof CtVariableRead<?>);
+    }
+
     @Override
     protected void check(StaticAnalysis staticAnalysis, DynamicAnalysis dynamicAnalysis) {
         staticAnalysis.processWith(new AbstractProcessor<CtVariable<?>>() {
@@ -68,26 +83,36 @@ public void process(CtVariable<?> ctVariable) {
                 }
 
                 CtType<?> parent = ctVariable.getParent(CtType.class);
-
-                if (parent == null) {
+                if (parent == null || ctVariable.getReference() == null) {
                     return;
                 }
 
                 Collection<CtFieldReference<?>> visibleFields = getAllVisibleFields(parent);
 
-                for (CtFieldReference<?> ctFieldReference : visibleFields) {
-                    if (ALLOWED_FIELDS.contains(ctFieldReference.getSimpleName()) || ctVariable.getReference() == null) {
-                        continue;
-                    }
-
-                    if (ctFieldReference.getSimpleName().equals(ctVariable.getSimpleName())
-                        && !ctFieldReference.equals(ctVariable.getReference())) {
-                        addLocalProblem(
-                            ctVariable,
-                            new LocalizedMessage("avoid-shadowing", Map.of("name", ctVariable.getSimpleName())),
-                            ProblemType.AVOID_SHADOWING
-                        );
-                    }
+                List<CtFieldReference<?>> hiddenFields = visibleFields.stream()
+                    .filter(ctFieldReference -> !ALLOWED_FIELDS.contains(ctFieldReference.getSimpleName()))
+                    .filter(ctFieldReference -> ctFieldReference.getSimpleName().equals(ctVariable.getSimpleName())
+                        && !ctFieldReference.equals(ctVariable.getReference()))
+                    .toList();
+
+                // if there are no fields hidden by the variable, skip them
+                if (hiddenFields.isEmpty()) {
+                    return;
+                }
+
+                CtElement variableParent = ctVariable.getParent();
+
+                // there might be multiple fields hidden by the variable (e.g. subclass hides superclass field)
+                boolean isFieldRead = hiddenFields.stream().anyMatch(ctFieldReference -> hasVariableReadIn(ctFieldReference.getFieldDeclaration(), variableParent));
+
+                // to reduce the number of annotations, we only report a problem if the variable AND the hidden field are read in
+                // the same context
+                if (hasVariableReadIn(ctVariable, variableParent) && isFieldRead) {
+                    addLocalProblem(
+                        ctVariable,
+                        new LocalizedMessage("avoid-shadowing", Map.of("name", ctVariable.getSimpleName())),
+                        ProblemType.AVOID_SHADOWING
+                    );
                 }
             }
         });