diff --git a/autograder-core/pom.xml b/autograder-core/pom.xml index 3085a325..3b34b80b 100644 --- a/autograder-core/pom.xml +++ b/autograder-core/pom.xml @@ -15,7 +15,7 @@ https://github.com/Feuermagier/autograder/autograder-core - 10.4.2 + 11.0.0 7.0.0-rc3 6.45.0 @@ -77,12 +77,6 @@ test - - org.eclipse.jdt - org.eclipse.jdt.core - 3.34.0 - - fr.inria.gforge.spoon @@ -93,11 +87,6 @@ org.slf4j slf4j-api - - - org.eclipse.jdt - org.eclipse.jdt.core - @@ -110,11 +99,6 @@ org.slf4j slf4j-api - - - org.eclipse.jdt - org.eclipse.jdt.core - diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/CharRange.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/CharRange.java index 1da6b6d8..debbaf76 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/CharRange.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/CharRange.java @@ -76,7 +76,7 @@ private static Optional> makeSuggestion(CtExpression fn.suggest( ctExpression.getFactory(), ctExpression, - ctExpression.getFactory().Type().CHARACTER + ctExpression.getFactory().Type().characterType() )); } diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/CollectionAddAll.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/CollectionAddAll.java index 57846689..c74bc3de 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/CollectionAddAll.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/CollectionAddAll.java @@ -39,6 +39,8 @@ ProblemType.COMMON_REIMPLEMENTATION_ADD_ALL }) public class CollectionAddAll extends IntegratedCheck { + private static final int MIN_ADD_ALL_SIZE = 4; + private record AddInvocation( CtVariableReference collection, CtExecutableReference executableReference, @@ -117,9 +119,13 @@ private void reportProblem(CtVariable ctVariable, List ctInvocation) { && target.getType().getTypeDeclaration() != null // the type with the size method must have an isEmpty method && target.getType().getTypeDeclaration().getMethod( - ctInvocation.getFactory().Type().BOOLEAN_PRIMITIVE, + ctInvocation.getFactory().Type().booleanPrimitiveType(), "isEmpty" ) != null && SpoonUtil.isSignatureEqualTo(ctInvocation.getExecutable(), int.class, "size"); diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseFormatString.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseFormatString.java index 5426b494..97e77add 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseFormatString.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseFormatString.java @@ -142,15 +142,15 @@ private CtExpression resolveExpression(CtExpression ctExpression) { && SpoonUtil.isTypeEqualTo(ctTypeAccess.getAccessedType(), java.lang.System.class) && SpoonUtil.isSignatureEqualTo( ctInvocation.getExecutable(), - typeFactory.STRING, + typeFactory.stringType(), "lineSeparator" )) { - return SpoonUtil.makeLiteral(typeFactory.STRING, "\n"); + return SpoonUtil.makeLiteral(typeFactory.stringType(), "\n"); } if (ctExpression instanceof CtLiteral ctLiteral - && SpoonUtil.areLiteralsEqual(ctLiteral, SpoonUtil.makeLiteral(typeFactory.CHARACTER_PRIMITIVE, '\n'))) { - return SpoonUtil.makeLiteral(typeFactory.STRING, "\n"); + && SpoonUtil.areLiteralsEqual(ctLiteral, SpoonUtil.makeLiteral(typeFactory.characterPrimitiveType(), '\n'))) { + return SpoonUtil.makeLiteral(typeFactory.stringType(), "\n"); } return ctExpression; diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RedundantIfForBooleanCheck.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RedundantIfForBooleanCheck.java index a275ffaf..469464cf 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RedundantIfForBooleanCheck.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RedundantIfForBooleanCheck.java @@ -17,12 +17,16 @@ import spoon.reflect.code.CtIf; import spoon.reflect.code.CtReturn; import spoon.reflect.code.CtStatement; +import spoon.reflect.declaration.CtMethod; import java.util.List; import java.util.Map; +import java.util.Set; @ExecutableCheck(reportedProblems = { ProblemType.REDUNDANT_IF_FOR_BOOLEAN }) public class RedundantIfForBooleanCheck extends IntegratedCheck { + private static final Set METHODS_TO_IGNORE = Set.of("equals", "hashCode"); + private String makeSuggestion(CtExpression ctExpression, Effect thenEffect, Effect elseEffect) { if (thenEffect instanceof AssignmentEffect thenAssignment && elseEffect instanceof AssignmentEffect) { return "%s = %s".formatted( @@ -129,8 +133,15 @@ protected void check(StaticAnalysis staticAnalysis, DynamicAnalysis dynamicAnaly staticAnalysis.processWith(new AbstractProcessor>() { @Override public void process(CtBlock block) { + CtMethod parentMethod = block.getParent(CtMethod.class); + if (parentMethod != null && METHODS_TO_IGNORE.contains(parentMethod.getSimpleName())) { + return; + } + List statements = SpoonUtil.getEffectiveStatements(block); + // TODO: write test + for (int i = 0; i < statements.size(); i++) { CtStatement statement = statements.get(i); diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RedundantNegationCheck.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RedundantNegationCheck.java index df271bfe..23dc41c8 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RedundantNegationCheck.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RedundantNegationCheck.java @@ -8,7 +8,10 @@ import de.firemage.autograder.core.integrated.SpoonUtil; import de.firemage.autograder.core.integrated.StaticAnalysis; +import de.firemage.autograder.core.integrated.evaluator.Evaluator; +import de.firemage.autograder.core.integrated.evaluator.fold.Fold; import spoon.processing.AbstractProcessor; +import spoon.reflect.code.CtBinaryOperator; import spoon.reflect.code.CtExpression; import spoon.reflect.code.CtUnaryOperator; import spoon.reflect.code.UnaryOperatorKind; @@ -17,31 +20,42 @@ @ExecutableCheck(reportedProblems = { ProblemType.REDUNDANT_NEGATION }) public class RedundantNegationCheck extends IntegratedCheck { + private record NegationFolder() implements Fold { + @Override + @SuppressWarnings("unchecked") + public CtExpression foldCtUnaryOperator(CtUnaryOperator ctUnaryOperator) { + // only check negations !(operand) + if (ctUnaryOperator.getKind() != UnaryOperatorKind.NOT) { + return ctUnaryOperator; + } + + CtExpression operand = ctUnaryOperator.getOperand(); + + // this negates the operand and optimizes it if possible + return (CtExpression) SpoonUtil.negate(operand); + } + } + @Override protected void check(StaticAnalysis staticAnalysis, DynamicAnalysis dynamicAnalysis) { - staticAnalysis.processWith(new AbstractProcessor>() { + staticAnalysis.processWith(new AbstractProcessor>() { @Override - public void process(CtUnaryOperator ctUnaryOperator) { - if (ctUnaryOperator.isImplicit() || !ctUnaryOperator.getPosition().isValidPosition()) { + public void process(CtExpression ctExpression) { + if (ctExpression.isImplicit() + || !ctExpression.getPosition().isValidPosition() + || ctExpression.getParent(CtExpression.class) != null) { return; } - // only check negations !(operand) - if (ctUnaryOperator.getKind() != UnaryOperatorKind.NOT) { - return; - } - - CtExpression operand = ctUnaryOperator.getOperand(); - // this negates the operand and optimizes it if possible - CtExpression negated = SpoonUtil.negate(operand); + CtExpression negated = new Evaluator(new NegationFolder()).evaluate(ctExpression); // if they are equal, the negation is not redundant - if (ctUnaryOperator.equals(negated)) { + if (ctExpression.equals(negated)) { return; } addLocalProblem( - ctUnaryOperator, + ctExpression, new LocalizedMessage( "common-reimplementation", Map.of("suggestion", negated.prettyprint()) diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RegexCheck.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RegexCheck.java index ba303a34..c2f0f0b1 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RegexCheck.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RegexCheck.java @@ -38,7 +38,7 @@ @ExecutableCheck(reportedProblems = {ProblemType.COMPLEX_REGEX}) public class RegexCheck extends IntegratedCheck { - private static final double MAX_ALLOWED_SCORE = 12.0; + private static final double MAX_ALLOWED_SCORE = 24.0; private static final List REGEX_HINTS = List.of("?", "<", ">", "+", "*", "[", "]", "$", "^", "|", "\\"); private static final int MIN_REGEX_HINTS = 2; diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RepeatedMathOperationCheck.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RepeatedMathOperationCheck.java index 4f266e80..59d4dbfc 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RepeatedMathOperationCheck.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RepeatedMathOperationCheck.java @@ -7,16 +7,27 @@ 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.evaluator.Evaluator; +import de.firemage.autograder.core.integrated.evaluator.fold.Fold; import spoon.processing.AbstractProcessor; import spoon.reflect.code.BinaryOperatorKind; import spoon.reflect.code.CtBinaryOperator; import spoon.reflect.code.CtExpression; import spoon.reflect.code.CtFieldRead; import spoon.reflect.code.CtVariableRead; +import spoon.reflect.code.UnaryOperatorKind; +import spoon.reflect.declaration.CtElement; +import spoon.reflect.factory.TypeFactory; import spoon.reflect.reference.CtVariableReference; -import java.util.Comparator; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.List; import java.util.Map; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.function.BiFunction; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -25,54 +36,156 @@ public class RepeatedMathOperationCheck extends IntegratedCheck { private static final Map OCCURRENCE_THRESHOLDS = Map.of(BinaryOperatorKind.PLUS, 2, BinaryOperatorKind.MUL, 3); - private record Variable(CtVariableReference ctVariableReference, CtExpression target) {} + private record Variable(CtVariableReference ctVariableReference, CtExpression target) { + } + + + private static List> splitOperator(CtBinaryOperator ctBinaryOperator, BinaryOperatorKind kind) { + List> result = new ArrayList<>(); + + if (ctBinaryOperator.getKind() != kind) { + return new ArrayList<>(List.of(ctBinaryOperator)); + } + + CtExpression left = ctBinaryOperator.getLeftHandOperand(); + CtExpression right = ctBinaryOperator.getRightHandOperand(); + + // The right hand side can also be a binary operator, e.g. a + (b + c) + if (right instanceof CtBinaryOperator rightOperator) { + List> rightOperands = splitOperator(rightOperator, kind); + // the right operands have to be added in reverse order + Collections.reverse(rightOperands); + result.addAll(rightOperands); + } else { + result.add(right); + } + + while (left instanceof CtBinaryOperator lhs && lhs.getKind() == kind) { + result.add(lhs.getRightHandOperand()); + left = lhs.getLeftHandOperand(); + } + + result.add(left); + + Collections.reverse(result); + + return result; + } + + /** + * This class optimizes repeated operations like `a + a + a + a` to `a * 4`. + */ + private record OperatorFolder(BinaryOperatorKind kind, int threshold, BiFunction, Integer, CtExpression> function) implements Fold { + @Override + public CtElement enter(CtElement ctElement) { + return this.fold(ctElement); + } + + @Override + public CtElement exit(CtElement ctElement) { + return ctElement; + } + + + @Override + @SuppressWarnings("unchecked") + public CtExpression foldCtBinaryOperator(CtBinaryOperator ctBinaryOperator) { + // skip if the operator is not supported + if (!OCCURRENCE_THRESHOLDS.containsKey(ctBinaryOperator.getKind()) || + !SpoonUtil.isPrimitiveNumeric(ctBinaryOperator.getType())) { + return ctBinaryOperator; + } + + List> operands = splitOperator(ctBinaryOperator, this.kind); + + Map, Integer> occurrences = operands.stream() + .collect(Collectors.toMap(o -> o, o -> 1, Integer::sum, LinkedHashMap::new)); + + // reconstruct the binary operator (note: this will destroy the original parentheses) + return (CtExpression) occurrences.entrySet() + .stream() + .map(entry -> { + var expression = entry.getKey(); + int count = entry.getValue(); + + if (count < this.threshold) { + return repeatExpression(this.kind, expression, count); + } else { + return this.function.apply(expression, count); + } + }) + .reduce((left, right) -> SpoonUtil.createBinaryOperator(left, right, this.kind)) + .orElseThrow(); + } + } + + + public static CtExpression repeatExpression(BinaryOperatorKind kind, CtExpression expression, int count) { + CtExpression[] array = new CtExpression[count - 1]; + Arrays.fill(array, expression); + return joinExpressions(kind, expression, array); + } + + public static CtExpression joinExpressions(BinaryOperatorKind kind, CtExpression first, CtExpression... others) { + return Arrays.stream(others).reduce(first, (left, right) -> SpoonUtil.createBinaryOperator(left, right, kind)); + } @Override protected void check(StaticAnalysis staticAnalysis, DynamicAnalysis dynamicAnalysis) { - staticAnalysis.processWith(new AbstractProcessor>() { + staticAnalysis.processWith(new AbstractProcessor>() { @Override - public void process(CtBinaryOperator operator) { - if (!OCCURRENCE_THRESHOLDS.containsKey(operator.getKind())) { + public void process(CtExpression ctExpression) { + if (ctExpression.isImplicit() + || !ctExpression.getPosition().isValidPosition() + // we only want to look at top level expressions: + || ctExpression.getParent(CtExpression.class) != null) { return; } - // Only look at the top statement - if (operator.getParent() instanceof CtBinaryOperator parent && - parent.getKind() == operator.getKind()) { - return; - } - - var occurrences = countOccurrences(operator, operator.getKind()); - - var optionalVariable = occurrences.entrySet().stream() - .filter(e -> e.getValue() >= OCCURRENCE_THRESHOLDS.get(operator.getKind())) - .max(Comparator.comparingInt(Map.Entry::getValue)); - - optionalVariable.ifPresent(ctVariableReferenceIntegerEntry -> { - Variable variable = ctVariableReferenceIntegerEntry.getKey(); - String variableName = "%s".formatted(variable.ctVariableReference().getSimpleName()); - if (variable.target() != null) { - variableName = "%s.%s".formatted( - variable.target().prettyprint(), - variable.ctVariableReference().getSimpleName() + AtomicInteger plusOptimizations = new AtomicInteger(); + AtomicInteger mulOptimizations = new AtomicInteger(); + + Fold plusFolder = new OperatorFolder( + BinaryOperatorKind.PLUS, + OCCURRENCE_THRESHOLDS.get(BinaryOperatorKind.PLUS), + (expression, count) -> { + plusOptimizations.addAndGet(1); + return SpoonUtil.createBinaryOperator( + expression, + SpoonUtil.makeLiteralNumber(expression.getType(), count), + BinaryOperatorKind.MUL ); } - - int count = ctVariableReferenceIntegerEntry.getValue(); - String suggestion = "%s * %d".formatted(variableName, count); - if (operator.getKind() == BinaryOperatorKind.MUL) { - suggestion = "Math.pow(%s, %d)".formatted(variableName, count); + ); + + Fold mulFolder = new OperatorFolder( + BinaryOperatorKind.MUL, + OCCURRENCE_THRESHOLDS.get(BinaryOperatorKind.MUL), + (expression, count) -> { + TypeFactory typeFactory = expression.getFactory().Type(); + mulOptimizations.addAndGet(1); + return SpoonUtil.createStaticInvocation( + typeFactory.get(java.lang.Math.class).getReference(), + "pow", + expression, + SpoonUtil.makeLiteralNumber(typeFactory.integerPrimitiveType(), count) + ); } + ); + CtExpression suggestion = new Evaluator(plusFolder).evaluate(ctExpression); + suggestion = new Evaluator(mulFolder).evaluate(suggestion); + + if (plusOptimizations.get() > 0 || mulOptimizations.get() > 0) { addLocalProblem( - operator, + ctExpression, new LocalizedMessage( "common-reimplementation", Map.of("suggestion", suggestion) ), ProblemType.REPEATED_MATH_OPERATION ); - }); + } } }); } diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/naming/BooleanIdentifierCheck.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/naming/BooleanIdentifierCheck.java index f81dfba3..062246be 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/naming/BooleanIdentifierCheck.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/naming/BooleanIdentifierCheck.java @@ -23,7 +23,7 @@ public void process(CtMethod ctMethod) { } String methodName = ctMethod.getSimpleName(); - if (ctMethod.getType().equals(ctMethod.getFactory().Type().createReference(boolean.class)) + if (ctMethod.getType().equals(ctMethod.getFactory().Type().booleanPrimitiveType()) && methodName.startsWith("get")) { String newName = "is" + methodName.substring(3); addLocalProblem( 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 830504f8..8aa61b32 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 @@ -201,7 +201,7 @@ public void process(CtField field) { && ctInvocation.getTarget() instanceof CtTypeAccess ctTypeAccess && SpoonUtil.isTypeEqualTo(ctTypeAccess.getAccessedType(), java.lang.System.class) && SpoonUtil.isSignatureEqualTo(ctInvocation.getExecutable(), String.class, "lineSeparator")) { - literal = SpoonUtil.makeLiteral(field.getFactory().Type().STRING, "\n"); + literal = SpoonUtil.makeLiteral(field.getFactory().Type().stringType(), "\n"); } else { return; } diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/naming/LinguisticNamingCheck.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/naming/LinguisticNamingCheck.java index 5dccb2e4..e66c1275 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/naming/LinguisticNamingCheck.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/naming/LinguisticNamingCheck.java @@ -24,6 +24,7 @@ @ExecutableCheck(reportedProblems = { ProblemType.CONFUSING_IDENTIFIER }) public class LinguisticNamingCheck extends IntegratedCheck { + private static final Set IGNORE_VARIABLES_WITH = Set.of("regex", "pattern"); private static final Set COMMON_BOOLEAN_GETTER_PREFIXES = Set.of( "is", "are", "can", "could", "must", "has", "have", "does", "will", "should", "would", "takes", "looks", "uses", "finds" @@ -55,6 +56,10 @@ private void reportProblem(String key, CtNamedElement ctNamedElement, Map void checkCtVariable(CtVariable ctVariable) { + if (IGNORE_VARIABLES_WITH.stream().anyMatch(s -> ctVariable.getSimpleName().toLowerCase().contains(s))) { + return; + } + if (hasBooleanPrefix(ctVariable) && !SpoonUtil.isBoolean(ctVariable)) { this.reportProblem( "linguistic-naming-boolean", 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 67937cfa..312ab4ea 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 @@ -194,6 +194,13 @@ public static boolean areLiteralsEqual( return valLeft.longValue() == valRight.longValue(); } + @SuppressWarnings("unchecked") + public static CtLiteral makeLiteralNumber(CtTypeReference ctTypeReference, Number number) { + Object value = FoldUtils.convert(ctTypeReference, number); + + return SpoonUtil.makeLiteral(ctTypeReference, (T) value); + } + /** * Makes a new literal with the given value and type. * @@ -360,7 +367,7 @@ private static CtBinaryOperator normalize(CtBinaryOperator ctBinaryOpe if (isCharacter.test(ctBinaryOperator.getRightHandOperand().getType())) { // for character use an integer literal step.setValue((char) 1); - step.setType(ctBinaryOperator.getFactory().Type().CHARACTER_PRIMITIVE); + step.setType(ctBinaryOperator.getFactory().Type().characterPrimitiveType()); } else { // this assumes that < and > are only used with numbers step.setValue(FoldUtils.convert(ctBinaryOperator.getRightHandOperand().getType(), ((Number) 1).doubleValue())); @@ -733,10 +740,17 @@ public static CtInvocation createStaticInvocation( CtExpression... parameters ) { Factory factory = targetType.getFactory(); - CtMethod methodHandle = targetType.getTypeDeclaration().getMethod( - methodName, - Arrays.stream(parameters).map(SpoonUtil::getExpressionType).toArray(CtTypeReference[]::new) - ); + + CtMethod methodHandle = null; + List> potentialMethods = targetType.getTypeDeclaration().getMethodsByName(methodName); + if (potentialMethods.size() == 1) { + methodHandle = (CtMethod) potentialMethods.get(0); + } else { + methodHandle = targetType.getTypeDeclaration().getMethod( + methodName, + Arrays.stream(parameters).map(SpoonUtil::getExpressionType).toArray(CtTypeReference[]::new) + ); + } return factory.createInvocation( factory.createTypeAccess(methodHandle.getDeclaringType().getReference()), diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/evaluator/OperatorHelper.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/evaluator/OperatorHelper.java index dcf083fb..30f24d97 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/evaluator/OperatorHelper.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/evaluator/OperatorHelper.java @@ -108,7 +108,7 @@ private static Optional> unaryNumericPromotion(CtTypeReferenc // if the operand is of type byte, short, or char, it is promoted to a value of type int by a widening // primitive conversion (§5.1.2). if (NUMBERS_PROMOTED_TO_INT.contains(operandType.getActualClass())) { - return Optional.of(operandType.getFactory().Type().INTEGER_PRIMITIVE); + return Optional.of(operandType.getFactory().Type().integerPrimitiveType()); } // otherwise, the operand is not converted at all. @@ -129,26 +129,26 @@ private static Optional> binaryNumericPromotion( return Optional.empty(); } - CtTypeReference doubleType = typeFactory.DOUBLE_PRIMITIVE; + CtTypeReference doubleType = typeFactory.doublePrimitiveType(); // If either operand is of type double, the other is converted to double. if (leftType.equals(doubleType) || rightType.equals(doubleType)) { return Optional.of(doubleType); } // Otherwise, if either operand is of type float, the other is converted to float. - CtTypeReference floatType = typeFactory.FLOAT_PRIMITIVE; + CtTypeReference floatType = typeFactory.floatPrimitiveType(); if (leftType.equals(floatType) || rightType.equals(floatType)) { return Optional.of(floatType); } // Otherwise, if either operand is of type long, the other is converted to long. - CtTypeReference longType = typeFactory.LONG_PRIMITIVE; + CtTypeReference longType = typeFactory.longPrimitiveType(); if (leftType.equals(longType) || rightType.equals(longType)) { return Optional.of(longType); } // Otherwise, both operands are converted to type int. - return Optional.of(typeFactory.INTEGER_PRIMITIVE); + return Optional.of(typeFactory.integerPrimitiveType()); } /** @@ -181,7 +181,7 @@ public static Optional> getPromotedType( switch (operator) { // logical operators case AND, OR -> { - CtTypeReference booleanType = typeFactory.BOOLEAN_PRIMITIVE; + CtTypeReference booleanType = typeFactory.booleanPrimitiveType(); if (!leftType.equals(booleanType) || !rightType.equals(booleanType)) { return Optional.empty(); } @@ -222,7 +222,7 @@ public static Optional> getPromotedType( // The equality operators may be used to compare two operands that are convertible (§5.1.8) // to numeric type, or two operands of type boolean or Boolean, or two operands that are each // of either reference type or the null type. All other cases result in a compile-time error. - CtTypeReference booleanType = typeFactory.BOOLEAN_PRIMITIVE; + CtTypeReference booleanType = typeFactory.booleanPrimitiveType(); return binaryNumericPromotion(leftType, rightType).or(() -> { // check if both operands are of type boolean or Boolean // if so they will be promoted to the primitive type boolean @@ -236,7 +236,7 @@ public static Optional> getPromotedType( // either operand to the type of the other by a casting conversion (§5.5). // The run-time values of the two operands would necessarily be unequal // (ignoring the case where both values are null). - CtTypeReference nullType = typeFactory.NULL_TYPE; + CtTypeReference nullType = typeFactory.nullType(); if (unboxedLeftType.equals(nullType)) { return Optional.of(unboxedRightType); } @@ -269,7 +269,7 @@ public static Optional> getPromotedType( // // If the type of either operand of a + operator is String, then the operation is // string concatenation. - CtTypeReference stringType = typeFactory.STRING; + CtTypeReference stringType = typeFactory.stringType(); if (leftType.equals(stringType) || rightType.equals(stringType)) { return Optional.of(stringType); } @@ -283,14 +283,14 @@ public static Optional> getPromotedType( CtTypeReference unboxedRightType = rightType.unbox(); Set> floatingPointNumbers = Set.of( - typeFactory.FLOAT_PRIMITIVE, - typeFactory.DOUBLE_PRIMITIVE + typeFactory.floatPrimitiveType(), + typeFactory.doublePrimitiveType() ); if (floatingPointNumbers.contains(unboxedLeftType) || floatingPointNumbers.contains(unboxedRightType)) { return Optional.empty(); } - if (unboxedLeftType.equals(unboxedRightType) && unboxedLeftType.equals(typeFactory.BOOLEAN_PRIMITIVE)) { + if (unboxedLeftType.equals(unboxedRightType) && unboxedLeftType.equals(typeFactory.booleanPrimitiveType())) { return Optional.of(unboxedLeftType); } @@ -336,8 +336,8 @@ public static Optional> getPromotedType( // See: https://docs.oracle.com/javase/specs/jls/se11/html/jls-15.html#jls-15.15.3 unaryNumericPromotion(operandType); case NOT -> { - if (operandType.unbox().equals(typeFactory.BOOLEAN_PRIMITIVE)) { - yield Optional.of(typeFactory.BOOLEAN_PRIMITIVE); + if (operandType.unbox().equals(typeFactory.booleanPrimitiveType())) { + yield Optional.of(typeFactory.booleanPrimitiveType()); } yield Optional.empty(); } diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/evaluator/fold/FoldUtils.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/evaluator/fold/FoldUtils.java index 2b9380b5..b9c6b6d2 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/evaluator/fold/FoldUtils.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/evaluator/fold/FoldUtils.java @@ -62,7 +62,7 @@ private static CtExpression inferTypeIfNeeded(CtExpression ctExpressio public static CtTypeReference inferType(CtBinaryOperator ctBinaryOperator) { return switch (ctBinaryOperator.getKind()) { - case AND, OR, INSTANCEOF, EQ, NE, LT, LE, GT, GE -> ctBinaryOperator.getFactory().Type().BOOLEAN_PRIMITIVE; + case AND, OR, INSTANCEOF, EQ, NE, LT, LE, GT, GE -> ctBinaryOperator.getFactory().Type().booleanPrimitiveType(); case SL, SR, USR, MUL, DIV, MOD, MINUS, PLUS, BITAND, BITXOR, BITOR -> OperatorHelper.getPromotedType( ctBinaryOperator.getKind(), inferTypeIfNeeded(ctBinaryOperator.getLeftHandOperand()), diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestCollectionAddAll.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestCollectionAddAll.java index c798cee6..dd9f50d1 100644 --- a/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestCollectionAddAll.java +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestCollectionAddAll.java @@ -48,12 +48,13 @@ public void foo(List list) { list.add(" "); list.add("a"); list.add("b"); + list.add("c"); } } """ ), PROBLEM_TYPES); - assertEqualsReimplementation(problems.next(), "list.addAll(List.of(\" \", \"a\", \"b\"))"); + assertEqualsReimplementation(problems.next(), "list.addAll(List.of(\" \", \"a\", \"b\", \"c\"))"); problems.assertExhausted(); } diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestRedundantIfForBooleanCheck.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestRedundantIfForBooleanCheck.java index eb5062fb..5cc39d12 100644 --- a/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestRedundantIfForBooleanCheck.java +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestRedundantIfForBooleanCheck.java @@ -307,6 +307,32 @@ boolean doB(int a) { problems.assertExhausted(); } + + @Test + void testInEquals() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + int a; + + @Override + public boolean equals(Object other) { + if (a == 0) { + return a == 1; + } + + return a == 2; + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test void testIgnoreInvalidIfWithValidElseIfElse() throws IOException, LinterException { ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestRedundantNegationCheck.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestRedundantNegationCheck.java index 2c5627df..45c8a219 100644 --- a/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestRedundantNegationCheck.java +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestRedundantNegationCheck.java @@ -53,7 +53,7 @@ void testRedundantNegation(String expression, String arguments, String expected) ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( JavaVersion.JAVA_17, "Test", - "public class Test { public void foo(%s) { System.out.println(%s); } }".formatted( + "public class Test { public void foo(%s) { var value = %s; } }".formatted( arguments, expression ) @@ -84,4 +84,27 @@ public void foo(List list) { problems.assertExhausted(); } + + @Test + void testNestedUnnecessaryNegation() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + import java.util.List; + + public class Test { + public void foo(int a, int b, int c) { + if (a == 0 || a == 1 && !(b == 0 || b == 1)) { + System.out.println("Hello"); + } + } + } + """ + ), PROBLEM_TYPES); + + assertEqualsRedundant(problems.next(), "(a == 0) || ((a == 1) && ((b != 0) && (b != 1)))"); + + problems.assertExhausted(); + } } diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestRepeatedMathOperationCheck.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestRepeatedMathOperationCheck.java index 52974d7c..2d00521a 100644 --- a/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestRepeatedMathOperationCheck.java +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestRepeatedMathOperationCheck.java @@ -59,6 +59,70 @@ public static void main(String[] args) { problems.assertExhausted(); } + @Test + void testMinimumThresholdPlus() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static void main(String[] args) { + int[] array = new int[10]; + int[] array2 = new int[10]; + + int a = 4; + int b = 3; + + int c = a + a; + int d = b + b + b; + int e = array.length + array.length + array.length + array.length; + + int f = array2.length + array2.length + array2.length + array2.length + array2.length; + } + } + """ + ), PROBLEM_TYPES); + + assertEqualsRepeat("a * 2", problems.next()); + assertEqualsRepeat("b * 3", problems.next()); + assertEqualsRepeat("array.length * 4", problems.next()); + assertEqualsRepeat("array2.length * 5", problems.next()); + + problems.assertExhausted(); + } + + @Test + void testMinimumThresholdMul() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static void main(String[] args) { + int[] array = new int[10]; + int[] array2 = new int[10]; + + int a = 4; + int b = 3; + + int c = a * a; + int d = b * b * b; + int e = array.length * array.length * array.length * array.length; + + int f = array2.length * array2.length * array2.length * array2.length * array2.length; + } + } + """ + ), PROBLEM_TYPES); + + assertEqualsRepeat("Math.pow(b, 3)", problems.next()); + assertEqualsRepeat("Math.pow(array.length, 4)", problems.next()); + assertEqualsRepeat("Math.pow(array2.length, 5)", problems.next()); + + problems.assertExhausted(); + } + + @Test void testFalsePositiveFieldAccess() throws IOException, LinterException { ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( @@ -77,4 +141,45 @@ public static void main(String[] args) { problems.assertExhausted(); } + + @Test + void testRecursiveSuggestion() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static void main(String[] args) { + int index = args.length; + + System.out.println(args[index + index + 1]); + int b = index * 2; + System.out.println(args[(index + index) + (b + b + b)]); + } + } + """ + ), PROBLEM_TYPES); + assertEqualsRepeat("System.out.println(args[index * 2 + 1])", problems.next()); + assertEqualsRepeat("System.out.println(args[index * 2 + b * 3])", problems.next()); + + problems.assertExhausted(); + } + + @Test + void testMulAndPlusComplexOptimization() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static int test(int a, int b, int c, int d) { + return a + a + (a + b - c) + 1 + d * a * a * (b * b * (a * a)); + } + } + """ + ), PROBLEM_TYPES); + assertEqualsRepeat("a * 2 + (a + b - c) + 1 + d * Math.pow(a, 4) * (b * b)", problems.next()); + + problems.assertExhausted(); + } } diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/naming/TestLinguisticNamingCheck.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/naming/TestLinguisticNamingCheck.java index c6c9d1cd..f0fa672f 100644 --- a/autograder-core/src/test/java/de/firemage/autograder/core/check/naming/TestLinguisticNamingCheck.java +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/naming/TestLinguisticNamingCheck.java @@ -161,4 +161,20 @@ public boolean isInvalidValue(String value) { problems.assertExhausted(); } + + @Test + void testVariablesToIgnore() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + private static final String HAS_VALUE_REGEX = "value"; // ok + private static final String HAS_VALUE_PATTERN = "value"; // ok + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } } diff --git a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/Regex/code/Test.java b/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/Regex/code/Test.java index a5e156b0..9a220f4e 100644 --- a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/Regex/code/Test.java +++ b/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/Regex/code/Test.java @@ -4,8 +4,8 @@ public class Test { private String noRegex = "Should we do this? I guess we shouldn't! f*ck you!"; - private String regex1 = "(foo)* [bar]+ x? x?"; /*# not ok #*/ - private String regex2 = "(?foo)"; /*# not ok #*/ + private String regex1 = "(foo)* [bar]+ x? x?"; /*# ok #*/ + private String regex2 = "(?foo)"; /*# ok #*/ private String regex3 = "^[a-z]+(?: \\S+)?$"; /*# not ok #*/ private String regex4 = "^(?\\d+)-->(?\\d+):(?\\d+)m,(?\\d+)x,(?\\d+)max$"; /*# not ok #*/ private String regex5 = "^(?\\d+),(?\\d+),(?\\d+),(?\\d+)$"; /*# not ok #*/ @@ -43,14 +43,14 @@ private void foo() { // test that context of regex is considered class RegexContext { - private static final String DEFINITELY_REGEX_1 = "(foo)* [bar]+ x? x?"; /*# not ok #*/ - private static final String DEFINITELY_REGEX_2 = "(foo)* [bar]+ x? x?"; /*# not ok #*/ - private static final String DEFINITELY_REGEX_3 = "(foo)* [bar]+ x? x?"; /*# not ok #*/ - private static final String DEFINITELY_REGEX_4 = "(foo)* [bar]+ x? x?"; /*# not ok #*/ - private static final String DEFINITELY_REGEX_5 = "(foo)* [bar]+ x? x?"; /*# not ok #*/ - private static final String DEFINITELY_REGEX_6 = "(foo)* [bar]+ x? x?"; /*# not ok #*/ - private static final String UNUSED_REGEX = "(foo)* [bar]+ x? x?"; /*# ok #*/ - private static final String NOT_USED_AS_REGEX = "(foo)* [bar]+ x? x?"; /*# ok #*/ + private static final String DEFINITELY_REGEX_1 = "^[a-z]+(?: \\S+)?$"; /*# not ok #*/ + private static final String DEFINITELY_REGEX_2 = "^[a-z]+(?: \\S+)?$"; /*# not ok #*/ + private static final String DEFINITELY_REGEX_3 = "^[a-z]+(?: \\S+)?$"; /*# not ok #*/ + private static final String DEFINITELY_REGEX_4 = "^[a-z]+(?: \\S+)?$"; /*# not ok #*/ + private static final String DEFINITELY_REGEX_5 = "^[a-z]+(?: \\S+)?$"; /*# not ok #*/ + private static final String DEFINITELY_REGEX_6 = "^[a-z]+(?: \\S+)?$"; /*# not ok #*/ + private static final String UNUSED_REGEX = "^[a-z]+(?: \\S+)?$"; /*# ok #*/ + private static final String NOT_USED_AS_REGEX = "^[a-z]+(?: \\S+)?$"; /*# ok #*/ private static final Pattern SYMBOL_REGEX = Pattern.compile("[0-9a-zA-Z]*"); /*# ok #*/ diff --git a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/Regex/config.txt b/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/Regex/config.txt index 9f9b1df5..933181b2 100644 --- a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/Regex/config.txt +++ b/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/Regex/config.txt @@ -1,7 +1,5 @@ complexity.RegexCheck Complex regex -Test.java:7 -Test.java:8 Test.java:9 Test.java:10 Test.java:11