Skip to content

Commit

Permalink
Merge pull request #371 from Luro02/main
Browse files Browse the repository at this point in the history 
Implement some things
  • Loading branch information
@Luro02
Luro02 authored Jan 13, 2024
2 parents d005b03 + d71ec10 commit e42d133
Show file tree
Hide file tree
Showing 25 changed files with 1,258 additions and 306 deletions.
3 changes: 2 additions & 1 deletion autograder-core/src/main/java/de/firemage/autograder/core/CodePosition.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import de.firemage.autograder.core.file.SourceInfo;
import de.firemage.autograder.core.file.SourcePath;
import spoon.reflect.code.CtLoop;
import spoon.reflect.cu.SourcePosition;
import spoon.reflect.declaration.CtElement;
import spoon.reflect.declaration.CtMethod;
Expand Down Expand Up @@ -32,7 +33,7 @@ public static CodePosition fromSourcePosition(
// Instead of highlighting all lines of a class or method, only highlight the first line.
//
// Someone might explicitly specify a source position, in which case it will differ from the element's position.
if ((ctElement instanceof CtType<?> || ctElement instanceof CtMethod<?>) && ctElement.getPosition().equals(sourcePosition)) {
if ((ctElement instanceof CtType<?> || ctElement instanceof CtMethod<?> || ctElement instanceof CtLoop) && ctElement.getPosition().equals(sourcePosition)) {
return new CodePosition(
sourceInfo,
relativePath,
Expand Down
5 changes: 5 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 @@ -90,6 +90,8 @@ public enum ProblemType {
COMMON_REIMPLEMENTATION_ARRAY_COPY,
COMMON_REIMPLEMENTATION_STRING_REPEAT,
COMMON_REIMPLEMENTATION_MAX_MIN,
COMMON_REIMPLEMENTATION_SQRT,
COMMON_REIMPLEMENTATION_HYPOT,
COMMON_REIMPLEMENTATION_ADD_ALL,
COMMON_REIMPLEMENTATION_ADD_ENUM_VALUES,
COMMON_REIMPLEMENTATION_ARRAYS_FILL,
Expand Down Expand Up @@ -145,5 +147,8 @@ public enum ProblemType {
ARRAY_AS_KEY_OF_SET_OR_MAP,
MULTIPLE_INLINE_STATEMENTS,
UNNECESSARY_BOXING,
AVOID_STRING_CONCAT,
UNNECESSARY_COMMENT,
OBJECT_DATATYPE,
REDUNDANT_UNINITIALIZED_VARIABLE
}
59 changes: 59 additions & 0 deletions autograder-core/src/main/java/de/firemage/autograder/core/check/api/AvoidStringConcat.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
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.CtRange;
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.CtInvocation;
import spoon.reflect.factory.Factory;
import spoon.reflect.reference.CtTypeReference;

import java.util.Map;
import java.util.Optional;
import java.util.Set;

@ExecutableCheck(reportedProblems = { ProblemType.AVOID_STRING_CONCAT })
public class AvoidStringConcat extends IntegratedCheck {
@Override
protected void check(StaticAnalysis staticAnalysis, DynamicAnalysis dynamicAnalysis) {
staticAnalysis.processWith(new AbstractProcessor<CtInvocation<?>>() {
@Override
public void process(CtInvocation<?> ctInvocation) {
if (ctInvocation.isImplicit()
|| !ctInvocation.getPosition().isValidPosition()) {
return;
}

if (ctInvocation.getTarget() == null
|| ctInvocation.getTarget().getType() == null
|| !SpoonUtil.isSignatureEqualTo(ctInvocation.getExecutable(), String.class, "concat", String.class)) {
return;
}

addLocalProblem(
ctInvocation,
new LocalizedMessage(
"common-reimplementation",
Map.of(
"suggestion", "%s + %s".formatted(
ctInvocation.getTarget().prettyprint(),
ctInvocation.getArguments().get(0).prettyprint()
)
)
),
ProblemType.AVOID_STRING_CONCAT
);
}
});
}
}
105 changes: 0 additions & 105 deletions ...ader-core/src/main/java/de/firemage/autograder/core/check/api/CommonReimplementation.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -150,110 +150,6 @@ private void checkArrayCopy(CtFor ctFor) {
}
}

private void checkMaxMin(CtIf ctIf) {
Set<BinaryOperatorKind> maxOperators = Set.of(BinaryOperatorKind.LT, BinaryOperatorKind.LE);
Set<BinaryOperatorKind> minOperators = Set.of(BinaryOperatorKind.GT, BinaryOperatorKind.GE);

// ensure that in the if block there is only one assignment to a variable
// and the condition is a binary operator with <, <=, > or >=
List<CtStatement> thenBlock = SpoonUtil.getEffectiveStatements(ctIf.getThenStatement());
if (thenBlock.size() != 1
|| !(thenBlock.get(0) instanceof CtAssignment<?, ?> thenAssignment)
|| !(thenAssignment.getAssigned() instanceof CtVariableWrite<?> ctVariableWrite)
|| !(ctIf.getCondition() instanceof CtBinaryOperator<Boolean> ctBinaryOperator)
|| (!maxOperators.contains(ctBinaryOperator.getKind()) && !minOperators.contains(ctBinaryOperator.getKind()))) {
return;
}

// keep track of the assigned variable (must be the same in the else block)
CtVariableReference<?> assignedVariable = ctVariableWrite.getVariable();

// this is the value that is assigned if the then-block is not executed
// The variable is not changed without an else-Block (this would be equivalent to an else { variable = variable; })
CtExpression<?> elseValue = ctIf.getFactory().createVariableRead(
assignedVariable.clone(),
assignedVariable.getModifiers().contains(ModifierKind.STATIC)
);
if (ctIf.getElseStatement() != null) {
List<CtStatement> elseBlock = SpoonUtil.getEffectiveStatements(ctIf.getElseStatement());
if (elseBlock.size() != 1
|| !(elseBlock.get(0) instanceof CtAssignment<?,?> elseAssignment)
|| !(elseAssignment.getAssigned() instanceof CtVariableAccess<?> elseAccess)
// ensure that the else block assigns to the same variable
|| !elseAccess.getVariable().equals(assignedVariable)) {
return;
}

elseValue = elseAssignment.getAssignment();
}

CtBinaryOperator<Boolean> condition = ctBinaryOperator;
// ensure that the else value is on the left side of the condition
if (ctBinaryOperator.getRightHandOperand().equals(elseValue)) {
condition = SpoonUtil.swapCtBinaryOperator(condition);
}

// if it is not on either side of the condition, return
if (!condition.getLeftHandOperand().equals(elseValue)) {
return;
}

// max looks like this:
// if (variable < max) {
// variable = max;
// }
//
// or with an explicit else block:
//
// if (max > expr) {
// v = max;
// } else {
// v = expr;
// }

if (maxOperators.contains(condition.getKind())) {
addLocalProblem(
ctIf,
new LocalizedMessage(
"common-reimplementation",
Map.of(
"suggestion", "%s = Math.max(%s, %s)".formatted(
ctVariableWrite.prettyprint(),
elseValue.prettyprint(),
condition.getRightHandOperand().prettyprint()
)
)
),
ProblemType.COMMON_REIMPLEMENTATION_MAX_MIN
);

return;
}

// if (variable > min) {
// variable = min;
// }

if (minOperators.contains(condition.getKind())) {
addLocalProblem(
ctIf,
new LocalizedMessage(
"common-reimplementation",
Map.of(
"suggestion", "%s = Math.min(%s, %s)".formatted(
ctVariableWrite.prettyprint(),
elseValue.prettyprint(),
condition.getRightHandOperand().prettyprint()
)
)
),
ProblemType.COMMON_REIMPLEMENTATION_MAX_MIN
);

return;
}
}

private void checkAddAll(CtForEach ctFor) {
List<CtStatement> statements = SpoonUtil.getEffectiveStatements(ctFor.getBody());
if (statements.size() != 1) {
Expand Down Expand Up @@ -654,7 +550,6 @@ public void visitCtIf(CtIf ctIf) {
return;
}

checkMaxMin(ctIf);
checkModulo(ctIf);
super.visitCtIf(ctIf);
}
Expand Down
Loading

0 comments on commit e42d133

Please sign in to comment.