From 0aa8430cb40405546f8827a4e7ecb658af8085da Mon Sep 17 00:00:00 2001 From: Pavel Dolgov Date: Tue, 20 Jun 2017 17:36:32 +0300 Subject: [PATCH] Java: Look for redundant assignment of a field in class and field initializers (IDEA-149587) --- .../analysis/HighlightControlFlowUtil.java | 2 +- .../defUse/DefUseInspectionBase.java | 148 ++++++++++++++++-- .../afterClassInitializer.java | 7 + .../afterFieldInitializer.java | 5 + .../beforeClassInitializer.java | 6 + .../beforeFieldInitializer.java | 5 + .../inspection/defUse/FieldInitializer.java | 91 +++++++++++ .../java/codeInspection/DefUseTest.java | 1 + 8 files changed, 248 insertions(+), 17 deletions(-) create mode 100644 java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/unusedAssignment/afterClassInitializer.java create mode 100644 java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/unusedAssignment/afterFieldInitializer.java create mode 100644 java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/unusedAssignment/beforeClassInitializer.java create mode 100644 java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/unusedAssignment/beforeFieldInitializer.java create mode 100644 java/java-tests/testData/inspection/defUse/FieldInitializer.java diff --git a/java/java-analysis-impl/src/com/intellij/codeInsight/daemon/impl/analysis/HighlightControlFlowUtil.java b/java/java-analysis-impl/src/com/intellij/codeInsight/daemon/impl/analysis/HighlightControlFlowUtil.java index 281382801fdb..e7e581ec4d93 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInsight/daemon/impl/analysis/HighlightControlFlowUtil.java +++ b/java/java-analysis-impl/src/com/intellij/codeInsight/daemon/impl/analysis/HighlightControlFlowUtil.java @@ -220,7 +220,7 @@ public class HighlightControlFlowUtil { * see JLS chapter 16 * @return true if variable assigned (maybe more than once) */ - private static boolean variableDefinitelyAssignedIn(@NotNull PsiVariable variable, @NotNull PsiElement context) { + public static boolean variableDefinitelyAssignedIn(@NotNull PsiVariable variable, @NotNull PsiElement context) { try { ControlFlow controlFlow = getControlFlow(context); return ControlFlowUtil.isVariableDefinitelyAssigned(variable, controlFlow); diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/defUse/DefUseInspectionBase.java b/java/java-analysis-impl/src/com/intellij/codeInspection/defUse/DefUseInspectionBase.java index 986d72a86368..cec0086a1378 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/defUse/DefUseInspectionBase.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/defUse/DefUseInspectionBase.java @@ -16,10 +16,12 @@ package com.intellij.codeInspection.defUse; import com.intellij.codeInsight.daemon.GroupNames; +import com.intellij.codeInsight.daemon.impl.analysis.HighlightControlFlowUtil; import com.intellij.codeInsight.daemon.impl.quickfix.RemoveUnusedVariableUtil; import com.intellij.codeInspection.*; import com.intellij.psi.*; import com.intellij.psi.controlFlow.DefUseUtil; +import com.intellij.psi.util.PsiTreeUtil; import com.intellij.util.ObjectUtils; import com.intellij.util.containers.ContainerUtil; import com.intellij.util.ui.JBUI; @@ -62,6 +64,11 @@ public class DefUseInspectionBase extends BaseJavaBatchLocalInspectionTool { checkCodeBlock((PsiCodeBlock)body, holder, isOnTheFly); } } + + @Override + public void visitField(PsiField field) { + checkField(field, holder, isOnTheFly); + } }; } @@ -89,25 +96,11 @@ public class DefUseInspectionBase extends BaseJavaBatchLocalInspectionTool { if (context instanceof PsiDeclarationStatement || context instanceof PsiResourceVariable) { if (info.isRead() && REPORT_REDUNDANT_INITIALIZER) { - List fixes = ContainerUtil.createMaybeSingletonList( - isOnTheFlyOrNoSideEffects(isOnTheFly, psiVariable, psiVariable.getInitializer()) ? createRemoveInitializerFix() : null); - holder.registerProblem(ObjectUtils.notNull(psiVariable.getInitializer(), psiVariable), - InspectionsBundle.message("inspection.unused.assignment.problem.descriptor2", - "" + psiVariable.getName() + "", "#ref #loc"), - ProblemHighlightType.LIKE_UNUSED_SYMBOL, - fixes.toArray(new LocalQuickFix[fixes.size()]) - ); + reportInitializerProblem(psiVariable, holder, isOnTheFly); } } else if (context instanceof PsiAssignmentExpression) { - final PsiAssignmentExpression assignment = (PsiAssignmentExpression)context; - List fixes = ContainerUtil.createMaybeSingletonList( - isOnTheFlyOrNoSideEffects(isOnTheFly, psiVariable, assignment.getRExpression()) ? createRemoveAssignmentFix() : null); - holder.registerProblem(assignment.getLExpression(), - InspectionsBundle.message("inspection.unused.assignment.problem.descriptor3", - ObjectUtils.assertNotNull(assignment.getRExpression()).getText(), "#ref" + " #loc"), - ProblemHighlightType.LIKE_UNUSED_SYMBOL, fixes.toArray(new LocalQuickFix[fixes.size()]) - ); + reportAssignmentProblem(psiVariable, (PsiAssignmentExpression)context, holder, isOnTheFly); } else { if (context instanceof PsiPrefixExpression && REPORT_PREFIX_EXPRESSIONS || @@ -120,6 +113,96 @@ public class DefUseInspectionBase extends BaseJavaBatchLocalInspectionTool { } } + private void reportInitializerProblem(PsiVariable psiVariable, ProblemsHolder holder, boolean isOnTheFly) { + List fixes = ContainerUtil.createMaybeSingletonList( + isOnTheFlyOrNoSideEffects(isOnTheFly, psiVariable, psiVariable.getInitializer()) ? createRemoveInitializerFix() : null); + holder.registerProblem(ObjectUtils.notNull(psiVariable.getInitializer(), psiVariable), + InspectionsBundle.message("inspection.unused.assignment.problem.descriptor2", + "" + psiVariable.getName() + "", "#ref #loc"), + ProblemHighlightType.LIKE_UNUSED_SYMBOL, + fixes.toArray(LocalQuickFix.EMPTY_ARRAY) + ); + } + + private void reportAssignmentProblem(PsiVariable psiVariable, + PsiAssignmentExpression assignment, + ProblemsHolder holder, + boolean isOnTheFly) { + List fixes = ContainerUtil.createMaybeSingletonList( + isOnTheFlyOrNoSideEffects(isOnTheFly, psiVariable, assignment.getRExpression()) ? createRemoveAssignmentFix() : null); + holder.registerProblem(assignment.getLExpression(), + InspectionsBundle.message("inspection.unused.assignment.problem.descriptor3", + ObjectUtils.assertNotNull(assignment.getRExpression()).getText(), "#ref" + " #loc"), + ProblemHighlightType.LIKE_UNUSED_SYMBOL, fixes.toArray(LocalQuickFix.EMPTY_ARRAY) + ); + } + + private void checkField(@NotNull PsiField field, @NotNull ProblemsHolder holder, boolean isOnTheFly) { + if (field.hasModifierProperty(PsiModifier.FINAL)) return; + final PsiClass psiClass = field.getContainingClass(); + if (psiClass == null) return; + final PsiClassInitializer[] classInitializers = psiClass.getInitializers(); + if (classInitializers.length == 0) return; + + final boolean isStatic = field.hasModifierProperty(PsiModifier.STATIC); + final boolean fieldHasInitializer = field.hasInitializer(); + final PsiClassInitializer initializerBeforeField = PsiTreeUtil.getPrevSiblingOfType(field, PsiClassInitializer.class); + final List fieldWrites = new ArrayList<>(); // class initializers and field initializer in the program order + + if (fieldHasInitializer && initializerBeforeField == null) { + fieldWrites.add(FieldWrite.createInitializer()); + } + for (PsiClassInitializer classInitializer : classInitializers) { + if (classInitializer.hasModifierProperty(PsiModifier.STATIC) == isStatic) { + final List assignments = collectAssignments(field, classInitializer); + if (!assignments.isEmpty()) { + boolean isDefinitely = HighlightControlFlowUtil.variableDefinitelyAssignedIn(field, classInitializer.getBody()); + fieldWrites.add(FieldWrite.createAssignments(isDefinitely, assignments)); + } + } + if (fieldHasInitializer && initializerBeforeField == classInitializer) { + fieldWrites.add(FieldWrite.createInitializer()); + } + } + Collections.reverse(fieldWrites); + + boolean wasDefinitelyAssigned = false; + for (final FieldWrite fieldWrite : fieldWrites) { + if (wasDefinitelyAssigned) { + if (fieldWrite.isInitializer()) { + reportInitializerProblem(field, holder, isOnTheFly); + } + else { + for (PsiAssignmentExpression assignment : fieldWrite.getAssignments()) { + reportAssignmentProblem(field, assignment, holder, isOnTheFly); + } + } + } + else if (fieldWrite.isDefinitely()) { + wasDefinitelyAssigned = true; + } + } + } + + @NotNull + private static List collectAssignments(@NotNull PsiField field, @NotNull PsiClassInitializer classInitializer) { + final List assignmentExpressions = new ArrayList<>(); + classInitializer.accept(new JavaRecursiveElementVisitor() { + @Override + public void visitAssignmentExpression(PsiAssignmentExpression expression) { + final PsiExpression lExpression = expression.getLExpression(); + if (lExpression instanceof PsiJavaReference && ((PsiJavaReference)lExpression).isReferenceTo(field)) { + final PsiExpression rExpression = expression.getRExpression(); + if (rExpression != null) { + assignmentExpressions.add(expression); + } + } + super.visitAssignmentExpression(expression); + } + }); + return assignmentExpressions; + } + private static boolean isOnTheFlyOrNoSideEffects(boolean isOnTheFly, PsiVariable psiVariable, PsiExpression initializer) { @@ -193,4 +276,37 @@ public class DefUseInspectionBase extends BaseJavaBatchLocalInspectionTool { public String getShortName() { return SHORT_NAME; } + + + private static class FieldWrite { + final boolean myDefinitely; + final List myAssignments; + + private FieldWrite(boolean definitely, List assignments) { + myDefinitely = definitely; + myAssignments = assignments; + } + + public boolean isDefinitely() { + return myDefinitely; + } + + public boolean isInitializer() { + return myAssignments == null; + } + + public List getAssignments() { + return myAssignments != null ? myAssignments : Collections.emptyList(); + } + + @NotNull + public static FieldWrite createInitializer() { + return new FieldWrite(true, null); + } + + @NotNull + public static FieldWrite createAssignments(boolean definitely, @NotNull List assignmentExpressions) { + return new FieldWrite(definitely, assignmentExpressions); + } + } } \ No newline at end of file diff --git a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/unusedAssignment/afterClassInitializer.java b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/unusedAssignment/afterClassInitializer.java new file mode 100644 index 000000000000..5eae65b40d46 --- /dev/null +++ b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/unusedAssignment/afterClassInitializer.java @@ -0,0 +1,7 @@ +// "Remove redundant assignment" "true" +class A { + static int n; + static { + } + static { n = 2; } +} \ No newline at end of file diff --git a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/unusedAssignment/afterFieldInitializer.java b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/unusedAssignment/afterFieldInitializer.java new file mode 100644 index 000000000000..3d872f9b232a --- /dev/null +++ b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/unusedAssignment/afterFieldInitializer.java @@ -0,0 +1,5 @@ +// "Remove redundant initializer" "true" +class A { + int n; + { n = 1; } +} \ No newline at end of file diff --git a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/unusedAssignment/beforeClassInitializer.java b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/unusedAssignment/beforeClassInitializer.java new file mode 100644 index 000000000000..17cc8f16e60e --- /dev/null +++ b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/unusedAssignment/beforeClassInitializer.java @@ -0,0 +1,6 @@ +// "Remove redundant assignment" "true" +class A { + static int n; + static { n = 1; } + static { n = 2; } +} \ No newline at end of file diff --git a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/unusedAssignment/beforeFieldInitializer.java b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/unusedAssignment/beforeFieldInitializer.java new file mode 100644 index 000000000000..0c7f9a6fb174 --- /dev/null +++ b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/unusedAssignment/beforeFieldInitializer.java @@ -0,0 +1,5 @@ +// "Remove redundant initializer" "true" +class A { + int n = 0; + { n = 1; } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/defUse/FieldInitializer.java b/java/java-tests/testData/inspection/defUse/FieldInitializer.java new file mode 100644 index 000000000000..8bdc06ac46c8 --- /dev/null +++ b/java/java-tests/testData/inspection/defUse/FieldInitializer.java @@ -0,0 +1,91 @@ +class C { + static boolean b = System.getProperty("foo") != null; + + static class C1 { + { s = "a"; } + String s = "b"; + } + static class C2 { + String s = "b"; + { s = "c"; } + } + static class C3 { + { s = "a"; } + String s; + { s = "c"; } + } + static class C4 { + { if (b) s = "a"; } + String s = "b"; + } + static class C5 { + String s = "b"; + { if (b) s = "c"; } + } + static class C6 { + String s = "b"; + { if (b) s = "c"; else s = "d"; } + } + static class C7 { + String s; + { s = "c"; } + } + static class C8 { + { s = "a"; } + String s; + } + static class C9 { + String s; + { + s = "b"; + if (b) s = "c"; + } + { s = "d"; } + } + + static class S1 { + static { s = "a"; } + static String s = "b"; + } + static class S2 { + static String s = "b"; + static { s = "c"; } + } + static class S3 { + static { s = "a"; } + static String s; + static { s = "c"; } + } + static class S4 { + static { if (b) s = "a"; } + static String s = "b"; + } + static class S5 { + static String s = "b"; + static { if (b) s = "c"; } + } + static class S6 { + static String s = "b"; + static { if (b) s = "c"; else s = "d"; } + } + static class S7 { + static String s; + static { s = "c"; } + } + static class S8 { + static { s = "a"; } + static String s; + } + static class S9 { + static String s; + static { + s = "b"; + if (b) s = "c"; + } + static { s = "d"; } + } + static class S10 { + static String s = "a"; + { s = "b"; } + } +} \ No newline at end of file diff --git a/java/java-tests/testSrc/com/intellij/java/codeInspection/DefUseTest.java b/java/java-tests/testSrc/com/intellij/java/codeInspection/DefUseTest.java index d0da52ba6c39..189b3851c06a 100644 --- a/java/java-tests/testSrc/com/intellij/java/codeInspection/DefUseTest.java +++ b/java/java-tests/testSrc/com/intellij/java/codeInspection/DefUseTest.java @@ -56,6 +56,7 @@ public class DefUseTest extends LightCodeInsightFixtureTestCase { public void testAssignmentsInLambdaBody() { doTest(); } public void testNestedTryFinallyInEndlessLoop() { doTest(); } public void testNestedTryFinallyInForLoop() { doTest(); } + public void testFieldInitializer() { doTest(); } private void doTest() { myFixture.enableInspections(new DefUseInspection());