Skip to content

Commit

Permalink
Merge pull request #597 from Luro02/main
Browse files Browse the repository at this point in the history 
Implement some things
  • Loading branch information
@Luro02
Luro02 authored Sep 24, 2024
2 parents bd4f739 + cdc6b6e commit eebee71
Show file tree
Hide file tree
Showing 20 changed files with 814 additions and 176 deletions.
5 changes: 5 additions & 0 deletions autograder-api/src/main/java/de/firemage/autograder/api/AbstractProblem.java
View file
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
package de.firemage.autograder.api;

import java.util.Optional;

public interface AbstractProblem {
String getCheckName();
Translatable getLinterName();
Translatable getExplanation();
String getDisplayLocation();
AbstractCodePosition getPosition();
String getType();
default Optional<Integer> getMaximumProblemsForCheck() {
return Optional.empty();
}
}
7 changes: 7 additions & 0 deletions autograder-core/src/main/java/de/firemage/autograder/core/Problem.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
import de.firemage.autograder.api.Translatable;
import de.firemage.autograder.core.check.Check;

import java.util.Optional;

/**
* Contains the default implementation of most {@link AbstractProblem} methods.
*/
Expand Down Expand Up @@ -63,6 +65,11 @@ public String getType() {
return this.problemType.toString();
}

@Override
public Optional<Integer> getMaximumProblemsForCheck() {
return this.getCheck().maximumProblems();
}

