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 ad42bfd2..63ce2592 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 @@ -14,6 +14,7 @@ public enum ProblemType { EQUALS_HASHCODE_COMPARABLE_CONTRACT, UNCHECKED_TYPE_CAST, TOO_MANY_EXCEPTIONS, + CONSTANT_NAME_CONTAINS_VALUE, DEPRECATED_COLLECTION_USED, COLLECTION_IS_EMPTY_REIMPLEMENTED, STRING_IS_EMPTY_REIMPLEMENTED, diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/naming/ConstantsHaveDescriptiveNamesCheck.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/naming/ConstantsHaveDescriptiveNamesCheck.java index 0f349c8d..061cfe51 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/naming/ConstantsHaveDescriptiveNamesCheck.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/naming/ConstantsHaveDescriptiveNamesCheck.java @@ -12,14 +12,13 @@ import spoon.reflect.code.CtLiteral; import spoon.reflect.code.CtTypeAccess; import spoon.reflect.declaration.CtField; -import spoon.reflect.declaration.CtVariable; import java.util.ArrayList; import java.util.List; import java.util.Map; import java.util.stream.Stream; -@ExecutableCheck(reportedProblems = {ProblemType.MEANINGLESS_CONSTANT_NAME}) +@ExecutableCheck(reportedProblems = {ProblemType.MEANINGLESS_CONSTANT_NAME, ProblemType.CONSTANT_NAME_CONTAINS_VALUE}) public class ConstantsHaveDescriptiveNamesCheck extends IntegratedCheck { private static final List NUMBER_PRE_SUFFIXES = List.of("index", "number", "value", "argument", "element", "param", "parameter", "arg", "group", "constant", "value_of"); @@ -145,20 +144,6 @@ private static boolean containsValueInName(String name, CtLiteral value) { return lowerCaseName.contains(valueString); } - private void reportProblem(String key, CtVariable ctVariable) { - this.addLocalProblem( - ctVariable, - new LocalizedMessage( - key, - Map.of( - "name", ctVariable.getSimpleName(), - "value", ctVariable.getDefaultExpression().prettyprint() - ) - ), - ProblemType.MEANINGLESS_CONSTANT_NAME - ); - } - @Override protected void check(StaticAnalysis staticAnalysis, DynamicAnalysis dynamicAnalysis) { staticAnalysis.processWith(new AbstractProcessor>() { @@ -189,9 +174,29 @@ public void process(CtField field) { if (literal.getValue() instanceof Integer v1 && isNonDescriptiveIntegerName(fieldName, v1) || literal.getValue() instanceof String v2 && isNonDescriptiveStringName(fieldName, v2)) { - reportProblem("constants-name-exp", field); + addLocalProblem( + field, + new LocalizedMessage( + "constants-name-exp", + Map.of( + "name", field.getSimpleName(), + "value", field.getDefaultExpression().prettyprint() + ) + ), + ProblemType.MEANINGLESS_CONSTANT_NAME + ); } else if (containsValueInName(fieldName, literal)) { - reportProblem("constants-name-exp-value", field); + addLocalProblem( + field, + new LocalizedMessage( + "constants-name-exp-value", + Map.of( + "name", field.getSimpleName(), + "value", field.getDefaultExpression().prettyprint() + ) + ), + ProblemType.CONSTANT_NAME_CONTAINS_VALUE + ); } } }); 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 aa75551a..070e468d 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 @@ -50,6 +50,7 @@ import spoon.reflect.factory.Factory; import spoon.reflect.factory.TypeFactory; import spoon.reflect.reference.CtExecutableReference; +import spoon.reflect.reference.CtFieldReference; import spoon.reflect.reference.CtReference; import spoon.reflect.reference.CtTypeParameterReference; import spoon.reflect.reference.CtTypeReference; @@ -61,9 +62,13 @@ import spoon.reflect.visitor.filter.SameFilter; import spoon.reflect.visitor.filter.VariableAccessFilter; +import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; +import java.util.Collections; +import java.util.Deque; +import java.util.HashSet; import java.util.Iterator; import java.util.LinkedHashSet; import java.util.List; @@ -809,14 +814,11 @@ public static boolean isStaticCallTo(CtInvocation invocation, String typeName } public static boolean isEffectivelyFinal(CtVariable ctVariable) { - CtModel ctModel = ctVariable.getFactory().getModel(); - if (ctVariable.getModifiers().contains(ModifierKind.FINAL)) { return true; } - List> variableUses = ctModel - .getElements(new BetterVariableAccessFilter<>(ctVariable)); + List> variableUses = SpoonUtil.findUsesOf(ctVariable); return variableUses.isEmpty() || variableUses.stream().noneMatch(variableAccess -> variableAccess instanceof CtVariableWrite); } @@ -829,23 +831,59 @@ public static Optional> getEffectivelyFinalExpression(CtVari return Optional.ofNullable(ctVariable.getDefaultExpression()); } + /** + * Checks if the given element is guaranteed to be immutable. + *

+ * Note that when this method returns {@code false}, the type might still be immutable. + * + * @param ctTypeReference the type to check + * @return true if the given element is guaranteed to be immutable, false otherwise + * @param the type of the element + */ public static boolean isImmutable(CtTypeReference ctTypeReference) { - CtType ctType = ctTypeReference.getTypeDeclaration(); - if (ctType == null) { - return false; - } + Deque> queue = new ArrayDeque<>(Collections.singletonList(ctTypeReference)); + Collection> visited = new HashSet<>(); - if (ctTypeReference.unbox().isPrimitive() || SpoonUtil.isTypeEqualTo(ctTypeReference, java.lang.String.class)) { - return true; - } + while (!queue.isEmpty()) { + CtType ctType = queue.removeFirst().getTypeDeclaration(); - if (ctType.isShadow()) { - return false; + // if the type is not in the classpath, null is returned + // in those cases, assume that the type is not immutable + if (ctType == null) { + return false; + } + + // skip types that have been checked (those are guaranteed to be immutable) + if (visited.contains(ctType)) { + continue; + } + + // primitive types and strings are immutable as well: + if (ctType.getReference().unbox().isPrimitive() + || SpoonUtil.isTypeEqualTo(ctType.getReference(), java.lang.String.class)) { + continue; + } + + // types that are not in the classpath like java.util.ArrayList are shadow types. + // the source code for those is missing, so it is impossible to check if they are immutable. + // => assume they are not immutable + if (ctType.isShadow()) { + return false; + } + + // for a type to be immutable, all of its fields must be final and immutable as well: + for (CtFieldReference ctFieldReference : ctType.getAllFields()) { + if (!SpoonUtil.isEffectivelyFinal(ctFieldReference.getFieldDeclaration())) { + return false; + } + + queue.add(ctFieldReference.getType()); + } + + visited.add(ctType); } - return ctType.getAllFields().stream() - .allMatch(ctFieldReference -> SpoonUtil.isEffectivelyFinal(ctFieldReference.getFieldDeclaration()) - && SpoonUtil.isImmutable(ctFieldReference.getType())); + return true; } public static boolean isTypeEqualTo(CtTypeReference ctType, Class... expected) { @@ -885,7 +923,7 @@ public static boolean isMainMethod(CtMethod method) { * @param ctElement the element to get the parents of * @return an iterable over all parents, the given element is not included */ - public static Iterable parents(CtElement ctElement) { + private static Iterable parents(CtElement ctElement) { return () -> new Iterator<>() { private CtElement current = ctElement; @@ -942,10 +980,6 @@ public static boolean isInnerClass(CtTypeMember type) { return type.getDeclaringType() != null; } - public static boolean isInnerClass(CtTypeReference ctTypeReference) { - return ctTypeReference.getDeclaringType() != null; - } - /** * Checks if the given method is overriding another method. *

@@ -1036,7 +1070,6 @@ public boolean matches(CtAbstractInvocation invocation) { CtExecutableReference invocationExecutable = invocation.getExecutable(); return invocationExecutable.equals(this.executable.getReference()) || this.executable.equals(invocationExecutable.getExecutableDeclaration()) - // TODO: consider removing this? || invocationExecutable.isOverriding(this.executable.getReference()); } } @@ -1047,7 +1080,6 @@ public boolean matches(CtExecutableReferenceExpression expression) { CtExecutableReference invocationExecutable = expression.getExecutable(); return invocationExecutable.equals(this.executable.getReference()) || this.executable.equals(invocationExecutable.getExecutableDeclaration()) - // TODO: consider removing this? || invocationExecutable.isOverriding(this.executable.getReference()); } } @@ -1124,9 +1156,12 @@ private static Filter buildExecutableFilter(CtExecutable ctExecuta // - CtVariable // - CtExecutable // - CtTypeMember - - public static List findUsesOf(CtVariable ctVariable) { - return SpoonUtil.findUses(ctVariable); + @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) { @@ -1137,7 +1172,6 @@ public static List findUsesOf(CtExecutable ctExecutable) { return SpoonUtil.findUses(ctExecutable); } - public static List findUses(CtElement ctElement) { return new ArrayList<>(ctElement.getFactory().getModel().getElements(new UsesFilter(ctElement))); } diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestCommonReimplementation.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestCommonReimplementation.java index 50ab96a8..03371601 100644 --- a/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestCommonReimplementation.java +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestCommonReimplementation.java @@ -342,6 +342,37 @@ public static Cell[] createCells(int n) { problems.assertExhausted(); } + @Test + void testArraysFillRecursiveType() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceStrings( + JavaVersion.JAVA_17, + Map.ofEntries( + Map.entry( + "Main", + """ + class PlayingFieldEntry { + static final PlayingFieldEntry FREE = new PlayingFieldEntry(); + } + + public class Main { + public static void main(String[] args) { + PlayingFieldEntry[] field = new PlayingFieldEntry[1]; + + for (int i = 0; i < field.length; i++) { + field[i] = PlayingFieldEntry.FREE; + } + } + } + """ + ) + ) + ), List.of(ProblemType.COMMON_REIMPLEMENTATION_ARRAYS_FILL)); + + assertEqualsReimplementation(problems.next(), "Arrays.fill(field, 0, field.length, PlayingFieldEntry.FREE)"); + + problems.assertExhausted(); + } + @Test void testEnumValuesAddAllUnorderedSet() throws LinterException, IOException { ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( diff --git a/sample_config.yaml b/sample_config.yaml index a1073087..e8a7f947 100644 --- a/sample_config.yaml +++ b/sample_config.yaml @@ -147,3 +147,4 @@ - OBJECT_DATATYPE - COMMON_REIMPLEMENTATION_SQRT - COMMON_REIMPLEMENTATION_HYPOT +- CONSTANT_NAME_CONTAINS_VALUE