Skip to content

Commit

Permalink
handle compact constructor correctly in leaked collection check
Browse files Browse the repository at this point in the history
  • Loading branch information
@Luro02
Luro02 committed May 16, 2024
1 parent c812402 commit f330c3d
Show file tree
Hide file tree
Showing 3 changed files with 193 additions and 39 deletions.
139 changes: 103 additions & 36 deletions ...rader-core/src/main/java/de/firemage/autograder/core/check/oop/LeakedCollectionCheck.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,8 @@
import de.firemage.autograder.core.integrated.StaticAnalysis;
import de.firemage.autograder.core.integrated.UsesFinder;
import spoon.reflect.code.CtAssignment;
import spoon.reflect.code.CtBlock;
import spoon.reflect.code.CtConstructorCall;
import spoon.reflect.code.CtExpression;
import spoon.reflect.code.CtFieldAccess;
import spoon.reflect.code.CtFieldRead;
import spoon.reflect.code.CtFieldWrite;
import spoon.reflect.code.CtLambda;
Expand All @@ -30,13 +28,15 @@
import spoon.reflect.declaration.CtModifiable;
import spoon.reflect.declaration.CtRecord;
import spoon.reflect.declaration.CtType;
import spoon.reflect.declaration.CtVariable;
import spoon.reflect.factory.Factory;
import spoon.reflect.reference.CtFieldReference;
import spoon.reflect.reference.CtLocalVariableReference;
import spoon.reflect.reference.CtParameterReference;
import spoon.reflect.reference.CtVariableReference;
import spoon.reflect.visitor.CtScanner;
import spoon.reflect.visitor.filter.TypeFilter;

import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.Optional;
Expand All @@ -53,23 +53,32 @@
// - The setter does not copy the list
// - The getter returns the list directly


// How to improve performance:
//
// 1. We visit each CtType and visit each CtConstructor
// 2. Based on the constructor we can infer if the field is
// - not copied
// - copied, but mutable
// - immutable
// 3. Then we check each CtMethod if the field is assigned a parameter (-> mutable from outside) or returned a direct reference (-> leaked)
//
// A major problem is that one can reassign references, so this has to be taken into account.