public ProblemType getProblemType() {
return problemType;
}
Expand Down
6 changes: 6 additions & 0 deletions autograder-core/src/main/java/de/firemage/autograder/core/ProblemType.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1030,6 +1030,12 @@ public enum ProblemType implements AbstractProblemType {
@HasFalsePositives
AVOID_STRING_CONCAT,

/**
* Reports code where {@link Character#isLetterOrDigit(char)} could be used.
*/
@HasFalsePositives
IS_LETTER_OR_DIGIT,

/**
* Reports unnecessary comments.
*/
Expand Down
119 changes: 98 additions & 21 deletions ...der-core/src/main/java/de/firemage/autograder/core/check/api/CheckIterableDuplicates.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@
import spoon.reflect.code.CtVariableRead;
import spoon.reflect.code.UnaryOperatorKind;
import spoon.reflect.reference.CtTypeReference;
import spoon.reflect.reference.CtVariableReference;

import java.util.ArrayList;
import java.util.List;
import java.util.Map;

Expand All @@ -45,6 +47,24 @@ private static String buildSuggestion(CtExpression<?> ctExpression, boolean isNe
return "new HashSet<>(%s).size() == %s".formatted(leftSide, rightSide);
}

private static <T> boolean isAddInvocationOnSet(CtInvocation<T> ctInvocation, CtVariableReference<?> argument) {
return TypeUtil.isTypeEqualTo(ctInvocation.getExecutable().getType(), boolean.class)
&& ctInvocation.getExecutable().getSimpleName().equals("add")
&& ctInvocation.getArguments().size() == 1
&& ctInvocation.getArguments().get(0) instanceof CtVariableRead<?> ctVariableRead
&& ctVariableRead.getVariable().equals(argument)
&& TypeUtil.isSubtypeOf(ctInvocation.getTarget().getType(), java.util.Set.class);
}

private static <T> boolean isContainsInvocationOnSet(CtInvocation<T> ctInvocation, CtVariableReference<?> argument) {
return TypeUtil.isTypeEqualTo(ctInvocation.getExecutable().getType(), boolean.class)
&& ctInvocation.getExecutable().getSimpleName().equals("contains")
&& ctInvocation.getArguments().size() == 1
&& ctInvocation.getArguments().get(0) instanceof CtVariableRead<?> ctVariableRead
&& ctVariableRead.getVariable().equals(argument)
&& TypeUtil.isSubtypeOf(ctInvocation.getTarget().getType(), java.util.Set.class);
}

@Override
protected void check(StaticAnalysis staticAnalysis) {
staticAnalysis.processWith(new AbstractProcessor<CtForEach>() {
Expand All @@ -55,34 +75,75 @@ public void process(CtForEach ctForEach) {
}

List<CtStatement> statements = StatementUtil.getEffectiveStatements(ctForEach.getBody());
if (statements.size() != 1 || !(statements.get(0) instanceof CtIf ctIf)) {
if (statements.isEmpty() || !(statements.get(0) instanceof CtIf ctIf) || ctIf.getThenStatement() == null) {
return;
}

// the if should only have a then statement
if (ctIf.getElseStatement() != null || ctIf.getThenStatement() == null) {
return;
}
// one can implement this in multiple ways, for example:
// for (var elem : list) {
// if (!set.add(elem)) {
// return false;
// }
// }
// or one could have a contains and then an add statement:
// for (var elem : list) {
// if (set.contains(elem)) {
// return false;
// }
// set.add(elem);
// }

List<CtStatement> ifStatements = StatementUtil.getEffectiveStatements(ctIf.getThenStatement());
if (ifStatements.isEmpty()) {
return;
}

CtLiteral<?> effectValue = null;
if (ifStatements.size() == 1
&& ifStatements.get(0) instanceof CtReturn<?> ctReturn
&& ctReturn.getReturnedExpression() instanceof CtLiteral<?> ctLiteral) {
effectValue = ctLiteral;
if ((ctIf.getElseStatement() != null || statements.size() == 2)
&& ctIf.getCondition() instanceof CtInvocation<?> ctInvocation
&& isContainsInvocationOnSet(ctInvocation, ctForEach.getVariable().getReference())) {
// it invokes contains, so the else must have the add invocation:

List<CtStatement> elseStatements = new ArrayList<>();
if (statements.size() == 2) {
elseStatements.add(statements.get(1));
} else {
elseStatements = StatementUtil.getEffectiveStatements(ctIf.getElseStatement());
}

CtLiteral<?> effectValue = getEffectValue(ifStatements);
if (effectValue != null
&& effectValue.getValue() instanceof Boolean value
&& elseStatements.size() == 1
&& elseStatements.get(0) instanceof CtInvocation<?> ctElseInvocation
&& isAddInvocationOnSet(ctElseInvocation, ctForEach.getVariable().getReference())) {
String suggestion = buildSuggestion(ctForEach.getExpression(), Boolean.TRUE.equals(value));

addLocalProblem(
ctForEach,
new LocalizedMessage(
"common-reimplementation",
Map.of(
"suggestion", suggestion
)
),
ProblemType.COMMON_REIMPLEMENTATION_ITERABLE_DUPLICATES
);
return;
}
}

if (ifStatements.size() == 2
&& ifStatements.get(0) instanceof CtAssignment<?,?> ctAssignment
&& ctAssignment.getAssignment() instanceof CtLiteral<?> ctLiteral
&& ifStatements.get(1) instanceof CtBreak) {
effectValue = ctLiteral;
if (statements.size() != 1) {
return;
}


// the if should only have a then statement
if (ctIf.getElseStatement() != null) {
return;
}

CtLiteral<?> effectValue = getEffectValue(ifStatements);

if (effectValue == null || !(effectValue.getValue() instanceof Boolean value)) {
return;
}
Expand All @@ -93,12 +154,7 @@ public void process(CtForEach ctForEach) {
if (!(ctIf.getCondition() instanceof CtUnaryOperator<Boolean> ctUnaryOperator
&& ctUnaryOperator.getKind() == UnaryOperatorKind.NOT
&& ctUnaryOperator.getOperand() instanceof CtInvocation<?> ctInvocation
&& TypeUtil.isTypeEqualTo(ctInvocation.getExecutable().getType(), boolean.class)
&& ctInvocation.getExecutable().getSimpleName().equals("add")
&& ctInvocation.getArguments().size() == 1
&& ctInvocation.getArguments().get(0) instanceof CtVariableRead<?> ctVariableRead
&& ctVariableRead.getVariable().equals(ctForEach.getVariable().getReference())
&& TypeUtil.isSubtypeOf(ctInvocation.getTarget().getType(), java.util.Set.class)))
&& isAddInvocationOnSet(ctInvocation, ctForEach.getVariable().getReference())))
{
return;
}
Expand All @@ -118,4 +174,25 @@ public void process(CtForEach ctForEach) {
}
});
}

// this extracts the return value from the statements, depending on where it is used it could be a
// return <value>
// or
// <variable> = <value>; break; (this would be used in a loop)
private static CtLiteral<?> getEffectValue(List<CtStatement> ifStatements) {
CtLiteral<?> effectValue = null;
if (ifStatements.size() == 1
&& ifStatements.get(0) instanceof CtReturn<?> ctReturn
&& ctReturn.getReturnedExpression() instanceof CtLiteral<?> ctLiteral) {
effectValue = ctLiteral;
}

if (ifStatements.size() == 2
&& ifStatements.get(0) instanceof CtAssignment<?,?> ctAssignment
&& ctAssignment.getAssignment() instanceof CtLiteral<?> ctLiteral
&& ifStatements.get(1) instanceof CtBreak) {
effectValue = ctLiteral;
}
return effectValue;
}
}
50 changes: 33 additions & 17 deletions ...ader-core/src/main/java/de/firemage/autograder/core/check/api/ForLoopCanBeInvocation.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import de.firemage.autograder.core.integrated.MethodUtil;
import de.firemage.autograder.core.integrated.TypeUtil;
import spoon.processing.AbstractProcessor;
import spoon.reflect.code.CtExpression;
import spoon.reflect.code.CtForEach;
import spoon.reflect.code.CtInvocation;
import spoon.reflect.code.CtStatement;
Expand All @@ -20,6 +21,13 @@

@ExecutableCheck(reportedProblems = { ProblemType.FOR_LOOP_CAN_BE_INVOCATION })
public class ForLoopCanBeInvocation extends IntegratedCheck {
static <T> boolean isCollectionAddInvocation(CtInvocation<T> ctInvocation) {
return ctInvocation.getTarget() != null
&& TypeUtil.isSubtypeOf(ctInvocation.getTarget().getType(), java.util.Collection.class)
&& MethodUtil.isSignatureEqualTo(ctInvocation.getExecutable(), boolean.class, "add", Object.class)
&& ctInvocation.getExecutable().getParameters().size() == 1;
}

@Override
protected void check(StaticAnalysis staticAnalysis) {
staticAnalysis.processWith(new AbstractProcessor<CtForEach>() {
Expand All @@ -34,28 +42,36 @@ public void process(CtForEach ctFor) {
return;
}

if (statements.get(0) instanceof CtInvocation<?> ctInvocation
&& TypeUtil.isSubtypeOf(ctInvocation.getTarget().getType(), java.util.Collection.class)
&& MethodUtil.isSignatureEqualTo(ctInvocation.getExecutable(), boolean.class, "add", Object.class)
&& ctInvocation.getExecutable().getParameters().size() == 1
// ensure that the add argument simply accesses the for variable:
// the body must be a single invocation of the add method on a collection
if (!(statements.get(0) instanceof CtInvocation<?> ctInvocation)
|| !isCollectionAddInvocation(ctInvocation)) {
return;
}

CtExpression<?> addArgument = ctInvocation.getArguments().get(0);
// allow explicit casting, for example you might do:
// for (int i : array) {
// collection.add((short) i);
// }
// which could not be replaced with a simple addAll invocation
if (!addArgument.getTypeCasts().isEmpty()) {
return;
}

// handle edge case where the variable is implicitly cast in the invocation (Character in List, but char in iterable)
List<CtTypeReference<?>> actualTypeArguments = ctInvocation.getTarget().getType().getActualTypeArguments();
if (!actualTypeArguments.isEmpty() && !ctFor.getVariable().getType().equals(actualTypeArguments.get(0))) {
return;
}

if (// ensure that the add argument simply accesses the for variable:
// for (T t : array) {
// collection.add(t);
// }
&& ctInvocation.getArguments().get(0) instanceof CtVariableRead<?> ctVariableRead
addArgument instanceof CtVariableRead<?> ctVariableRead
&& ctVariableRead.getVariable().equals(ctFor.getVariable().getReference())) {

// allow explicit casting
if (!ctInvocation.getArguments().get(0).getTypeCasts().isEmpty()) {
return;
}

// handle edge case where the variable is implicitly cast in the invocation (Character in List, but char in iterable)
List<CtTypeReference<?>> actualTypeArguments = ctInvocation.getTarget().getType().getActualTypeArguments();
if (!actualTypeArguments.isEmpty() && !ctFor.getVariable().getType().equals(actualTypeArguments.get(0))) {
return;
}

// special case for arrays
String addAllArg = ctFor.getExpression().toString();
if (ctFor.getExpression().getType().isArray()) {
addAllArg = "Arrays.asList(%s)".formatted(addAllArg);
Expand Down
Loading

0 comments on commit eebee71

Please sign in to comment.