mirror of
https://gitflic.ru/project/openide/openide.git
synced 2026-09-27 10:03:11 +07:00
[java-inspections] Refactor and improve MismatchedCollectionQueryUpdate
1. Track separately updates that have no effect on empty collection 2. Extract MismatchedQueryUpdateRunner to avoid lots of arguments 3. Make QueryUpdateInfo immutable record 4. Track variables containing derived collections 5. Track iterators as derived collections Fixes IDEA-356315 Incorrect suggestion of Stream.toList() when followed by Iterator.remove() GitOrigin-RevId: 3b7fd99b2310f123e924034f7c623bd01e4e4c40
This commit is contained in:
Tagir Valeev
committed by
intellij-monorepo-bot
intellij-monorepo-bot
parent
4c5baed284
commit
dd15592444
@@ -123,7 +123,9 @@ mismatched.read.write.array.display.name=Mismatched read and write of array
|
||||
mismatched.read.write.array.problem.descriptor.write.not.read=Contents of array <code>#ref</code> are written to, but never read #loc
|
||||
mismatched.read.write.array.problem.descriptor.read.not.write=Contents of array <code>#ref</code> are read, but never written to #loc
|
||||
mismatched.update.collection.display.name=Mismatched query and update of collection
|
||||
mismatched.update.collection.problem.descriptor.updated.not.queried=Contents of collection <code>#ref</code> are updated, but never queried #loc
|
||||
mismatched.update.collection.problem.description.no.effect.updates=Update operations on empty collection <code>#ref</code> have no effect #loc
|
||||
mismatched.update.collection.problem.description.updated.not.queried=Contents of collection <code>#ref</code> are updated, but never queried #loc
|
||||
mismatched.update.collection.problem.description.queried.empty=Contents of empty collection <code>#ref</code> are queried, but it's never populated #loc
|
||||
mismatched.update.collection.problem.description.queried.not.updated=Contents of collection <code>#ref</code> are queried, but never updated #loc
|
||||
rename.quickfix=Rename
|
||||
renameto.quickfix=Rename to ''{0}''
|
||||
|
||||
@@ -20,7 +20,6 @@ import com.intellij.psi.*;
|
||||
import com.intellij.psi.util.InheritanceUtil;
|
||||
import com.intellij.psi.util.PsiUtil;
|
||||
import com.intellij.util.ObjectUtils;
|
||||
import com.intellij.util.containers.ContainerUtil;
|
||||
import com.siyeh.ig.callMatcher.CallMatcher;
|
||||
import org.jetbrains.annotations.Contract;
|
||||
import org.jetbrains.annotations.NonNls;
|
||||
@@ -45,7 +44,8 @@ public final class CollectionUtils {
|
||||
CallMatcher.anyOf(
|
||||
CallMatcher.instanceCall(CommonClassNames.JAVA_UTIL_MAP, "keySet", "values", "entrySet").parameterCount(0),
|
||||
CallMatcher.instanceCall("java.util.NavigableMap", "descendingKeySet", "descendingMap", "navigableKeySet").parameterCount(0),
|
||||
CallMatcher.instanceCall("java.util.NavigableSet", "descendingSet").parameterCount(0)
|
||||
CallMatcher.instanceCall("java.util.NavigableSet", "descendingSet").parameterCount(0),
|
||||
CallMatcher.instanceCall("java.util.SequencedCollection", "reversed").parameterCount(0)
|
||||
);
|
||||
|
||||
/**
|
||||
|
||||
+166
-118
@@ -41,7 +41,6 @@ import org.jetbrains.annotations.NonNls;
|
||||
import org.jetbrains.annotations.NotNull;
|
||||
import org.jetbrains.annotations.Nullable;
|
||||
|
||||
import java.util.Collections;
|
||||
import java.util.HashSet;
|
||||
import java.util.List;
|
||||
import java.util.Set;
|
||||
@@ -58,6 +57,9 @@ public final class MismatchedCollectionQueryUpdateInspection extends BaseInspect
|
||||
"synchronizedSortedMap", "synchronizedSortedSet");
|
||||
private static final CallMatcher DERIVED = CallMatcher.anyOf(
|
||||
CollectionUtils.DERIVED_COLLECTION,
|
||||
CallMatcher.instanceCall(CommonClassNames.JAVA_UTIL_COLLECTION, "iterator").parameterCount(0),
|
||||
CallMatcher.instanceCall(CommonClassNames.JAVA_UTIL_LIST, "listIterator").parameterCount(0),
|
||||
CallMatcher.instanceCall(CommonClassNames.JAVA_UTIL_LIST, "listIterator").parameterTypes("int"),
|
||||
CallMatcher.instanceCall(CommonClassNames.JAVA_UTIL_LIST, "subList"),
|
||||
CallMatcher.instanceCall("java.util.SortedMap", "headMap", "tailMap", "subMap"),
|
||||
CallMatcher.instanceCall(CommonClassNames.JAVA_UTIL_SORTED_SET, "headSet", "tailSet", "subSet"));
|
||||
@@ -71,6 +73,19 @@ public final class MismatchedCollectionQueryUpdateInspection extends BaseInspect
|
||||
private static final @NonNls Set<String> COLLECTIONS_UPDATES = Set.of("addAll", "fill", "copy", "replaceAll", "sort");
|
||||
private static final Set<String> COLLECTIONS_ALL =
|
||||
StreamEx.of(COLLECTIONS_QUERIES).append(COLLECTIONS_UPDATES).toImmutableSet();
|
||||
private static final @NotNull CallMatcher QUERY_ITERATOR_METHODS =
|
||||
CallMatcher.anyOf(
|
||||
CallMatcher.instanceCall(CommonClassNames.JAVA_UTIL_ITERATOR, "hasNext", "next", "forEachRemaining"),
|
||||
CallMatcher.instanceCall("java.util.ListIterator", "hasPrevious", "previous", "nextIndex", "previousIndex")
|
||||
);
|
||||
private static final @NotNull CallMatcher NO_UPDATE_FOR_EMPTY =
|
||||
CallMatcher.anyOf(
|
||||
CallMatcher.instanceCall(CommonClassNames.JAVA_UTIL_ITERATOR, "remove").parameterCount(0),
|
||||
CallMatcher.instanceCall("java.util.ListIterator", "set").parameterCount(1),
|
||||
CallMatcher.instanceCall(CommonClassNames.JAVA_UTIL_COLLECTION, "remove", "clear", "removeAll", "retainAll", "removeIf"),
|
||||
CallMatcher.instanceCall(CommonClassNames.JAVA_UTIL_LIST, "sort", "replaceAll"),
|
||||
CallMatcher.instanceCall(CommonClassNames.JAVA_UTIL_MAP, "replaceAll", "replace", "remove")
|
||||
);
|
||||
private static final Set<String> defaultQueryNames =
|
||||
Set.of("contains", "copyInto", "equals", "forEach", "get", "hashCode", "indexOf", "iterator", "lastIndexOf", "parallelStream", "peek",
|
||||
"propertyNames", "save", "size", "store", "stream", "toArray", "toString", "write");
|
||||
@@ -86,6 +101,8 @@ public final class MismatchedCollectionQueryUpdateInspection extends BaseInspect
|
||||
@SuppressWarnings("PublicField")
|
||||
public final ExternalizableStringSet ignoredClasses = new ExternalizableStringSet();
|
||||
|
||||
private final MismatchedQueryUpdateRunner myRunner = new MismatchedQueryUpdateRunner(queryNames, updateNames, ignoredClasses);
|
||||
|
||||
@Override
|
||||
public @NotNull OptPane getOptionsPane() {
|
||||
return pane(
|
||||
@@ -107,13 +124,7 @@ public final class MismatchedCollectionQueryUpdateInspection extends BaseInspect
|
||||
|
||||
@Override
|
||||
public @NotNull String buildErrorString(Object... infos) {
|
||||
final boolean updated = ((Boolean)infos[0]).booleanValue();
|
||||
if (updated) {
|
||||
return InspectionGadgetsBundle.message("mismatched.update.collection.problem.descriptor.updated.not.queried");
|
||||
}
|
||||
else {
|
||||
return InspectionGadgetsBundle.message("mismatched.update.collection.problem.description.queried.not.updated");
|
||||
}
|
||||
return (String)infos[0];
|
||||
}
|
||||
|
||||
@Override
|
||||
@@ -131,46 +142,84 @@ public final class MismatchedCollectionQueryUpdateInspection extends BaseInspect
|
||||
return new MismatchedCollectionQueryUpdateVisitor();
|
||||
}
|
||||
|
||||
private static QueryUpdateInfo getCollectionQueryUpdateInfo(@Nullable PsiVariable variable,
|
||||
PsiElement context,
|
||||
Set<String> queryNames,
|
||||
Set<String> updateNames) {
|
||||
return getCollectionQueryUpdateInfo(variable, context, queryNames, updateNames, Set.of());
|
||||
}
|
||||
private static QueryUpdateInfo getCollectionQueryUpdateInfo(@Nullable PsiVariable variable,
|
||||
PsiElement context,
|
||||
Set<String> queryNames,
|
||||
Set<String> updateNames,
|
||||
Set<PsiReferenceExpression> ignored) {
|
||||
QueryUpdateInfo info = new QueryUpdateInfo();
|
||||
Visitor visitor = new Visitor(variable, queryNames, updateNames, info, ignored);
|
||||
context.accept(visitor);
|
||||
return info;
|
||||
private record MismatchedQueryUpdateRunner(
|
||||
@NotNull Set<String> queryNames,
|
||||
@NotNull Set<String> updateNames,
|
||||
@NotNull Set<String> ignoredClasses
|
||||
) {
|
||||
@NotNull QueryUpdateInfo compute(@Nullable PsiVariable variable,
|
||||
@Nullable PsiElement context,
|
||||
@NotNull Set<PsiReferenceExpression> ignored) {
|
||||
if (context == null) return QueryUpdateInfo.UNKNOWN;
|
||||
if (variable != null && !checkVariable(variable)) {
|
||||
return QueryUpdateInfo.UNKNOWN;
|
||||
}
|
||||
Visitor visitor = new Visitor(variable, queryNames, updateNames, ignored);
|
||||
context.accept(visitor);
|
||||
return visitor.getInfo();
|
||||
}
|
||||
|
||||
@NotNull QueryUpdateInfo compute(@NotNull PsiVariable variable, @NotNull Set<PsiReferenceExpression> ignored) {
|
||||
final PsiElement context = variable instanceof PsiField ? PsiUtil.getTopLevelClass(variable) :
|
||||
PsiTreeUtil.getParentOfType(variable, PsiCodeBlock.class);
|
||||
return compute(variable, context, ignored);
|
||||
}
|
||||
|
||||
@NotNull QueryUpdateInfo processSingleRef(@NotNull PsiExpression ref) {
|
||||
Visitor visitor = new Visitor(null, queryNames, updateNames, Set.of());
|
||||
visitor.process(ref);
|
||||
return visitor.getInfo();
|
||||
}
|
||||
|
||||
private boolean checkVariable(@NotNull PsiVariable variable) {
|
||||
if (variable instanceof PsiField field) {
|
||||
if (!field.hasModifierProperty(PsiModifier.PRIVATE)) {
|
||||
PsiClass aClass = field.getContainingClass();
|
||||
if (aClass == null || !aClass.hasModifierProperty(PsiModifier.PRIVATE) || field.hasModifierProperty(PsiModifier.PUBLIC)) {
|
||||
// Public field within private class can be written/read via reflection even without setAccessible hacks,
|
||||
// so we don't analyze such fields to reduce false-positives
|
||||
return false;
|
||||
}
|
||||
}
|
||||
}
|
||||
else if (!(variable instanceof PsiLocalVariable)) {
|
||||
return false;
|
||||
}
|
||||
final PsiType type = variable.getType();
|
||||
if (!CollectionUtils.isCollectionClassOrInterface(type)) {
|
||||
return false;
|
||||
}
|
||||
return !ContainerUtil.exists(ignoredClasses, className -> InheritanceUtil.isInheritor(type, className));
|
||||
}
|
||||
}
|
||||
|
||||
private static class Visitor extends JavaRecursiveElementWalkingVisitor {
|
||||
final PsiVariable myVariable;
|
||||
final Set<String> myQueryNames;
|
||||
final Set<String> myUpdateNames;
|
||||
final QueryUpdateInfo myInfo;
|
||||
final Set<PsiReferenceExpression> myIgnored = new HashSet<>();
|
||||
final @Nullable PsiVariable myVariable;
|
||||
final @NotNull Set<PsiVariable> myDerivedVariables = new HashSet<>();
|
||||
final @NotNull Set<String> myQueryNames;
|
||||
final @NotNull Set<String> myUpdateNames;
|
||||
final @NotNull Set<PsiReferenceExpression> myIgnored = new HashSet<>();
|
||||
boolean myQueried;
|
||||
boolean myUpdated;
|
||||
boolean myUpdatedForNonEmpty;
|
||||
boolean myKnownEmpty;
|
||||
|
||||
private Visitor(@Nullable PsiVariable variable,
|
||||
@NotNull Set<String> queryNames,
|
||||
@NotNull Set<String> updateNames,
|
||||
@NotNull QueryUpdateInfo info) {
|
||||
@NotNull Set<PsiReferenceExpression> ignored) {
|
||||
myVariable = variable;
|
||||
myQueryNames = queryNames;
|
||||
myUpdateNames = updateNames;
|
||||
myInfo = info;
|
||||
}
|
||||
private Visitor(@Nullable PsiVariable variable,
|
||||
@NotNull Set<String> queryNames,
|
||||
@NotNull Set<String> updateNames,
|
||||
@NotNull QueryUpdateInfo info,
|
||||
@NotNull Set<PsiReferenceExpression> ignored) {
|
||||
this(variable, queryNames, updateNames, info);
|
||||
myIgnored.addAll(ignored);
|
||||
if (variable != null) {
|
||||
myKnownEmpty = definitelyEmptyCollection(variable.getInitializer());
|
||||
}
|
||||
}
|
||||
|
||||
private static boolean definitelyEmptyCollection(@Nullable PsiExpression initializer) {
|
||||
return initializer == null || ExpressionUtils.nonStructuralChildren(initializer)
|
||||
.allMatch(e -> ConstructionUtils.isEmptyCollectionInitializer(e));
|
||||
}
|
||||
|
||||
@Override
|
||||
@@ -181,8 +230,13 @@ public final class MismatchedCollectionQueryUpdateInspection extends BaseInspect
|
||||
makeUpdated();
|
||||
makeQueried();
|
||||
}
|
||||
} else if (ref.isReferenceTo(myVariable) && !myIgnored.contains(ref)) {
|
||||
process(findEffectiveReference(ref));
|
||||
}
|
||||
else {
|
||||
if (ref.resolve() instanceof PsiVariable target) {
|
||||
if ((target.equals(myVariable) || myDerivedVariables.contains(target)) && !myIgnored.contains(ref)) {
|
||||
process(ref);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -190,25 +244,37 @@ public final class MismatchedCollectionQueryUpdateInspection extends BaseInspect
|
||||
public void visitThisExpression(@NotNull PsiThisExpression expression) {
|
||||
super.visitThisExpression(expression);
|
||||
if (myVariable == null) {
|
||||
process(findEffectiveReference(expression));
|
||||
process(expression);
|
||||
}
|
||||
}
|
||||
|
||||
private void makeUpdated() {
|
||||
myInfo.updated = true;
|
||||
if (myInfo.queried) {
|
||||
myUpdated = true;
|
||||
if (myQueried) {
|
||||
stopWalking();
|
||||
}
|
||||
}
|
||||
|
||||
private void makeUpdatedForNonEmpty() {
|
||||
myUpdatedForNonEmpty = true;
|
||||
}
|
||||
|
||||
private void makeQueried() {
|
||||
myInfo.queried = true;
|
||||
if (myInfo.updated) {
|
||||
myQueried = true;
|
||||
if (myUpdated) {
|
||||
stopWalking();
|
||||
}
|
||||
}
|
||||
|
||||
@NotNull QueryUpdateInfo getInfo() {
|
||||
return new QueryUpdateInfo(myQueried, myUpdated, myUpdatedForNonEmpty, myKnownEmpty);
|
||||
}
|
||||
|
||||
private void process(PsiExpression reference) {
|
||||
doProcess(findEffectiveReference(reference));
|
||||
}
|
||||
|
||||
private void doProcess(PsiExpression reference) {
|
||||
PsiMethodCallExpression qualifiedCall = ExpressionUtils.getCallForQualifier(reference);
|
||||
if (qualifiedCall != null) {
|
||||
processQualifiedCall(qualifiedCall);
|
||||
@@ -228,6 +294,10 @@ public final class MismatchedCollectionQueryUpdateInspection extends BaseInspect
|
||||
}
|
||||
return;
|
||||
}
|
||||
if (myVariable != null && parent instanceof PsiLocalVariable derived && !VariableAccessUtils.variableIsAssigned(derived)) {
|
||||
myDerivedVariables.add(derived);
|
||||
return;
|
||||
}
|
||||
if (parent instanceof PsiMethodReferenceExpression methodReference) {
|
||||
processQualifiedMethodReference(methodReference);
|
||||
return;
|
||||
@@ -237,11 +307,12 @@ public final class MismatchedCollectionQueryUpdateInspection extends BaseInspect
|
||||
return;
|
||||
}
|
||||
if (parent instanceof PsiAssignmentExpression assignment && assignment.getLExpression() == reference) {
|
||||
process(findEffectiveReference(assignment));
|
||||
process(assignment);
|
||||
PsiExpression rValue = assignment.getRExpression();
|
||||
if (rValue == null) return;
|
||||
if (ExpressionUtils.nonStructuralChildren(rValue)
|
||||
.allMatch(MismatchedCollectionQueryUpdateInspection::isEmptyCollectionInitializer)) {
|
||||
myKnownEmpty = myKnownEmpty && definitelyEmptyCollection(rValue);
|
||||
return;
|
||||
}
|
||||
if (ExpressionUtils.nonStructuralChildren(rValue)
|
||||
@@ -312,6 +383,14 @@ public final class MismatchedCollectionQueryUpdateInspection extends BaseInspect
|
||||
private void processQualifiedCall(PsiMethodCallExpression call) {
|
||||
boolean voidContext = ExpressionUtils.isVoidContext(call);
|
||||
String name = call.getMethodExpression().getReferenceName();
|
||||
PsiExpression qualifier = call.getMethodExpression().getQualifierExpression();
|
||||
if (qualifier != null && InheritanceUtil.isInheritor(qualifier.getType(), CommonClassNames.JAVA_UTIL_ITERATOR)) {
|
||||
makeQueried();
|
||||
if (!QUERY_ITERATOR_METHODS.test(call)) {
|
||||
makeUpdatedCall(call);
|
||||
}
|
||||
return;
|
||||
}
|
||||
boolean queryQualifier = isQueryUpdateMethodName(name, myQueryNames);
|
||||
boolean updateQualifier = isQueryUpdateMethodName(name, myUpdateNames);
|
||||
if (queryQualifier &&
|
||||
@@ -319,7 +398,7 @@ public final class MismatchedCollectionQueryUpdateInspection extends BaseInspect
|
||||
makeQueried();
|
||||
}
|
||||
if (updateQualifier) {
|
||||
makeUpdated();
|
||||
makeUpdatedCall(call);
|
||||
if (!voidContext) {
|
||||
makeQueried();
|
||||
}
|
||||
@@ -356,6 +435,15 @@ public final class MismatchedCollectionQueryUpdateInspection extends BaseInspect
|
||||
}
|
||||
}
|
||||
|
||||
private void makeUpdatedCall(PsiMethodCallExpression call) {
|
||||
if (NO_UPDATE_FOR_EMPTY.test(call)) {
|
||||
makeUpdatedForNonEmpty();
|
||||
}
|
||||
else {
|
||||
makeUpdated();
|
||||
}
|
||||
}
|
||||
|
||||
private static boolean mayHaveSideEffect(PsiExpression fn) {
|
||||
if (fn instanceof PsiLambdaExpression) {
|
||||
PsiElement body = ((PsiLambdaExpression)fn).getBody();
|
||||
@@ -474,51 +562,55 @@ public final class MismatchedCollectionQueryUpdateInspection extends BaseInspect
|
||||
return immutable && !SideEffectChecker.mayHaveSideEffects(call);
|
||||
}
|
||||
|
||||
private static class QueryUpdateInfo {
|
||||
boolean updated;
|
||||
boolean queried;
|
||||
private record QueryUpdateInfo(boolean queried, boolean updated, boolean updatedForNonEmpty, boolean knownEmpty) {
|
||||
static final QueryUpdateInfo UNKNOWN = new QueryUpdateInfo(true, true, true, false);
|
||||
}
|
||||
|
||||
private class MismatchedCollectionQueryUpdateVisitor extends BaseInspectionVisitor {
|
||||
private void register(PsiVariable variable, boolean written) {
|
||||
private void check(@NotNull PsiVariable variable) {
|
||||
QueryUpdateInfo info = myRunner.compute(variable, Set.of());
|
||||
final boolean written = info.updated || updatedViaInitializer(variable);
|
||||
final boolean read = info.queried || queriedViaInitializer(variable);
|
||||
if (!written && info.updatedForNonEmpty && info.knownEmpty) {
|
||||
registerVariableError(variable,
|
||||
InspectionGadgetsBundle.message("mismatched.update.collection.problem.description.no.effect.updates"));
|
||||
return;
|
||||
}
|
||||
if (read == written) {
|
||||
return;
|
||||
}
|
||||
String message = written ? InspectionGadgetsBundle.message("mismatched.update.collection.problem.description.updated.not.queried") :
|
||||
info.knownEmpty ? InspectionGadgetsBundle.message(
|
||||
"mismatched.update.collection.problem.description.queried.empty") :
|
||||
InspectionGadgetsBundle.message("mismatched.update.collection.problem.description.queried.not.updated");
|
||||
if (written) {
|
||||
PsiExpression initializer = variable.getInitializer();
|
||||
if (initializer != null) {
|
||||
List<PsiExpression> expressions = ExpressionUtils.nonStructuralChildren(initializer).toList();
|
||||
if (!ContainerUtil.and(expressions, MismatchedCollectionQueryUpdateInspection::isCollectionInitializer)) {
|
||||
expressions.stream().filter(MismatchedCollectionQueryUpdateInspection::isCollectionInitializer)
|
||||
.forEach(emptyCollection -> registerError(emptyCollection, Boolean.TRUE));
|
||||
.forEach(emptyCollection -> registerError(emptyCollection, message));
|
||||
return;
|
||||
}
|
||||
}
|
||||
}
|
||||
registerVariableError(variable, written);
|
||||
registerVariableError(variable, message);
|
||||
}
|
||||
|
||||
@Override
|
||||
public void visitField(@NotNull PsiField field) {
|
||||
super.visitField(field);
|
||||
QueryUpdateInfo info = getCollectionQueryUpdateInfo(field, updateNames, queryNames, ignoredClasses);
|
||||
if (info == null) return;
|
||||
final boolean written = info.updated || updatedViaInitializer(field);
|
||||
final boolean read = info.queried || queriedViaInitializer(field);
|
||||
if (read == written || UnusedSymbolUtil.isImplicitWrite(field) || UnusedSymbolUtil.isImplicitRead(field)) {
|
||||
if (UnusedSymbolUtil.isImplicitWrite(field) || UnusedSymbolUtil.isImplicitRead(field)) {
|
||||
// Even implicit read of the mutable collection field may cause collection change
|
||||
return;
|
||||
}
|
||||
register(field, written);
|
||||
check(field);
|
||||
}
|
||||
|
||||
@Override
|
||||
public void visitLocalVariable(@NotNull PsiLocalVariable variable) {
|
||||
super.visitLocalVariable(variable);
|
||||
QueryUpdateInfo info = getCollectionQueryUpdateInfo(variable, updateNames, queryNames, ignoredClasses, Set.of());
|
||||
if (info == null) return;
|
||||
final boolean written = info.updated || updatedViaInitializer(variable);
|
||||
final boolean read = info.queried || queriedViaInitializer(variable);
|
||||
if (read != written) {
|
||||
register(variable, written);
|
||||
}
|
||||
check(variable);
|
||||
}
|
||||
|
||||
private boolean updatedViaInitializer(PsiVariable variable) {
|
||||
@@ -531,7 +623,7 @@ public final class MismatchedCollectionQueryUpdateInspection extends BaseInspect
|
||||
if (initializer instanceof PsiNewExpression newExpression) {
|
||||
final PsiAnonymousClass anonymousClass = newExpression.getAnonymousClass();
|
||||
if (anonymousClass != null) {
|
||||
if (getCollectionQueryUpdateInfo(null, anonymousClass, queryNames, updateNames).updated) {
|
||||
if (myRunner.compute(null, anonymousClass, Set.of()).updated) {
|
||||
return true;
|
||||
}
|
||||
final ThisPassedAsArgumentVisitor visitor = new ThisPassedAsArgumentVisitor();
|
||||
@@ -552,48 +644,6 @@ public final class MismatchedCollectionQueryUpdateInspection extends BaseInspect
|
||||
}
|
||||
}
|
||||
|
||||
private static boolean checkVariable(PsiVariable variable, PsiElement context, Set<String> ignoredClasses) {
|
||||
if (context == null) {
|
||||
return false;
|
||||
}
|
||||
final PsiType type = variable.getType();
|
||||
if (!CollectionUtils.isCollectionClassOrInterface(type)) {
|
||||
return false;
|
||||
}
|
||||
return !ContainerUtil.exists(ignoredClasses, className -> InheritanceUtil.isInheritor(type, className));
|
||||
}
|
||||
|
||||
private static QueryUpdateInfo getCollectionQueryUpdateInfo(@NotNull PsiField field,
|
||||
Set<String> updateNames,
|
||||
Set<String> queryNames,
|
||||
Set<String> ignoredClasses) {
|
||||
if (!field.hasModifierProperty(PsiModifier.PRIVATE)) {
|
||||
PsiClass aClass = field.getContainingClass();
|
||||
if (aClass == null || !aClass.hasModifierProperty(PsiModifier.PRIVATE) || field.hasModifierProperty(PsiModifier.PUBLIC)) {
|
||||
// Public field within private class can be written/read via reflection even without setAccessible hacks,
|
||||
// so we don't analyze such fields to reduce false-positives
|
||||
return null;
|
||||
}
|
||||
}
|
||||
final PsiClass containingClass = PsiUtil.getTopLevelClass(field);
|
||||
if (!checkVariable(field, containingClass, ignoredClasses)) {
|
||||
return null;
|
||||
}
|
||||
return getCollectionQueryUpdateInfo(field, containingClass, queryNames, updateNames);
|
||||
}
|
||||
|
||||
private static QueryUpdateInfo getCollectionQueryUpdateInfo(@NotNull PsiLocalVariable variable,
|
||||
Set<String> updateNames,
|
||||
Set<String> queryNames,
|
||||
Set<String> ignoredClasses,
|
||||
Set<PsiReferenceExpression> ignored) {
|
||||
final PsiCodeBlock codeBlock = PsiTreeUtil.getParentOfType(variable, PsiCodeBlock.class);
|
||||
if (!checkVariable(variable, codeBlock, ignoredClasses)) {
|
||||
return null;
|
||||
}
|
||||
return getCollectionQueryUpdateInfo(variable, codeBlock, queryNames, updateNames, ignored);
|
||||
}
|
||||
|
||||
/**
|
||||
* @param collectCall a method call returning a list (e.g. {@code Stream.of(1, 2, 3).collect(Collectors.toList())})
|
||||
* @return {@code true} if it's known that the result of {@code collectCall} is never updated;
|
||||
@@ -602,10 +652,10 @@ public final class MismatchedCollectionQueryUpdateInspection extends BaseInspect
|
||||
@Contract("null -> false")
|
||||
public static boolean isUnmodified(@Nullable PsiMethodCallExpression collectCall) {
|
||||
if (collectCall == null) return false;
|
||||
final PsiExpression effectiveReference = findEffectiveReference(collectCall);
|
||||
QueryUpdateInfo info = new QueryUpdateInfo();
|
||||
new Visitor(null, defaultQueryNames, defaultUpdateNames, info).process(effectiveReference);
|
||||
if (!info.updated) return true;
|
||||
final PsiExpression effectiveReference = findEffectiveReference(collectCall);
|
||||
MismatchedQueryUpdateRunner runner = new MismatchedQueryUpdateRunner(defaultQueryNames, defaultUpdateNames, Set.of());
|
||||
QueryUpdateInfo info = runner.processSingleRef(collectCall);
|
||||
if (!info.updated && !info.updatedForNonEmpty) return true;
|
||||
PsiElement parent = effectiveReference.getParent();
|
||||
Set<PsiReferenceExpression> ignored = new HashSet<>();
|
||||
if (parent instanceof PsiAssignmentExpression assignmentExpression) {
|
||||
@@ -614,12 +664,10 @@ public final class MismatchedCollectionQueryUpdateInspection extends BaseInspect
|
||||
parent = referenceExpression.resolve();
|
||||
}
|
||||
}
|
||||
if (!(parent instanceof PsiLocalVariable || parent instanceof PsiField)) return false;
|
||||
if (parent instanceof PsiField) {
|
||||
info = getCollectionQueryUpdateInfo((PsiField)parent, defaultUpdateNames, defaultQueryNames, Collections.emptySet());
|
||||
} else {
|
||||
info = getCollectionQueryUpdateInfo((PsiLocalVariable)parent, defaultUpdateNames, defaultQueryNames, Collections.emptySet(), ignored);
|
||||
if (parent instanceof PsiVariable variable) {
|
||||
info = runner.compute(variable, variable instanceof PsiField ? Set.of() : ignored);
|
||||
return !info.updated && !info.updatedForNonEmpty;
|
||||
}
|
||||
return info != null && !info.updated;
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
+50
-16
@@ -22,8 +22,8 @@ import java.util.concurrent.*;
|
||||
|
||||
public class MismatchedCollectionQueryUpdate {
|
||||
private Set foo = new HashSet();
|
||||
private Set <warning descr="Contents of collection 'foo2' are queried, but never updated">foo2</warning> = new HashSet();
|
||||
private Set <warning descr="Contents of collection 'bar' are queried, but never updated">bar</warning> ;
|
||||
private Set <warning descr="Contents of empty collection 'foo2' are queried, but it's never populated">foo2</warning> = new HashSet();
|
||||
private Set <warning descr="Contents of empty collection 'bar' are queried, but it's never populated">bar</warning> ;
|
||||
private Set bar2 = new HashSet(foo2);
|
||||
private Set bal ;
|
||||
private Set<String> injected = new HashSet<>();
|
||||
@@ -86,7 +86,7 @@ public class MismatchedCollectionQueryUpdate {
|
||||
}
|
||||
|
||||
Object[] testRemoveFromAnotherCollection(List<List<String>> list) {
|
||||
List<String> <warning descr="Contents of collection 'l' are queried, but never updated">l</warning> = new ArrayList<>();
|
||||
List<String> <warning descr="Contents of empty collection 'l' are queried, but it's never populated">l</warning> = new ArrayList<>();
|
||||
list.remove(l);
|
||||
process(list);
|
||||
return l.toArray();
|
||||
@@ -95,7 +95,7 @@ public class MismatchedCollectionQueryUpdate {
|
||||
native void process(List<List<String>> list);
|
||||
|
||||
void testPureMethod() {
|
||||
List<String> <warning descr="Contents of collection 'list' are queried, but never updated">list</warning> = new ArrayList<>();
|
||||
List<String> <warning descr="Contents of empty collection 'list' are queried, but it's never populated">list</warning> = new ArrayList<>();
|
||||
if(hasNull(list)) {
|
||||
System.out.println("has nulls!");
|
||||
}
|
||||
@@ -134,13 +134,13 @@ public class MismatchedCollectionQueryUpdate {
|
||||
}
|
||||
|
||||
boolean testSeparateInitialization() {
|
||||
List<String> <warning descr="Contents of collection 'list' are queried, but never updated">list</warning>;
|
||||
List<String> <warning descr="Contents of empty collection 'list' are queried, but it's never populated">list</warning>;
|
||||
list = new ArrayList<>();
|
||||
return list.isEmpty();
|
||||
}
|
||||
|
||||
void testKeySet() {
|
||||
Map<String, String> <warning descr="Contents of collection 'map' are queried, but never updated">map</warning> = new HashMap<>();
|
||||
Map<String, String> <warning descr="Contents of empty collection 'map' are queried, but it's never populated">map</warning> = new HashMap<>();
|
||||
for(String s : map.keySet()) {
|
||||
System.out.println(s);
|
||||
}
|
||||
@@ -163,18 +163,40 @@ public class MismatchedCollectionQueryUpdate {
|
||||
System.out.println(i.next());
|
||||
}
|
||||
|
||||
void testListIterator2() {
|
||||
List<String> <warning descr="Contents of empty collection 'test' are queried, but it's never populated">test</warning> = new ArrayList<String>();
|
||||
ListIterator<String> i = test.listIterator(0);
|
||||
System.out.println(i.next());
|
||||
}
|
||||
|
||||
void testListIterator3() {
|
||||
List<String> <warning descr="Update operations on empty collection 'test' have no effect">test</warning> = new ArrayList<String>();
|
||||
ListIterator<String> iter = test.listIterator();
|
||||
while(iter.hasNext()) {
|
||||
iter.next();
|
||||
iter.set("foo");
|
||||
}
|
||||
}
|
||||
|
||||
void testIterator() {
|
||||
List<String> <warning descr="Contents of collection 'test' are queried, but never updated">test</warning> = new ArrayList<String>();
|
||||
List<String> <warning descr="Update operations on empty collection 'test' have no effect">test</warning> = new ArrayList<String>();
|
||||
Iterator<String> i = test.iterator();
|
||||
while(i.hasNext()) {
|
||||
if(i.next() == null) {
|
||||
// Normally iterator cannot add, remove only. If collection is always empty (not updated in any other way),
|
||||
// this is useless anyways
|
||||
i.remove();
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
void derivedWrite() {
|
||||
List<String> <warning descr="Contents of collection 'list' are updated, but never queried">list</warning> = new ArrayList<>();
|
||||
for (int i = 0; i < 10; i++) {
|
||||
list.add(String.valueOf(i));
|
||||
}
|
||||
List<String> subList = list.subList(0, 10);
|
||||
subList.add("Hi!");
|
||||
}
|
||||
|
||||
void add(TestPrivateClass tpc) {
|
||||
tpc.field.add("foo");
|
||||
}
|
||||
@@ -300,8 +322,8 @@ public class MismatchedCollectionQueryUpdate {
|
||||
}
|
||||
|
||||
void methodArgument() {
|
||||
List<String> <warning descr="Contents of collection 'foos' are updated, but never queried">foos</warning> = new ArrayList<>();
|
||||
List<String> <warning descr="Contents of collection 'bars' are queried, but never updated">bars</warning> = new ArrayList<>();
|
||||
List<String> <warning descr="Update operations on empty collection 'foos' have no effect">foos</warning> = new ArrayList<>();
|
||||
List<String> <warning descr="Contents of empty collection 'bars' are queried, but it's never populated">bars</warning> = new ArrayList<>();
|
||||
|
||||
foos.removeAll( bars );
|
||||
|
||||
@@ -310,6 +332,18 @@ public class MismatchedCollectionQueryUpdate {
|
||||
m(other);
|
||||
}
|
||||
|
||||
private final List<String> <warning descr="Update operations on empty collection 'noAdd' have no effect">noAdd</warning> = new ArrayList<>();
|
||||
void updateEmptyAndQuery() {
|
||||
noAdd.remove("xyz");
|
||||
if (noAdd.indexOf("a") > 10) {
|
||||
noAdd.remove("b");
|
||||
}
|
||||
}
|
||||
|
||||
void sortEmpty() {
|
||||
noAdd.sort(Comparator.naturalOrder());
|
||||
}
|
||||
|
||||
void m(List l) {}
|
||||
|
||||
void initializationInsideMethodCall() {
|
||||
@@ -347,7 +381,7 @@ public class MismatchedCollectionQueryUpdate {
|
||||
|
||||
public void foofoo()
|
||||
{
|
||||
final Map<String, String> <warning descr="Contents of collection 'anotherMap' are queried, but never updated">anotherMap</warning> = new HashMap<String, String>();
|
||||
final Map<String, String> <warning descr="Contents of empty collection 'anotherMap' are queried, but it's never populated">anotherMap</warning> = new HashMap<String, String>();
|
||||
final SortedMap<String, String> map = new TreeMap<String, String>(anotherMap);
|
||||
final Iterator<String> it = map.keySet().iterator();
|
||||
while(it.hasNext()){
|
||||
@@ -444,12 +478,12 @@ class CollectionsUser {
|
||||
}
|
||||
|
||||
void c() {
|
||||
List<String> <warning descr="Contents of collection 'l' are queried, but never updated">l</warning> = new ArrayList();
|
||||
List<String> <warning descr="Contents of empty collection 'l' are queried, but it's never populated">l</warning> = new ArrayList();
|
||||
final int frequency = Collections.frequency(l, "one");
|
||||
}
|
||||
|
||||
List<String> d() {
|
||||
List<String> <warning descr="Contents of collection 'l' are queried, but never updated">l</warning> = new ArrayList();
|
||||
List<String> <warning descr="Contents of empty collection 'l' are queried, but it's never populated">l</warning> = new ArrayList();
|
||||
return Collections.unmodifiableList(l);
|
||||
}
|
||||
|
||||
@@ -573,7 +607,7 @@ class SomeList extends ArrayList<Integer> {
|
||||
|
||||
class UnmodifiableTernaryTest {
|
||||
private final List<String> myList = new ArrayList<>();
|
||||
private final List<String> <warning descr="Contents of collection 'myList2' are queried, but never updated">myList2</warning> = new ArrayList<>();
|
||||
private final List<String> <warning descr="Contents of empty collection 'myList2' are queried, but it's never populated">myList2</warning> = new ArrayList<>();
|
||||
|
||||
void add() {
|
||||
myList.add("foo");
|
||||
@@ -609,7 +643,7 @@ class InLambdaTest {
|
||||
|
||||
class AssertTest {
|
||||
void test() {
|
||||
List<String> <warning descr="Contents of collection 'list' are queried, but never updated">list</warning> = new ArrayList<>();
|
||||
List<String> <warning descr="Contents of empty collection 'list' are queried, but it's never populated">list</warning> = new ArrayList<>();
|
||||
assert list.isEmpty() : list;
|
||||
}
|
||||
}
|
||||
|
||||
+23
@@ -164,6 +164,29 @@ class TernaryTest {
|
||||
}
|
||||
}
|
||||
|
||||
// IDEA-356315
|
||||
class MissingIteratorRemoveAnalysis {
|
||||
public void problem() {
|
||||
final List<String> strings = Stream.of("anything", "at", "all")
|
||||
.collect(Collectors.toList());
|
||||
|
||||
for(Iterator<String> iterator = strings.iterator(); iterator.hasNext(); ) {
|
||||
String string = iterator.next();
|
||||
iterator.remove();
|
||||
}
|
||||
}
|
||||
|
||||
public void noRemove() {
|
||||
final List<String> strings = Stream.of("anything", "at", "all")
|
||||
.<warning descr="'collect(toList())' can be replaced with 'toList()'">collect(Collectors.toList())</warning>;
|
||||
|
||||
for(Iterator<String> iterator = strings.iterator(); iterator.hasNext(); ) {
|
||||
String string = iterator.next();
|
||||
System.out.println(string);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
//class TODO() {
|
||||
//
|
||||
// void testMustHave1(Stream<String> stream) {
|
||||
|
||||
Reference in New Issue
Block a user