diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/ProblemType.java b/autograder-core/src/main/java/de/firemage/autograder/core/ProblemType.java index 4d734e2d..95183623 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/ProblemType.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/ProblemType.java @@ -120,10 +120,9 @@ public enum ProblemType implements AbstractProblemType { IMPLEMENT_COMPARABLE, /** - * Reports magic strings. + * Reports magic literals. */ - @HasFalsePositives - MAGIC_STRING, + MAGIC_LITERAL, /** * Checks if a constant has its value in its name. For example `public static final int TEN = 10;`. @@ -502,6 +501,14 @@ public enum ProblemType implements AbstractProblemType { @HasFalsePositives TRY_CATCH_COMPLEXITY, + /** + * Reports code where the try block contains statements that do not throw an exception and could therefore be moved outside the try block. + *
+ * It might have false-positives, because it is difficult to detect what code throws which exceptions. + */ + @HasFalsePositives + TRY_BLOCK_SIZE, + /** * Reports static blocks in classes. */ diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/comment/UnnecessaryComment.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/comment/UnnecessaryComment.java index 27e74d7c..6c2002ba 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/comment/UnnecessaryComment.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/comment/UnnecessaryComment.java @@ -8,6 +8,7 @@ import de.firemage.autograder.core.integrated.StaticAnalysis; import spoon.processing.AbstractProcessor; import spoon.reflect.code.CtComment; +import spoon.reflect.code.CtJavaDoc; import spoon.reflect.declaration.CtElement; import java.util.ArrayList; @@ -30,7 +31,7 @@ private void checkComments(Collection comments) { } for (CtComment ctComment : comments) { - if (ctComment.getContent().isBlank()) { + if (ctComment.getContent().isBlank() && !(ctComment instanceof CtJavaDoc ctJavaDoc && ctJavaDoc.getJavadocElements().isEmpty())) { addLocalProblem( ctComment, new LocalizedMessage("unnecessary-comment-empty"), diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/TryCatchComplexity.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/TryCatchComplexity.java index 9a8c9e99..e85ebaa1 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/TryCatchComplexity.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/TryCatchComplexity.java @@ -73,6 +73,8 @@ private List filterMethodInvocations(List statements) private List extractMethodInvocations(List statements) { return statements.stream().filter(MethodUtil::isInvocation).toList(); } + + // TODO: this could be combined with the duplicatecode check code private void visitStatement(CtStatement statement, List allStatements) { // The CtStatement may be null if the body of an if statement is empty. for example "if (true);" if (statement == null || allStatements.size() > MAX_ALLOWED_STATEMENTS) { diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/exceptions/TryBlockSize.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/exceptions/TryBlockSize.java new file mode 100644 index 00000000..13648974 --- /dev/null +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/exceptions/TryBlockSize.java @@ -0,0 +1,215 @@ +package de.firemage.autograder.core.check.exceptions; + +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.MethodUtil; +import de.firemage.autograder.core.integrated.StatementUtil; +import de.firemage.autograder.core.integrated.StaticAnalysis; +import de.firemage.autograder.core.integrated.TypeUtil; +import de.firemage.autograder.core.integrated.UsesFinder; +import spoon.processing.AbstractProcessor; +import spoon.reflect.code.CtCatch; +import spoon.reflect.code.CtCatchVariable; +import spoon.reflect.code.CtConstructorCall; +import spoon.reflect.code.CtExecutableReferenceExpression; +import spoon.reflect.code.CtExpression; +import spoon.reflect.code.CtInvocation; +import spoon.reflect.code.CtNewClass; +import spoon.reflect.code.CtStatement; +import spoon.reflect.code.CtThrow; +import spoon.reflect.code.CtTry; +import spoon.reflect.cu.SourcePosition; +import spoon.reflect.declaration.CtElement; +import spoon.reflect.declaration.CtType; +import spoon.reflect.reference.CtExecutableReference; +import spoon.reflect.reference.CtTypeReference; +import spoon.reflect.visitor.CtScanner; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Set; +import java.util.function.Predicate; + +@ExecutableCheck(reportedProblems = { ProblemType.TRY_BLOCK_SIZE }) +public class TryBlockSize extends IntegratedCheck { + private static boolean noneThrow(CtStatement ctStatement, Predicate> isMatch) { + List> thrownExceptions = new ArrayList<>(); + ctStatement.accept(new CtScanner() { + @Override + public void visitCtThrow(CtThrow ctThrow) { + thrownExceptions.add(ctThrow.getThrownExpression().getType()); + super.visitCtThrow(ctThrow); + } + + private void recordExecutableReference(CtExecutableReference ctExecutableReference) { + var executable = MethodUtil.getExecutableDeclaration(ctExecutableReference); + if (executable != null) { + thrownExceptions.addAll(executable.getThrownTypes()); + } + } + + @Override + public void visitCtInvocation(CtInvocation invocation) { + this.recordExecutableReference(invocation.getExecutable()); + super.visitCtInvocation(invocation); + } + + @Override + public void visitCtConstructorCall(CtConstructorCall ctConstructorCall) { + this.recordExecutableReference(ctConstructorCall.getExecutable()); + super.visitCtConstructorCall(ctConstructorCall); + } + + + @Override + public void visitCtNewClass(CtNewClass ctNewClass) { + this.recordExecutableReference(ctNewClass.getExecutable()); + super.visitCtNewClass(ctNewClass); + } + + @Override + public > void visitCtExecutableReferenceExpression(CtExecutableReferenceExpression expression) { + this.recordExecutableReference(expression.getExecutable()); + super.visitCtExecutableReferenceExpression(expression); + } + }); + + return thrownExceptions.stream().noneMatch(isMatch); + } + + private static String formatSourceRange(List ctElements) { + if (ctElements.isEmpty()) { + return null; + } + + SourcePosition position = ctElements.get(0).getPosition(); + String result = "L%d".formatted(position.getLine()); + + if (position.getLine() == position.getEndLine() && ctElements.size() == 1) { + return result; + } + + int endLine = position.getEndLine(); + if (ctElements.size() > 1) { + endLine = ctElements.get(ctElements.size() - 1).getPosition().getEndLine(); + } + + return result + "-%d".formatted(endLine); + } + + @Override + public void check(StaticAnalysis staticAnalysis) { + staticAnalysis.processWith(new AbstractProcessor() { + @Override + public void process(CtTry ctTry) { + if (ctTry.isImplicit() || !ctTry.getPosition().isValidPosition()) { + return; + } + + List statements = StatementUtil.getEffectiveStatements(ctTry.getBody()); + if (statements.isEmpty()) { + return; + } + + // these are all exceptions that are caught by the try-catch block + Set> caughtExceptions = ctTry.getCatchers() + .stream() + .map(CtCatch::getParameter) + .map(CtCatchVariable::getMultiTypes) + .flatMap(List::stream) + // filter out RuntimeExceptions, because they are hard to track via code analysis + .filter(type -> !TypeUtil.isSubtypeOf(type, java.lang.RuntimeException.class)) + .map(CtTypeReference::getTypeDeclaration) + .filter(Objects::nonNull) + .collect(CoreUtil.toIdentitySet()); + + // in case only RuntimeExceptions are caught, ignore the block + if (caughtExceptions.isEmpty()) { + return; + } + + // The noneThrow method will extract thrown types from the given statement and call this predicate with them. + // + // The predicate then checks if any of the thrown types are caught by the try-catch block. + Predicate> isMatch = ctTypeReference -> { + var type = ctTypeReference.getTypeDeclaration(); + + // this can happen, but I don't remember when this happens + if (type == null) { + return false; + } + + // here it checks via the subtype relation, because subtypes are instances of their parent type. + return caughtExceptions.stream().anyMatch(caughtException -> UsesFinder.isSubtypeOf(type, caughtException)); + }; + + // TODO: what about code like this? + // + // try { + // var variable = methodThatThrows(); + // + // // code that does not throw, but uses the variable + // System.out.println(variable); + // } catch (InvalidArgumentException e) { + // // handle exception + // } + // + // Should that code be linted? + // TODO: if it should, document a possible solution for this in the wiki + + // go through each statement and check which do not throw exceptions that are later caught (these are irrelevant) + List irrelevantLeadingStatements = new ArrayList<>(); + CtStatement lastCheckedStatement = null; + for (CtStatement statement : statements) { + lastCheckedStatement = statement; + if (!noneThrow(statement, isMatch)) { + break; + } + + irrelevantLeadingStatements.add(statement); + } + + List irrelevantTrailingStatements = new ArrayList<>(); + for (int i = statements.size() - 1; i >= 0; i--) { + CtStatement statement = statements.get(i); + if (statement == lastCheckedStatement || !noneThrow(statement, isMatch)) { + break; + } + + irrelevantTrailingStatements.add(statement); + } + + Collections.reverse(irrelevantTrailingStatements); + + if (!irrelevantLeadingStatements.isEmpty() || !irrelevantTrailingStatements.isEmpty()) { + String start = formatSourceRange(irrelevantLeadingStatements); + String end = formatSourceRange(irrelevantTrailingStatements); + + String result = start; + if (start == null) { + result = end; + } else if (end != null) { + result = "%s, %s".formatted(start, end); + } + + addLocalProblem( + ctTry, + new LocalizedMessage( + "try-block-size", + Map.of( + "lines", Objects.requireNonNull(result) + ) + ), + ProblemType.TRY_BLOCK_SIZE + ); + } + } + }); + } +} diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/general/MagicLiteral.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/general/MagicLiteral.java new file mode 100644 index 00000000..166e2b3f --- /dev/null +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/general/MagicLiteral.java @@ -0,0 +1,88 @@ +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.integrated.IntegratedCheck; +import de.firemage.autograder.core.integrated.MethodUtil; +import de.firemage.autograder.core.integrated.StaticAnalysis; +import de.firemage.autograder.core.integrated.TypeUtil; +import spoon.reflect.CtModel; +import spoon.reflect.code.CtLiteral; +import spoon.reflect.declaration.CtField; +import spoon.reflect.declaration.CtMethod; +import spoon.reflect.declaration.CtModule; +import spoon.reflect.visitor.CtScanner; + +import java.util.Map; +import java.util.Optional; +import java.util.Set; + +/** + * Checks for magic literals in the code. + * + * @author Tobias Thirolf + * @author Lucas Altenau + */ +@ExecutableCheck(reportedProblems = { ProblemType.MAGIC_LITERAL }) +public class MagicLiteral extends IntegratedCheck { + private static final Set DEFAULT_IGNORED_NUMBERS = Set.of(-1.0, 0.0, 1.0, 2.0); + + private void visitLiteral(String magicType, CtLiteral ctLiteral) { + CtMethod parentMethod = ctLiteral.getParent(CtMethod.class); + // allow magic literals in hashCode methods (some implementations use prime numbers) + if (parentMethod != null && TypeUtil.isTypeEqualTo(parentMethod.getType(), int.class) && parentMethod.getSimpleName().equals("hashCode") + && MethodUtil.isOverriddenMethod(parentMethod)) { + return; + } + + CtField parent = ctLiteral.getParent(CtField.class); + if (parent == null || !parent.isFinal()) { + this.addLocalProblem( + ctLiteral, + new LocalizedMessage( + "magic-literal", + Map.of( + "value", ctLiteral.toString().replace("\n", "\\n").replace("\r", "\\r"), + "type", magicType + ) + ), + ProblemType.MAGIC_LITERAL + ); + } + } + + @Override + protected void check(StaticAnalysis staticAnalysis) { + CtModel submissionModel = staticAnalysis.getModel(); + + for (CtModule module : submissionModel.getAllModules()) { + module.accept(new CtScanner() { + @Override + public void visitCtLiteral(CtLiteral ctLiteral) { + if (ctLiteral.isImplicit() || !ctLiteral.getPosition().isValidPosition() || ctLiteral.getType() == null) { + super.visitCtLiteral(ctLiteral); + return; + } + + if (ctLiteral.getType().isPrimitive()) { + if (ctLiteral.getValue() instanceof Number number && !DEFAULT_IGNORED_NUMBERS.contains(number.doubleValue())) { + visitLiteral("number", ctLiteral); + } else if (ctLiteral.getValue() instanceof Character) { + visitLiteral("char", ctLiteral); + } + } else if (ctLiteral.getValue() instanceof String string && !string.isEmpty()) { + visitLiteral("string", ctLiteral); + } + + super.visitCtLiteral(ctLiteral); + } + }); + } + } + + @Override + public Optional maximumProblems() { + return Optional.of(1); + } +} diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/general/MagicString.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/general/MagicString.java deleted file mode 100644 index 683ac723..00000000 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/general/MagicString.java +++ /dev/null @@ -1,85 +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.integrated.IntegratedCheck; -import de.firemage.autograder.core.integrated.StaticAnalysis; -import de.firemage.autograder.core.integrated.TypeUtil; -import spoon.processing.AbstractProcessor; -import spoon.reflect.code.CtLiteral; -import spoon.reflect.declaration.CtField; -import spoon.reflect.declaration.CtType; -import spoon.reflect.visitor.Filter; -import spoon.reflect.visitor.filter.CompositeFilter; -import spoon.reflect.visitor.filter.FilteringOperator; -import spoon.reflect.visitor.filter.TypeFilter; - -import java.util.Collection; -import java.util.HashSet; -import java.util.List; -import java.util.Map; -import java.util.Optional; - -@ExecutableCheck(reportedProblems = { ProblemType.MAGIC_STRING }) - -public class MagicString extends IntegratedCheck { - private static final Filter> IS_MAGIC_STRING = ctLiteral -> { - // ignore empty values - if (ctLiteral.getValue().isEmpty()) { - return false; - } - - // ignore strings that are in constants: - CtField parent = ctLiteral.getParent(CtField.class); - if (parent != null && parent.isStatic() && parent.isFinal()) { - return false; - } - - return true; - }; - - @Override - protected void check(StaticAnalysis staticAnalysis) { - staticAnalysis.processWith(new AbstractProcessor>() { - - @Override - @SuppressWarnings("unchecked") - public void process(CtType ctType) { - if (ctType.isImplicit() || !ctType.getPosition().isValidPosition()) { - return; - } - - List> magicStrings = ctType.getElements(new CompositeFilter<>( - FilteringOperator.INTERSECTION, - new TypeFilter<>(CtLiteral.class), - element -> element.getType() != null && TypeUtil.isTypeEqualTo(element.getType(), String.class), - IS_MAGIC_STRING - )); - - Collection reportedStrings = new HashSet<>(); - for (CtLiteral magicString : magicStrings) { - if (!reportedStrings.add(magicString.getValue())) { - continue; - } - - addLocalProblem( - magicString, - new LocalizedMessage( - "magic-string", - Map.of( - "value", magicString.getValue() - ) - ), - ProblemType.MAGIC_STRING - ); - } - } - }); - } - - @Override - public Optional maximumProblems() { - return Optional.of(1); - } -} diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/oop/LeakedCollectionCheck.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/oop/LeakedCollectionCheck.java index 8ca12338..7b27b40b 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/oop/LeakedCollectionCheck.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/oop/LeakedCollectionCheck.java @@ -300,7 +300,7 @@ private void checkCtExecutableReturn(CtExecutable ctExecutable) { // a lambda like () -> true does not have a body, but an expression which is a return statement // this case is handled here - if (statements.isEmpty() && ctExecutable instanceof CtLambda ctLambda) { + if (statements.isEmpty() && ctExecutable instanceof CtLambda ctLambda && ctLambda.getExpression() != null) { statements = List.of(createCtReturn(ctLambda.getExpression().clone())); } diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/structure/DuplicateIfBlock.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/structure/DuplicateIfBlock.java index 66da7c69..c9063c83 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/structure/DuplicateIfBlock.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/structure/DuplicateIfBlock.java @@ -12,39 +12,117 @@ import spoon.reflect.code.CtStatement; import spoon.reflect.declaration.CtElement; +import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Collections; import java.util.IdentityHashMap; import java.util.List; import java.util.Map; +import java.util.Queue; import java.util.Set; import java.util.function.Predicate; import java.util.stream.Collectors; @ExecutableCheck(reportedProblems = { ProblemType.DUPLICATE_IF_BLOCK }) public class DuplicateIfBlock extends IntegratedCheck { - private static List getDuplicates(CtIf ctIf, Predicate isDuplicate) { - List result = new ArrayList<>(); - - List followingStatements; + private static List getElseStatements(CtIf ctIf, List followingStatements) { + List elseStatements; if (ctIf.getElseStatement() == null) { - // the if does not have an else, so check if the following statements are duplicate if blocks - followingStatements = StatementUtil.getNextStatements(ctIf); - + // check if the if block is terminal, because when it is not, we cannot merge it with the following statements + // with the if block, because they are executed after the if block has been executed. + // + // if (condition) { + // System.out.println("a"); + // } + // + // System.out.println("a"); + // + // if (otherCondition) { + // System.out.println("a"); + // } + if (ctIf.getThenStatement() == null || !StatementUtil.isTerminal(ctIf.getThenStatement())) { + return null; + } + elseStatements = followingStatements; } else { - // the if has an else, so check if the else has an if with a duplicate block - followingStatements = StatementUtil.getEffectiveStatements(ctIf.getElseStatement()); + // there is an explicit else block + List statements = StatementUtil.getEffectiveStatements(ctIf.getElseStatement()); + // if the then block is not terminal, we cannot merge the else block if there is more than an if statement in it + // + // if (condition) { + // System.out.println("a"); + // } else { + // if (otherCondition) { + // System.out.println("a"); + // } + // + // System.out.println("b"); + // } + if (ctIf.getThenStatement() != null && !StatementUtil.isTerminal(ctIf.getThenStatement()) && statements.size() > 1) { + return null; + } + + elseStatements = new ArrayList<>(statements); + // if the then block is terminal, we can merge the else block with the following statements + if (ctIf.getThenStatement() != null && StatementUtil.isTerminal(ctIf.getThenStatement())) { + elseStatements.addAll(followingStatements); + } + } + + return elseStatements; + } + + // This function extracts duplicates from the if/else-if/else blocks that could be merged into one if block. + private static List listDuplicates(CtIf ctIf, Predicate isDuplicate) { + List result = new ArrayList<>(); + + List elseStatements = getElseStatements(ctIf, StatementUtil.getNextStatements(ctIf)); + if (elseStatements == null) { + return result; } - for (var statement : followingStatements) { + Queue queue = new ArrayDeque<>(elseStatements); + while (!queue.isEmpty()) { + CtStatement statement = queue.poll(); + + // these are the conditions for merging the previous if blocks with the current one: if (!(statement instanceof CtIf nextIf) || nextIf.getThenStatement() == null || !isDuplicate.test(nextIf.getThenStatement())) { break; } - if (nextIf.getElseStatement() == null) { + // Now we need to check the else block, for example in the following code one can not merge the if blocks: + // + // if (i <= 0) { + // System.out.println("a"); + // } else { + // if (i + i > 1) { + // System.out.println("a"); + // } else { + // System.out.println("c"); + // } + // } + + // if there is no else block, we can merge the if blocks + if (nextIf.getElseStatement() == null && StatementUtil.isTerminal(nextIf.getThenStatement())) { + result.add(nextIf); + continue; + } + + // Otherwise call the function which adjusts the else statements for possible merging + List nextElseStatements = getElseStatements(nextIf, new ArrayList<>(queue)); + if (nextElseStatements == null) { + break; + } + + // the else block seems to be mergeable: + if (nextElseStatements.isEmpty()) { result.add(nextIf); + break; } + + result.add(nextIf); + queue = new ArrayDeque<>(nextElseStatements); } return result; @@ -60,17 +138,15 @@ public void process(CtIf ctIf) { return; } - // what we check: - // - if (condition) followed by another if (condition) - // - if (condition) with an else { if (condition) } + List duplicates = listDuplicates( + ctIf, + ctStatement -> DuplicateCatchBlock.isDuplicateBlock(ctIf.getThenStatement(), ctStatement, difference -> false) + ); - // for the merging to work, the if block must be terminal (i.e. ends with return, throw, break, continue) - if (ctIf.getThenStatement() == null || !StatementUtil.isTerminal(ctIf.getThenStatement())) { + if (duplicates.stream().anyMatch(visited::contains)) { return; } - List duplicates = getDuplicates(ctIf, ctStatement -> DuplicateCatchBlock.isDuplicateBlock(ctIf.getThenStatement(), ctStatement, difference -> false)); - if (!duplicates.isEmpty()) { visited.add(ctIf); visited.addAll(duplicates); diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/CoreUtil.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/CoreUtil.java index b2d37faa..959c4675 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/CoreUtil.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/CoreUtil.java @@ -11,9 +11,14 @@ import java.io.File; import java.util.Arrays; +import java.util.Collections; +import java.util.IdentityHashMap; import java.util.Optional; +import java.util.Set; import java.util.StringJoiner; import java.util.function.Consumer; +import java.util.stream.Collector; +import java.util.stream.Collectors; /** * Utility class for functionality that does not fit in any other utility class. @@ -73,6 +78,10 @@ public static void visitCtCompilationUnit(CtModel ctModel, Consumer Collector> toIdentitySet() { + return Collectors.toCollection(() -> Collections.newSetFromMap(new IdentityHashMap<>())); + } + /** * Converts the provided source position into a human-readable string. * diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/MethodUtil.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/MethodUtil.java index 03e2d75d..9d34f546 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/MethodUtil.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/MethodUtil.java @@ -109,6 +109,10 @@ public static boolean isInOverridingMethod(CtElement ctElement) { return MethodHierarchy.isOverridingMethod(ctMethod); } + public static boolean isOverriddenMethod(CtMethod ctMethod) { + return MethodHierarchy.isOverriddenMethod(ctMethod); + } + /** * Checks if the given method is an invocation. * @param statement which is checked @@ -129,6 +133,16 @@ public static boolean isInMainMethod(CtElement ctElement) { return MethodUtil.isMainMethod(ctMethod); } + /** + * Returns the declaration of the given executable reference. + * + * @param ctExecutableReference the reference to get the declaration of + * @return the declaration or null if the declaration could not be found + */ + public static CtExecutable getExecutableDeclaration(CtExecutableReference ctExecutableReference) { + return UsesFinder.getExecutableDeclaration(ctExecutableReference); + } + public static boolean isGetter(CtMethod method) { return method.getSimpleName().startsWith("get") && method.getParameters().isEmpty() diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/StatementUtil.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/StatementUtil.java index 3137478c..346a05e5 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/StatementUtil.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/StatementUtil.java @@ -169,4 +169,29 @@ public static List getCasesEffects(Iterable> ctCases return effects; } + + /** + * Factory method to create a {@link CtBlock} with the given statements. + * + * @param firstStatement the first statement of the block + * @param otherStatements the other statements of the block + * @return the created block, note that the statements are cloned + */ + public static CtBlock createCtBlock(CtStatement firstStatement, List otherStatements) { + CtBlock ctBlock = firstStatement.getFactory().createCtBlock(firstStatement.clone()); + + for (CtStatement statement : otherStatements) { + ctBlock.addStatement(statement.clone()); + } + + return ctBlock; + } + + public static CtBlock createCtBlock(List statements) { + if (statements.isEmpty()) { + return null; + } + + return createCtBlock(statements.get(0), statements.subList(1, statements.size())); + } } diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/UsesFinder.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/UsesFinder.java index 3be22dbb..ef168482 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/UsesFinder.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/UsesFinder.java @@ -185,6 +185,11 @@ public static boolean isAccessingVariable(CtVariable ctVariable, CtVariableAc public static CtVariable getDeclaredVariable(CtVariableAccess ctVariableAccess) { return UsesFinder.getFor(ctVariableAccess).scanner.variableAccessDeclarations.getOrDefault(ctVariableAccess, null); } + + static CtExecutable getExecutableDeclaration(CtExecutableReference ctExecutableReference) { + return UsesFinder.getFor(ctExecutableReference).scanner.executableDeclarations.computeIfAbsent(ctExecutableReference, CtExecutableReference::getExecutableDeclaration); + } + /** * The scanner searches for uses of supported code elements in a single pass over the entire model. * Since inserting into the hash map requires (amortized) constant time, usage search runs in O(n) time. @@ -199,6 +204,7 @@ private static class UsesScanner extends CtScanner { private final Map> executableUses = new IdentityHashMap<>(); private final Map> typeUses = new IdentityHashMap<>(); private final Map> subtypes = new IdentityHashMap<>(); + private final Map executableDeclarations = new IdentityHashMap<>(); // Caches the current instanceof pattern variables, since Spoon doesn't track them yet // We are conservative: A pattern introduces a variable until the end of the current block @@ -335,6 +341,7 @@ private void recordTypeParameterReference(CtTypeParameterReference reference) { private void recordExecutableReference(CtExecutableReference reference, CtElement referencingElement) { var executable = reference.getExecutableDeclaration(); if (executable != null) { + this.executableDeclarations.put(reference, executable); var uses = this.executableUses.computeIfAbsent(executable, k -> new ArrayList<>()); uses.add(referencingElement); } diff --git a/autograder-core/src/main/resources/strings.de.ftl b/autograder-core/src/main/resources/strings.de.ftl index 94754cc6..3b0cd660 100644 --- a/autograder-core/src/main/resources/strings.de.ftl +++ b/autograder-core/src/main/resources/strings.de.ftl @@ -134,6 +134,7 @@ exception-message = Eine Exception sollte immer eine Nachricht dabei haben, die number-format-exception-ignored = 'NumberFormatException' sollte gefangen werden und entweder behandelt oder durch eine eigene Exception ersetzt werden. +try-block-size = Keine der im catch-Block gefangenen Exceptions, wird in den Zeilen {$lines} geworfen. Diese Zeilen sollten vor oder nach dem try-catch-Block stehen. # General compare-objects-exp = Der Typ '{$type}' sollte 'equals' implementieren und nicht über 'toString' verglichen werden. @@ -177,7 +178,7 @@ binary-operator-on-boolean = Statt '|' und '&' sollte man '||' und '&&' verwende object-datatype = Statt dem Datentyp 'Object', sollte die Variable '{$variable}' einen konkreten oder generischen Datentyp haben. -magic-string = Der String '{$value}' sollte in eine Konstante ausgelagert werden. +magic-literal = "{$value} ist ein(e) magic {$type}." loop-should-be-do-while = Diese Schleife sollte eine do-while Schleife sein, weil der Code vor der Schleife, der gleiche wie in der Schleife ist: {$suggestion} diff --git a/autograder-core/src/main/resources/strings.en.ftl b/autograder-core/src/main/resources/strings.en.ftl index f40c6b73..875e03c4 100644 --- a/autograder-core/src/main/resources/strings.en.ftl +++ b/autograder-core/src/main/resources/strings.en.ftl @@ -139,6 +139,8 @@ exception-message = An exception should always have a message that explains what number-format-exception-ignored = 'NumberFormatException' should be caught and/or replaced by self-defined exception. +try-block-size = None of the exceptions caught in the catch block are thrown in the lines {$lines}. These lines should be placed before or after the try-catch block. + # General compare-objects-exp = Implement an equals method for type '{$type}' and use it for comparisons @@ -180,7 +182,7 @@ binary-operator-on-boolean = Instead of '|' and '&' one should use '||' and '&&' object-datatype = Instead of the datatype 'Object', the variable '{$variable}' should have a concrete or generic datatype. -magic-string = The string '{$value}' should be in a constant. +magic-literal = "{$value} is a magic {$type}." loop-should-be-do-while = This loop should be a do-while loop, because the code before the loop is the same as the code in the loop: {$suggestion} diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/comment/TestUnnecessaryComment.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/comment/TestUnnecessaryComment.java index 1d5bd418..f0a8fd94 100644 --- a/autograder-core/src/test/java/de/firemage/autograder/core/check/comment/TestUnnecessaryComment.java +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/comment/TestUnnecessaryComment.java @@ -101,4 +101,48 @@ public Test() { problems.assertExhausted(); } + + @Test + void testPartiallyFilledJavadoc() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator( + StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "edu.kit.kastel.Main", + """ + package edu.kit.kastel; + + /** + * + * @author uxxxx + */ + public class Main { + } + """ + ), + PROBLEM_TYPES + ); + + problems.assertExhausted(); + } + + @Test + void testPartiallyFilledJavadocNoPackage() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator( + StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + /** + * + * @author uxxxx + */ + public class Main { + } + """ + ), + PROBLEM_TYPES + ); + + problems.assertExhausted(); + } } diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/exceptions/TestTryBlockSize.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/exceptions/TestTryBlockSize.java new file mode 100644 index 00000000..0f2fd538 --- /dev/null +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/exceptions/TestTryBlockSize.java @@ -0,0 +1,221 @@ +package de.firemage.autograder.core.check.exceptions; + +import de.firemage.autograder.api.JavaVersion; +import de.firemage.autograder.api.LinterException; +import de.firemage.autograder.core.LocalizedMessage; +import de.firemage.autograder.core.Problem; +import de.firemage.autograder.core.ProblemType; +import de.firemage.autograder.core.check.AbstractCheckTest; +import de.firemage.autograder.core.file.StringSourceInfo; +import org.junit.jupiter.api.Test; + +import java.io.IOException; +import java.util.List; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +class TestTryBlockSize extends AbstractCheckTest { + private static final List PROBLEM_TYPES = List.of(ProblemType.TRY_BLOCK_SIZE); + + void assertUnnecessaryStatements(Problem problem, String lines) { + assertEquals( + this.linter.translateMessage(new LocalizedMessage( + "try-block-size", + Map.of( + "lines", lines + ) + )), + this.linter.translateMessage(problem.getExplanation()) + ); + } + + @Test + void testNothingThrows() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + public class Main { + private Main() { + } + + public static void main(String[] args) { + try { + System.out.println("Hello"); + System.out.println("World"); + System.out.println("!"); + } catch (Exception e) { + System.out.println("Error"); + } + } + } + """ + ), PROBLEM_TYPES); + + assertUnnecessaryStatements(problems.next(), "L7-9"); + + problems.assertExhausted(); + } + + @Test + void testMotivation() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + import java.util.Scanner; + + class InvalidArgumentException extends Exception { + public InvalidArgumentException(String message) { + super(message); + } + } + + public class Main { + private Main() { + } + + public static void main(String[] args) { + try { + Scanner scanner = new Scanner(System.in); + + int[] array = new int[5]; + int index = 0; + String line = scanner.nextLine(); + + for (String part : line.split(":", -1)) { + array[index] = parseNumber(part); + index += 1; + } + + for (int i = 0; i < array.length; i++) { + System.out.println(array[i]); + } + } catch (InvalidArgumentException e) { + System.out.println("Error, %s".formatted(e.getMessage())); + } + } + + private static int parseNumber(String string) throws InvalidArgumentException { + try { + return Integer.parseInt(string); + } catch (NumberFormatException e) { + throw new InvalidArgumentException("The input \\"%s\\" is not a valid number.".formatted(string)); + } + } + } + """ + ), PROBLEM_TYPES); + + assertUnnecessaryStatements(problems.next(), "L15-19, L26-28"); + + problems.assertExhausted(); + } + + @Test + void testOnlyTrailing() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + import java.util.Scanner; + + class InvalidArgumentException extends Exception { + public InvalidArgumentException(String message) { + super(message); + } + } + + public class Main { + private Main() { + } + + public static void main(String[] args) { + Scanner scanner = new Scanner(System.in); + + int[] array = new int[5]; + int index = 0; + String line = scanner.nextLine(); + + try { + for (String part : line.split(":", -1)) { + array[index] = parseNumber(part); + index += 1; + } + + for (int i = 0; i < array.length; i++) { + System.out.println(array[i]); + } + } catch (InvalidArgumentException e) { + System.out.println("Error, %s".formatted(e.getMessage())); + } + } + + private static int parseNumber(String string) throws InvalidArgumentException { + try { + return Integer.parseInt(string); + } catch (NumberFormatException e) { + throw new InvalidArgumentException("The input \\"%s\\" is not a valid number.".formatted(string)); + } + } + } + """ + ), PROBLEM_TYPES); + + assertUnnecessaryStatements(problems.next(), "L26-28"); + + problems.assertExhausted(); + } + + + @Test + void testUsesVariable() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + import java.util.Scanner; + + class InvalidArgumentException extends Exception { + public InvalidArgumentException(String message) { + super(message); + } + } + + public class Main { + private Main() { + } + + public static void main(String[] args) { + Scanner scanner = new Scanner(System.in); + String line = scanner.nextLine(); + + try { + int value = parseNumber(line); + + for (int i = 0; i < 5; i++) { + System.out.println(value); + } + System.out.println(value); + } catch (InvalidArgumentException e) { + System.out.println("Error, %s".formatted(e.getMessage())); + } + } + + private static int parseNumber(String string) throws InvalidArgumentException { + try { + return Integer.parseInt(string); + } catch (NumberFormatException e) { + throw new InvalidArgumentException("The input \\"%s\\" is not a valid number.".formatted(string)); + } + } + } + """ + ), PROBLEM_TYPES); + + assertUnnecessaryStatements(problems.next(), "L20-23"); + + problems.assertExhausted(); + } +} diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/general/TestMagicString.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/general/TestMagicLiteral.java similarity index 63% rename from autograder-core/src/test/java/de/firemage/autograder/core/check/general/TestMagicString.java rename to autograder-core/src/test/java/de/firemage/autograder/core/check/general/TestMagicLiteral.java index bb9575a7..e618cf73 100644 --- a/autograder-core/src/test/java/de/firemage/autograder/core/check/general/TestMagicString.java +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/general/TestMagicLiteral.java @@ -8,6 +8,8 @@ import de.firemage.autograder.api.JavaVersion; import de.firemage.autograder.core.file.StringSourceInfo; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; import java.io.IOException; import java.util.List; @@ -15,14 +17,24 @@ import static org.junit.jupiter.api.Assertions.assertEquals; -class TestMagicString extends AbstractCheckTest { - private static final List PROBLEM_TYPES = List.of(ProblemType.MAGIC_STRING); +class TestMagicLiteral extends AbstractCheckTest { + private static final List PROBLEM_TYPES = List.of(ProblemType.MAGIC_LITERAL); + + void assertMagicLiteral(Problem problem, String value) { + String type = "number"; + if (value.contains("'")) { + type = "character"; + } else if (value.contains("\"")) { + type = "string"; + } - void assertMagicString(Problem problem, String value) { assertEquals( this.linter.translateMessage(new LocalizedMessage( - "magic-string", - Map.of("value", value) + "magic-literal", + Map.of( + "type", type, + "value", value + ) )), this.linter.translateMessage(problem.getExplanation()) ); @@ -51,9 +63,9 @@ private static void doSomething(String string) { """ ), PROBLEM_TYPES); - assertMagicString(problems.next(), "Hello World"); - assertMagicString(problems.next(), "value"); - assertMagicString(problems.next(), "some error message"); + assertMagicLiteral(problems.next(), "\"Hello World\""); + assertMagicLiteral(problems.next(), "\"value\""); + assertMagicLiteral(problems.next(), "\"some error message\""); problems.assertExhausted(); } @@ -112,7 +124,7 @@ public enum EnumTest { """ ), PROBLEM_TYPES); - assertMagicString(problems.next(), "NotThursday2"); + assertMagicLiteral(problems.next(), "\"NotThursday2\""); problems.assertExhausted(); } @@ -166,10 +178,74 @@ private Function getFunction() { """ ), PROBLEM_TYPES); - assertMagicString(problems.next(), "Hello World"); - assertMagicString(problems.next(), "value"); - assertMagicString(problems.next(), "some error message"); - assertMagicString(problems.next(), "whatever"); + assertMagicLiteral(problems.next(), "\"Hello World\""); + assertMagicLiteral(problems.next(), "\"value\""); + assertMagicLiteral(problems.next(), "\"some error message\""); + assertMagicLiteral(problems.next(), "\"whatever\""); + + problems.assertExhausted(); + } + + @ParameterizedTest + @CsvSource( + delimiter = '|', + useHeadersInDisplayName = true, + value = { + " Expression | IsMagicLiteral ", + " \"a\" | true ", + " \"\" | false ", + " 0 | false ", + " 1 | false ", + " 2 | false ", + " -1 | false ", + " -2 | false ", + // + " -0.0f | false ", + " 0.0f | false ", + " 1.0f | false ", + " 1.1F | true ", + " 0.0 | false ", + } + ) + void testValues(String expression, boolean isMagicLiteral) throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static void main(String[] args) { + var value = %s; + } + } + """.formatted(expression) + ), PROBLEM_TYPES); + + if (isMagicLiteral) { + assertMagicLiteral(problems.next(), expression); + } + + problems.assertExhausted(); + + } + + @Test + void testMagicNumber() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + import java.util.function.Function; + + public class Test { + public static void main(String[] args) { + float magicFloat = 1.0f; //# ok + String empty = "" + 5; + } + } + """ + ), PROBLEM_TYPES); + + assertMagicLiteral(problems.next(), "5"); problems.assertExhausted(); } diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/structure/TestDuplicateIfBlock.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/structure/TestDuplicateIfBlock.java index 7dc8f82d..cf75b336 100644 --- a/autograder-core/src/test/java/de/firemage/autograder/core/check/structure/TestDuplicateIfBlock.java +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/structure/TestDuplicateIfBlock.java @@ -9,6 +9,8 @@ import de.firemage.autograder.core.file.StringSourceInfo; import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; import java.io.IOException; import java.util.List; @@ -51,7 +53,7 @@ public static void main(String[] args) {} } @Test - void testIfElse() throws IOException, LinterException { + void testIfElseIfTerminal() throws IOException, LinterException { ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( JavaVersion.JAVA_17, "Main", @@ -79,8 +81,61 @@ public static void main(String[] args) {} problems.assertExhausted(); } + + @Test + void testIfImplicitElseIf() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + public class Main { + void foo(int i) { + if (i == 0) { + System.out.println("zero"); + } else { + if (i == 1) { + System.out.println("zero"); + } + } + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testIfElseIfExtraStatement() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + public class Main { + void foo(int i) { + if (i == 0) { + System.out.println("zero"); + } else { + if (i == 1) { + System.out.println("zero"); + } + + System.out.println("extra"); + } + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + @Test - void testIfElseExtraStatement() throws IOException, LinterException { + void testIfElseIfExtraStatementTerminal() throws IOException, LinterException { ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( JavaVersion.JAVA_17, "Main", @@ -144,14 +199,38 @@ public static void main(String[] args) {} } """ ), PROBLEM_TYPES); - + // The above would be equivalent to a: + // + // if (a) { + // System.out.println("zero"); + // return; + // } else if (b) { + // System.out.println("zero"); + // return; + // } else if (c) { + // System.out.println("zero"); + // return; + // } else if (d) { + // System.out.println("zero"); + // return; + // } else { + // } + // + // Which could be simplified to a single if statement: + // + // if (a || b || c || d) { + // System.out.println("zero"); + // return; + // } + // + // The check will not detect this. assertDuplicate(problems.next(), List.of("i == 2", "i == 3")); problems.assertExhausted(); } @Test - void testFollowingIf() throws IOException, LinterException { + void testFollowingIfTerminal() throws IOException, LinterException { ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( JavaVersion.JAVA_17, "Main", @@ -188,4 +267,215 @@ public static void main(String[] args) {} problems.assertExhausted(); } + + @Test + void testFollowingIf() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + public class Main { + void foo(int i) { + if (i == 0) { + System.out.println("zero"); + } + + if (i == 1) { + System.out.println("zero"); + } + + if (i == 2) { + System.out.println("zero"); + } + + if (i == 3) { + System.out.println("1"); + } + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testIfElseIfElseTerminal() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + public class Main { + boolean foo(int i) { + if (i <= 0) { + return false; + } else if (i + i > 1) { + return false; + } else { + return true; + } + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + assertDuplicate(problems.next(), List.of("i <= 0", "i + i > 1")); + + problems.assertExhausted(); + } + + + @Test + void testIfElseIfElse() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + public class Main { + void foo(int i) { + if (i <= 0) { + System.out.println("a"); + } else { + if (i + i > 1) { + System.out.println("a"); + } else { + System.out.println("b"); + } + } + + System.out.println("c"); + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + assertDuplicate(problems.next(), List.of("i <= 0", "i + i > 1")); + + problems.assertExhausted(); + } + + + @Test + void testIfElseIfNoElse() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + public class Main { + void foo(int i) { + if (i <= 0) { + System.out.println("a"); + } else { + if (i + i > 1) { + System.out.println("a"); + } + } + + System.out.println("c"); + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testIfElseIfElseIfElse() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + public class Main { + boolean isDoorOpen(int number, boolean[] doors, String[] sweets) { + if (number < 1) { + return false; + } else if (number > sweets.length) { + return false; + } else if (doors[number - 1]) { + return true; + } else { + return false; + } + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + assertDuplicate(problems.next(), List.of("number < 1", "number > sweets.length")); + + problems.assertExhausted(); + } + + @Test + void testDuplicateNestedIfElseIf() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + public class Main { + void foo(int i) { + if (i <= 0) { + System.out.println("a"); + } else { + if (i + i > 1) { + if (i + 5 > 5) { + System.out.println("a"); + } else { + System.out.println("b"); + } + } else { + System.out.println("c"); + } + } + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testDuplicateNestedIfElseIfTerminal() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + public class Main { + int foo(int i) { + if (i <= 0) { + return 0; + } else { + if (i + i > 1) { + if (i + 5 > 5) { + return 0; + } else { + return 1; + } + } else { + return 2; + } + } + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } } diff --git a/pom.xml b/pom.xml index de175ae3..cd639e4b 100644 --- a/pom.xml +++ b/pom.xml @@ -35,13 +35,14 @@ https://github.com/Feuermagier/autograder/tree/main + UTF-8 @@ -53,7 +54,7 @@ 2.0.16 2.18.0 - 11.1.1-SNAPSHOT + 11.1.1-beta-18 0.70 0.10.2 diff --git a/sample_config.yaml b/sample_config.yaml index 64ad4af1..d98dac1d 100644 --- a/sample_config.yaml +++ b/sample_config.yaml @@ -143,7 +143,7 @@ problemsToReport: - COMMON_REIMPLEMENTATION_SQRT - COMMON_REIMPLEMENTATION_HYPOT - CONSTANT_NAME_CONTAINS_VALUE - - MAGIC_STRING + - MAGIC_LITERAL - IMPLEMENT_COMPARABLE - SIMPLIFY_ARRAYS_FILL - COMMON_REIMPLEMENTATION_ITERABLE_DUPLICATES @@ -163,3 +163,4 @@ problemsToReport: - SIMPLIFY_STRING_SUBSTRING - DUPLICATE_CATCH_BLOCK - DUPLICATE_IF_BLOCK + - TRY_BLOCK_SIZE