diff --git a/java/java-impl/src/com/intellij/codeInspection/IdempotentLoopBodyInspection.java b/java/java-impl/src/com/intellij/codeInspection/IdempotentLoopBodyInspection.java index 6532f323f665..7b3f16c75278 100644 --- a/java/java-impl/src/com/intellij/codeInspection/IdempotentLoopBodyInspection.java +++ b/java/java-impl/src/com/intellij/codeInspection/IdempotentLoopBodyInspection.java @@ -2,17 +2,14 @@ package com.intellij.codeInspection; import com.intellij.psi.*; +import com.intellij.psi.controlFlow.*; import com.intellij.psi.util.PsiUtil; import com.siyeh.ig.psiutils.SideEffectChecker; -import com.siyeh.ig.psiutils.VariableAccessUtils; import one.util.streamex.StreamEx; import org.jetbrains.annotations.NotNull; -import org.jetbrains.annotations.Nullable; -import java.util.Collections; -import java.util.HashSet; -import java.util.Set; -import java.util.function.Function; +import java.util.Collection; +import java.util.List; import static com.intellij.util.ObjectUtils.tryCast; @@ -25,147 +22,71 @@ public class IdempotentLoopBodyInspection extends AbstractBaseJavaLocalInspectio public void visitWhileStatement(PsiWhileStatement loop) { PsiExpression condition = loop.getCondition(); if (condition == null || SideEffectChecker.mayHaveSideEffects(condition)) return; - if (isIdempotent(loop.getBody())) { - holder.registerProblem(loop.getFirstChild(), InspectionsBundle.message("inspection.idempotent.loop.body")); + PsiStatement body = loop.getBody(); + if (body == null) return; + if (mayHaveUndesiredSideEffects(loop, body)) return; + final ControlFlow controlFlow; + try { + controlFlow = + ControlFlowFactory.getInstance(loop.getProject()).getControlFlow(body, LocalsOrMyInstanceFieldsControlFlowPolicy.getInstance()); } + catch (AnalysisCanceledException ignored) { + return; + } + + checkControlFlow(loop, controlFlow, holder); } @Override public void visitForStatement(PsiForStatement loop) { PsiExpression condition = loop.getCondition(); if (condition != null && SideEffectChecker.mayHaveSideEffects(condition)) return; - if (isIdempotent(loop.getBody(), loop.getUpdate())) { + PsiStatement body = loop.getBody(); + if (body == null) return; + PsiStatement update = loop.getUpdate(); + if (mayHaveUndesiredSideEffects(loop, body) || update != null && mayHaveUndesiredSideEffects(loop, update)) return; + ControlFlow controlFlow; + try { + controlFlow = + ControlFlowFactory.getInstance(loop.getProject()).getControlFlow(loop, LocalsOrMyInstanceFieldsControlFlowPolicy.getInstance()); + } + catch (AnalysisCanceledException ignored) { + return; + } + int start = controlFlow.getStartOffset(body); + int end = controlFlow.getEndOffset(update == null ? body : update); + if(start == -1 || end == -1) return; + controlFlow = new ControlFlowSubRange(controlFlow, start, end); + + checkControlFlow(loop, controlFlow, holder); + } + + private void checkControlFlow(PsiLoopStatement loop, ControlFlow bodyFlow, @NotNull ProblemsHolder holder) { + Collection variables = ControlFlowUtil.getWrittenVariables(bodyFlow, 0, bodyFlow.getSize(), true); + if (variables.isEmpty()) return; + List reads = ControlFlowUtil.getReadBeforeWrite(bodyFlow); + if (StreamEx.of(reads).map(PsiReferenceExpression::resolve).select(PsiVariable.class).noneMatch(variables::contains)) { holder.registerProblem(loop.getFirstChild(), InspectionsBundle.message("inspection.idempotent.loop.body")); } } - private boolean isIdempotent(PsiStatement... statements) { - Set variables = extractWrites(statements); - if (variables == null || variables.isEmpty()) return false; - if (!(variables instanceof HashSet)) { - variables = new HashSet<>(variables); - } - for (PsiStatement statement : statements) { - if (usesInputVariable(statement, variables)) { - return false; - } - } - return true; - } - - private boolean usesInputVariable(PsiStatement statement, Set variables) { - if (statement == null) return false; - if (statement instanceof PsiBlockStatement) { - for (PsiStatement st : ((PsiBlockStatement)statement).getCodeBlock().getStatements()) { - if (usesInputVariable(st, variables)) { - return true; - } - } - return false; - } - if (statement instanceof PsiExpressionStatement) { - PsiAssignmentExpression assignment = tryCast(((PsiExpressionStatement)statement).getExpression(), PsiAssignmentExpression.class); - if (assignment != null) { - if (anyVariableIsUsed(assignment.getRExpression(), variables)) return true; - PsiReferenceExpression ref = - tryCast(PsiUtil.skipParenthesizedExprDown(assignment.getLExpression()), PsiReferenceExpression.class); - if (ref != null) { - PsiElement var = ref.resolve(); - if (var instanceof PsiVariable) { - variables.remove(var); + private boolean mayHaveUndesiredSideEffects(PsiLoopStatement loop, @NotNull PsiElement element) { + return SideEffectChecker.mayHaveSideEffects(element, e -> { + if (e instanceof PsiContinueStatement && ((PsiContinueStatement)e).findContinuedStatement() == loop) return true; + if (e instanceof PsiLocalVariable) return true; + if (e instanceof PsiAssignmentExpression) { + PsiAssignmentExpression assignment = (PsiAssignmentExpression)e; + if (assignment.getOperationTokenType() == JavaTokenType.EQ) { + PsiReferenceExpression ref = + tryCast(PsiUtil.skipParenthesizedExprDown(assignment.getLExpression()), PsiReferenceExpression.class); + if (ref != null) { + PsiElement target = ref.resolve(); + if (target instanceof PsiLocalVariable || target instanceof PsiParameter) return true; } } - return false; } - } - if (statement instanceof PsiIfStatement) { - PsiIfStatement ifStatement = (PsiIfStatement)statement; - if (anyVariableIsUsed(ifStatement.getCondition(), variables)) return true; - Set thenVars = new HashSet<>(variables); - if (usesInputVariable(ifStatement.getThenBranch(), thenVars)) return true; - Set elseVars = new HashSet<>(variables); - if (usesInputVariable(ifStatement.getElseBranch(), elseVars)) return true; - thenVars.addAll(elseVars); - variables.retainAll(thenVars); return false; - } - if (statement instanceof PsiDeclarationStatement) { - StreamEx.of(((PsiDeclarationStatement)statement).getDeclaredElements()).select(PsiVariable.class) - .forEach(variables::remove); - } - return anyVariableIsUsed(statement, variables); - } - - private boolean anyVariableIsUsed(@Nullable PsiElement statement, @NotNull Set variables) { - return VariableAccessUtils.collectUsedVariables(statement).stream().anyMatch(variables::contains); - } - - /** - * Extract written variables from statement which may affect the next iteration - * @param statement - * @return list of written variables or null if the statement may have unknown side effects, thus further analysis is impossible. - */ - @Nullable - private Set extractWrites(@Nullable PsiStatement statement) { - if (statement == null || - statement instanceof PsiEmptyStatement || - (statement instanceof PsiContinueStatement && ((PsiContinueStatement)statement).getLabelIdentifier() == null)) { - return Collections.emptySet(); - } - if (statement instanceof PsiBlockStatement) { - PsiStatement[] statements = ((PsiBlockStatement)statement).getCodeBlock().getStatements(); - return extractWrites(statements); - } - if (statement instanceof PsiDeclarationStatement) { - PsiElement[] elements = ((PsiDeclarationStatement)statement).getDeclaredElements(); - for (PsiElement element : elements) { - if (!(element instanceof PsiLocalVariable)) return null; - PsiLocalVariable var = (PsiLocalVariable)element; - PsiExpression initializer = var.getInitializer(); - if (initializer != null && SideEffectChecker.mayHaveSideEffects(initializer)) return null; - } - return Collections.emptySet(); - } - if (statement instanceof PsiExpressionStatement) { - PsiAssignmentExpression assignment = tryCast(((PsiExpressionStatement)statement).getExpression(), PsiAssignmentExpression.class); - if (assignment == null || - assignment.getOperationTokenType() != JavaTokenType.EQ || - assignment.getRExpression() == null || - SideEffectChecker.mayHaveSideEffects(assignment.getRExpression())) { - return null; - } - PsiReferenceExpression ref = - tryCast(PsiUtil.skipParenthesizedExprDown(assignment.getLExpression()), PsiReferenceExpression.class); - if (ref == null) return null; - PsiElement var = ref.resolve(); - if (var instanceof PsiLocalVariable || var instanceof PsiParameter) { - return Collections.singleton((PsiVariable)var); - } - } - if (statement instanceof PsiIfStatement) { - PsiIfStatement ifStatement = (PsiIfStatement)statement; - PsiExpression condition = ifStatement.getCondition(); - if (condition == null || SideEffectChecker.mayHaveSideEffects(condition)) return null; - Set thenResult = extractWrites(ifStatement.getThenBranch()); - if (thenResult == null) return null; - Set elseResult = extractWrites(ifStatement.getElseBranch()); - if (elseResult == null) return null; - if (thenResult.isEmpty()) return elseResult; - if (elseResult.isEmpty()) return thenResult; - return StreamEx.of(thenResult, elseResult).toFlatCollection(Function.identity(), HashSet::new); - } - return null; - } - - @Nullable - private Set extractWrites(PsiStatement... statements) { - Set result = new HashSet<>(); - for (PsiStatement subStatement : statements) { - Set subResult = extractWrites(subStatement); - if (subResult == null) return null; - result.addAll(subResult); - } - return result; + }); } }; } diff --git a/java/java-psi-impl/src/com/intellij/psi/controlFlow/ControlFlowSubRange.java b/java/java-psi-impl/src/com/intellij/psi/controlFlow/ControlFlowSubRange.java index f9da387e8f03..7ed8fea59719 100644 --- a/java/java-psi-impl/src/com/intellij/psi/controlFlow/ControlFlowSubRange.java +++ b/java/java-psi-impl/src/com/intellij/psi/controlFlow/ControlFlowSubRange.java @@ -24,12 +24,12 @@ import java.util.ArrayList; import java.util.List; public class ControlFlowSubRange implements ControlFlow { - private final ControlFlowImpl myControlFlow; + private final ControlFlow myControlFlow; private final int myStart; private final int myEnd; private List myInstructions; - public ControlFlowSubRange(ControlFlowImpl controlFlow, int start, int end) { + public ControlFlowSubRange(ControlFlow controlFlow, int start, int end) { myControlFlow = controlFlow; myStart = start; myEnd = end; diff --git a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/bugs/MismatchedStringBuilderQueryUpdateInspection.java b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/bugs/MismatchedStringBuilderQueryUpdateInspection.java index 3836f0d1074d..5f95b034c0b2 100644 --- a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/bugs/MismatchedStringBuilderQueryUpdateInspection.java +++ b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/bugs/MismatchedStringBuilderQueryUpdateInspection.java @@ -261,7 +261,8 @@ public class MismatchedStringBuilderQueryUpdateInspection extends BaseInspection if (hasReferenceToVariable(variable, qualifierExpression)) { PsiElement parent = PsiTreeUtil.getParentOfType(expression, PsiStatement.class, PsiLambdaExpression.class); if (parent instanceof PsiStatement && - !SideEffectChecker.mayHaveSideEffects(parent, this::isSideEffectFreeBuilderMethodCall)) { + !SideEffectChecker.mayHaveSideEffects( + parent, e -> e instanceof PsiMethodCallExpression && isSideEffectFreeBuilderMethodCall((PsiMethodCallExpression)e))) { return; } queried = true; diff --git a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/SideEffectChecker.java b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/SideEffectChecker.java index aa185a42cf70..44ab6bb4c0c2 100644 --- a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/SideEffectChecker.java +++ b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/SideEffectChecker.java @@ -65,8 +65,8 @@ public class SideEffectChecker { return visitor.mayHaveSideEffects(); } - public static boolean mayHaveSideEffects(@NotNull PsiElement element, Predicate shouldIgnoreCall) { - final SideEffectsVisitor visitor = new SideEffectsVisitor(null, shouldIgnoreCall); + public static boolean mayHaveSideEffects(@NotNull PsiElement element, Predicate shouldIgnoreElement) { + final SideEffectsVisitor visitor = new SideEffectsVisitor(null, shouldIgnoreElement); element.accept(visitor); return visitor.mayHaveSideEffects(); } @@ -86,39 +86,39 @@ public class SideEffectChecker { private static class SideEffectsVisitor extends JavaRecursiveElementWalkingVisitor { private @Nullable final List mySideEffects; boolean found; - final Predicate myIgnoredCallPredicate; + final Predicate myIgnorePredicate; SideEffectsVisitor(@Nullable List sideEffects) { this(sideEffects, call -> false); } - SideEffectsVisitor(@Nullable List sideEffects, Predicate predicate) { - myIgnoredCallPredicate = predicate; + SideEffectsVisitor(@Nullable List sideEffects, Predicate predicate) { + myIgnorePredicate = predicate; mySideEffects = sideEffects; } - private void addSideEffect(PsiElement element) { + private boolean addSideEffect(PsiElement element) { + if (myIgnorePredicate.test(element)) return false; found = true; if(mySideEffects != null) { mySideEffects.add(element); } else { stopWalking(); } + return true; } @Override public void visitAssignmentExpression(@NotNull PsiAssignmentExpression expression) { - addSideEffect(expression); + if (addSideEffect(expression)) return; + super.visitAssignmentExpression(expression); } @Override public void visitMethodCallExpression(@NotNull PsiMethodCallExpression expression) { - if (!myIgnoredCallPredicate.test(expression)) { - final PsiMethod method = expression.resolveMethod(); - if (!isPure(method)) { - addSideEffect(expression); - return; - } + final PsiMethod method = expression.resolveMethod(); + if (!isPure(method)) { + if (addSideEffect(expression)) return; } super.visitMethodCallExpression(expression); } @@ -132,8 +132,7 @@ public class SideEffectChecker { @Override public void visitNewExpression(@NotNull PsiNewExpression expression) { if(!isSideEffectFreeConstructor(expression)) { - addSideEffect(expression); - return; + if (addSideEffect(expression)) return; } super.visitNewExpression(expression); } @@ -142,20 +141,21 @@ public class SideEffectChecker { public void visitUnaryExpression(@NotNull PsiUnaryExpression expression) { final IElementType tokenType = expression.getOperationTokenType(); if (tokenType.equals(JavaTokenType.PLUSPLUS) || tokenType.equals(JavaTokenType.MINUSMINUS)) { - addSideEffect(expression); - return; + if (addSideEffect(expression)) return; } super.visitUnaryExpression(expression); } @Override - public void visitDeclarationStatement(PsiDeclarationStatement statement) { - addSideEffect(statement); + public void visitVariable(PsiVariable variable) { + if (addSideEffect(variable)) return; + super.visitVariable(variable); } @Override public void visitBreakStatement(PsiBreakStatement statement) { - addSideEffect(statement); + if (addSideEffect(statement)) return; + super.visitBreakStatement(statement); } @Override @@ -165,17 +165,20 @@ public class SideEffectChecker { @Override public void visitContinueStatement(PsiContinueStatement statement) { - addSideEffect(statement); + if (addSideEffect(statement)) return; + super.visitContinueStatement(statement); } @Override public void visitReturnStatement(PsiReturnStatement statement) { - addSideEffect(statement); + if (addSideEffect(statement)) return; + super.visitReturnStatement(statement); } @Override public void visitThrowStatement(PsiThrowStatement statement) { - addSideEffect(statement); + if (addSideEffect(statement)) return; + super.visitThrowStatement(statement); } @Override