From f3c4a52eceeb7fb882f8eda5a9d573bee8e19c97 Mon Sep 17 00:00:00 2001 From: peter Date: Tue, 6 Oct 2015 16:28:34 +0200 Subject: [PATCH] dfa: don't suggest to assert/surround-with-if possible NPEs on volatile fields, suggest to extract a local variable instead (IDEA-145668) --- .../dataFlow/DataFlowInspectionBase.java | 15 +++++- .../dataFlow/DataFlowInspection.java | 46 +++++++++++++++++-- .../fixture/VolatileFieldNPEFixes.java | 10 ++++ .../DataFlowInspectionTest.java | 7 +++ 4 files changed, 73 insertions(+), 5 deletions(-) create mode 100644 java/java-tests/testData/inspection/dataFlow/fixture/VolatileFieldNPEFixes.java diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspectionBase.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspectionBase.java index d2d6500de158..f2362ac4371f 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspectionBase.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspectionBase.java @@ -225,7 +225,10 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool { try { final List fixes = new SmartList(); - if (!(qualifier instanceof PsiLiteralExpression && ((PsiLiteralExpression)qualifier).getValue() == null)) { + if (isVolatileFieldReference(qualifier)) { + ContainerUtil.addIfNotNull(fixes, createIntroduceVariableFix(qualifier)); + } + else if (!(qualifier instanceof PsiLiteralExpression && ((PsiLiteralExpression)qualifier).getValue() == null)) { if (PsiUtil.getLanguageLevel(qualifier).isAtLeast(LanguageLevel.JDK_1_4)) { final Project project = qualifier.getProject(); final PsiElementFactory elementFactory = JavaPsiFacade.getInstance(project).getElementFactory(); @@ -250,6 +253,16 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool { } } + @Nullable + protected LocalQuickFixOnPsiElement createIntroduceVariableFix(PsiExpression expression) { + return null; + } + + private static boolean isVolatileFieldReference(PsiExpression qualifier) { + PsiElement target = qualifier instanceof PsiReferenceExpression ? ((PsiReferenceExpression)qualifier).resolve() : null; + return target instanceof PsiField && ((PsiField)target).hasModifierProperty(PsiModifier.VOLATILE); + } + protected LocalQuickFix createAssertFix(PsiBinaryExpression binary, PsiExpression expression) { return null; } diff --git a/java/java-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspection.java b/java/java-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspection.java index fd426f327a16..db944713a346 100644 --- a/java/java-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspection.java +++ b/java/java-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspection.java @@ -19,14 +19,18 @@ import com.intellij.codeInsight.NullableNotNullDialog; import com.intellij.codeInspection.*; import com.intellij.ide.DataManager; import com.intellij.openapi.actionSystem.CommonDataKeys; +import com.intellij.openapi.actionSystem.DataContext; import com.intellij.openapi.project.Project; import com.intellij.openapi.project.ProjectManager; -import com.intellij.psi.JavaTokenType; -import com.intellij.psi.PsiAssignmentExpression; -import com.intellij.psi.PsiBinaryExpression; -import com.intellij.psi.PsiExpression; +import com.intellij.openapi.util.AsyncResult; +import com.intellij.psi.*; import com.intellij.psi.tree.IElementType; +import com.intellij.refactoring.JavaRefactoringActionHandlerFactory; +import com.intellij.refactoring.RefactoringActionHandler; import com.intellij.refactoring.util.RefactoringUtil; +import com.intellij.util.Consumer; +import org.jetbrains.annotations.Nls; +import org.jetbrains.annotations.NotNull; import javax.swing.*; import javax.swing.event.ChangeEvent; @@ -64,6 +68,40 @@ public class DataFlowInspection extends DataFlowInspectionBase { return RefactoringUtil.getParentStatement(expression, false) == null ? null : new AddAssertStatementFix(binary); } + @Override + protected LocalQuickFixOnPsiElement createIntroduceVariableFix(final PsiExpression expression) { + return new LocalQuickFixOnPsiElement(expression) { + @NotNull + @Override + public String getText() { + return "Introduce Local Variable"; + } + + @Override + public void invoke(@NotNull final Project project, + @NotNull PsiFile file, + @NotNull final PsiElement startElement, + @NotNull PsiElement endElement) { + final RefactoringActionHandler handler = JavaRefactoringActionHandlerFactory.getInstance().createIntroduceVariableHandler(); + final AsyncResult dataContextContainer = DataManager.getInstance().getDataContextFromFocus(); + dataContextContainer.doWhenDone(new Consumer() { + @Override + public void consume(DataContext dataContext) { + handler.invoke(project, new PsiElement[]{startElement}, dataContext); + } + }); + + } + + @Nls + @NotNull + @Override + public String getFamilyName() { + return getText(); + } + }; + } + private class OptionsPanel extends JPanel { private final JCheckBox myIgnoreAssertions; private final JCheckBox myReportConstantReferences; diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/VolatileFieldNPEFixes.java b/java/java-tests/testData/inspection/dataFlow/fixture/VolatileFieldNPEFixes.java new file mode 100644 index 000000000000..a9f77db79796 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/fixture/VolatileFieldNPEFixes.java @@ -0,0 +1,10 @@ +import org.jetbrains.annotations.Nullable; +class Test { + @Nullable volatile String x; + + public void foo() { + if (x != null) { + System.out.println(x.substring(1)); + } + } +} \ No newline at end of file diff --git a/java/java-tests/testSrc/com/intellij/codeInspection/DataFlowInspectionTest.java b/java/java-tests/testSrc/com/intellij/codeInspection/DataFlowInspectionTest.java index 195e4395322d..66abd2827752 100644 --- a/java/java-tests/testSrc/com/intellij/codeInspection/DataFlowInspectionTest.java +++ b/java/java-tests/testSrc/com/intellij/codeInspection/DataFlowInspectionTest.java @@ -376,6 +376,13 @@ public class DataFlowInspectionTest extends LightCodeInsightFixtureTestCase { assertEmpty(myFixture.filterAvailableIntentions("Simplify")); } + public void testVolatileFieldNPEFixes() { + doTest(); + assertEmpty(myFixture.filterAvailableIntentions("Surround")); + assertEmpty(myFixture.filterAvailableIntentions("Assert")); + assertNotEmpty(myFixture.filterAvailableIntentions("Introduce Local Variable")); + } + public void testAssertThat() { myFixture.addClass("package org.hamcrest; public class CoreMatchers { " + "public static Matcher notNullValue() {}\n" +