Implement some things

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

@@ -20,6 +20,7 @@ public enum ProblemType {
     COMMENTED_OUT_CODE,
     INCONSISTENT_COMMENT_LANGUAGE,
     MUTABLE_ENUM,
+    CHAR_RANGE,
     INVALID_COMMENT_LANGUAGE,
     JAVADOC_STUB_DESCRIPTION,
     JAVADOC_STUB_PARAMETER_TAG,
@@ -87,6 +88,7 @@ public enum ProblemType {
     USE_ENTRY_SET,
     EMPTY_BLOCK,
     UNUSED_CODE_ELEMENT,
+    UNUSED_CODE_ELEMENT_PRIVATE,
     SIMILAR_IDENTIFIER,
     REPEATED_MATH_OPERATION,
     STATIC_FIELD_SHOULD_BE_INSTANCE,

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

@@ -0,0 +1,196 @@
+package de.firemage.autograder.core.check.api;
+
+import de.firemage.autograder.core.LocalizedMessage;
+import de.firemage.autograder.core.ProblemType;
+import de.firemage.autograder.core.check.ExecutableCheck;
+import de.firemage.autograder.core.dynamic.DynamicAnalysis;
+import de.firemage.autograder.core.integrated.IntegratedCheck;
+import de.firemage.autograder.core.integrated.SpoonUtil;
+import de.firemage.autograder.core.integrated.StaticAnalysis;
+import org.apache.commons.lang3.Range;
+import spoon.processing.AbstractProcessor;
+import spoon.reflect.code.BinaryOperatorKind;
+import spoon.reflect.code.CtBinaryOperator;
+import spoon.reflect.code.CtExpression;
+import spoon.reflect.code.CtLiteral;
+import spoon.reflect.declaration.CtTypedElement;
+
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.function.UnaryOperator;
+
+@ExecutableCheck(reportedProblems = { ProblemType.CHAR_RANGE })
+public class CharRange extends IntegratedCheck {
+    private static final Set<BinaryOperatorKind> RANGE_OPERATORS = Set.of(
+        BinaryOperatorKind.LT,
+        BinaryOperatorKind.LE,
+        BinaryOperatorKind.GT,
+        BinaryOperatorKind.GE
+    );
+
+    private static final Map<Range<Character>, UnaryOperator<String>> KNOWN_RANGES = Map.of(
+        Range.between('a', 'z'), arg -> "Character.isAlphabetic(%s) && Character.isLowerCase(%s)".formatted(arg, arg),
+        Range.between('A', 'Z'), arg -> "Character.isAlphabetic(%s) && Character.isUpperCase(%s)".formatted(arg, arg),
+        Range.between('0', '9'), arg -> "Character.isDigit(%s)".formatted(arg)
+    );
+
+    private static boolean isOfTypeCharacter(CtTypedElement<?> ctTypedElement) {
+        return SpoonUtil.isTypeEqualTo(ctTypedElement.getType(), java.lang.Character.class, char.class);
+    }
+
+    private static BinaryOperatorKind swapOperator(BinaryOperatorKind operator) {
+        return switch (operator) {
+            // a < b => b > a
+            case LT -> BinaryOperatorKind.GT;
+            // a <= b => b >= a
+            case LE -> BinaryOperatorKind.GE;
+            // a >= b => b <= a
+            case GE -> BinaryOperatorKind.LE;
+            // a > b => b < a
+            case GT -> BinaryOperatorKind.LT;
+            default -> operator;
+        };
+    }
+
+    private record CtRange<T extends Comparable<T>>(
+        CtExpression<T> ctExpression,
+        BinaryOperatorKind operator,
+        CtLiteral<T> ctLiteral) {
+
+        @SuppressWarnings("unchecked")
+        public static Optional<CtRange<Character>> of(CtBinaryOperator<Boolean> ctBinaryOperator) {
+            // <expr> <op> <literal>
+            CtExpression<Character> expression;
+            BinaryOperatorKind operator;
+            CtLiteral<Character> literal;
+
+            // <expr> <op> <literal>
+            if (SpoonUtil.resolveCtExpression(ctBinaryOperator.getRightHandOperand()) instanceof CtLiteral<?> ctLiteral
+                && isOfTypeCharacter(ctLiteral)) {
+                literal = (CtLiteral<Character>) ctLiteral;
+                operator = ctBinaryOperator.getKind();
+                expression = (CtExpression<Character>) ctBinaryOperator.getLeftHandOperand();
+                // <literal> <op> <expr>
+            } else if (SpoonUtil.resolveCtExpression(ctBinaryOperator.getLeftHandOperand()) instanceof CtLiteral<?> ctLiteral
+                && isOfTypeCharacter(ctLiteral)) {
+                literal = (CtLiteral<Character>) ctLiteral;
+                expression = (CtExpression<Character>) ctBinaryOperator.getRightHandOperand();
+                // must swap literal and expression, to do that, the operator must be inverted:
+                // a <= b => b >= a
+                // a < b => b > a
+                // a >= b => b <= a
+                // a > b => b < a
+                operator = swapOperator(ctBinaryOperator.getKind());
+            } else {
+                return Optional.empty();
+            }
+
+            // adjust the literal if the operator is < or >
+            if (operator == BinaryOperatorKind.LT) {
+                // <expr> < <literal> => <expr> <= <literal> - 1
+                literal.setValue((char) (literal.getValue() - 1));
+                operator = BinaryOperatorKind.LE;
+            } else if (ctBinaryOperator.getKind() == BinaryOperatorKind.GT) {
+                // <expr> > <literal> => <expr> >= <literal> + 1
+                literal.setValue((char) (literal.getValue() + 1));
+                operator = BinaryOperatorKind.GE;
+            }
+
+            return Optional.of(new CtRange<>(expression, operator, literal));
+        }
+
+        private static <T> CtLiteral<T> minValue(CtLiteral<T> ctLiteral) {
+            CtLiteral result = ctLiteral.getFactory().createLiteral();
+            result.setBase(ctLiteral.getBase());
+
+            if (ctLiteral.getValue() instanceof Integer) {
+                result.setValue(Integer.MIN_VALUE);
+            } else if (ctLiteral.getValue() instanceof Character) {
+                result.setValue(Character.MIN_VALUE);
+            } else if (ctLiteral.getValue() instanceof Long) {
+                result.setValue(Long.MIN_VALUE);
+            }
+
+            return result;
+        }
+
+        private static <T> CtLiteral<T> maxValue(CtLiteral<T> ctLiteral) {
+            CtLiteral result = ctLiteral.getFactory().createLiteral();
+            result.setBase(ctLiteral.getBase());
+
+            if (ctLiteral.getValue() instanceof Integer) {
+                result.setValue(Integer.MAX_VALUE);
+            } else if (ctLiteral.getValue() instanceof Character) {
+                result.setValue(Character.MAX_VALUE);
+            } else if (ctLiteral.getValue() instanceof Long) {
+                result.setValue(Long.MAX_VALUE);
+            }
+
+            return result;
+        }
+
+        public Range<T> toRange() {
+            // <expr> <op> <literal>
+            if (this.operator == BinaryOperatorKind.LE) {
+                return Range.between(minValue(this.ctLiteral).getValue(), this.ctLiteral.getValue());
+            } else if (this.operator == BinaryOperatorKind.GE) {
+                // <expr> >= <literal>
+                return Range.between(this.ctLiteral.getValue(), maxValue(this.ctLiteral).getValue());
+            } else {
+                throw new IllegalStateException("Unsupported operator: " + this.operator);
+            }
+        }
+    }
+
+    @Override
+    protected void check(StaticAnalysis staticAnalysis, DynamicAnalysis dynamicAnalysis) {
+        staticAnalysis.processWith(new AbstractProcessor<CtBinaryOperator<Boolean>>() {
+            @Override
+            @SuppressWarnings("unchecked")
+            public void process(CtBinaryOperator<Boolean> ctBinaryOperator) {
+                if (ctBinaryOperator.isImplicit()
+                    || !ctBinaryOperator.getPosition().isValidPosition()
+                    || !SpoonUtil.isTypeEqualTo(ctBinaryOperator.getType(), java.lang.Boolean.class, boolean.class)
+                    || ctBinaryOperator.getKind() != BinaryOperatorKind.AND
+                    || !(ctBinaryOperator.getLeftHandOperand() instanceof CtBinaryOperator<?> left)
+                    || !(ctBinaryOperator.getRightHandOperand() instanceof CtBinaryOperator<?> right)
+                    || !RANGE_OPERATORS.contains(left.getKind())
+                    || !RANGE_OPERATORS.contains(right.getKind())
+                ) {
+                    return;
+                }
+
+                // structure must be one of:
+                // - (<expr> <op> <literal>) && (<expr> <op> <literal>)
+                // - (<literal> <op> <expr>) && (<literal> <op> <expr>)
+                // or swapped
+
+                CtRange<Character> leftRange = CtRange.of((CtBinaryOperator<Boolean>) left).orElse(null);
+                CtRange<Character> rightRange = CtRange.of((CtBinaryOperator<Boolean>) right).orElse(null);
+                if (leftRange == null || rightRange == null) {
+                    return;
+                }
+
+                // skip them if they do not check the same variable
+                if (!leftRange.ctExpression().equals(rightRange.ctExpression())) {
+                    return;
+                }
+
+                Range<Character> intersection = leftRange.toRange().intersectionWith(rightRange.toRange());
+
+                UnaryOperator<String> suggester = KNOWN_RANGES.get(intersection);
+                if (suggester != null) {
+                    addLocalProblem(
+                        ctBinaryOperator,
+                        new LocalizedMessage(
+                            "char-range",
+                            Map.of("suggestion", suggester.apply(leftRange.ctExpression().prettyprint()))
+                        ),
+                        ProblemType.CHAR_RANGE
+                    );
+                }
+            }
+        });
+    }
+}

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

@@ -67,7 +67,7 @@ private static Visibility getVisibility(CtTypeMember ctTypeMember, CtReference c
 
         // if there are no references, the member itself will be returned
         if (ctTypeMember == commonParent) {
-            return Visibility.PRIVATE;
+            return Visibility.of(ctTypeMember);
         }
 
         // if all references are in the same type as the member, it can be private

autograder-core/src/main/java/de/firemage/autograder/core/check/oop/ConcreteCollectionCheck.java

@@ -177,14 +177,7 @@ public void visitCtRecord(CtRecord ctRecord) {
                 this.scan(CtRole.ANNOTATION, ctRecord.getAnnotations());
                 this.scan(CtRole.INTERFACE, ctRecord.getSuperInterfaces());
                 this.scan(CtRole.TYPE_PARAMETER, ctRecord.getFormalCtTypeParameters());
-                for (CtRecordComponent component : ctRecord.getRecordComponents()) {
-                    CtField<?> ctField = component.toField();
-                    // the toField does not set the parent
-                    // this is a problem, because when one queries the AST, one might get this node
-                    // and it will not have a parent
-                    ctField.setParent(ctRecord);
-                    this.visitCtField(ctField);
-                }
+                ctRecord.getFields().forEach(this::visitCtField);
                 this.scan(CtRole.COMMENT, ctRecord.getComments());
                 this.exit(ctRecord);
             }

autograder-core/src/main/java/de/firemage/autograder/core/check/unnecessary/UnusedCodeElementCheck.java

@@ -3,22 +3,93 @@
 import de.firemage.autograder.core.LocalizedMessage;
 import de.firemage.autograder.core.ProblemType;
 import de.firemage.autograder.core.check.ExecutableCheck;
-import de.firemage.autograder.core.pmd.PMDCheck;
-import net.sourceforge.pmd.lang.java.rule.bestpractices.UnusedFormalParameterRule;
-import net.sourceforge.pmd.lang.java.rule.bestpractices.UnusedLocalVariableRule;
-import net.sourceforge.pmd.lang.java.rule.bestpractices.UnusedPrivateFieldRule;
-import net.sourceforge.pmd.lang.java.rule.bestpractices.UnusedPrivateMethodRule;
-
-import java.util.List;
-
-@ExecutableCheck(reportedProblems = {ProblemType.UNUSED_CODE_ELEMENT})
-public class UnusedCodeElementCheck extends PMDCheck {
-    public UnusedCodeElementCheck() {
-        super(new LocalizedMessage("unused-element-exp"), List.of(
-                new UnusedLocalVariableRule(),
-                new UnusedFormalParameterRule(),
-                new UnusedPrivateFieldRule(),
-                new UnusedPrivateMethodRule()),
-            ProblemType.UNUSED_CODE_ELEMENT);
+import de.firemage.autograder.core.dynamic.DynamicAnalysis;
+import de.firemage.autograder.core.integrated.IntegratedCheck;
+import de.firemage.autograder.core.integrated.SpoonUtil;
+import de.firemage.autograder.core.integrated.StaticAnalysis;
+import spoon.reflect.code.CtLocalVariable;
+import spoon.reflect.declaration.CtField;
+import spoon.reflect.declaration.CtMethod;
+import spoon.reflect.declaration.CtModifiable;
+import spoon.reflect.declaration.CtNamedElement;
+import spoon.reflect.declaration.CtParameter;
+import spoon.reflect.declaration.CtTypeParameter;
+import spoon.reflect.reference.CtReference;
+import spoon.reflect.visitor.CtScanner;
+import spoon.reflect.visitor.filter.DirectReferenceFilter;
+
+import java.util.Map;
+
+@ExecutableCheck(reportedProblems = { ProblemType.UNUSED_CODE_ELEMENT, ProblemType.UNUSED_CODE_ELEMENT_PRIVATE })
+public class UnusedCodeElementCheck extends IntegratedCheck {
+    private void checkUnused(CtNamedElement ctElement, CtReference ctReference) {
+        if (ctElement.isImplicit() || !ctElement.getPosition().isValidPosition()) {
+            return;
+        }
+
+        boolean isUnused = ctElement.getFactory()
+            .getModel()
+            .getElements(new DirectReferenceFilter<>(ctReference))
+            .isEmpty();
+
+
+        ProblemType problemType = ProblemType.UNUSED_CODE_ELEMENT;
+        if (ctElement instanceof CtModifiable ctModifiable && ctModifiable.isPrivate()) {
+            problemType = ProblemType.UNUSED_CODE_ELEMENT_PRIVATE;
+        }
+
+        if (isUnused) {
+            addLocalProblem(
+                ctElement,
+                new LocalizedMessage("unused-element", Map.of("name", ctElement.getSimpleName())),
+                problemType
+            );
+        }
+    }
+
+    @Override
+    protected void check(StaticAnalysis staticAnalysis, DynamicAnalysis dynamicAnalysis) {
+        staticAnalysis.getModel().getRootPackage().accept(new CtScanner() {
+            @Override
+            public <T> void visitCtLocalVariable(CtLocalVariable<T> ctLocalVariable) {
+                checkUnused(ctLocalVariable, ctLocalVariable.getReference());
+                super.visitCtLocalVariable(ctLocalVariable);
+            }
+
+            @Override
+            public <T> void visitCtMethod(CtMethod<T> ctMethod) {
+                if (SpoonUtil.isOverriddenMethod(ctMethod) || SpoonUtil.isMainMethod(ctMethod)) {
+                    super.visitCtMethod(ctMethod);
+                    return;
+                }
+
+                checkUnused(ctMethod, ctMethod.getReference());
+                super.visitCtMethod(ctMethod);
+            }
+
+            @Override
+            public <T> void visitCtParameter(CtParameter<T> ctParameter) {
+                if (SpoonUtil.isInOverriddenMethod(ctParameter) || SpoonUtil.isInMainMethod(ctParameter)) {
+                    super.visitCtParameter(ctParameter);
+                    return;
+                }
+
+                checkUnused(ctParameter, ctParameter.getReference());
+
+                super.visitCtParameter(ctParameter);
+            }
+
+            @Override
+            public void visitCtTypeParameter(CtTypeParameter ctTypeParameter) {
+                checkUnused(ctTypeParameter, ctTypeParameter.getReference());
+                super.visitCtTypeParameter(ctTypeParameter);
+            }
+
+            @Override
+            public <T> void visitCtField(CtField<T> ctField) {
+                checkUnused(ctField, ctField.getReference());
+                super.visitCtField(ctField);
+            }
+        });
     }
 }