@ExecutableCheck(reportedProblems = {
ProblemType.LEAKED_COLLECTION_RETURN,
ProblemType.LEAKED_COLLECTION_ASSIGN
})
public class LeakedCollectionCheck extends IntegratedCheck {
/**
* Checks if the field can be mutated from the outside.
* Checks if the field can be mutated (if it is not okay to return a reference to it without copying it).
* <p>
* Because we do not want to lint when people store an immutable copy via List.of() or similar.
*
* @param ctFieldAccess the accessed field
* @param ctField the field to check
* @return true if the field can be mutated, false otherwise
*/
private static boolean canBeMutated(CtFieldAccess<?> ctFieldAccess) {
CtFieldReference<?> ctFieldReference = ctFieldAccess.getVariable();
CtField<?> ctField = ctFieldReference.getFieldDeclaration();

private static boolean canBeMutated(CtField<?> ctField) {
// arrays are always mutable, there is no immutable array
if (ctField.getType().isArray()) {
return true;
Expand All @@ -80,35 +89,50 @@ private static boolean canBeMutated(CtFieldAccess<?> ctFieldAccess) {
return false;
}

// TODO: what is this supposed to be? I don't understand why this function exists

// TODO: what if the default value is always overwritten in the constructor? Then it is never mutable?

// 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 guaranteed to be mutable
return UsesFinder.variableUses(ctField)
.ofType(CtVariableWrite.class)
.filterDirectParent(CtAssignment.class, a -> isMutableAssignee(a.getAssignment()))
.hasAny();
}

/**
* Checks if the given expression is mutable (it would be problematic to return a reference to it without copying it).
*
* @param ctExpression the expression to check
* @return true if the expression is mutable, false otherwise
*/
private static boolean isMutableAssignee(CtExpression<?> ctExpression) {
if (ctExpression instanceof CtNewArray<?>) {
return true;
}

if (!SpoonUtil.isSubtypeOf(ctExpression.getType(), java.util.Collection.class)) {
// not a collection
return false;
}

// something like new ArrayList<>() is always mutable
if (ctExpression instanceof CtConstructorCall<?>) {
return true;
}

if (ctExpression instanceof CtVariableRead<?> ctVariableRead
&& ctVariableRead.getVariable() instanceof CtLocalVariableReference<?> ctLocalVariableReference) {
CtLocalVariable<?> ctVariable = ctLocalVariableReference.getDeclaration();
CtExecutable<?> ctExecutable = ctExpression.getParent(CtExecutable.class);

// if a variable is assigned to the field, one has to check if the variable is mutable
if (ctExpression instanceof CtVariableRead<?> ctVariableRead) {
CtVariable<?> ctVariable = SpoonUtil.getVariableDeclaration(ctVariableRead.getVariable());

// the variable is mutable if it is assigned a mutable value in the declaration or through any write
return (ctVariable.getDefaultExpression() != null && isMutableAssignee(ctVariable.getDefaultExpression()))
|| UsesFinder.variableUses(ctVariable)
.ofType(CtVariableWrite.class)
Expand All @@ -118,15 +142,13 @@ private static boolean isMutableAssignee(CtExpression<?> ctExpression) {

// if a public method assigns a parameter to a field that parameter can be mutated from the outside
// => the field is mutable assignee
CtExecutable<?> ctExecutable = ctExpression.getParent(CtExecutable.class);
return ctExecutable instanceof CtModifiable ctModifiable
if (ctExecutable instanceof CtModifiable ctModifiable
&& !ctModifiable.isPrivate()
&& hasAssignedParameterReference(ctExpression, ctExecutable);
}
&& hasAssignedParameterReference(ctExpression, ctExecutable)) {
return true;
}

private static boolean hasAssignedParameterReference(CtExpression<?> ctExpression, CtExecutable<?> ctExecutable) {
return ctExpression instanceof CtVariableRead<?> ctVariableRead
&& isParameterOf(ctVariableRead.getVariable(), ctExecutable);
return false;
}

private static boolean isParameterOf(CtVariableReference<?> ctVariableReference, CtExecutable<?> ctExecutable) {
Expand All @@ -138,6 +160,53 @@ private static boolean isParameterOf(CtVariableReference<?> ctVariableReference,
return ctExecutable.getParameters().stream().anyMatch(ctParameter -> ctParameter == declaration);
}

private static List<CtExpression<?>> findPreviousAssignee(CtVariableRead<?> ctVariableRead) {
List<CtExpression<?>> result = new ArrayList<>();
CtExecutable<?> ctExecutable = ctVariableRead.getParent(CtExecutable.class);

boolean foundPreviousAssignment = false;
CtStatement currentStatement = ctVariableRead.getParent(CtStatement.class);
for (CtStatement ctStatement : SpoonUtil.getEffectiveStatements(ctExecutable.getBody()).reversed()) {
if (!foundPreviousAssignment) {
if (ctStatement == currentStatement) {
foundPreviousAssignment = true;
}

continue;
}

if (ctStatement instanceof CtAssignment<?,?> ctAssignment
&& ctAssignment.getAssigned() instanceof CtVariableWrite<?> ctVariableWrite
&& ctVariableWrite.getVariable().equals(ctVariableRead.getVariable())) {

result.add(ctAssignment.getAssignment());
}
}

return result;
}

private static boolean hasAssignedParameterReference(CtExpression<?> ctExpression, CtExecutable<?> ctExecutable) {
if (ctExpression instanceof CtVariableRead<?> ctVariableRead && isParameterOf(ctVariableRead.getVariable(), ctExecutable)) {
// There is a special-case: one can reassign the parameter to itself with a different value:
//
// values = List.copyOf(values);
// this.values = values;
//
// Here it is guaranteed that the field is not mutably assigned.

List<CtExpression<?>> previousAssignees = findPreviousAssignee(ctVariableRead);

if (!previousAssignees.isEmpty()) {
return hasAssignedParameterReference(previousAssignees.getFirst(), ctExecutable);
}

return true;
}

return false;
}

private void checkCtExecutableReturn(CtExecutable<?> ctExecutable) {
List<CtStatement> statements = SpoonUtil.getEffectiveStatements(ctExecutable.getBody());

Expand All @@ -162,24 +231,19 @@ private void checkCtExecutableReturn(CtExecutable<?> ctExecutable) {
}).toList();

for (CtReturn<?> ctReturn : returns) {
if (ctReturn.getReturnedExpression() == null) {
continue;
}

CtExpression<?> returnedExpression = ctReturn.getReturnedExpression();

if (!(returnedExpression instanceof CtFieldRead<?> ctFieldRead)) {
continue;
}

CtField<?> field = ctFieldRead.getVariable().getFieldDeclaration();

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

if (canBeMutated(ctFieldRead)) {
if (canBeMutated(field)) {
addLocalProblem(
SpoonUtil.findValidPosition(ctExecutable),
new LocalizedMessage(
Expand All @@ -195,6 +259,12 @@ private void checkCtExecutableReturn(CtExecutable<?> ctExecutable) {
}
}

/**
* Checks if the executable has an assignment to a field that is assigned a parameter reference,
* then it could be mutated from the outside.
*
* @param ctExecutable the executable to check
*/
private void checkCtExecutableAssign(CtExecutable<?> ctExecutable) {
if (ctExecutable instanceof CtModifiable ctModifiable && ctModifiable.isPrivate()) {
return;
Expand All @@ -207,8 +277,7 @@ private void checkCtExecutableAssign(CtExecutable<?> ctExecutable) {
}

if (hasAssignedParameterReference(ctAssignment.getAssignment(), ctExecutable)
&& ctFieldWrite.getVariable().getFieldDeclaration().isPrivate()
&& canBeMutated(ctFieldWrite)) {
&& ctFieldWrite.getVariable().getFieldDeclaration().isPrivate()) {
String methodName = ctExecutable.getSimpleName();
if (methodName.equals("<init>")) {
methodName = ctExecutable.getParent(CtType.class).getSimpleName();
Expand Down Expand Up @@ -239,15 +308,13 @@ private static CtReturn<?> createCtReturn(CtExpression<?> ctExpression) {
private static CtMethod<?> fixRecordAccessor(CtRecord ctRecord, CtMethod<?> ctMethod) {
Factory factory = ctMethod.getFactory();
CtMethod<?> result = ctMethod.clone();
CtBlock<?> ctBody = factory.createBlock();
CtFieldRead ctFieldRead = factory.createFieldRead();

ctFieldRead.setTarget(null);
ctFieldRead.setVariable(ctRecord.getField(ctMethod.getSimpleName()).getReference());
ctFieldRead.setType(result.getType());

ctBody.setStatements(List.of(createCtReturn(ctFieldRead)));
result.setBody(ctBody);
result.setBody(createCtReturn(ctFieldRead));
result.setParent(ctRecord);

return result;
Expand Down
21 changes: 18 additions & 3 deletions autograder-core/src/main/java/de/firemage/autograder/core/integrated/SpoonUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@
import spoon.reflect.reference.CtReference;
import spoon.reflect.reference.CtTypeParameterReference;
import spoon.reflect.reference.CtTypeReference;
import spoon.reflect.reference.CtVariableReference;
import spoon.reflect.visitor.Filter;
import spoon.reflect.visitor.filter.CompositeFilter;
import spoon.reflect.visitor.filter.FilteringOperator;
import spoon.reflect.visitor.filter.TypeFilter;
Expand Down Expand Up @@ -877,6 +879,7 @@ public static boolean isEffectivelyFinal(CtVariable<?> ctVariable) {
if (ctVariable.getModifiers().contains(ModifierKind.FINAL)) {
return true;
}

return UsesFinder.variableUses(ctVariable).ofType(CtVariableWrite.class).hasNone();
}

Expand Down Expand Up @@ -1112,7 +1115,7 @@ public static boolean isInOverridingMethod(CtElement ctElement) {
*/
public static boolean isInvocation(CtStatement statement) {
return statement instanceof CtInvocation<?> || statement instanceof CtConstructorCall<?> ||
statement instanceof CtLambda<?>;
statement instanceof CtLambda<?>;
}

public static boolean isInMainMethod(CtElement ctElement) {
Expand All @@ -1137,11 +1140,23 @@ public static CtElement getReferenceDeclaration(CtReference ctReference) {
target = ctExecutableReference.getExecutableDeclaration();
}

if (target == null && ctReference instanceof CtFieldReference<?> ctFieldReference) {
if (target == null && ctReference instanceof CtVariableReference<?> ctVariableReference) {
target = getVariableDeclaration(ctVariableReference);
}

return target;
}

public static CtVariable<?> getVariableDeclaration(CtVariableReference<?> ctVariableReference) {
// this might be null if the reference is not in the source path
// for example, when the reference points to a java.lang type
CtVariable<?> target = ctVariableReference.getDeclaration();

if (target == null && ctVariableReference instanceof CtFieldReference<?> ctFieldReference) {
target = ctFieldReference.getFieldDeclaration();
}

if (target == null && ctReference instanceof CtLocalVariableReference<?> ctLocalVariableReference) {
if (target == null && ctVariableReference instanceof CtLocalVariableReference<?> ctLocalVariableReference) {
target = getLocalVariableDeclaration(ctLocalVariableReference);
}

Expand Down
72 changes: 72 additions & 0 deletions ...r-core/src/test/java/de/firemage/autograder/core/check/oop/TestLeakedCollectionCheck.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -836,4 +836,76 @@ public List<String> get() {

problems.assertExhausted();
}

@Test
void testCompactConstructorImmutable() throws IOException, LinterException {
ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString(
JavaVersion.JAVA_17,
"Zoo",
"""
import java.util.List;
import java.util.Collection;
import java.util.ArrayList;
public record Zoo(String name, Collection<String> animals) {
public Zoo {
animals = List.copyOf(animals);
}
}
"""
), PROBLEM_TYPES);

problems.assertExhausted();
}

@Test
void testCompactConstructorLeakedAssign() throws IOException, LinterException {
ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString(
JavaVersion.JAVA_17,
"Zoo",
"""
import java.util.List;
import java.util.Collection;
import java.util.ArrayList;
public record Zoo(String name, Collection<String> animals) {
public Zoo {
animals = animals;
}
public Collection<String> animals() {
return new ArrayList<>(animals);
}
}
"""
), PROBLEM_TYPES);

assertEqualsLeakedAssign(problems.next(), "Zoo", "animals");

problems.assertExhausted();
}


@Test
void testCompactConstructorCopied() throws IOException, LinterException {
ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString(
JavaVersion.JAVA_17,
"Zoo",
"""
import java.util.List;
import java.util.Collection;
import java.util.ArrayList;
public record Zoo(String name, Collection<String> animals) {
public Zoo {
animals = new ArrayList<>(animals);
}
}
"""
), PROBLEM_TYPES);

assertEqualsLeakedReturn(problems.next(), "animals", "animals");

problems.assertExhausted();
}
}

0 comments on commit f330c3d

Please sign in to comment.