Skip to content

Commit

Permalink
introduce common api for finding uses of an element
Browse files Browse the repository at this point in the history
  • Loading branch information
Luro02 committed Apr 23, 2024
1 parent 6fc515b commit 8712e44
Show file tree
Hide file tree
Showing 13 changed files with 300 additions and 120 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ private void checkArraysFill(CtFor ctFor) {

CtElement loopVariable = SpoonUtil.getReferenceDeclaration(forLoopRange.loopVariable());
// return if the for loop uses the loop variable (would not be a simple repetition)
if (SpoonUtil.hasAnyUsesIn(loopVariable, ctAssignment.getAssignment(), e -> true)) {
if (SpoonUtil.hasAnyUsesIn(loopVariable, ctAssignment.getAssignment())) {
return;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
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.uses.UsesFinder;
import spoon.processing.AbstractProcessor;
import spoon.reflect.code.CtAssignment;
import spoon.reflect.code.CtLocalVariable;
Expand Down Expand Up @@ -50,12 +51,7 @@ public void process(CtAssignment<?, ?> ctAssignment) {
return;
}


if (followingStatements.stream().noneMatch(statement -> SpoonUtil.hasAnyUsesIn(
ctLocalVariable,
statement,
element -> element instanceof CtVariableRead<?>
))) {
if (followingStatements.stream().noneMatch(statement -> UsesFinder.ofVariableRead(ctLocalVariable).in(statement).hasAny())) {
addLocalProblem(
ctAssignment,
new LocalizedMessage(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,8 @@
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.uses.UsesFinder;
import spoon.processing.AbstractProcessor;
import spoon.reflect.code.CtLocalVariable;
import spoon.reflect.code.CtVariableRead;
import spoon.reflect.declaration.CtConstructor;
import spoon.reflect.declaration.CtElement;
import spoon.reflect.declaration.CtMethod;
Expand Down Expand Up @@ -48,18 +47,6 @@ private static Collection<CtFieldReference<?>> getAllVisibleFields(CtTypeInforma
return result;
}

/**
* Searches for a variable read of the given variable in the given element.
*
* @param ctVariable the variable that should be read
* @param in the element to search in
* @return true if a variable read was found, false otherwise
* @param <T> the type of the variable
*/
private static <T> boolean hasVariableReadIn(CtVariable<T> ctVariable, CtElement in) {
return SpoonUtil.findUsesIn(ctVariable, in).stream().anyMatch(ctElement -> ctElement instanceof CtVariableRead<?>);
}

@Override
protected void check(StaticAnalysis staticAnalysis) {
staticAnalysis.processWith(new AbstractProcessor<CtVariable<?>>() {
Expand Down Expand Up @@ -104,11 +91,11 @@ public void process(CtVariable<?> ctVariable) {
CtElement variableParent = ctVariable.getParent();

// there might be multiple fields hidden by the variable (e.g. subclass hides superclass field)
boolean isFieldRead = hiddenFields.stream().anyMatch(ctFieldReference -> hasVariableReadIn(ctFieldReference.getFieldDeclaration(), variableParent));
boolean isFieldRead = hiddenFields.stream().anyMatch(ctFieldReference -> UsesFinder.ofVariableRead(ctFieldReference.getFieldDeclaration()).in(variableParent).hasAny());

// to reduce the number of annotations, we only report a problem if the variable AND the hidden field are read in
// the same context
if (hasVariableReadIn(ctVariable, variableParent) && isFieldRead) {
if (UsesFinder.ofVariableRead(ctVariable).in(variableParent).hasAny() && isFieldRead) {
addLocalProblem(
ctVariable,
new LocalizedMessage("avoid-shadowing", Map.of("name", ctVariable.getSimpleName())),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
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.uses.UsesFinder;
import spoon.processing.AbstractProcessor;
import spoon.reflect.code.CtFieldWrite;
import spoon.reflect.declaration.CtConstructor;
Expand Down Expand Up @@ -38,11 +39,8 @@ public void process(CtField<?> ctField) {
}

// check if the field is written to in constructors, which is fine if it does not have an explicit value
boolean hasWriteInConstructor = SpoonUtil.hasAnyUses(
ctField,
ctElement -> ctElement instanceof CtFieldWrite<?> ctFieldWrite
&& ctFieldWrite.getParent(CtConstructor.class) != null
);
boolean hasWriteInConstructor = UsesFinder.ofVariableWrite(ctField)
.hasAnyMatch(ctFieldWrite -> ctFieldWrite.getParent(CtConstructor.class) != null);

// we need to check if the field is explicitly initialized, because this is not allowed:
//
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
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.uses.UsesFinder;
import spoon.processing.AbstractProcessor;
import spoon.reflect.code.CtArrayRead;
import spoon.reflect.code.CtBodyHolder;
Expand Down Expand Up @@ -64,20 +65,15 @@ public class ForToForEachLoop extends IntegratedCheck {
public static Optional<CtVariable<?>> getForEachLoopVariable(
CtBodyHolder ctBodyHolder,
ForLoopRange forLoopRange,
Function<CtVariableAccess<?>, Optional<CtVariableAccess<?>>> getPotentialLoopVariableAccess
Function<? super CtVariableAccess<?>, Optional<CtVariableAccess<?>>> getPotentialLoopVariableAccess
) {
CtVariable<?> loopVariable = forLoopRange.loopVariable().getDeclaration();

// if a ForLoopRange exists, it is guranteed that the variable is incremented by 1 for each step
// if a ForLoopRange exists, it is guaranteed that the variable is incremented by 1 for each step

CtVariable<?> ctVariable = null;
CtVariableAccess<?> expectedAccess = null;
for (CtElement use : SpoonUtil.findUsesIn(loopVariable, ctBodyHolder.getBody())) {
if (!(use instanceof CtVariableAccess<?> ctVariableAccess)) {
throw new IllegalStateException(
"SpoonUtil.findUsesIn returned non-variable access for '%s' as input".formatted(loopVariable));
}

for (CtVariableAccess<?> ctVariableAccess : UsesFinder.of(loopVariable).in(ctBodyHolder.getBody()).all()) {
// We need to get the variable on which the access is performed (e.g. in a[i] we need to get a)
CtVariableAccess<?> potentialLoopVariableAccess = getPotentialLoopVariableAccess.apply(ctVariableAccess)
.orElse(null);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
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.uses.UsesFinder;
import spoon.processing.AbstractProcessor;
import spoon.reflect.code.BinaryOperatorKind;
import spoon.reflect.code.CtAssignment;
Expand Down Expand Up @@ -102,8 +103,7 @@ private static LoopSuggestion getCounter(CtLoop ctLoop) {
CtStatement finalLastStatement = lastStatement;
boolean isUpdatedMultipleTimes = statements.stream()
.filter(statement -> statement != finalLastStatement)
.anyMatch(
statement -> SpoonUtil.hasAnyUsesIn(ctLocalVariable, statement, ctElement -> ctElement instanceof CtVariableWrite<?>));
.anyMatch(statement -> UsesFinder.ofVariableWrite(ctLocalVariable).in(statement).hasAny());

if (isUpdatedMultipleTimes) {
return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
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.uses.UsesFinder;
import spoon.processing.AbstractProcessor;
import spoon.reflect.CtModel;
import spoon.reflect.declaration.CtElement;
Expand Down Expand Up @@ -59,7 +60,7 @@ public String toString() {
private static Visibility getVisibility(CtTypeMember ctTypeMember) {
CtModel ctModel = ctTypeMember.getFactory().getModel();

List<CtElement> references = SpoonUtil.findUsesOf(ctTypeMember);
List<CtElement> references = UsesFinder.of(ctTypeMember).all();

CtElement commonParent = SpoonUtil.findCommonParent(ctTypeMember, references);
CtType<?> declaringType = ctTypeMember.getDeclaringType();
Expand Down Expand Up @@ -124,7 +125,7 @@ public void process(CtTypeMember ctTypeMember) {
.getFields()
.stream()
// filter out the field itself and those that do not reference the field
.filter(field -> field != ctField && !SpoonUtil.findUsesIn(ctField, field).isEmpty())
.filter(field -> field != ctField && SpoonUtil.hasAnyUsesIn(ctField, field))
.map(UseDifferentVisibility::getVisibility)
.max(Visibility::compareTo);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
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.uses.UsesFinder;
import spoon.reflect.code.CtAssignment;
import spoon.reflect.code.CtBlock;
import spoon.reflect.code.CtConstructorCall;
Expand All @@ -31,10 +32,12 @@
import spoon.reflect.reference.CtFieldReference;
import spoon.reflect.reference.CtVariableReference;
import spoon.reflect.visitor.CtScanner;
import spoon.reflect.visitor.Filter;

import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.function.Predicate;

// What should be supported:
// - Any mutable collection (List, Set, Map, ...)
Expand All @@ -54,16 +57,18 @@
})
public class LeakedCollectionCheck extends IntegratedCheck {
private void checkCtExecutableReturn(CtExecutable<?> ctExecutable) {
// parent will be null, if the return is in a constructor.
// constructors are not considered methods.
List<CtStatement> statements = SpoonUtil.getEffectiveStatements(ctExecutable.getBody());

// a lambda like () -> true does not have a body, but an expression which is a return statement
// this case is handled here
if (statements.isEmpty() && ctExecutable instanceof CtLambda<?> ctLambda) {
statements = List.of(createCtReturn(ctLambda.getExpression().clone()));
}

if (statements.isEmpty()
// we should not check private methods (for those it should be okay to return a not-copied collection)
|| (ctExecutable instanceof CtModifiable ctModifiable && ctModifiable.isPrivate())
// TODO: shouldn't we check ALL returns (like in an if-else block)?
|| !(statements.getLast() instanceof CtReturn<?> ctReturn)
|| ctReturn.getReturnedExpression() == null) {
return;
Expand All @@ -74,9 +79,16 @@ private void checkCtExecutableReturn(CtExecutable<?> ctExecutable) {
if (!(returnedExpression instanceof CtFieldRead<?> ctFieldRead)) {
return;
}

// TODO: could this crash?
CtField<?> field = ctFieldRead.getVariable().getFieldDeclaration();

if (canBeMutated(ctFieldRead) && field.isPrivate()) {
// if the field is not private, it can not be leaked/mutated anyway.
if (!field.isPrivate()) {
return;
}

if (canBeMutated(ctFieldRead)) {
addLocalProblem(
SpoonUtil.findValidPosition(ctExecutable),
new LocalizedMessage(
Expand All @@ -97,7 +109,7 @@ private void checkCtExecutableAssign(CtExecutable<?> ctExecutable) {
}

for (CtStatement ctStatement : SpoonUtil.getEffectiveStatements(ctExecutable.getBody())) {
if (!(ctStatement instanceof CtAssignment<?,?> ctAssignment)
if (!(ctStatement instanceof CtAssignment<?, ?> ctAssignment)
|| !(ctAssignment.getAssigned() instanceof CtFieldWrite<?> ctFieldWrite)) {
continue;
}
Expand Down Expand Up @@ -197,7 +209,14 @@ public void visitCtAnonymousExecutable(CtAnonymousExecutable ctAnonymousExecutab
});
}

// checks if the variable can be mutated from the outside
/**
* Checks if the field can be mutated from the outside.
* <p>
* Because we do not want to lint when people store an immutable copy via List.of() or similar.
*
* @param ctFieldAccess the accessed field
* @return true if the field can be mutated, false otherwise
*/
private static boolean canBeMutated(CtFieldAccess<?> ctFieldAccess) {
CtFieldReference<?> ctFieldReference = ctFieldAccess.getVariable();
CtField<?> ctField = ctFieldReference.getFieldDeclaration();
Expand All @@ -212,18 +231,20 @@ private static boolean canBeMutated(CtFieldAccess<?> ctFieldAccess) {
return false;
}

// if the field has a default value that is mutable, the field is mutable
if (ctField.getAssignment() != null && isMutableAssignee(ctField.getAssignment())) {
return true;
}

// there are some special classes that are always immutable
// for those we check if they were assigned that special construct
//
// (e.g. List.of(), Set.of(), Map.of(), Collections.unmodifiableList(), ...)

// we check if there is a write to the field with a value that is guranteed to be mutable
return SpoonUtil.hasAnyUses(ctField, ctElement -> ctElement instanceof CtFieldWrite<?> ctVariableWrite
&& ctVariableWrite.getVariable().equals(ctFieldReference)
&& ctVariableWrite.getParent() instanceof CtAssignment<?,?> ctAssignment
&& ctAssignment.getAssigned() == ctVariableWrite
&& isMutableAssignee(ctAssignment.getAssignment()))
|| (ctField.getAssignment() != null && isMutableAssignee(ctField.getAssignment()));
return UsesFinder.ofVariableWrite(ctField)
.hasAnyMatch(ctFieldWrite -> ctFieldWrite.getParent() instanceof CtAssignment<?, ?> ctAssignment
&& isMutableAssignee(ctAssignment.getAssignment()));
}

private static boolean isMutableAssignee(CtExpression<?> ctExpression) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
package de.firemage.autograder.core.check.utils;

import java.util.Iterator;
import java.util.function.Function;
import java.util.stream.Stream;

public sealed interface Option<T> extends Iterable<T> permits Option.Some, Option.None {
static <T> Option<T> ofNullable(T value) {
return value == null ? new None<>() : new Some<>(value);
}

default T unwrap() {
return switch (this) {
case Some<T> (var value) -> value;
case None<T> ignored -> throw new IllegalStateException("Expected Some value, but got None.");
};
}

default boolean isSome() {
return this instanceof Some;
}

default <U> Option<U> map(Function<T, U> function) {
return switch (this) {
case Some<T>(var value) -> new Some<>(function.apply(value));
case None<T> ignored -> new None<>();
};
}

/**
* Returns the value if it is present or null if it is not.
*
* @return the value or null
*/
default T nullable() {
return switch (this) {
case Some<T>(var value) -> value;
case None<T> ignored -> null;
};
}

default Stream<T> stream() {
return switch (this) {
case Some<T>(var value) -> Stream.of(value);
case None<T> ignored -> Stream.empty();
};
}

@Override
default Iterator<T> iterator() {
return stream().iterator();
}

record None<T>() implements Option<T> {
}

record Some<T>(T value) implements Option<T> {
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public static Optional<ForLoopRange> fromCtFor(CtFor ctFor) {
&& ctVariable.getReference().equals(ctVariableAccess.getVariable()))
.orElse(false)
// the loop variable must not be used after the loop
&& SpoonUtil.getNextStatements(ctFor).stream().allMatch(statement -> SpoonUtil.findUsesIn(localVariable, statement).isEmpty())
&& SpoonUtil.getNextStatements(ctFor).stream().noneMatch(statement -> SpoonUtil.hasAnyUsesIn(localVariable, statement))
) {
potentialLoopVariable = localVariable;
} else if (ctFor.getForInit().size() == 1 && ctFor.getForInit().get(0) instanceof CtLocalVariable<?> ctLocalVariable) {
Expand Down
Loading

0 comments on commit 8712e44

Please sign in to comment.