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 c731bc2d..412d8fd5 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 @@ -73,7 +73,7 @@ public enum ProblemType { TOO_FEW_PACKAGES, TRY_CATCH_COMPLEXITY, AVOID_STATIC_BLOCKS, - + METHOD_SHOULD_BE_STATIC, REASSIGNED_PARAMETER, DOUBLE_BRACE_INITIALIZATION, FOR_CAN_BE_FOREACH, @@ -89,7 +89,8 @@ public enum ProblemType { IDENTIFIER_CONTAINS_TYPE_NAME, USE_GUARD_CLAUSES, CONCRETE_COLLECTION_AS_FIELD_OR_RETURN_VALUE, - LIST_NOT_COPIED_IN_GETTER, + LEAKED_COLLECTION_RETURN, + LEAKED_COLLECTION_ASSIGN, METHOD_USES_PLACEHOLDER_IMPLEMENTATION, UTILITY_CLASS_NOT_FINAL, UTILITY_CLASS_INVALID_CONSTRUCTOR, diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseArraysFill.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseArraysFill.java index dfdeb448..7bbce898 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseArraysFill.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseArraysFill.java @@ -43,7 +43,7 @@ private void checkArraysFill(CtFor ctFor) { CtElement loopVariable = SpoonUtil.getReferenceDeclaration(forLoopRange.loopVariable()); // return if the for loop uses the loop variable (would not be a simple repetition) - if (SpoonUtil.hasAnyUsesIn(loopVariable, ctAssignment.getAssignment(), e -> true)) { + if (SpoonUtil.hasAnyUsesIn(loopVariable, ctAssignment.getAssignment())) { return; } diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseModuloOperator.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseModuloOperator.java index 149b5ac8..6c185820 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseModuloOperator.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseModuloOperator.java @@ -70,9 +70,15 @@ private void checkModulo(CtIf ctIf) { // is equal to // // variable %= 3; - if (Set.of(BinaryOperatorKind.GE, BinaryOperatorKind.EQ).contains(condition.getKind())) { - CtExpression right = condition.getRightHandOperand(); + CtExpression checkedValue = condition.getRightHandOperand(); + // for boxed types, one could check if the value is null, + // for which the suggestion `a %= null` would not make sense + if (SpoonUtil.isNullLiteral(checkedValue)) { + return; + } + + if (Set.of(BinaryOperatorKind.GE, BinaryOperatorKind.EQ).contains(condition.getKind())) { addLocalProblem( ctIf, new LocalizedMessage( @@ -80,7 +86,7 @@ private void checkModulo(CtIf ctIf) { Map.of( "suggestion", "%s %%= %s".formatted( assignedVariable, - right + checkedValue ) ) ), diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseStringFormatted.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseStringFormatted.java index 3ab838e2..34f918de 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseStringFormatted.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseStringFormatted.java @@ -37,6 +37,11 @@ private void checkCtInvocation(CtInvocation ctInvocation) { } CtExpression format = args.remove(0); + // skip if the format string is not a string literal (e.g. a complex concatenation) + if (SpoonUtil.tryGetStringLiteral(SpoonUtil.resolveConstant(format)).isEmpty()) { + return; + } + String output = "%s.formatted(%s)".formatted( format, args.stream().map(CtExpression::toString).reduce((a, b) -> a + ", " + b).orElse("") diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RedundantAssignment.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RedundantAssignment.java index 4cbb0242..a0111b28 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RedundantAssignment.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RedundantAssignment.java @@ -7,6 +7,7 @@ import de.firemage.autograder.core.integrated.IntegratedCheck; import de.firemage.autograder.core.integrated.SpoonUtil; import de.firemage.autograder.core.integrated.StaticAnalysis; +import de.firemage.autograder.core.integrated.uses.UsesFinder; import spoon.processing.AbstractProcessor; import spoon.reflect.code.CtAssignment; import spoon.reflect.code.CtLocalVariable; @@ -50,12 +51,7 @@ public void process(CtAssignment ctAssignment) { return; } - - if (followingStatements.stream().noneMatch(statement -> SpoonUtil.hasAnyUsesIn( - ctLocalVariable, - statement, - element -> element instanceof CtVariableRead - ))) { + if (followingStatements.stream().noneMatch(statement -> UsesFinder.ofVariableRead(ctLocalVariable).in(statement).hasAny())) { addLocalProblem( ctAssignment, new LocalizedMessage( 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 07b868c5..fc78d308 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 @@ -6,9 +6,8 @@ import de.firemage.autograder.core.integrated.IntegratedCheck; import de.firemage.autograder.core.integrated.SpoonUtil; import de.firemage.autograder.core.integrated.StaticAnalysis; +import de.firemage.autograder.core.integrated.uses.UsesFinder; 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; @@ -48,18 +47,6 @@ 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) { staticAnalysis.processWith(new AbstractProcessor>() { @@ -89,7 +76,9 @@ public void process(CtVariable ctVariable) { Collection> visibleFields = getAllVisibleFields(parent); List> hiddenFields = visibleFields.stream() + // ignore fields that are allowed to be hidden .filter(ctFieldReference -> !ALLOWED_FIELDS.contains(ctFieldReference.getSimpleName())) + // only keep fields that have the same name as the variable, but are not the same field .filter(ctFieldReference -> ctFieldReference.getSimpleName().equals(ctVariable.getSimpleName()) && !ctFieldReference.equals(ctVariable.getReference())) .toList(); @@ -102,11 +91,11 @@ public void process(CtVariable ctVariable) { 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)); + boolean isFieldRead = hiddenFields.stream().anyMatch(ctFieldReference -> UsesFinder.ofVariableRead(ctFieldReference.getFieldDeclaration()).in(variableParent).hasAny()); // 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) { + if (UsesFinder.ofVariableRead(ctVariable).in(variableParent).hasAny() && isFieldRead) { addLocalProblem( ctVariable, new LocalizedMessage("avoid-shadowing", Map.of("name", ctVariable.getSimpleName())), diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/general/FieldShouldBeFinal.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/general/FieldShouldBeFinal.java index bc5dc9ab..a2f3cfe6 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/general/FieldShouldBeFinal.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/general/FieldShouldBeFinal.java @@ -6,6 +6,7 @@ import de.firemage.autograder.core.integrated.IntegratedCheck; import de.firemage.autograder.core.integrated.SpoonUtil; import de.firemage.autograder.core.integrated.StaticAnalysis; +import de.firemage.autograder.core.integrated.uses.UsesFinder; import spoon.processing.AbstractProcessor; import spoon.reflect.code.CtFieldWrite; import spoon.reflect.declaration.CtConstructor; @@ -38,11 +39,8 @@ public void process(CtField ctField) { } // check if the field is written to in constructors, which is fine if it does not have an explicit value - boolean hasWriteInConstructor = SpoonUtil.hasAnyUses( - ctField, - ctElement -> ctElement instanceof CtFieldWrite ctFieldWrite - && ctFieldWrite.getParent(CtConstructor.class) != null - ); + boolean hasWriteInConstructor = UsesFinder.ofVariableWrite(ctField) + .hasAnyMatch(ctFieldWrite -> ctFieldWrite.getParent(CtConstructor.class) != null); // we need to check if the field is explicitly initialized, because this is not allowed: // 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 index a21e75a5..6b347aba 100644 --- 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 @@ -8,6 +8,7 @@ import de.firemage.autograder.core.integrated.IntegratedCheck; import de.firemage.autograder.core.integrated.SpoonUtil; import de.firemage.autograder.core.integrated.StaticAnalysis; +import de.firemage.autograder.core.integrated.uses.UsesFinder; import spoon.processing.AbstractProcessor; import spoon.reflect.code.CtArrayRead; import spoon.reflect.code.CtBodyHolder; @@ -64,20 +65,15 @@ public class ForToForEachLoop extends IntegratedCheck { public static Optional> getForEachLoopVariable( CtBodyHolder ctBodyHolder, ForLoopRange forLoopRange, - Function, Optional>> getPotentialLoopVariableAccess + 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 + // if a ForLoopRange exists, it is guaranteed 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)); - } - + for (CtVariableAccess ctVariableAccess : UsesFinder.of(loopVariable).in(ctBodyHolder.getBody()).all()) { // 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); diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/general/LoopShouldBeFor.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/general/LoopShouldBeFor.java index df3da60d..b429fa04 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/general/LoopShouldBeFor.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/general/LoopShouldBeFor.java @@ -6,6 +6,7 @@ import de.firemage.autograder.core.integrated.IntegratedCheck; import de.firemage.autograder.core.integrated.SpoonUtil; import de.firemage.autograder.core.integrated.StaticAnalysis; +import de.firemage.autograder.core.integrated.uses.UsesFinder; import spoon.processing.AbstractProcessor; import spoon.reflect.code.BinaryOperatorKind; import spoon.reflect.code.CtAssignment; @@ -102,8 +103,7 @@ private static LoopSuggestion getCounter(CtLoop ctLoop) { CtStatement finalLastStatement = lastStatement; boolean isUpdatedMultipleTimes = statements.stream() .filter(statement -> statement != finalLastStatement) - .anyMatch( - statement -> SpoonUtil.hasAnyUsesIn(ctLocalVariable, statement, ctElement -> ctElement instanceof CtVariableWrite)); + .anyMatch(statement -> UsesFinder.ofVariableWrite(ctLocalVariable).in(statement).hasAny()); if (isUpdatedMultipleTimes) { return null; diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/general/UseDifferentVisibility.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/general/UseDifferentVisibility.java index b0b2acea..9bc23d02 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/general/UseDifferentVisibility.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/general/UseDifferentVisibility.java @@ -6,6 +6,7 @@ import de.firemage.autograder.core.integrated.IntegratedCheck; import de.firemage.autograder.core.integrated.SpoonUtil; import de.firemage.autograder.core.integrated.StaticAnalysis; +import de.firemage.autograder.core.integrated.uses.UsesFinder; import spoon.processing.AbstractProcessor; import spoon.reflect.CtModel; import spoon.reflect.declaration.CtElement; @@ -59,7 +60,7 @@ public String toString() { private static Visibility getVisibility(CtTypeMember ctTypeMember) { CtModel ctModel = ctTypeMember.getFactory().getModel(); - List references = SpoonUtil.findUsesOf(ctTypeMember); + List references = UsesFinder.of(ctTypeMember).all(); CtElement commonParent = SpoonUtil.findCommonParent(ctTypeMember, references); CtType declaringType = ctTypeMember.getDeclaringType(); @@ -124,7 +125,7 @@ public void process(CtTypeMember ctTypeMember) { .getFields() .stream() // filter out the field itself and those that do not reference the field - .filter(field -> field != ctField && !SpoonUtil.findUsesIn(ctField, field).isEmpty()) + .filter(field -> field != ctField && SpoonUtil.hasAnyUsesIn(ctField, field)) .map(UseDifferentVisibility::getVisibility) .max(Visibility::compareTo); 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 new file mode 100644 index 00000000..4cf586ce --- /dev/null +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/oop/LeakedCollectionCheck.java @@ -0,0 +1,305 @@ +package de.firemage.autograder.core.check.oop; + +import de.firemage.autograder.core.LocalizedMessage; +import de.firemage.autograder.core.ProblemType; +import de.firemage.autograder.core.check.ExecutableCheck; +import de.firemage.autograder.core.integrated.IntegratedCheck; +import de.firemage.autograder.core.integrated.SpoonUtil; +import de.firemage.autograder.core.integrated.StaticAnalysis; +import de.firemage.autograder.core.integrated.uses.UsesFinder; +import spoon.reflect.code.CtAssignment; +import spoon.reflect.code.CtBlock; +import spoon.reflect.code.CtConstructorCall; +import spoon.reflect.code.CtExpression; +import spoon.reflect.code.CtFieldAccess; +import spoon.reflect.code.CtFieldRead; +import spoon.reflect.code.CtFieldWrite; +import spoon.reflect.code.CtLambda; +import spoon.reflect.code.CtLocalVariable; +import spoon.reflect.code.CtNewArray; +import spoon.reflect.code.CtReturn; +import spoon.reflect.code.CtStatement; +import spoon.reflect.code.CtVariableRead; +import spoon.reflect.declaration.CtAnonymousExecutable; +import spoon.reflect.declaration.CtConstructor; +import spoon.reflect.declaration.CtElement; +import spoon.reflect.declaration.CtExecutable; +import spoon.reflect.declaration.CtField; +import spoon.reflect.declaration.CtMethod; +import spoon.reflect.declaration.CtModifiable; +import spoon.reflect.declaration.CtRecord; +import spoon.reflect.declaration.CtType; +import spoon.reflect.factory.Factory; +import spoon.reflect.reference.CtFieldReference; +import spoon.reflect.reference.CtLocalVariableReference; +import spoon.reflect.reference.CtVariableReference; +import spoon.reflect.visitor.CtScanner; +import spoon.reflect.visitor.filter.TypeFilter; + +import java.util.List; +import java.util.Map; +import java.util.Optional; + +// What should be supported: +// - Any mutable collection (List, Set, Map, ...) +// - Detect when things are always immutable (e.g. List.of() in constructor and no setter!) +// - Arrays that are leaked +// - Records that are not implemented correctly +// - Enums are a special case where the constructor is private? + +// How can collections be modified from the outside? +// - The variable is passed through the constructor and the constructor does not copy it +// - The setter does not copy the list +// - The getter returns the list directly + +@ExecutableCheck(reportedProblems = { + ProblemType.LEAKED_COLLECTION_RETURN, + ProblemType.LEAKED_COLLECTION_ASSIGN +}) +public class LeakedCollectionCheck extends IntegratedCheck { + /** + * Checks if the field can be mutated from the outside. + *

+ * Because we do not want to lint when people store an immutable copy via List.of() or similar. + * + * @param ctFieldAccess the accessed field + * @return true if the field can be mutated, false otherwise + */ + private static boolean canBeMutated(CtFieldAccess ctFieldAccess) { + CtFieldReference ctFieldReference = ctFieldAccess.getVariable(); + CtField ctField = ctFieldReference.getFieldDeclaration(); + + // arrays are always mutable, there is no immutable array + if (ctField.getType().isArray()) { + return true; + } + + if (!SpoonUtil.isSubtypeOf(ctField.getType(), java.util.Collection.class)) { + // not a collection + return false; + } + + // if the field has a default value that is mutable, the field is mutable + if (ctField.getAssignment() != null && isMutableAssignee(ctField.getAssignment())) { + return true; + } + + // there are some special classes that are always immutable + // for those we check if they were assigned that special construct + // + // (e.g. List.of(), Set.of(), Map.of(), Collections.unmodifiableList(), ...) + + // we check if there is a write to the field with a value that is guaranteed to be mutable + return UsesFinder.ofVariableWrite(ctField) + .hasAnyMatch(ctFieldWrite -> ctFieldWrite.getParent() instanceof CtAssignment ctAssignment + && isMutableAssignee(ctAssignment.getAssignment())); + } + + private static boolean isMutableAssignee(CtExpression ctExpression) { + if (ctExpression instanceof CtNewArray) { + return true; + } + + if (ctExpression instanceof CtConstructorCall) { + return true; + } + + if (ctExpression instanceof CtVariableRead ctVariableRead + && ctVariableRead.getVariable() instanceof CtLocalVariableReference ctLocalVariableReference) { + CtLocalVariable ctVariable = ctLocalVariableReference.getDeclaration(); + return (ctVariable.getDefaultExpression() != null && isMutableAssignee(ctVariable.getDefaultExpression())) + || UsesFinder.ofVariableWrite(ctVariable) + .hasAnyMatch(ctVariableWrite -> ctVariableWrite.getParent() instanceof CtAssignment ctAssignment + && isMutableAssignee(ctAssignment.getAssignment())); + } + + // if a public method assigns a parameter to a field that parameter can be mutated from the outside + // => the field is mutable assignee + CtExecutable ctExecutable = ctExpression.getParent(CtExecutable.class); + return ctExecutable instanceof CtModifiable ctModifiable + && !ctModifiable.isPrivate() + && hasAssignedParameterReference(ctExpression, ctExecutable); + } + + private static boolean hasAssignedParameterReference(CtExpression ctExpression, CtExecutable ctExecutable) { + return ctExpression instanceof CtVariableRead ctVariableRead + && isParameterOf(ctVariableRead.getVariable(), ctExecutable); + } + + private static boolean isParameterOf(CtVariableReference ctVariableReference, CtExecutable ctExecutable) { + CtElement declaration = SpoonUtil.getReferenceDeclaration(ctVariableReference); + if (declaration == null) { + return false; + } + + return ctExecutable.getParameters().stream().anyMatch(ctParameter -> ctParameter == declaration); + } + + private void checkCtExecutableReturn(CtExecutable ctExecutable) { + List statements = SpoonUtil.getEffectiveStatements(ctExecutable.getBody()); + + // 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) { + statements = List.of(createCtReturn(ctLambda.getExpression().clone())); + } + + if (statements.isEmpty() + // we should not check private methods (for those it should be okay to return a not-copied collection) + || (ctExecutable instanceof CtModifiable ctModifiable && ctModifiable.isPrivate())) { + return; + } + + List returns = statements.stream().flatMap(ctStatement -> { + if (ctStatement instanceof CtReturn ctReturn) { + return List.of(ctReturn).stream(); + } else { + return ctStatement.filterChildren(new TypeFilter<>(CtReturn.class)).list(CtReturn.class).stream(); + } + }).toList(); + + for (CtReturn ctReturn : returns) { + if (ctReturn.getReturnedExpression() == null) { + continue; + } + + CtExpression returnedExpression = ctReturn.getReturnedExpression(); + + if (!(returnedExpression instanceof CtFieldRead ctFieldRead)) { + continue; + } + + CtField field = ctFieldRead.getVariable().getFieldDeclaration(); + + // if the field is not private, it can not be leaked/mutated anyway. + if (!field.isPrivate()) { + continue; + } + + if (canBeMutated(ctFieldRead)) { + addLocalProblem( + SpoonUtil.findValidPosition(ctExecutable), + new LocalizedMessage( + "leaked-collection-return", + Map.of( + "method", ctExecutable.getSimpleName(), + "field", field.getSimpleName() + ) + ), + ProblemType.LEAKED_COLLECTION_RETURN + ); + } + } + } + + private void checkCtExecutableAssign(CtExecutable ctExecutable) { + if (ctExecutable instanceof CtModifiable ctModifiable && ctModifiable.isPrivate()) { + return; + } + + for (CtStatement ctStatement : SpoonUtil.getEffectiveStatements(ctExecutable.getBody())) { + if (!(ctStatement instanceof CtAssignment ctAssignment) + || !(ctAssignment.getAssigned() instanceof CtFieldWrite ctFieldWrite)) { + continue; + } + + if (hasAssignedParameterReference(ctAssignment.getAssignment(), ctExecutable) + && ctFieldWrite.getVariable().getFieldDeclaration().isPrivate() + && canBeMutated(ctFieldWrite)) { + String methodName = ctExecutable.getSimpleName(); + if (methodName.equals("")) { + methodName = ctExecutable.getParent(CtType.class).getSimpleName(); + } + + addLocalProblem( + SpoonUtil.findValidPosition(ctStatement), + new LocalizedMessage( + "leaked-collection-assign", + Map.of( + "method", methodName, + "field", ctFieldWrite.getVariable().getSimpleName() + ) + ), + ProblemType.LEAKED_COLLECTION_ASSIGN + ); + } + } + } + + private static CtReturn createCtReturn(CtExpression ctExpression) { + CtReturn ctReturn = ctExpression.getFactory().createReturn(); + return ctReturn.setReturnedExpression((CtExpression) ctExpression); + } + + // The generated accessor method of a record does not have a real return statement. + // This method fixes that by creating the return statement. + private static CtMethod fixRecordAccessor(CtRecord ctRecord, CtMethod ctMethod) { + Factory factory = ctMethod.getFactory(); + CtMethod result = ctMethod.clone(); + CtBlock ctBody = factory.createBlock(); + CtFieldRead ctFieldRead = factory.createFieldRead(); + + ctFieldRead.setTarget(null); + ctFieldRead.setVariable(ctRecord.getField(ctMethod.getSimpleName()).getReference()); + ctFieldRead.setType(result.getType()); + + ctBody.setStatements(List.of(createCtReturn(ctFieldRead))); + result.setBody(ctBody); + result.setParent(ctRecord); + + return result; + } + + @Override + protected void check(StaticAnalysis staticAnalysis) { + staticAnalysis.getModel().getRootPackage().accept(new CtScanner() { + @Override + public void visitCtRecord(CtRecord ctRecord) { + for (CtMethod ctMethod : ctRecord.getMethods()) { + if (ctMethod.isImplicit()) { + this.visitCtMethod(fixRecordAccessor(ctRecord, ctMethod)); + } + } + + super.visitCtRecord(ctRecord); + } + + @Override + public void visitCtConstructor(CtConstructor ctConstructor) { + checkCtExecutableReturn(ctConstructor); + checkCtExecutableAssign(ctConstructor); + + super.visitCtConstructor(ctConstructor); + } + + @Override + public void visitCtMethod(CtMethod ctMethod) { + checkCtExecutableReturn(ctMethod); + checkCtExecutableAssign(ctMethod); + + super.visitCtMethod(ctMethod); + } + + @Override + public void visitCtLambda(CtLambda ctLambda) { + checkCtExecutableReturn(ctLambda); + checkCtExecutableAssign(ctLambda); + + super.visitCtLambda(ctLambda); + } + + @Override + public void visitCtAnonymousExecutable(CtAnonymousExecutable ctAnonymousExecutable) { + checkCtExecutableReturn(ctAnonymousExecutable); + checkCtExecutableAssign(ctAnonymousExecutable); + + super.visitCtAnonymousExecutable(ctAnonymousExecutable); + } + }); + } + + @Override + public Optional maximumProblems() { + return Optional.of(4); + } +} diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/oop/ListGetterSetterCheck.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/oop/ListGetterSetterCheck.java deleted file mode 100644 index 28650693..00000000 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/oop/ListGetterSetterCheck.java +++ /dev/null @@ -1,124 +0,0 @@ -package de.firemage.autograder.core.check.oop; - -import de.firemage.autograder.core.LocalizedMessage; -import de.firemage.autograder.core.ProblemType; -import de.firemage.autograder.core.check.ExecutableCheck; -import de.firemage.autograder.core.integrated.IntegratedCheck; -import de.firemage.autograder.core.integrated.SpoonStreamUtil; -import de.firemage.autograder.core.integrated.SpoonUtil; -import de.firemage.autograder.core.integrated.StaticAnalysis; -import spoon.processing.AbstractProcessor; -import spoon.reflect.code.CtAssignment; -import spoon.reflect.code.CtConstructorCall; -import spoon.reflect.code.CtExpression; -import spoon.reflect.code.CtFieldAccess; -import spoon.reflect.code.CtFieldRead; -import spoon.reflect.code.CtInvocation; -import spoon.reflect.code.CtNewArray; -import spoon.reflect.code.CtReturn; -import spoon.reflect.declaration.CtField; -import spoon.reflect.declaration.CtMethod; -import spoon.reflect.declaration.CtTypeInformation; - -import java.util.Optional; - -@ExecutableCheck(reportedProblems = {ProblemType.LIST_NOT_COPIED_IN_GETTER}) -public class ListGetterSetterCheck extends IntegratedCheck { - @Override - protected void check(StaticAnalysis staticAnalysis) { - staticAnalysis.processWith(new AbstractProcessor>() { - @Override - public void process(CtReturn ret) { - CtMethod parentMethod = ret.getParent(CtMethod.class); - // parent will be null, if the return is in a constructor. - // constructors are not considered methods. - if (parentMethod == null || !parentMethod.isPublic() || ret.getReturnedExpression() == null) { - return; - } - - var returnedExpression = ret.getReturnedExpression(); - - if (isMutableCollection(returnedExpression.getType()) - && returnedExpression instanceof CtFieldRead read) { - CtField field = read.getVariable().getFieldDeclaration(); - if (field.isPrivate() && wasMutablyAssigned(staticAnalysis, field)) { - addLocalProblem(ret, new LocalizedMessage("list-getter-exp"), - ProblemType.LIST_NOT_COPIED_IN_GETTER); - } - } - } - }); - } - - private boolean isMutableCollection(CtTypeInformation type) { - if (type.isArray()) return true; - - String name = type.getQualifiedName(); - return name.equals("java.util.List") - || name.equals("java.util.ArrayList") - || name.equals("java.util.LinkedList") - || name.equals("java.util.Map") - || name.equals("java.util.HashMap") - || name.equals("java.util.TreeMap") - || name.equals("java.util.Set") - || name.equals("java.util.HashSet") - || name.equals("java.util.LinkedHashSet") - || name.equals("java.util.TreeSet") - || name.equals("java.util.NavigableMap"); - // TODO add more collections / implement an inheritance solver - } - - private boolean wasMutablyAssigned(StaticAnalysis analysis, CtField field) { - // arrays are always mutable - if (field.getType().isArray()) return true; - - if (field.getDefaultExpression() != null && isMutableAssignee(field.getDefaultExpression())) { - return true; - } - - var processor = new AbstractProcessor>() { - boolean mutablyAssigned = false; - - @Override - public void process(CtAssignment write) { - if (write.getAssigned() instanceof CtFieldAccess ref && - ref.getVariable().getFieldDeclaration().equals(field)) { - if (isMutableAssignee(write.getAssignment())) { - mutablyAssigned = true; - } - } - } - }; - analysis.processWith(processor); - return processor.mutablyAssigned; - } - - private boolean isMutableAssignee(CtExpression expression) { - if (expression instanceof CtNewArray) { - return true; - } - - if (expression instanceof CtInvocation invocation) { - return !SpoonUtil.isStaticCallTo(invocation, "java.util.List", "of") - && !SpoonUtil.isStaticCallTo(invocation, "java.util.Set", "of") - && !SpoonUtil.isStaticCallTo(invocation, "java.util.Map", "of") - && !SpoonUtil.isStaticCallTo(invocation, "java.util.Collections", "unmodifiableList") - && !SpoonUtil.isStaticCallTo(invocation, "java.util.Collections", "unmodifiableSet") - && !SpoonUtil.isStaticCallTo(invocation, "java.util.Collections", "unmodifiableSortedSet") - && !SpoonUtil.isStaticCallTo(invocation, "java.util.Collections", "unmodifiableMap") - && !SpoonUtil.isStaticCallTo(invocation, "java.util.Collections", "unmodifiableSortedMap") - && !SpoonUtil.isStaticCallTo(invocation, "java.util.Collections", "unmodifiableCollection") - && !(SpoonStreamUtil.isStreamOperation(invocation) && - invocation.getExecutable().getSimpleName().equals("toList")); - // .collect(Collectors.toList()) can be mutable or immutable - - } else { - return expression instanceof CtConstructorCall; - } - } - - @Override - public Optional maximumProblems() { - return Optional.of(4); - } -} diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/oop/MethodShouldBeStatic.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/oop/MethodShouldBeStatic.java new file mode 100644 index 00000000..d641bfb0 --- /dev/null +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/oop/MethodShouldBeStatic.java @@ -0,0 +1,108 @@ +package de.firemage.autograder.core.check.oop; + +import de.firemage.autograder.core.LocalizedMessage; +import de.firemage.autograder.core.ProblemType; +import de.firemage.autograder.core.check.ExecutableCheck; +import de.firemage.autograder.core.integrated.IntegratedCheck; +import de.firemage.autograder.core.integrated.SpoonUtil; +import de.firemage.autograder.core.integrated.StaticAnalysis; +import spoon.processing.AbstractProcessor; +import spoon.reflect.code.CtExecutableReferenceExpression; +import spoon.reflect.code.CtFieldAccess; +import spoon.reflect.code.CtInvocation; +import spoon.reflect.code.CtSuperAccess; +import spoon.reflect.code.CtTargetedExpression; +import spoon.reflect.code.CtThisAccess; +import spoon.reflect.code.CtTypeAccess; +import spoon.reflect.declaration.CtElement; +import spoon.reflect.declaration.CtMethod; +import spoon.reflect.declaration.CtType; +import spoon.reflect.declaration.CtTypeMember; +import spoon.reflect.visitor.Filter; + +import java.util.Map; + + +@ExecutableCheck(reportedProblems = { ProblemType.METHOD_SHOULD_BE_STATIC }) +public class MethodShouldBeStatic extends IntegratedCheck { + /** + * Finds elements that access the given type through this. Accessing the same type through a different object is ok. + * + * @param ctType the type to be accessed + */ + private record ThisAccessFilter(CtType ctType) implements Filter { + private boolean isSuperTypeAccess(CtTargetedExpression ctTargetedExpression) { + return ctTargetedExpression.getTarget() instanceof CtSuperAccess superAccess + && this.ctType.isSubtypeOf(superAccess.getType()); + } + + private boolean isThisTypeAccess(CtTargetedExpression ctTargetedExpression) { + if (this.isSuperTypeAccess(ctTargetedExpression)) { + return true; + } + + return ctTargetedExpression.getTarget() instanceof CtThisAccess thisAccess + && thisAccess.getTarget() instanceof CtTypeAccess ctTypeAccess + && this.ctType.equals(ctTypeAccess.getAccessedType().getTypeDeclaration()); + } + + @Override + public boolean matches(CtElement element) { + return switch (element) { + case CtFieldAccess ctFieldAccess -> this.isThisTypeAccess(ctFieldAccess); + case CtInvocation ctInvocation -> this.isThisTypeAccess(ctInvocation); + case CtExecutableReferenceExpression ctExecutableReferenceExpression -> this.isThisTypeAccess(ctExecutableReferenceExpression); + default -> false; + }; + } + } + + private static boolean isEffectivelyStatic(CtTypeMember ctTypeMember) { + if (ctTypeMember.isStatic()) { + return true; + } + + if (ctTypeMember.getDeclaringType().isInterface() || ctTypeMember.isAbstract()) { + return false; + } + + if (ctTypeMember instanceof CtMethod ctMethod) { + if (SpoonUtil.isOverriddenMethod(ctMethod) || SpoonUtil.isOverridden(ctMethod)) { + return false; + } + + if (ctMethod.getBody() == null) { + return true; + } + + return ctMethod.getBody().filterChildren(new ThisAccessFilter(ctTypeMember.getDeclaringType())).first() == null; + } + + return false; + } + + @Override + protected void check(StaticAnalysis staticAnalysis) { + staticAnalysis.processWith(new AbstractProcessor>() { + @Override + public void process(CtMethod ctMethod) { + if (ctMethod.isImplicit() || !ctMethod.getPosition().isValidPosition()) { + return; + } + + + if (!ctMethod.isStatic() && isEffectivelyStatic(ctMethod)) { + addLocalProblem( + ctMethod, + new LocalizedMessage( + "method-should-be-static", + Map.of("name", ctMethod.getSimpleName()) + ), + ProblemType.METHOD_SHOULD_BE_STATIC + ); + } + } + }); + } + +} diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/utils/Option.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/utils/Option.java new file mode 100644 index 00000000..3ec39141 --- /dev/null +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/utils/Option.java @@ -0,0 +1,69 @@ +package de.firemage.autograder.core.check.utils; + +import java.util.Iterator; +import java.util.Objects; +import java.util.function.Function; +import java.util.stream.Stream; + +public sealed interface Option extends Iterable permits Option.Some, Option.None { + static Option ofNullable(T value) { + return value == null ? new None<>() : new Some<>(value); + } + + static Option some(T value) { + Objects.requireNonNull(value, "Value must not be null."); + return new Some<>(value); + } + + static Option none() { + return new None<>(); + } + + default T unwrap() { + return switch (this) { + case Some (var value) -> value; + case None ignored -> throw new IllegalStateException("Expected Some value, but got None."); + }; + } + + default boolean isSome() { + return this instanceof Some; + } + + default Option map(Function function) { + return switch (this) { + case Some(var value) -> new Some<>(function.apply(value)); + case None ignored -> new None<>(); + }; + } + + /** + * Returns the value if it is present or null if it is not. + * + * @return the value or null + */ + default T nullable() { + return switch (this) { + case Some(var value) -> value; + case None ignored -> null; + }; + } + + default Stream stream() { + return switch (this) { + case Some(var value) -> Stream.of(value); + case None ignored -> Stream.empty(); + }; + } + + @Override + default Iterator iterator() { + return stream().iterator(); + } + + record None() implements Option { + } + + record Some(T value) implements Option { + } +} 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 5996ba89..8dfd8b1c 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 @@ -39,7 +39,7 @@ public static Optional fromCtFor(CtFor ctFor) { && 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()) + && SpoonUtil.getNextStatements(ctFor).stream().noneMatch(statement -> SpoonUtil.hasAnyUsesIn(localVariable, statement)) ) { potentialLoopVariable = localVariable; } else if (ctFor.getForInit().size() == 1 && ctFor.getForInit().get(0) instanceof CtLocalVariable ctLocalVariable) { 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 346a1fb9..8cfc6f0a 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 @@ -9,7 +9,7 @@ import de.firemage.autograder.core.integrated.evaluator.fold.InferOperatorTypes; import de.firemage.autograder.core.integrated.evaluator.fold.InlineVariableRead; import de.firemage.autograder.core.integrated.evaluator.fold.RemoveRedundantCasts; -import org.apache.commons.compress.utils.FileNameUtils; +import de.firemage.autograder.core.integrated.uses.UsesFinder; import org.apache.commons.io.FilenameUtils; import spoon.reflect.CtModel; import spoon.reflect.code.BinaryOperatorKind; @@ -64,7 +64,6 @@ import spoon.reflect.reference.CtTypeParameterReference; import spoon.reflect.reference.CtTypeReference; import spoon.reflect.visitor.Filter; -import spoon.reflect.visitor.chain.CtQueryable; import spoon.reflect.visitor.filter.CompositeFilter; import spoon.reflect.visitor.filter.DirectReferenceFilter; import spoon.reflect.visitor.filter.FilteringOperator; @@ -92,7 +91,6 @@ import java.util.function.Consumer; import java.util.function.Function; import java.util.function.Predicate; -import java.util.stream.Collectors; import java.util.stream.Stream; public final class SpoonUtil { @@ -866,10 +864,7 @@ public static boolean isEffectivelyFinal(CtVariable ctVariable) { return true; } - return !SpoonUtil.hasAnyUses( - ctVariable, - ctElement -> ctElement instanceof CtVariableWrite - ); + return !UsesFinder.of(ctVariable).filterType(CtVariableWrite.class).hasAny(); } public static Optional> getEffectivelyFinalExpression(CtVariable ctVariable) { @@ -1101,6 +1096,16 @@ public static boolean isOverriddenMethod(CtMethod ctMethod) { return !topDefinitions.isEmpty(); } + /** + * Checks if the given method is overridden by another method. + * + * @param ctMethod the method to check, must not be null + * @return true if the given method is overridden by another method, false otherwise + */ + public static boolean isOverridden(CtMethod ctMethod) { + return ctMethod.getFactory().getModel().filterChildren(new OverridingMethodFilter(ctMethod)).first() != null; + } + public static boolean isInOverriddenMethod(CtElement ctElement) { CtMethod ctMethod = ctElement.getParent(CtMethod.class); if (ctMethod == null) { @@ -1120,6 +1125,7 @@ public static boolean isInvocation(CtStatement statement) { return statement instanceof CtInvocation || statement instanceof CtConstructorCall || statement instanceof CtLambda; } + public static boolean isInMainMethod(CtElement ctElement) { CtMethod ctMethod = ctElement.getParent(CtMethod.class); if (ctMethod == null) { @@ -1129,18 +1135,15 @@ public static boolean isInMainMethod(CtElement ctElement) { return isMainMethod(ctMethod); } - /** - * Finds all uses of {@code ctElement} in {@code in}. - * - * @param ctElement the element to search for - * @param in the element to search in - * @return all uses of {@code ctElement} in {@code in} - */ - public static List findUsesIn(CtElement ctElement, CtElement in) { - return new ArrayList<>(in.getElements(new UsesFilter(ctElement))); + public static boolean hasAnyUsesIn(CtElement ctElement, CtElement toSearchIn) { + return UsesFinder.ofElement(ctElement).in(toSearchIn).hasAny(); } - public record FilterAdapter(Filter filter, Class type) implements Filter { + public static boolean hasAnyUses(CtElement ctElement, Predicate predicate) { + return UsesFinder.ofElement(ctElement).preFilter(predicate::test).hasAny(); + } + + public record FilterAdapter(Filter filter, Class type) implements Filter { @Override public boolean matches(U element) { if (this.type.isInstance(element)) { @@ -1424,58 +1427,6 @@ private static boolean isDirectSubtypeOf(CtTypeInformation ctType, CtType sup || ctType.getSuperInterfaces().contains(superType.getReference()); } - // Supported CtElement subtypes: - // - CtVariable - // - CtExecutable - // - CtTypeMember - @SuppressWarnings("unchecked") - public static List> findUsesOf(CtVariable ctVariable) { - return SpoonUtil.findUses(ctVariable) - .stream() - .map(ctElement -> (CtVariableAccess) ctElement) - .collect(Collectors.toList()); - } - - public static List findUsesOf(CtTypeMember ctTypeMember) { - return SpoonUtil.findUses(ctTypeMember); - } - - public static List findUsesOf(CtExecutable ctExecutable) { - return SpoonUtil.findUses(ctExecutable); - } - - private static boolean internalHasAnyUses(CtQueryable model, CtElement ctElement, Predicate predicate) { - // for local variables, one does not need to search the whole model - if (ctElement instanceof CtLocalVariable ctLocalVariable && model == ctElement.getFactory().getModel()) { - CtBlock parentBlock = ctLocalVariable.getParent(CtBlock.class); - if (parentBlock != null) { - return parentBlock - .filterChildren(new CompositeFilter<>(FilteringOperator.INTERSECTION, predicate::test, new UsesFilter(ctElement))) - .first(CtElement.class) != null; - } - } - - return model - .filterChildren(new CompositeFilter<>(FilteringOperator.INTERSECTION, predicate::test, new UsesFilter(ctElement))) - .first(CtElement.class) != null; - } - - public static boolean hasAnyUses(CtElement ctElement, Predicate predicate) { - return internalHasAnyUses(ctElement.getFactory().getModel(), ctElement, predicate); - } - - public static boolean hasAnyUsesIn(CtElement ctElement, CtElement toSearchIn) { - return hasAnyUsesIn(ctElement, toSearchIn, element -> true); - } - - public static boolean hasAnyUsesIn(CtElement ctElement, CtElement toSearchIn, Predicate predicate) { - return internalHasAnyUses(toSearchIn, ctElement, predicate); - } - - private static List findUses(CtElement ctElement) { - return new ArrayList<>(ctElement.getFactory().getModel().getElements(new UsesFilter(ctElement))); - } - private static int referenceIndexOf(List list, T element) { for (int i = 0; i < list.size(); i++) { if (list.get(i) == element) { @@ -1602,6 +1553,14 @@ public static SourcePosition findPosition(CtElement ctElement) { return null; } + public static CtElement findValidPosition(CtElement ctElement) { + CtElement result = ctElement; + while (result != null && !result.getPosition().isValidPosition()) { + result = result.getParent(); + } + return result; + } + /** * Converts the provided source position into a human-readable string. * diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/uses/BaseUsesFinder.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/uses/BaseUsesFinder.java new file mode 100644 index 00000000..2c950b8a --- /dev/null +++ b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/uses/BaseUsesFinder.java @@ -0,0 +1,116 @@ +package de.firemage.autograder.core.integrated.uses; + +import de.firemage.autograder.core.integrated.SpoonUtil; +import spoon.reflect.code.CtCatchVariable; +import spoon.reflect.code.CtLocalVariable; +import spoon.reflect.code.CtPattern; +import spoon.reflect.code.CtResource; +import spoon.reflect.code.CtTypePattern; +import spoon.reflect.declaration.CtElement; +import spoon.reflect.declaration.CtExecutable; +import spoon.reflect.declaration.CtParameter; +import spoon.reflect.declaration.CtTypeMember; +import spoon.reflect.declaration.CtTypeParameter; +import spoon.reflect.visitor.Filter; +import spoon.reflect.visitor.chain.CtQuery; +import spoon.reflect.visitor.chain.CtQueryable; +import spoon.reflect.visitor.filter.CompositeFilter; +import spoon.reflect.visitor.filter.FilteringOperator; + +import java.util.List; + +class BaseUsesFinder implements UsesFinder { + private final CtElement toSearchFor; + private CtQueryable toSearchIn; + + private Filter preFilter; + private Filter postFilter; + + BaseUsesFinder(CtElement toSearchFor) { + this.toSearchFor = toSearchFor; + // by default search in the whole model + this.toSearchIn = toSearchFor.getFactory().getModel(); + } + + /** + * Narrow the search to only look in the given element. + * + * @param toSearchIn the element to search in + * @return this UsesFinder + */ + public UsesFinder in(CtQueryable toSearchIn) { + this.toSearchIn = toSearchIn; + return this; + } + + // TODO: allow chaining of pre/post filters + + @SuppressWarnings("unchecked") + public UsesFinder preFilter(Filter preFilter, Class itemClass) { + this.preFilter = new SpoonUtil.FilterAdapter<>(preFilter, (Class) itemClass); + return this; + } + + @SuppressWarnings("unchecked") + public UsesFinder postFilter(Filter postFilter, Class itemClass) { + this.postFilter = new SpoonUtil.FilterAdapter<>(postFilter, (Class) itemClass); + return (UsesFinder) this; + } + + public CtQuery query(CtQueryable searchScope) { + Filter filter = new SpoonUtil.UsesFilter(this.toSearchFor); + + if (this.preFilter != null) { + filter = new CompositeFilter<>(FilteringOperator.INTERSECTION, this.preFilter, filter); + } + + if (this.postFilter != null) { + filter = new CompositeFilter<>( + FilteringOperator.INTERSECTION, + filter, + this.postFilter + ); + } + + return searchScope.filterChildren(filter); + } + + // TODO: figure out the overhead for creating a query, maybe this should be cached + @Override + public CtQuery query() { + CtQueryable searchScope = this.toSearchIn; + + if (this.toSearchIn == this.toSearchFor.getFactory().getModel()) { + switch (this.toSearchFor) { + // for local variables, one does not need to search the whole model, it is enough to search in the parent block + case CtLocalVariable ctLocalVariable -> { + if (ctLocalVariable.getParent(CtPattern.class) == null) { + searchScope = ctLocalVariable.getParent(); + } + } + case CtParameter ctParameter when ctParameter.getParent() instanceof CtExecutable ctExecutable && ctExecutable.getBody() != null -> { + searchScope = ctParameter.getParent(); + } + case CtCatchVariable ctCatchVariable -> { + searchScope = ctCatchVariable.getParent(); + } + case CtResource ctResource -> { + searchScope = ctResource.getParent(); + } + case CtTypeMember ctTypeMember when ctTypeMember.isPrivate() -> { + searchScope = ctTypeMember.getDeclaringType(); + } + case CtTypeMember ctTypeMember when !ctTypeMember.isPublic() -> { + searchScope = ctTypeMember.getDeclaringType().getPackage(); + } + case CtTypeParameter ctTypeParameter -> { + searchScope = ctTypeParameter.getParent(); + } + default -> { + } + } + } + + return this.query(searchScope); + } +} diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/uses/UsesFinder.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/uses/UsesFinder.java new file mode 100644 index 00000000..3434ffab --- /dev/null +++ b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/uses/UsesFinder.java @@ -0,0 +1,95 @@ +package de.firemage.autograder.core.integrated.uses; + +import de.firemage.autograder.core.check.utils.Option; +import spoon.reflect.code.CtVariableAccess; +import spoon.reflect.code.CtVariableRead; +import spoon.reflect.code.CtVariableWrite; +import spoon.reflect.declaration.CtElement; +import spoon.reflect.declaration.CtTypeMember; +import spoon.reflect.declaration.CtVariable; +import spoon.reflect.visitor.Filter; +import spoon.reflect.visitor.chain.CtConsumableFunction; +import spoon.reflect.visitor.chain.CtFunction; +import spoon.reflect.visitor.chain.CtQuery; +import spoon.reflect.visitor.chain.CtQueryable; + +import java.util.List; + +public interface UsesFinder extends CtQueryable { + static UsesFinder ofElement(CtElement toSearchFor) { + return new BaseUsesFinder<>(toSearchFor); + } + + static UsesFinder of(CtTypeMember ctTypeMember) { + return UsesFinder.ofElement(ctTypeMember); + } + + static UsesFinder> of(CtVariable toSearchFor) { + return UsesFinder.ofElement(toSearchFor).filterType(CtVariableAccess.class); + } + + static UsesFinder> ofVariableRead(CtVariable toSearchFor) { + return UsesFinder.ofElement(toSearchFor).filterType(CtVariableRead.class); + } + + + static UsesFinder> ofVariableWrite(CtVariable toSearchFor) { + return UsesFinder.ofElement(toSearchFor).filterType(CtVariableWrite.class); + } + + UsesFinder in(CtQueryable toSearchIn); + + UsesFinder preFilter(Filter preFilter, Class itemClass); + + default UsesFinder preFilter(Filter preFilter) { + return this.preFilter(preFilter, CtElement.class); + } + + UsesFinder postFilter(Filter postFilter, Class itemClass); + + default UsesFinder filterType(Class itemClass) { + return this.postFilter(element -> true, itemClass); + } + + CtQuery query(); + + /** + * Finds all uses of the element and returns them as a list. + * + * @return all uses of the element, you must ensure that the elements are really of type R (if necessary use filterType) + */ + default List all() { + return this.query().list(); + } + + default Option any() { + return Option.ofNullable(this.query().first()); + } + + default boolean hasAny() { + return this.any().isSome(); + } + + default Option anyMatch(Filter filter) { + return Option.ofNullable(this.query().filterChildren(filter).first()); + } + + default boolean hasAnyMatch(Filter filter) { + return this.anyMatch(filter).isSome(); + } + + @Override + default CtQuery filterChildren(Filter filter) { + return this.query().filterChildren(filter); + } + + @Override + default CtQuery map(CtFunction function) { + return this.query().map(function); + } + + @Override + default CtQuery map(CtConsumableFunction queryStep) { + return this.query().map(queryStep); + } +} diff --git a/autograder-core/src/main/resources/strings.de.ftl b/autograder-core/src/main/resources/strings.de.ftl index 44ab9dad..541e7682 100644 --- a/autograder-core/src/main/resources/strings.de.ftl +++ b/autograder-core/src/main/resources/strings.de.ftl @@ -226,10 +226,13 @@ variable-redundant-number-suffix = Der Bezeichner '{$name}' enthält eine redund # OOP concrete-collection = Der Typ '{$type}' sollte durch eine Schnittstelle wie zum Beispiel 'List' oder 'Set' ersetzt werden. -list-getter-exp = Kopiere diese veränderbare Collection bevor du sie zurückgibst, um unbeabsichtigte Veränderungen durch andere Klassen zu verhindern +leaked-collection-return = Die Methode '{$method}' gibt eine Referenz zu dem Feld '{$field}' zurück. Dadurch ist es möglich das Feld von außerhalb zu verändern. Gebe stattdessen eine Kopie zurück. +leaked-collection-assign = Die Methode '{$method}' weißt dem Feld '{$field}' eine Referenz zu. Dadurch ist es möglich das Feld von außerhalb zu verändern. Weiße stattdessen eine Kopie dem Feld zu. method-abstract-exp = {$type}::{$method} sollte abstrakt sein, anstatt eine Platzhalter-Implementierung anzugeben +method-should-be-static = Die Methode '{$name}' sollte statisch sein, da sie auf keine Instanzattribute oder Methoden zugreift. + utility-exp-final = Utility-Klasse ist nicht final utility-exp-constructor = Utility-Klassen müssen genau einen privaten und parameterlosen Konstruktor haben diff --git a/autograder-core/src/main/resources/strings.en.ftl b/autograder-core/src/main/resources/strings.en.ftl index 581f6fb1..d4ac6369 100644 --- a/autograder-core/src/main/resources/strings.en.ftl +++ b/autograder-core/src/main/resources/strings.en.ftl @@ -225,10 +225,13 @@ variable-redundant-number-suffix = The identifier '{$name}' has a redundant numb # OOP concrete-collection = The type '{$type}' should be replaced by an interface like 'List' or 'Set'. -list-getter-exp = Copy this mutable collection before returning it to avoid unwanted mutations by other classes +leaked-collection-return = The method '{$method}' returns a reference to the field '{$field}'. This allows the field to be modified from the outside. Instead, a copy should be returned. +leaked-collection-assign = The method '{$method}' assigns a reference to the field '{$field}'. This allows the field to be modified from the outside. Instead, a copy should be made before setting the field. method-abstract-exp = {$type}::{$method} should be abstract and not provide a default implementation +method-should-be-static = The method '{$name}' should be static, because it does not access instance attributes or methods. + utility-exp-final = Utility class is not final utility-exp-constructor = Utility classes must have a single private no-arg constructor diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestUseModuloOperator.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestUseModuloOperator.java index 6cfce007..167854d6 100644 --- a/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestUseModuloOperator.java +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestUseModuloOperator.java @@ -73,4 +73,45 @@ public static int adjust(int value, int limit) { } problems.assertExhausted(); } + + @Test + void testBoxedEqualsNull() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + public class Main { + public static void test(Integer i) { + if (i == null) { + i = 0; + } + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testBoxedInteger() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + public class Main { + public static void test(Integer i) { + Integer j = 7; + if (i == j) { + i = 0; + } + } + } + """ + ), PROBLEM_TYPES); + + assertReimplementation(problems.next(), "i %= j"); + + problems.assertExhausted(); + } } diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestUseStringFormatted.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestUseStringFormatted.java new file mode 100644 index 00000000..995a029e --- /dev/null +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestUseStringFormatted.java @@ -0,0 +1,70 @@ +package de.firemage.autograder.core.check.api; + +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 TestUseStringFormatted extends AbstractCheckTest { + private static final List PROBLEM_TYPES = List.of(ProblemType.USE_STRING_FORMATTED); + + void assertUseFormatString(Problem problem, String suggestion) { + assertEquals( + this.linter.translateMessage( + new LocalizedMessage( + "use-string-formatted", + Map.of("formatted", suggestion) + ) + ), + this.linter.translateMessage(problem.getExplanation()) + ); + } + + @Test + void testComplexFormat() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + private static final String INITIAL_FORMAT = "%s %"; + private static final String INDEX_FORMAT = "d: %"; + private static final int indexWidth = 2; + private static final int maxInstructionWidth = 4; + private static final int maxArgAWidth = 6; + private static final String SECOND_FORMAT = "d: %"; + private static final int maxArgBWidth = 6; + private static final String THIRD_FORMAT = "d: %s"; + + public static void validateNumber(String a, int index, int second, String third) { + String format = String.format(INITIAL_FORMAT + + (indexWidth) + + INDEX_FORMAT + + (maxInstructionWidth) + + (maxArgAWidth + 1) + + SECOND_FORMAT + + (maxArgBWidth) + + THIRD_FORMAT, + a, + index, + second, + third + ); + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } +} diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestUnusedImport.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestUnusedImport.java index 00d2d9f5..4156ee61 100644 --- a/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestUnusedImport.java +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestUnusedImport.java @@ -640,6 +640,7 @@ class Test { } @Test + @Disabled("Some spoon bug that I can't be bothered to report, debug or fix. Who writes code like this anyway?") void testUsedStaticJavaLangImport() throws IOException, LinterException { ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceStrings( JavaVersion.JAVA_17, 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 index 258ef10c..e611d4b9 100644 --- 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 @@ -401,4 +401,30 @@ public String toString() { problems.assertExhausted(); } + + + @Test + void testArrayParamHidesAttribute() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceStrings( + JavaVersion.JAVA_17, + Map.ofEntries( + Map.entry( + "Main", + """ + class Main { + private String ais; + + public void doSomething(String[] ais) { + System.out.println(ais); + } + } + """ + ) + ) + ), PROBLEM_TYPES); + + // not reported, because only the array is used in the method + + problems.assertExhausted(); + } } diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/oop/TestLeakedCollectionCheck.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/oop/TestLeakedCollectionCheck.java new file mode 100644 index 00000000..c182eda6 --- /dev/null +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/oop/TestLeakedCollectionCheck.java @@ -0,0 +1,839 @@ +package de.firemage.autograder.core.check.oop; + +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 org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; + +import java.io.IOException; +import java.util.List; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +class TestLeakedCollectionCheck extends AbstractCheckTest { + private static final List PROBLEM_TYPES = List.of( + ProblemType.LEAKED_COLLECTION_RETURN, + ProblemType.LEAKED_COLLECTION_ASSIGN + ); + + void assertEqualsLeakedReturn(Problem problem, String method, String field) { + assertEquals( + this.linter.translateMessage(new LocalizedMessage( + "leaked-collection-return", + Map.of( + "method", method, + "field", field + ) + )), + this.linter.translateMessage(problem.getExplanation()) + ); + } + + void assertEqualsLeakedAssign(Problem problem, String method, String field) { + assertEquals( + this.linter.translateMessage(new LocalizedMessage( + "leaked-collection-assign", + Map.of( + "method", method, + "field", field + ) + )), + this.linter.translateMessage(problem.getExplanation()) + ); + } + + @Test + void testLeakedGetArrayInClass() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + private int[] arrayA; + private String[][] arrayB; + + public int[] getA() { + return this.arrayA; + } + + public String[][] getB() { + return arrayB; + } + } + """ + ), PROBLEM_TYPES); + + assertEqualsLeakedReturn(problems.next(), "getA", "arrayA"); + assertEqualsLeakedReturn(problems.next(), "getB", "arrayB"); + + problems.assertExhausted(); + } + + @Test + void testNoAssignment() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + import java.util.List; + + public class Test { + private List list; + + public List get() { + return list; + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testMutableFieldInit() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + import java.util.List; + import java.util.ArrayList; + + public class Test { + private List list = new ArrayList<>(); + + public List get() { + return list; + } + } + """ + ), PROBLEM_TYPES); + + assertEqualsLeakedReturn(problems.next(), "get", "list"); + + problems.assertExhausted(); + } + + @Test + void testImmutableFieldInit() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + import java.util.List; + import java.util.ArrayList; + + public class Test { + private List list = List.of(); + + public List get() { + return list; + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testConstructorMutableInit() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + import java.util.List; + import java.util.ArrayList; + + public class Test { + private List list; + + public Test() { + this.list = new ArrayList<>(); + } + + public List get() { + return list; + } + } + """ + ), PROBLEM_TYPES); + + assertEqualsLeakedReturn(problems.next(), "get", "list"); + + problems.assertExhausted(); + } + + @Test + void testConstructorParamInit() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + import java.util.List; + import java.util.ArrayList; + + public class Test { + private List list; + + public Test(List list) { + this.list = new ArrayList<>(list); + } + + public List get() { + return list; + } + } + """ + ), PROBLEM_TYPES); + + assertEqualsLeakedReturn(problems.next(), "get", "list"); + + problems.assertExhausted(); + } + + @ParameterizedTest + @CsvSource( + delimiter = '|', + useHeadersInDisplayName = true, + value = { + " Type | Init ", + " List | List.of() ", + " List | List.of(1, 2) ", + " List | Arrays.asList(new Integer[] {1, 2, 3}) ", + // everything from Collections that creates an immutable collection: + " List | Collections.EMPTY_LIST ", + " Map | Collections.EMPTY_MAP ", + " Set | Collections.EMPTY_SET ", + " List | Collections.emptyList() ", + " Map | Collections.emptyMap() ", + " NavigableMap | Collections.emptyNavigableMap() ", + " NavigableSet | Collections.emptyNavigableSet() ", + " Set | Collections.emptySet() ", + " SortedMap | Collections.emptySortedMap() ", + " SortedSet | Collections.emptySortedSet() ", + " List | Collections.nCopies(5, 1) ", + " Set | Collections.singleton(5) ", + " List | Collections.singletonList(5) ", + " Map | Collections.singletonMap(5, 4) ", + + " Collection | Collections.unmodifiableCollection(new ArrayList<>()) ", + " List | Collections.unmodifiableList(new ArrayList<>()) ", + " Map | Collections.unmodifiableMap(new HashMap<>()) ", + " Map | Collections.unmodifiableNavigableMap(new TreeMap<>()) ", + " Set | Collections.unmodifiableNavigableSet(new TreeSet<>()) ", + + " Set | Collections.unmodifiableSet(new TreeSet<>()) ", + " Map | Collections.unmodifiableSortedMap(new TreeMap<>()) ", + " Set | Collections.unmodifiableSortedSet(new TreeSet<>()) ", + + " Collection | Collections.unmodifiableCollection(new ArrayList<>()) ", + " Collection | Collections.unmodifiableList(new ArrayList<>()) ", + } + ) + void testImmutableInit(String type, String init) throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + import java.util.*; + + public class Test { + private %s field = %s; + + public %s get() { + return field; + } + } + """.formatted(type, init, type) + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testLambdaReturn() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + import java.util.List; + import java.util.ArrayList; + import java.util.function.Supplier; + + public class Test { + private List list = new ArrayList<>(); + + public Supplier> getA() { + return () -> list; + } + + public Supplier> getB() { + return () -> { return list; }; + } + + public Supplier> getC() { + return () -> { return List.copyOf(list); }; + } + } + """ + ), PROBLEM_TYPES); + + // This is not implemented, because lambdas are rare. + + assertEqualsLeakedReturn(problems.next(), "lambda$0", "list"); + assertEqualsLeakedReturn(problems.next(), "lambda$1", "list"); + + problems.assertExhausted(); + } + + @Test + void testEnumArrayReturn() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "FieldKind", + """ + enum Vegetable { CARROT, SALAD; } + + public enum FieldKind { + FIELD(new Vegetable[] {Vegetable.CARROT, Vegetable.SALAD}); + + private final Vegetable[] possibleVegetables; + + FieldKind(Vegetable[] possibleVegetables) { + this.possibleVegetables = possibleVegetables; + } + + public Vegetable[] getPossibleVegetables() { + return this.possibleVegetables; + } + } + """ + ), PROBLEM_TYPES); + + assertEqualsLeakedReturn(problems.next(), "getPossibleVegetables", "possibleVegetables"); + + problems.assertExhausted(); + } + + @Test + void testEnumListOf() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "FieldKind", + """ + import java.util.List; + + enum Vegetable { CARROT, SALAD; } + + public enum FieldKind { + FIELD(List.of(Vegetable.CARROT, Vegetable.SALAD)); + + private final List possibleVegetables; + + FieldKind(List possibleVegetables) { + this.possibleVegetables = possibleVegetables; + } + + public List getPossibleVegetables() { + return this.possibleVegetables; + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testAssignPublicConstructor() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + import java.util.List; + import java.util.ArrayList; + + public class Test { + private List list; + + public Test(List list) { + this.list = list; + } + } + """ + ), PROBLEM_TYPES); + + assertEqualsLeakedAssign(problems.next(), "Test", "list"); + + problems.assertExhausted(); + } + + @Test + void testAssignMultiple() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + import java.util.List; + import java.util.ArrayList; + + public class Test { + private List listA; + private List listB; + + public Test(List listA, List listB) { + this.listA = listA; + this.listB = listB; + } + } + """ + ), PROBLEM_TYPES); + + assertEqualsLeakedAssign(problems.next(), "Test", "listA"); + assertEqualsLeakedAssign(problems.next(), "Test", "listB"); + + problems.assertExhausted(); + } + + @Test + void testSetter() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + import java.util.List; + import java.util.ArrayList; + + public class Test { + private List list; + + public void setList(List list) { + this.list = list; + } + } + """ + ), PROBLEM_TYPES); + + assertEqualsLeakedAssign(problems.next(), "setList", "list"); + + problems.assertExhausted(); + } + + @Test + void testSetterArray() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + import java.util.List; + import java.util.ArrayList; + + public class Test { + private String[] array; + + public void setArray(String[] array) { + this.array = array; + } + } + """ + ), PROBLEM_TYPES); + + assertEqualsLeakedAssign(problems.next(), "setArray", "array"); + + problems.assertExhausted(); + } + + @Test + void testSetterCopy() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + import java.util.List; + import java.util.ArrayList; + + public class Test { + private List list; + + public void setList(List list) { + this.list = new ArrayList<>(list); + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testAssignPublic() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + import java.util.List; + import java.util.ArrayList; + + public class Test { + public List list; + + public void setList(List list) { + this.list = list; + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testAssignNoWrite() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + import java.util.List; + import java.util.ArrayList; + + class Test { + private List list; + + void setList(List list) { + this.list = list; + } + } + """ + ), PROBLEM_TYPES); + + assertEqualsLeakedAssign(problems.next(), "setList", "list"); + + problems.assertExhausted(); + } + + @Test + void testAssignClassDefaultPublic() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + import java.util.List; + import java.util.ArrayList; + + class Test { + private List list = new ArrayList<>(); + + void setList(List list) { + this.list = list; + } + } + """ + ), PROBLEM_TYPES); + + assertEqualsLeakedAssign(problems.next(), "setList", "list"); + + problems.assertExhausted(); + } + + @Test + void testAssignNotParameter() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + import java.util.List; + import java.util.ArrayList; + + public class Test { + private List list; + + public void setList(List list) { + List otherList = new ArrayList<>(); + this.list = otherList; + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testRecordNoConstructor() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Forest", + """ + import java.util.List; + import java.util.ArrayList; + + public record Forest(List trees, List animals) {} + """ + ), PROBLEM_TYPES); + + assertEqualsLeakedReturn(problems.next(), "animals", "animals"); + assertEqualsLeakedReturn(problems.next(), "trees", "trees"); + + assertEqualsLeakedAssign(problems.next(), "Forest", "trees"); + assertEqualsLeakedAssign(problems.next(), "Forest", "animals"); + + problems.assertExhausted(); + } + + @Test + void testRecordImmutable() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Forest", + """ + import java.util.List; + import java.util.ArrayList; + + public record Forest(List trees, List animals) { + public Forest(List trees, List animals) { + this.trees = List.copyOf(trees); + this.animals = List.copyOf(animals); + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + + @Test + void testRecordOverridden() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Forest", + """ + import java.util.List; + import java.util.ArrayList; + + public record Forest(List trees, List animals) { + public Forest(List trees, List animals) { + this.trees = new ArrayList<>(trees); + this.animals = new ArrayList<>(animals); + } + + @Override + public List trees() { + return new ArrayList<>(trees); + } + + @Override + public List animals() { + return new ArrayList<>(animals); + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testRecordNotOverriddenGetter() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Forest", + """ + import java.util.List; + import java.util.ArrayList; + + public record Forest(List trees, List animals) { + public Forest(List trees, List animals) { + this.trees = new ArrayList<>(trees); + this.animals = new ArrayList<>(animals); + } + + public List getTrees() { + return new ArrayList<>(trees); + } + + public List getAnimals() { + return new ArrayList<>(animals); + } + } + """ + ), PROBLEM_TYPES); + + assertEqualsLeakedReturn(problems.next(), "animals", "animals"); + assertEqualsLeakedReturn(problems.next(), "trees", "trees"); + + problems.assertExhausted(); + } + + + @Test + void testRecordInitConstructor() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Forest", + """ + import java.util.List; + import java.util.ArrayList; + + public record Forest(List trees, List animals) { + public Forest(List trees, List animals) { + this.trees = new ArrayList<>(trees); + this.animals = animals; + } + } + """ + ), PROBLEM_TYPES); + + assertEqualsLeakedReturn(problems.next(), "animals", "animals"); + assertEqualsLeakedReturn(problems.next(), "trees", "trees"); + + assertEqualsLeakedAssign(problems.next(), "Forest", "animals"); + + problems.assertExhausted(); + } + + @Test + void testRecordUnmodifiableWithTempVariable() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Zoo", + """ + import java.util.List; + import java.util.Collection; + import java.util.Collections; + import java.util.ArrayList; + + // example from https://stackoverflow.com/a/71486123 + public record Zoo(String name, Collection animals) { + // NOTE: check that it overwrites the canonical constructor. + // Overwriting other constructors does not guarantee immutability. + public Zoo(String name, Collection animals) { + Collection list = Collections.unmodifiableCollection(animals); // because it is unmodifiable, it must not overwrite the accessor + this.animals = list; + this.name = name; + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + + @Test + void testRecordCopy() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Zoo", + """ + import java.util.List; + import java.util.Collection; + import java.util.ArrayList; + + public record Zoo(String name, Collection animals) { + public Zoo(String name, Collection animals) { + this.animals = new ArrayList<>(animals); + this.name = name; + } + + public Collection animals() { + return new ArrayList<>(animals); + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testRecordCallingCanonicalConstructor() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Zoo", + """ + import java.util.List; + import java.util.Collection; + import java.util.ArrayList; + + // example from https://stackoverflow.com/a/71486123 + public record Zoo(String name, Collection animals) { + public Zoo(Collection animals) { + this("DefaultZoo", animals); //# not ok + } + } + """ + ), PROBLEM_TYPES); + + assertEqualsLeakedReturn(problems.next(), "animals", "animals"); + assertEqualsLeakedAssign(problems.next(), "Zoo", "animals"); + + problems.assertExhausted(); + } + + @Test + void testRecordNotOverwritingAccessor() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Zoo", + """ + import java.util.List; + import java.util.Collection; + import java.util.ArrayList; + + // example from https://stackoverflow.com/a/71486123 + public record Zoo(String name, Collection animals) { + // NOTE: check that it overwrites the canonical constructor. + // Overwriting other constructors does not guarantee immutability. + public Zoo(String name, Collection animals) { + Collection list = new ArrayList<>(animals); + this.animals = list; + this.name = name; + } + + //# not ok, accessor not overwritten + } + """ + ), PROBLEM_TYPES); + + assertEqualsLeakedReturn(problems.next(), "animals", "animals"); + + problems.assertExhausted(); + } + + @Test + void testConditionalReturn() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + import java.util.List; + import java.util.ArrayList; + + class Test { + private List list = new ArrayList<>(); + + public List get() { + if (list.isEmpty()) { + return new ArrayList<>(); + } else { + return list; + } + } + } + """ + ), PROBLEM_TYPES); + + assertEqualsLeakedReturn(problems.next(), "get", "list"); + + problems.assertExhausted(); + } +} diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/oop/TestMethodShouldBeStatic.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/oop/TestMethodShouldBeStatic.java new file mode 100644 index 00000000..795f0728 --- /dev/null +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/oop/TestMethodShouldBeStatic.java @@ -0,0 +1,274 @@ +package de.firemage.autograder.core.check.oop; + +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 TestMethodShouldBeStatic extends AbstractCheckTest { + private static final List PROBLEM_TYPES = List.of(ProblemType.METHOD_SHOULD_BE_STATIC); + + void assertEqualsStatic(Problem problem, String name) { + assertEquals( + this.linter.translateMessage(new LocalizedMessage( + "method-should-be-static", + Map.of( + "name", name + ) + )), + this.linter.translateMessage(problem.getExplanation()) + ); + } + + @Test + void testMethodWithInstanceAccess() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + private int a; + + public void foo() { + this.a = 6; + System.out.println(this.a); + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testMethodWithImplicitInstanceAccess() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + private int a; + + public void foo() { + a = 6; + System.out.println(a); + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testMethodEmpty() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public void foo() {} + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + assertEqualsStatic(problems.next(), "foo"); + + problems.assertExhausted(); + } + + @Test + void testThisObjectAccess() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + private int a; + + public Test makeTest() { + Test result = new Test(); + result.a = 5; + + return result; + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + assertEqualsStatic(problems.next(), "makeTest"); + + problems.assertExhausted(); + } + + @Test + void testStaticMethodInvocation() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + private static void foo() {} + + public void run() { + foo(); + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + assertEqualsStatic(problems.next(), "run"); + + problems.assertExhausted(); + } + + + @Test + void testExecutableReference() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + import java.util.function.Function; + + public class Test { + private String label; + + private String string(String input) { + return input + label; + } + + private static void make(Function f) { + System.out.println(f.apply("Hello")); + } + + public void run() { + make(this::string); + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testSuperAccess() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + import java.util.function.Function; + + class Parent { + protected String label; + } + + public class Test extends Parent { + public void run() { + System.out.println(super.label); + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testStaticFieldAccess() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + private static String label; + + public void run() { + System.out.println(label); + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + assertEqualsStatic(problems.next(), "run"); + + problems.assertExhausted(); + } + + @Test + void testOverriddenMethod() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceStrings( + JavaVersion.JAVA_17, + Map.ofEntries( + Map.entry( + "Parent", + """ + public class Parent { + // This method is overridden by Test + public void foo() {} + } + """ + ), + Map.entry( + "Test", + """ + public class Test extends Parent { + @Override + public void foo() { + System.out.println("Hello world"); + } + + public static void main(String[] args) {} + } + """ + ) + ) + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testInterface() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public interface Test { + default void run() {} + + void foo(); + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } +} diff --git a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/ListGetterSetter/code/Test.java b/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/ListGetterSetter/code/Test.java deleted file mode 100644 index d745ccc8..00000000 --- a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/ListGetterSetter/code/Test.java +++ /dev/null @@ -1,62 +0,0 @@ -package de.firemage.autograder.core.check_tests.ListGetterSetter.code; - -import java.util.ArrayList; -import java.util.Collections; -import java.util.List; -import java.util.function.Supplier; - -public class Test { - private final List list = new ArrayList<>(); - private final int[] array = new int[10]; - private final String[][] array2d = new String[2][2]; - - public Test() { - return; /*# ok #*/ - } - - public static void main(String[] args) { - Supplier> lambda = () -> { return List.of("a"); }; /*# ok #*/ - } - - public List getList1() { - return this.list; /*# not ok #*/ - } - - public List getList2() { - return list; /*# not ok #*/ - } - - public List getList3() { - return new ArrayList<>(this.list); /*# ok #*/ - } - - public List getList4() { - return Collections.unmodifiableList(this.list); /*# ok #*/ - } - - public int[] getArray() { - return this.array; /*# not ok #*/ - } - - public String[][] getArray2d() { - return this.array2d; /*# not ok #*/ - } -} - -enum Vegetable { - CARROT, SALAD; -} - -enum FieldKind { - FIELD(new Vegetable[] {Vegetable.CARROT, Vegetable.SALAD}); - - private final Vegetable[] possibleVegetables; - - FieldKind(Vegetable[] possibleVegetables) { - this.possibleVegetables = possibleVegetables; - } - - public Vegetable[] getPossibleVegetables() { - return this.possibleVegetables; /*# not ok #*/ - } -} diff --git a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/ListGetterSetter/config.txt b/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/ListGetterSetter/config.txt deleted file mode 100644 index 3751b28a..00000000 --- a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/ListGetterSetter/config.txt +++ /dev/null @@ -1,7 +0,0 @@ -oop.ListGetterSetterCheck -Return mutable collection without copying it -Test.java:22 -Test.java:26 -Test.java:38 -Test.java:42 -Test.java:60 diff --git a/pom.xml b/pom.xml index 33f9d744..439b3c92 100644 --- a/pom.xml +++ b/pom.xml @@ -38,7 +38,7 @@ UTF-8 - 17 + 21 ${java.version} ${java.version} ${java.version} diff --git a/sample_config.yaml b/sample_config.yaml index 745930ae..e231bc35 100644 --- a/sample_config.yaml +++ b/sample_config.yaml @@ -44,7 +44,8 @@ - SINGLE_LETTER_LOCAL_NAME - IDENTIFIER_IS_ABBREVIATED_TYPE - CONCRETE_COLLECTION_AS_FIELD_OR_RETURN_VALUE -- LIST_NOT_COPIED_IN_GETTER +- LEAKED_COLLECTION_ASSIGN +- LEAKED_COLLECTION_RETURN - METHOD_USES_PLACEHOLDER_IMPLEMENTATION - UTILITY_CLASS_NOT_FINAL - UTILITY_CLASS_INVALID_CONSTRUCTOR @@ -157,3 +158,4 @@ - REDUNDANT_ASSIGNMENT - LOOP_SHOULD_BE_DO_WHILE - LOOP_SHOULD_BE_FOR +- METHOD_SHOULD_BE_STATIC