diff --git a/autograder-cmd/src/main/java/de/firemage/autograder/cmd/Application.java b/autograder-cmd/src/main/java/de/firemage/autograder/cmd/Application.java index 8a4f2db9..dc5df00c 100644 --- a/autograder-cmd/src/main/java/de/firemage/autograder/cmd/Application.java +++ b/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 { private CommandSpec spec; private final TempLocation tempLocation; + private final Set 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 getChecks() throws IOException { + List 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 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 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 statusConsumer = status -> diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/CodePosition.java b/autograder-core/src/main/java/de/firemage/autograder/core/CodePosition.java index c2235cef..225f3410 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/CodePosition.java +++ b/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, diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/Linter.java b/autograder-core/src/main/java/de/firemage/autograder/core/Linter.java index 8c532416..2e8e6daf 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/Linter.java +++ b/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,6 +45,7 @@ public final class Linter { private final boolean disableDynamicAnalysis; private final ClassLoader classLoader; private final int maxProblemsPerCheck; + private final Predicate isExcluded; private Linter( Locale locale, @@ -51,7 +53,8 @@ private Linter( int threads, boolean disableDynamicAnalysis, ClassLoader classLoader, - int maxProblemsPerCheck + int maxProblemsPerCheck, + Predicate 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 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 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 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); } diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/CommonReimplementation.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/CommonReimplementation.java index bd610e63..55c005ce 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/CommonReimplementation.java +++ b/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 ctLiteral + && ctLiteral.getValue() == 0 + && ForToForEachLoop.findIterable(forLoopRange).isPresent()) { + return; + } + this.addLocalProblem( ctFor, new LocalizedMessage( diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseFormatString.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseFormatString.java index 4c1a6642..3ec4dd56 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseFormatString.java +++ b/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> getFormatArgs(CtBinaryOperator ctBinaryOperator) { List> result = new ArrayList<>(); @@ -73,6 +74,12 @@ private String buildFormattedString(Iterable> 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> 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) { diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RedundantIfForBooleanCheck.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RedundantIfForBooleanCheck.java index c5e034a2..a275ffaf 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RedundantIfForBooleanCheck.java +++ b/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; } diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/general/AvoidShadowing.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/general/AvoidShadowing.java index 8bd850fe..c458f6bf 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/general/AvoidShadowing.java +++ b/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 ALLOWED_FIELDS = List.of("serialVersionUID"); @@ -46,6 +49,18 @@ private static Collection> 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 the type of the variable + */ + private static boolean hasVariableReadIn(CtVariable 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>() { @@ -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> 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> 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 + ); } } }); diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/general/ForToForEachCheck.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/general/ForToForEachCheck.java deleted file mode 100644 index 85fae761..00000000 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/general/ForToForEachCheck.java +++ /dev/null @@ -1,15 +0,0 @@ -package de.firemage.autograder.core.check.general; - -import de.firemage.autograder.core.LocalizedMessage; -import de.firemage.autograder.core.ProblemType; -import de.firemage.autograder.core.check.ExecutableCheck; -import de.firemage.autograder.core.pmd.PMDCheck; -import net.sourceforge.pmd.lang.java.rule.bestpractices.ForLoopCanBeForeachRule; - -@ExecutableCheck(reportedProblems = {ProblemType.FOR_CAN_BE_FOREACH}) -public class ForToForEachCheck extends PMDCheck { - public ForToForEachCheck() { - super(new LocalizedMessage("for-foreach"), - new ForLoopCanBeForeachRule(), ProblemType.FOR_CAN_BE_FOREACH); - } -} diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/general/ForToForEachLoop.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/general/ForToForEachLoop.java new file mode 100644 index 00000000..634a1a39 --- /dev/null +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/general/ForToForEachLoop.java @@ -0,0 +1,228 @@ +package de.firemage.autograder.core.check.general; + +import de.firemage.autograder.core.LocalizedMessage; +import de.firemage.autograder.core.ProblemType; +import de.firemage.autograder.core.check.ExecutableCheck; +import de.firemage.autograder.core.dynamic.DynamicAnalysis; +import de.firemage.autograder.core.integrated.ForLoopRange; +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.CtArrayRead; +import spoon.reflect.code.CtBodyHolder; +import spoon.reflect.code.CtExpression; +import spoon.reflect.code.CtFieldRead; +import spoon.reflect.code.CtFor; +import spoon.reflect.code.CtInvocation; +import spoon.reflect.code.CtLiteral; +import spoon.reflect.code.CtVariableAccess; +import spoon.reflect.code.CtVariableRead; +import spoon.reflect.declaration.CtElement; +import spoon.reflect.declaration.CtVariable; +import spoon.reflect.reference.CtArrayTypeReference; +import spoon.reflect.reference.CtTypeReference; +import spoon.reflect.reference.CtVariableReference; + +import java.util.Map; +import java.util.Optional; +import java.util.function.Function; + +@ExecutableCheck(reportedProblems = {ProblemType.FOR_CAN_BE_FOREACH}) +public class ForToForEachLoop extends IntegratedCheck { + private static final Function, Optional>> LOOP_VARIABLE_ACCESS_STRING = ctVariableAccess -> { + if (ctVariableAccess.getParent() instanceof CtInvocation ctInvocation + && SpoonUtil.isSignatureEqualTo(ctInvocation.getExecutable(), char.class, "charAt", int.class) + && ctInvocation.getTarget() instanceof CtVariableAccess variableAccess) { + return Optional.of(variableAccess); + } + + return Optional.empty(); + }; + + private static final Function, Optional>> LOOP_VARIABLE_ACCESS_ARRAY = ctVariableAccess -> { + if (ctVariableAccess.getParent() instanceof CtArrayRead arrayAccess + && arrayAccess.getTarget() instanceof CtVariableAccess variableAccess) { + return Optional.of(variableAccess); + } + + return Optional.empty(); + }; + + public static final Function, Optional>> LOOP_VARIABLE_ACCESS_LIST = ctVariableAccess -> { + if (ctVariableAccess.getParent() instanceof CtInvocation ctInvocation + // && SpoonUtil.isSignatureEqualTo(ctInvocation.getExecutable(), Object.class, "get", int.class) + && ctInvocation.getExecutable().getSimpleName().equals("get") + && ctInvocation.getTarget() instanceof CtVariableAccess variableAccess) { + return Optional.of(variableAccess); + } + + return Optional.empty(); + }; + + public static Optional> getForEachLoopVariable( + CtBodyHolder ctBodyHolder, + ForLoopRange forLoopRange, + Function, Optional>> getPotentialLoopVariableAccess + ) { + CtVariable loopVariable = forLoopRange.loopVariable().getDeclaration(); + + // if a ForLoopRange exists, it is guranteed that the variable is incremented by 1 for each step + + CtVariable ctVariable = null; + CtVariableAccess expectedAccess = null; + for (CtElement use : SpoonUtil.findUsesIn(loopVariable, ctBodyHolder.getBody())) { + if (!(use instanceof CtVariableAccess ctVariableAccess)) { + throw new IllegalStateException( + "SpoonUtil.findUsesIn returned non-variable access for '%s' as input".formatted(loopVariable)); + } + + // We need to get the variable on which the access is performed (e.g. in a[i] we need to get a) + CtVariableAccess potentialLoopVariableAccess = getPotentialLoopVariableAccess.apply(ctVariableAccess) + .orElse(null); + + if (!(potentialLoopVariableAccess instanceof CtVariableRead)) { + return Optional.empty(); + } + + if (expectedAccess == null) { + expectedAccess = potentialLoopVariableAccess; + } + + if (!expectedAccess.equals(potentialLoopVariableAccess)) { + return Optional.empty(); + } + + CtVariableReference potentialVariable = potentialLoopVariableAccess.getVariable(); + + if (potentialVariable.getDeclaration() == null) { + return Optional.empty(); + } + + if (ctVariable == null) { + ctVariable = potentialVariable.getDeclaration(); + } + + // check if the variable is the same for all accesses, otherwise it cannot be used in a for-each loop + if (!ctVariable.equals(potentialVariable.getDeclaration())) { + return Optional.empty(); + } + } + + return Optional.ofNullable(ctVariable); + } + + // The condition of a for loop will look like + // - i < array.length + // - i < collection.size() + // - i < string.length() + // + // based on the above, we can find the variable that should be iterated over + public static Optional> findIterable(ForLoopRange forLoopRange) { + CtExpression end = forLoopRange.end(); + + // check if the end condition is array.length + if (end instanceof CtFieldRead ctFieldRead + && ctFieldRead.getVariable().getSimpleName().equals("length") + && ctFieldRead.getTarget() instanceof CtVariableAccess target + && target.getType().isArray()) { + return Optional.ofNullable(target.getVariable().getDeclaration()); + } + + // check if the end condition is collection.size() + if (end instanceof CtInvocation ctInvocation + && SpoonUtil.isSignatureEqualTo(ctInvocation.getExecutable(), int.class, "size") + && ctInvocation.getTarget() instanceof CtVariableAccess target + && SpoonUtil.isSubtypeOf(target.getType(), java.util.Collection.class)) { + return Optional.ofNullable(target.getVariable().getDeclaration()); + } + + // check if the end condition is string.length() + if (end instanceof CtInvocation ctInvocation + && SpoonUtil.isSignatureEqualTo(ctInvocation.getExecutable(), int.class, "length") + && ctInvocation.getTarget() instanceof CtVariableAccess target + && SpoonUtil.isTypeEqualTo(target.getType(), java.lang.String.class)) { + return Optional.ofNullable(target.getVariable().getDeclaration()); + } + + + return Optional.empty(); + } + + @Override + protected void check(StaticAnalysis staticAnalysis, DynamicAnalysis dynamicAnalysis) { + staticAnalysis.processWith(new AbstractProcessor() { + @Override + public void process(CtFor ctFor) { + if (ctFor.isImplicit() || !ctFor.getPosition().isValidPosition()) { + return; + } + + ForLoopRange forLoopRange = ForLoopRange.fromCtFor(ctFor).orElse(null); + if (forLoopRange == null) { + return; + } + + // for loop must start at index 0 + if (!(forLoopRange.start() instanceof CtLiteral ctLiteral) || ctLiteral.getValue() != 0) { + return; + } + + CtVariable iterable = findIterable(forLoopRange).orElse(null); + if (iterable == null) { + return; + } + + CtTypeReference elementType = ctFor.getFactory().createCtTypeReference(java.lang.Object.class); + String iterableExpression = iterable.getSimpleName(); + + Function, Optional>> getPotentialLoopVariableAccess; + if (SpoonUtil.isString(iterable.getType())) { + getPotentialLoopVariableAccess = LOOP_VARIABLE_ACCESS_STRING; + + iterableExpression = "%s.toCharArray()".formatted(iterableExpression); + elementType = ctFor.getFactory().createCtTypeReference(char.class); + } else if (SpoonUtil.isSubtypeOf(iterable.getType(), java.util.List.class)) { + getPotentialLoopVariableAccess = LOOP_VARIABLE_ACCESS_LIST; + + // size != 1, if the list is a raw type: List list = new ArrayList(); + if (iterable.getType().getActualTypeArguments().size() == 1) { + elementType = iterable.getType().getActualTypeArguments().get(0).unbox(); + } + } else if (iterable.getType() instanceof CtArrayTypeReference arrayTypeReference) { + getPotentialLoopVariableAccess = LOOP_VARIABLE_ACCESS_ARRAY; + + elementType = arrayTypeReference.getComponentType(); + } else { + // unknown iterable type + return; + } + + CtVariable ctLoopVariable = getForEachLoopVariable( + ctFor, + forLoopRange, + getPotentialLoopVariableAccess + ).orElse(null); + + if (!iterable.equals(ctLoopVariable)) { + return; + } + + + addLocalProblem( + ctFor, + new LocalizedMessage( + "common-reimplementation", + Map.of( + "suggestion", "for (%s value : %s) { ... }".formatted( + elementType.prettyprint(), + iterableExpression + ) + ) + ), + ProblemType.FOR_CAN_BE_FOREACH + ); + } + }); + } +} diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/ForLoopRange.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/ForLoopRange.java index 7c1129c7..5996ba89 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/ForLoopRange.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/ForLoopRange.java @@ -5,10 +5,12 @@ import spoon.reflect.code.CtExpression; import spoon.reflect.code.CtFor; import spoon.reflect.code.CtLocalVariable; +import spoon.reflect.code.CtStatementList; import spoon.reflect.code.CtUnaryOperator; import spoon.reflect.code.CtVariableAccess; import spoon.reflect.code.CtVariableWrite; import spoon.reflect.code.UnaryOperatorKind; +import spoon.reflect.declaration.CtVariable; import spoon.reflect.reference.CtLocalVariableReference; import java.util.Optional; @@ -21,14 +23,38 @@ public record ForLoopRange( @SuppressWarnings("unchecked") public static Optional fromCtFor(CtFor ctFor) { - // ensure that the loop has exactly one variable initialized with a literal value - if (ctFor.getForInit().size() != 1 - || !(ctFor.getForInit().get(0) instanceof CtLocalVariable ctLocalVariable) - || !SpoonUtil.isTypeEqualTo(ctLocalVariable.getType(), int.class, Integer.class) - || ctLocalVariable.getDefaultExpression() == null) { + CtLocalVariable potentialLoopVariable = null; + + // check if the loop variable is not declared in the for loop + if (ctFor.getForInit().isEmpty() + // then look at the variable used in the break condition + && ctFor.getExpression() instanceof CtBinaryOperator ctBinaryOperator + && ctBinaryOperator.getLeftHandOperand() instanceof CtVariableAccess ctVariableAccess + && ctVariableAccess.getVariable() != null + // check that this variable is a local variable + && ctVariableAccess.getVariable().getDeclaration() instanceof CtLocalVariable localVariable + // which is declared before the loop + && SpoonUtil.getPreviousStatement(ctFor) + .map(statement -> statement instanceof CtVariable ctVariable + && ctVariable.getReference().equals(ctVariableAccess.getVariable())) + .orElse(false) + // the loop variable must not be used after the loop + && SpoonUtil.getNextStatements(ctFor).stream().allMatch(statement -> SpoonUtil.findUsesIn(localVariable, statement).isEmpty()) + ) { + potentialLoopVariable = localVariable; + } else if (ctFor.getForInit().size() == 1 && ctFor.getForInit().get(0) instanceof CtLocalVariable ctLocalVariable) { + potentialLoopVariable = ctLocalVariable; + } + + if (potentialLoopVariable == null + || !SpoonUtil.isTypeEqualTo(potentialLoopVariable.getType(), int.class, Integer.class) + || potentialLoopVariable.getDefaultExpression() == null) { return Optional.empty(); } + CtLocalVariable ctLocalVariable = (CtLocalVariable) potentialLoopVariable; + + // ensure that the loop has exactly one variable initialized with a literal value CtExpression start = SpoonUtil.resolveCtExpression(ctLocalVariable.getDefaultExpression()); // ensure that it is initialized with some integer diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/SpoonUtil.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/SpoonUtil.java index 6dda541a..aa75551a 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/SpoonUtil.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/SpoonUtil.java @@ -1161,6 +1161,20 @@ public static Optional getPreviousStatement(CtStatement ctStatement return Optional.empty(); } + public static List getNextStatements(CtStatement ctStatement) { + List result = new ArrayList<>(); + if (ctStatement.getParent() instanceof CtStatementList ctStatementList) { + List statements = ctStatementList.getStatements(); + int index = statements.indexOf(ctStatement); + + if (index > 0) { + result.addAll(statements.subList(index + 1, statements.size())); + } + } + + return result; + } + public static Optional tryMakeEffect(CtStatement ctStatement) { return TerminalStatement.of(ctStatement).or(() -> AssignmentStatement.of(ctStatement)); } diff --git a/autograder-core/src/main/resources/strings.de.ftl b/autograder-core/src/main/resources/strings.de.ftl index 4e2fb9d8..41cfca97 100644 --- a/autograder-core/src/main/resources/strings.de.ftl +++ b/autograder-core/src/main/resources/strings.de.ftl @@ -151,8 +151,6 @@ double-brace-init = Die obskure 'Double Brace'-Syntax sollte vermieden werden field-local-exp = Das Attribut '{$field}' der Klasse {$class} sollte eine lokale Variable sein, da sie in jeder Methode vor dem ersten Lesen überschrieben wird -for-foreach = for-Schleife sollte eine for-each-Schleife sein - missing-override = '{$name}' sollte eine '@Override'-Annotation haben, siehe https://stackoverflow.com/a/94411/7766117. system-specific-linebreak = Systemabhängiger Zeilenumbruch (\n) benutzt. Besser ist System.lineSeparator() oder (falls es sich um einen format-String handelt) '%n'. diff --git a/autograder-core/src/main/resources/strings.en.ftl b/autograder-core/src/main/resources/strings.en.ftl index 22a29f7f..ad3a4d91 100644 --- a/autograder-core/src/main/resources/strings.en.ftl +++ b/autograder-core/src/main/resources/strings.en.ftl @@ -153,8 +153,6 @@ double-brace-init = Don't use the obscure 'double brace initialization' syntax field-local-exp = Field '{$field}' of class {$class} should be converted to a local variable as every method overwrites it before reading it -for-foreach = for-loop should be a for-each-loop - missing-override = '{$name}' should have an '@Override'-annotation, see https://stackoverflow.com/a/94411/7766117. system-specific-linebreak = Always use system-independent line breaks such as the value obtained from System.lineSeparator() or %n in format strings diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestUseFormatString.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestUseFormatString.java index b2fd38f3..6a51ce94 100644 --- a/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestUseFormatString.java +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestUseFormatString.java @@ -34,7 +34,7 @@ void assertUseFormatString(String expected, Problem problem) { } @Test - void testSimpleArrayCopy() throws LinterException, IOException { + void testMotivatingExample() throws LinterException, IOException { ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( JavaVersion.JAVA_17, "Test", @@ -80,4 +80,50 @@ public static void main(String[] args) { assertUseFormatString("stringBuilder.append(\"[%s]\".formatted(args[0]))", problems.next()); problems.assertExhausted(); } + + @Test + void testMinimumStringConstantLiterals() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Field", + """ + public class Field { + private static final String OPEN_BRACKET = "["; + private static final String CLOSE_BRACKET = "]"; + + private int number; + + @Override + public String toString() { + return OPEN_BRACKET + number + CLOSE_BRACKET; + } + } + """ + ), PROBLEM_TYPES); + + assertUseFormatString("\"[%d]\".formatted(number)", problems.next()); + problems.assertExhausted(); + } + + @Test + void testTooFewStringLiterals() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Field", + """ + public class Field { + private String left; + private int number; + private String right; + + @Override + public String toString() { + return this.left + number + this.right + "."; + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } } diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/general/TestAvoidShadowing.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/general/TestAvoidShadowing.java new file mode 100644 index 00000000..258ef10c --- /dev/null +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/general/TestAvoidShadowing.java @@ -0,0 +1,404 @@ +package de.firemage.autograder.core.check.general; + +import de.firemage.autograder.core.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.compiler.JavaVersion; +import de.firemage.autograder.core.file.StringSourceInfo; +import org.junit.jupiter.api.Test; + +import java.io.IOException; +import java.util.List; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +class TestAvoidShadowing extends AbstractCheckTest { + private static final String LOCALIZED_MESSAGE_KEY = "avoid-shadowing"; + private static final List PROBLEM_TYPES = List.of(ProblemType.AVOID_SHADOWING); + + private void assertEqualsHidden(String name, Problem problem) { + assertEquals( + this.linter.translateMessage( + new LocalizedMessage( + LOCALIZED_MESSAGE_KEY, + Map.of("name", name) + )), + this.linter.translateMessage(problem.getExplanation()) + ); + } + + @Test + void testHiddenUnusedParentField() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceStrings( + JavaVersion.JAVA_17, + Map.ofEntries( + Map.entry( + "Parent", + """ + public class Parent { + protected int number; + + @Override + public String toString() { + return "Parent(%d)".formatted(this.number); + } + } + """ + ), + Map.entry( + "Main", + """ + public class Main extends Parent { + int number; /*# ok, because super.number is not used #*/ + + public Main() { + super(); + this.number = 5; + } + + public void doSomething() { + System.out.println(this.number); + } + + public static void main(String[] args) { + Main main = new Main(); + main.doSomething(); + } + } + """ + ) + ) + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testHiddenParentField() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceStrings( + JavaVersion.JAVA_17, + Map.ofEntries( + Map.entry( + "Parent", + """ + public class Parent { + protected int number; + + @Override + public String toString() { + return "Parent(%d)".formatted(this.number); + } + } + """ + ), + Map.entry( + "Main", + """ + public class Main extends Parent { + int number; /*# ok, because super.number is not used #*/ + + public Main() { + super(); + this.number = 5; + } + + public void doSomething() { + // both fields are used: + System.out.println(this.number); + System.out.println(super.number); + } + + public static void main(String[] args) { + Main main = new Main(); + main.doSomething(); + } + } + """ + ) + ) + ), PROBLEM_TYPES); + + assertEqualsHidden("number", problems.next()); + + problems.assertExhausted(); + } + + @Test + void testLocalVariableHidesField() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceStrings( + JavaVersion.JAVA_17, + Map.ofEntries( + Map.entry( + "Main", + """ + public class Main { + int number; + + public Main() { + super(); + this.number = 5; + } + + public void doSomething() { + int number = 6; + + System.out.println(this.number); + System.out.println(number); + } + + + public static void main(String[] args) { + Main main = new Main(); + main.doSomething(); + } + } + """ + ) + ) + ), PROBLEM_TYPES); + + assertEqualsHidden("number", problems.next()); + + problems.assertExhausted(); + } + + @Test + void testLocalVariableHidesFields() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceStrings( + JavaVersion.JAVA_17, + Map.ofEntries( + Map.entry( + "Parent", + """ + public class Parent { + protected int number; + + @Override + public String toString() { + return "Parent(%d)".formatted(this.number); + } + } + """ + ), + Map.entry( + "Main", + """ + public class Main extends Parent { + int number; /*# ok, because super.number is not used #*/ + + public Main() { + super(); + this.number = 5; + } + + public void doSomething() { + int number = 6; + + System.out.println(this.number); + System.out.println(super.number); + System.out.println(number); + } + + public static void main(String[] args) { + Main main = new Main(); + main.doSomething(); + } + } + """ + ) + ) + ), PROBLEM_TYPES); + + assertEqualsHidden("number", problems.next()); + assertEqualsHidden("number", problems.next()); + + problems.assertExhausted(); + } + + @Test + void testStatic() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceStrings( + JavaVersion.JAVA_17, + Map.ofEntries( + Map.entry( + "Parent", + """ + public class Parent { + protected int number; + + @Override + public String toString() { + return "Parent(%d)".formatted(this.number); + } + } + """ + ), + Map.entry( + "Main", + """ + public class Main extends Parent { + static int number; + + public void doSomething() { + int number = 6; + + System.out.println(Main.number); + System.out.println(super.number); + System.out.println(number); + } + + public static void main(String[] args) { + Main main = new Main(); + main.doSomething(); + } + } + """ + ) + ) + ), PROBLEM_TYPES); + + assertEqualsHidden("number", problems.next()); + assertEqualsHidden("number", problems.next()); + + problems.assertExhausted(); + } + + @Test + void testInheritance() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceStrings( + JavaVersion.JAVA_17, + Map.ofEntries( + Map.entry( + "A", + """ + class A { + protected int a; + int x; + static int y; + private int z; + } + """ + ), + Map.entry( + "B", + """ + class B extends A { + private int b; + private final int c; + + public B(int b, int c) { + this.b = b; + this.c = c; + + System.out.println(b); + System.out.println(this.b); + System.out.println(c); + System.out.println(this.c); + } + + public void setB(int b) { + this.b = b; + } + + private void foo2(int b) {} + private void foo() { + int a = 3; /*# not ok #*/ + int x = 4; /*# not ok #*/ + int y = 5; /*# not ok #*/ + final int z = 5; /*# ok #*/ + + System.out.println("" + a + this.a); + System.out.println("" + x + this.x); + System.out.println("" + y + A.y); + System.out.println("" + z); + } + } + """ + ), + Map.entry( + "C", + """ + class C extends A { + protected int a; /*# not ok #*/ + int x; /*# not ok #*/ + static int y; /*# not ok #*/ + private int z; /*# ok #*/ + + void doSomething() { + System.out.println("" + super.a + this.a); + System.out.println("" + super.x + this.x); + System.out.println("" + A.y + C.y); + System.out.println("" + z); + } + } + """ + ) + ) + ), PROBLEM_TYPES); + + assertEqualsHidden("a", problems.next()); + assertEqualsHidden("x", problems.next()); + assertEqualsHidden("y", problems.next()); + + assertEqualsHidden("a", problems.next()); + assertEqualsHidden("x", problems.next()); + assertEqualsHidden("y", problems.next()); + + problems.assertExhausted(); + } + + @Test + void testException() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceStrings( + JavaVersion.JAVA_17, + Map.ofEntries( + Map.entry( + "SomeException", + """ + class SomeException extends IllegalArgumentException { + @java.io.Serial + private static final long serialVersionUID = -4491591333105161142L; /*# ok #*/ + + public SomeException(String message) { + super(message + serialVersionUID); + } + } + """ + ) + ) + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testInStaticContext() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceStrings( + JavaVersion.JAVA_17, + Map.ofEntries( + Map.entry( + "Main", + """ + class Main { + private String string; + + public static void doSomething(String string) { + System.out.println(string); + } + + @Override + public String toString() { + return this.string; + } + } + """ + ) + ) + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } +} diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/general/TestForToForEachLoop.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/general/TestForToForEachLoop.java new file mode 100644 index 00000000..f5483312 --- /dev/null +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/general/TestForToForEachLoop.java @@ -0,0 +1,432 @@ +package de.firemage.autograder.core.check.general; + +import de.firemage.autograder.core.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.compiler.JavaVersion; +import de.firemage.autograder.core.file.StringSourceInfo; +import org.junit.jupiter.api.Test; + +import java.io.IOException; +import java.util.List; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +class TestForToForEachLoop extends AbstractCheckTest { + private static final List PROBLEM_TYPES = List.of(ProblemType.FOR_CAN_BE_FOREACH); + + void assertEqualsForEach(Problem problem, String type, String iterable) { + assertEquals( + this.linter.translateMessage(new LocalizedMessage( + "common-reimplementation", + Map.of("suggestion", "for (%s value : %s) { ... }".formatted(type, iterable)) + )), + this.linter.translateMessage(problem.getExplanation()) + ); + } + + @Test + void testArray() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + class Test { + public static void main(String[] args) { + //# not ok + for (int i = 0; i < args.length; i++) { + System.out.println(args[i]); + } + + //# ok + for (int i = 0; i < args.length; i++) { + args[i] = "Hello World"; + + System.out.println(args[i]); + } + + int[][] doubleArray = new int[10][10]; + + //# not ok + for (int i = 0; i < doubleArray.length; i++) { + System.out.println(doubleArray[i]); + } + + //# ok + for (int i = 0; i < doubleArray.length; i++) { + doubleArray[i][i] = 1; + + System.out.println(doubleArray[i]); + } + } + } + """ + ), PROBLEM_TYPES); + + assertEqualsForEach(problems.next(), "String", "args"); + assertEqualsForEach(problems.next(), "int[]", "doubleArray"); + + problems.assertExhausted(); + } + + @Test + void testList() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + import java.util.List; + import java.util.ArrayList; + + class Test { + public static void main(String[] args) { + List list = List.of(1, 2, 3, 4, 5, 6); + + for (int i = 0; i < list.size(); i++) { //# not ok + System.out.println(list.get(i)); + } + + for (int i = 0; i < list.size(); i++) { //# not ok + System.out.println(list.get(i)); + System.out.println(list.get(i)); + } + + for (int i = 0; i < list.size(); i += 2) { //# ok + System.out.println(list.get(i)); + } + + for (int i = 0; i < list.size() - 1; i += 2) { //# ok + System.out.println(list.get(i)); + } + + for (int i = 0; i < list.size(); i++) { //# ok + System.out.println(i); + } + + for (int i : list) { //# ok + System.out.println(i); + } + } + + void lessEqualCondition(List lo) { + for (int i = 0; i <= lo.size() - 1; i++) { /*# not ok #*/ + System.out.println(lo.get(i)); + } + } + + void concreteList(ArrayList arrayList) { + for (int i = 0; i < arrayList.size(); i++) { /*# not ok #*/ + System.out.println(arrayList.get(i)); + } + } + + void listWithIndexUse(List withIndexUse) { + for (int i = 0; i < withIndexUse.size(); i++) { /*# ok #*/ + System.out.println(i + ": " + withIndexUse.get(i)); + } + } + + + protected static final char[] filter(char[] chars, char removeChar) { + int count = 0; + for (int i = 0; i < chars.length; i++) { /*# not ok #*/ + if (chars[i] == removeChar) { + count++; + } + } + + char[] results = new char[chars.length - count]; + + int index = 0; + for (int i = 0; i < chars.length; i++) { /*# not ok #*/ + if (chars[i] != removeChar) { + results[index++] = chars[i]; + } + } + return results; + } + + private void fofo(List mList) { + for (int i = 0; i < mList.size(); i++) { /*# ok #*/ + mList.get(i).setIndex(i); + } + } + + interface Foo { + void setIndex(int i); + } + + } + """ + ), PROBLEM_TYPES); + + assertEqualsForEach(problems.next(), "int", "list"); + assertEqualsForEach(problems.next(), "int", "list"); + assertEqualsForEach(problems.next(), "String", "lo"); + assertEqualsForEach(problems.next(), "String", "arrayList"); + assertEqualsForEach(problems.next(), "char", "chars"); + assertEqualsForEach(problems.next(), "char", "chars"); + + problems.assertExhausted(); + } + + @Test + void testReverseAccess() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + import java.util.List; + + class Test { + void loop(List l) { + for (int i = l.size() - 1; i > 0; i-= 1) { /*# ok #*/ + System.out.println(i + ": " + l.get(i)); + } + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testIndexInitOutsideLoop() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + import java.util.List; + + class Test { + void loop(List l) { + int i = 0; + for (; i < l.size(); i++) { /*# not ok #*/ + System.out.println(l.get(i)); + } + } + + void loop2(List usedAfterLoop) { + int i = 0; + for (; i < usedAfterLoop.size(); i++) { /*# ok; i is used after loop #*/ + System.out.println(usedAfterLoop.get(i)); + + if (usedAfterLoop.get(i).equals("foo")) { + break; + } + } + + System.out.println(usedAfterLoop.get(i)); + } + } + """ + ), PROBLEM_TYPES); + + assertEqualsForEach(problems.next(), "String", "l"); + + problems.assertExhausted(); + } + + @Test + void testIndexNotZero() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + import java.util.List; + + class Test { + void loop(List filters, StringBuilder builder) { + for (int i = 1; i < filters.size(); i++) { /*# ok #*/ + builder.append(' '); + builder.append(filters.get(i)); + } + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testAccessNextElement() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + class Test { + static String findOptionalStringValue(String[] args, String name, String defaultValue) { + for (int i = 0; i < args.length; i++) { /*# ok #*/ + if (args[i].equals(name)) { + return args[i + 1]; + } + } + return defaultValue; + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testOnlyArrayWrite() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + class Test { + private String newString() { + int strLength = randomInt(1, 100); + + char[] chars = new char[strLength]; + for (int i = 0; i < chars.length; i++) { /*# ok #*/ + chars[i] = randomCharIn("123"); + } + return new String(chars); + } + + private int randomInt(int min, int max) { + return 42; + } + + private char randomCharIn(String s) { + return '1'; + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testTransferListIndexToArrayIndex() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + import java.util.List; + import java.util.ArrayList; + + class Test { + private int[] hashes = new int[10]; + + public void foo() { + List stringList = new ArrayList<>(); + + this.hashes = new int[stringList.size()]; + for (int i = 0; i < stringList.size(); i++) { /*# ok #*/ + this.hashes[i] = stringList.get(i).hashCode(); + } + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testIndexSameArrayFieldDifferentInstances() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + class Test { + final int hashes[] = new int[6]; + + public boolean equals(Test other) { + for (int i = 0; i < hashes.length; i++) { /*# ok #*/ + if (this.hashes[i] != other.hashes[i]) + return false; + } + + return true; + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testLoopWithDifferentListInCondition() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + import java.util.List; + import java.util.ArrayList; + + class Test { + void loop(List l) { + List l2 = new ArrayList<>(l); + for (int i = 0; i < l.size(); i++) { /*# ok #*/ + System.out.println(l2.get(i)); + } + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testGenerics() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + import java.util.List; + + class Test { + void loop(List list) { + for (int i = 0; i < list.size(); i++) { + System.out.println(list.get(i)); + } + } + } + """ + ), PROBLEM_TYPES); + + assertEqualsForEach(problems.next(), "T", "list"); + + problems.assertExhausted(); + } + + @Test + void testFieldAccess() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Node", + """ + class Node { + Node[] children; + + public Object childrenAccept(Object data) { + if (children != null) { + for (int i = 0; i < children.length; ++i) { /*# not ok #*/ + Node apexNode = (Node) children[i]; + System.out.println(apexNode); + } + } + return data; + } + } + """ + ), PROBLEM_TYPES); + + assertEqualsForEach(problems.next(), "Node", "children"); + + problems.assertExhausted(); + } +} diff --git a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/AvoidShadowing/code/Test.java b/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/AvoidShadowing/code/Test.java deleted file mode 100644 index 9171ef9e..00000000 --- a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/AvoidShadowing/code/Test.java +++ /dev/null @@ -1,69 +0,0 @@ -package de.firemage.autograder.core.check_tests.AvoidShadowing.code; - -public class Test { - int x; - static int y; - private int z; - - public void foo() { - int x = 4; /*# not ok #*/ - int y = 5; /*# not ok #*/ - final int z = 5; /*# not ok #*/ - } -} - -class A { - protected int a; - int x; - static int y; - private int z; -} - -class B extends A { - private final int b; - private final int c; - - public B(int b, int c) { /*# ok #*/ - this.b = b; - this.c = c; - } - - private void foo2(int b) {} /*# not ok #*/ - private void foo() { - int a = 3; /*# not ok #*/ - int x = 4; /*# not ok #*/ - int y = 5; /*# not ok #*/ - final int z = 5; /*# ok #*/ - } -} - -class C extends A { - protected int a; /*# not ok #*/ - int x; /*# not ok #*/ - static int y; /*# not ok #*/ - private int z; /*# ok #*/ -} - -class SomeException extends IllegalArgumentException { - @java.io.Serial - private static final long serialVersionUID = -4491591333105161142L; /*# ok #*/ - public SomeException(String message) { - super(message); - } -} - -class Parent { - void parent(int x) {} -} - -class Child extends Parent { - private int x; - - void parent(int x) {} /*# ok #*/ -} - -class ShadowInStaticContext { - private String string; - - public static void doSomething(String string) {} /*# ok #*/ -} diff --git a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/AvoidShadowing/config.txt b/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/AvoidShadowing/config.txt deleted file mode 100644 index 0939e259..00000000 --- a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/AvoidShadowing/config.txt +++ /dev/null @@ -1,12 +0,0 @@ -general.AvoidShadowing -Don't hide fields with variables. -Test.java:9 -Test.java:10 -Test.java:11 -Test.java:31 -Test.java:33 -Test.java:34 -Test.java:35 -Test.java:41 -Test.java:42 -Test.java:43 diff --git a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/ClosedSetOfValues/config.txt b/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/ClosedSetOfValues/config.txt index 9a43eeab..1cf424be 100644 --- a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/ClosedSetOfValues/config.txt +++ b/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/ClosedSetOfValues/config.txt @@ -5,5 +5,5 @@ Test.java:23-35 Test.java:40 Test.java:41 Test.java:42 -Test.java:71-101 +Test.java:71 Test.java:138-144 diff --git a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/ForToForeach/code/Test.java b/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/ForToForeach/code/Test.java deleted file mode 100644 index f556acc2..00000000 --- a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/ForToForeach/code/Test.java +++ /dev/null @@ -1,203 +0,0 @@ -package de.firemage.autograder.core.check_tests.ForToForeach.code; - -import java.util.*; - -public class Test { - public static void main(String[] args) { - List list = List.of(1, 2, 3, 4, 5, 6); - - for (int i = 0; i < list.size(); i++) { /*# not ok; Should be for-each #*/ - System.out.println(list.get(i)); - } - - for (int i = 0; i < list.size(); i++) { /*# not ok; Should be for-each #*/ - System.out.println(list.get(i)); - System.out.println(list.get(i)); - } - - for (int i = 0; i < list.size(); i += 2) { /*# false positive; see https://github.com/pmd/pmd/issues/4569 #*/ - System.out.println(list.get(i)); - } - - for (int i = 0; i < list.size() - 1; i += 2) { /*# ok #*/ - System.out.println(list.get(i)); - } - - for (int i = 0; i < list.size(); i++) { /*# ok #*/ - System.out.println(i); - } - - for (int i : list) { /*# ok #*/ - System.out.println(i); - } - } -} - -class PmdExample1 { - void loop(List l) { - for (int i = 0; i < l.size(); i++) { /*# not ok #*/ - System.out.println(l.get(i)); - } - } -} - -class PmdExample2 { - void loop(List lo) { - for (int i = 0; i <= lo.size() - 1; i++) { /*# not ok #*/ - System.out.println(lo.get(i)); - } - } -} - -class PmdExample3 { - void loop(ArrayList l) { - for (int i = 0; i < l.size(); i++) { /*# not ok #*/ - System.out.println(l.get(i)); - } - } -} - -class Node { - Node[] children; - - public Object childrenAccept(Object data) { - if (children != null) { - for (int i = 0; i < children.length; ++i) { /*# not ok #*/ - Node apexNode = (Node) children[i]; - System.out.println(apexNode); - } - } - return data; - } -} - - -class PmdExample5 { - protected static final char[] filter(char[] chars, char removeChar) { - int count = 0; - for (int i = 0; i < chars.length; i++) { /*# not ok #*/ - if (chars[i] == removeChar) { - count++; - } - } - - char[] results = new char[chars.length - count]; - - int index = 0; - for (int i = 0; i < chars.length; i++) { /*# not ok #*/ - if (chars[i] != removeChar) { - results[index++] = chars[i]; - } - } - return results; - } -} - -class PmdExample9 { - void loop(List l) { - int i = 0; - for (; i < l.size(); i++) { /*# not ok #*/ - System.out.println(l.get(i)); - } - } -} - -class PmdExample6 { - void loop(List l) { - for (int i = 0; i < l.size(); i++) { /*# ok #*/ - System.out.println(i + ": " + l.get(i)); - } - } -} - -class PmdExample7 { - void loop(List l) { - List l2 = new ArrayList<>(l); - for (int i = 0; i < l.size(); i++) { /*# ok #*/ - System.out.println(l2.get(i)); - } - } -} - -class PmdExample8 { - void loop(List l) { - for (int i = l.size() - 1; i > 0; i-= 1) { /*# ok #*/ - System.out.println(i + ": " + l.get(i)); - } - } -} - -class PmdExample10 { - void loop(List filters, StringBuilder builder) { - for (int i = 1; i < filters.size(); i++) { /*# ok #*/ - builder.append(' '); - builder.append(filters.get(i)); - } - } -} - -class PmdExample11 { - private static String findOptionalStringValue(String[] args, String name, String defaultValue) { - for (int i = 0; i < args.length; i++) { /*# ok #*/ - if (args[i].equals(name)) { - return args[i + 1]; - } - } - return defaultValue; - } -} - -class PmdExample12 { - private String newString() { - int strLength = randomInt(1, 100); - - char[] chars = new char[strLength]; - for (int i = 0; i < chars.length; i++) { /*# ok #*/ - chars[i] = randomCharIn("123"); - } - return new String(chars); - } - - private int randomInt(int min, int max) { - return 42; - } - - private char randomCharIn(String s) { - return '1'; - } -} - -class PmdExample13 { - private int[] hashes = new int[10]; - public void foo() { - List stringList = new ArrayList<>(); - - this.hashes = new int[stringList.size()]; - for (int i = 0; i < stringList.size(); i++) { /*# ok #*/ - this.hashes[i] = stringList.get(i).hashCode(); - } - } -} - -class PmdExample14 { - - final int hashes[] = new int[6]; - - public void foo(PmdExample14 other) { - for (int i = 0; i < hashes.length; i++) { /*# ok #*/ - if (this.hashes[i] == other.hashes[i]) - throw new IllegalStateException(); - } - } -} - -class PmdExample15 { - private void fofo(List mList) { - for (int i = 0; i < mList.size(); i++) { /*# ok #*/ - mList.get(i).setIndex(i); - } - } - interface Foo { - void setIndex(int i); - } -} diff --git a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/ForToForeach/config.txt b/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/ForToForeach/config.txt deleted file mode 100644 index 01a2905d..00000000 --- a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/ForToForeach/config.txt +++ /dev/null @@ -1,13 +0,0 @@ -general.ForToForEachCheck -For loop should be for-each loop -Test.java:9 -Test.java:13 -# this is a false-positive: https://github.com/pmd/pmd/issues/4569 -Test.java:18 -Test.java:38 -Test.java:46 -Test.java:54 -Test.java:65 -Test.java:78 -Test.java:87 -Test.java:99 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 index e2932184..a82a7d41 100644 --- 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 @@ -1,9 +1,9 @@ oop.MethodShouldBeAbstractCheck Method should be abstract instead of providing a dummy implementation -Test.java:32-34 -Test.java:36-38 -Test.java:44-46 -Test.java:48-50 -Test.java:52-54 -Test.java:56-58 -Test.java:60-62 +Test.java:32 +Test.java:36 +Test.java:44 +Test.java:48 +Test.java:52 +Test.java:56 +Test.java:60