From 261d0170305dac25815d049874d00a85a71d866d Mon Sep 17 00:00:00 2001 From: Dmitry Batkovich Date: Tue, 26 Jul 2016 11:17:18 +0300 Subject: [PATCH 1/5] quick fix batch application: getWorkingQuickFix uses QuickFix#getFamilyName to find suitable fixes (IDEA-155841) --- .../StreamApiMigrationInspection.java | 27 ++++++------------- .../ex/LocalQuickFixWrapper.java | 16 ++--------- 2 files changed, 10 insertions(+), 33 deletions(-) diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/StreamApiMigrationInspection.java b/java/java-analysis-impl/src/com/intellij/codeInspection/StreamApiMigrationInspection.java index 18f947b96716..815d035a6feb 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/StreamApiMigrationInspection.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/StreamApiMigrationInspection.java @@ -22,7 +22,6 @@ import com.intellij.codeInsight.daemon.impl.analysis.HighlightControlFlowUtil; import com.intellij.codeInspection.ui.SingleCheckboxOptionsPanel; import com.intellij.openapi.diagnostic.Logger; import com.intellij.openapi.project.Project; -import com.intellij.openapi.util.Condition; import com.intellij.openapi.util.text.StringUtil; import com.intellij.pom.java.LanguageLevel; import com.intellij.psi.*; @@ -129,10 +128,10 @@ public class StreamApiMigrationInspection extends BaseJavaBatchLocalInspectionTo } else if (REPLACE_TRIVIAL_FOREACH || !isTrivial(body, statement.getIterationParameter())) { final List fixes = new ArrayList(); - fixes.add(new ReplaceWithForeachFix()); + fixes.add(new ReplaceWithForeachCallFix("forEach")); if (extractIfStatement(body) != null) { //for .stream() - fixes.add(new ReplaceWithForeachOrderedFix()); + fixes.add(new ReplaceWithForeachCallFix("forEachOrdered")); } holder.registerProblem(iteratedValue, "Can be replaced with foreach call", ProblemHighlightType.GENERIC_ERROR_OR_WARNING, @@ -285,22 +284,12 @@ public class StreamApiMigrationInspection extends BaseJavaBatchLocalInspectionTo return mapperCall instanceof PsiReferenceExpression && ((PsiReferenceExpression)mapperCall).resolve() == parameter; } - private static class ReplaceWithForeachFix extends ReplaceWithForeachCallFix { - @Override - protected String getForEachMethodName() { - return "forEach"; - } - } + private static class ReplaceWithForeachCallFix implements LocalQuickFix { + private final String myForEachMethodName; - private static class ReplaceWithForeachOrderedFix extends ReplaceWithForeachCallFix { - @Override - protected String getForEachMethodName() { - return "forEachOrdered"; + protected ReplaceWithForeachCallFix(String forEachMethodName) { + myForEachMethodName = forEachMethodName; } - } - - private static abstract class ReplaceWithForeachCallFix implements LocalQuickFix { - protected abstract String getForEachMethodName(); @NotNull @Override @@ -311,7 +300,7 @@ public class StreamApiMigrationInspection extends BaseJavaBatchLocalInspectionTo @NotNull @Override public String getFamilyName() { - return "Replace with " + getForEachMethodName(); + return "Replace with " + myForEachMethodName; } @Override @@ -336,7 +325,7 @@ public class StreamApiMigrationInspection extends BaseJavaBatchLocalInspectionTo body = thenBranch; } - buffer.append(".").append(getForEachMethodName()).append("("); + buffer.append(".").append(myForEachMethodName).append("("); final String functionalExpressionText = createForEachFunctionalExpressionText(project, body, parameter); final PsiElementFactory elementFactory = JavaPsiFacade.getElementFactory(project); diff --git a/platform/lang-impl/src/com/intellij/codeInspection/ex/LocalQuickFixWrapper.java b/platform/lang-impl/src/com/intellij/codeInspection/ex/LocalQuickFixWrapper.java index 5982609cff91..810c6d6a7626 100644 --- a/platform/lang-impl/src/com/intellij/codeInspection/ex/LocalQuickFixWrapper.java +++ b/platform/lang-impl/src/com/intellij/codeInspection/ex/LocalQuickFixWrapper.java @@ -61,26 +61,14 @@ public class LocalQuickFixWrapper extends QuickFixAction { @Nullable private QuickFix getWorkingQuickFix(@NotNull QuickFix[] fixes) { - final QuickFix exactResult = getWorkingQuickFix(fixes, true); - return exactResult != null ? exactResult : getWorkingQuickFix(fixes, false); - } - - @Nullable - private QuickFix getWorkingQuickFix(@NotNull QuickFix[] fixes, boolean exact) { for (QuickFix fix : fixes) { - if (!checkFix(exact, myFix, fix)) continue; - if (myFix instanceof IntentionWrapper && fix instanceof IntentionWrapper) { - if (!checkFix(exact, ((IntentionWrapper)myFix).getAction(), ((IntentionWrapper)fix).getAction())) continue; + if (fix.getFamilyName().equals(myFix.getFamilyName())) { + return fix; } - return fix; } return null; } - private static boolean checkFix(boolean exact, T thisFix, T fix) { - return exact ? thisFix.getClass() == fix.getClass() : thisFix.getClass().isInstance(fix); - } - @Override protected boolean applyFix(@NotNull RefEntity[] refElements) { return true; From 8cf7f1da84a27ecd5db28eb29e40f40073acc7e2 Mon Sep 17 00:00:00 2001 From: Dmitry Batkovich Date: Tue, 26 Jul 2016 11:18:45 +0300 Subject: [PATCH 2/5] QuickFixGetFamilyNameViolationInspection updated to provide less false-positives and be more green --- ...ckFixGetFamilyNameViolationInspection.java | 42 +++++++++++++++---- ...va => NotViolatedByExternalParameter.java} | 2 +- ...onByField.java => NotViolatedByField.java} | 2 +- .../NotViolatedByGetName.java | 16 +++++++ ...a => ViolationByPsiElementFieldUsage.java} | 5 ++- ...xGetFamilyNameViolationInspectionTest.java | 12 ++++-- 6 files changed, 65 insertions(+), 14 deletions(-) rename plugins/devkit/testData/inspections/getFamilyNameViolation/{ViolationByExternalParameter.java => NotViolatedByExternalParameter.java} (64%) rename plugins/devkit/testData/inspections/getFamilyNameViolation/{ViolationByField.java => NotViolatedByField.java} (60%) create mode 100644 plugins/devkit/testData/inspections/getFamilyNameViolation/NotViolatedByGetName.java rename plugins/devkit/testData/inspections/getFamilyNameViolation/{ViolationByGetName.java => ViolationByPsiElementFieldUsage.java} (72%) diff --git a/plugins/devkit/src/inspections/QuickFixGetFamilyNameViolationInspection.java b/plugins/devkit/src/inspections/QuickFixGetFamilyNameViolationInspection.java index 8446d03a51c3..5d8498a7ea22 100644 --- a/plugins/devkit/src/inspections/QuickFixGetFamilyNameViolationInspection.java +++ b/plugins/devkit/src/inspections/QuickFixGetFamilyNameViolationInspection.java @@ -17,9 +17,13 @@ package org.jetbrains.idea.devkit.inspections; import com.intellij.codeInspection.*; import com.intellij.openapi.diagnostic.Logger; +import com.intellij.openapi.extensions.AreaInstance; +import com.intellij.openapi.vfs.VirtualFile; +import com.intellij.pom.Navigatable; import com.intellij.psi.*; import com.intellij.psi.util.InheritanceUtil; import com.intellij.psi.util.PsiTreeUtil; +import com.intellij.util.containers.ContainerUtil; import gnu.trove.THashSet; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -33,6 +37,11 @@ import java.util.Set; public class QuickFixGetFamilyNameViolationInspection extends DevKitInspectionBase { private final static Logger LOG = Logger.getInstance(QuickFixGetFamilyNameViolationInspection.class); + private final static Set BASE_CONTEXT_AWARE_CLASSES = ContainerUtil.newHashSet(PsiElement.class.getName(), + Navigatable.class.getName(), + AreaInstance.class.getName(), + VirtualFile.class.getName()); + @Nullable @Override public ProblemDescriptor[] checkMethod(@NotNull PsiMethod method, @NotNull InspectionManager manager, boolean isOnTheFly) { @@ -55,28 +64,35 @@ public class QuickFixGetFamilyNameViolationInspection extends DevKitInspectionBa if (!processed.add(method) || method.hasModifierProperty(PsiModifier.STATIC)) return false; final PsiCodeBlock body = method.getBody(); if (body == null) return false; + if (isContextDependentType(method.getReturnType())) { + return true; + } final Collection referenceIterator = PsiTreeUtil.findChildrenOfType(body, PsiJavaCodeReferenceElement.class); for (PsiJavaCodeReferenceElement reference : referenceIterator) { final PsiElement resolved = reference.resolve(); if (resolved instanceof PsiVariable) { - if ((resolved instanceof PsiLocalVariable || resolved instanceof PsiParameter) && !PsiTreeUtil.isAncestor(body, resolved, false)) { - return true; - } - if (resolved instanceof PsiField && !((PsiField)resolved).hasModifierProperty(PsiModifier.STATIC)) { + if (!(resolved instanceof PsiField && ((PsiField)resolved).hasModifierProperty(PsiModifier.STATIC)) && isContextDependentType(((PsiVariable)resolved).getType())) { return true; } } - if (resolved instanceof PsiMethod && !((PsiMethod)resolved).hasModifierProperty(PsiModifier.STATIC)) { - final PsiClass resolvedContainingClass = ((PsiMethod)resolved).getContainingClass(); + if (resolved instanceof PsiMethod) { + final PsiMethod resolvedMethod = (PsiMethod)resolved; + final PsiClass resolvedContainingClass = resolvedMethod.getContainingClass(); + //if (resolvedMethod.getName().equals("getName") && + // resolvedMethod.getParameterList().getParametersCount() == 0 && + // !resolvedMethod.hasModifierProperty(PsiModifier.STATIC) && + // InheritanceUtil.isInheritor(resolvedContainingClass, QuickFix.class.getName())) { + // return true; + //} final PsiClass methodContainingClass = method.getContainingClass(); if (resolvedContainingClass != null && methodContainingClass != null && (methodContainingClass == resolvedContainingClass || methodContainingClass.isInheritor(resolvedContainingClass, true))) { - if (doesMethodViolate((PsiMethod)resolved, processed)) { + if (doesMethodViolate(resolvedMethod, processed)) { return true; } } @@ -84,4 +100,16 @@ public class QuickFixGetFamilyNameViolationInspection extends DevKitInspectionBa } return false; } + + private static boolean isContextDependentType(@Nullable PsiType type) { + if (type == null) return false; + + for (String aClass : BASE_CONTEXT_AWARE_CLASSES) { + if (InheritanceUtil.isInheritor(type, aClass)) { + return true; + } + } + + return false; + } } diff --git a/plugins/devkit/testData/inspections/getFamilyNameViolation/ViolationByExternalParameter.java b/plugins/devkit/testData/inspections/getFamilyNameViolation/NotViolatedByExternalParameter.java similarity index 64% rename from plugins/devkit/testData/inspections/getFamilyNameViolation/ViolationByExternalParameter.java rename to plugins/devkit/testData/inspections/getFamilyNameViolation/NotViolatedByExternalParameter.java index dc4014552931..af497ebf5053 100644 --- a/plugins/devkit/testData/inspections/getFamilyNameViolation/ViolationByExternalParameter.java +++ b/plugins/devkit/testData/inspections/getFamilyNameViolation/NotViolatedByExternalParameter.java @@ -8,7 +8,7 @@ class A { return "some name"; }; - public String getFamilyName() { + public String getFamilyName() { return someParameter + "123"; }; }; diff --git a/plugins/devkit/testData/inspections/getFamilyNameViolation/ViolationByField.java b/plugins/devkit/testData/inspections/getFamilyNameViolation/NotViolatedByField.java similarity index 60% rename from plugins/devkit/testData/inspections/getFamilyNameViolation/ViolationByField.java rename to plugins/devkit/testData/inspections/getFamilyNameViolation/NotViolatedByField.java index c948ae538462..44ff062f6740 100644 --- a/plugins/devkit/testData/inspections/getFamilyNameViolation/ViolationByField.java +++ b/plugins/devkit/testData/inspections/getFamilyNameViolation/NotViolatedByField.java @@ -8,7 +8,7 @@ class MyQuickFix implements QuickFix { return "some name"; }; - public String getFamilyName() { + public String getFamilyName() { return someField + getName() + "123"; }; diff --git a/plugins/devkit/testData/inspections/getFamilyNameViolation/NotViolatedByGetName.java b/plugins/devkit/testData/inspections/getFamilyNameViolation/NotViolatedByGetName.java new file mode 100644 index 000000000000..4f465db74d12 --- /dev/null +++ b/plugins/devkit/testData/inspections/getFamilyNameViolation/NotViolatedByGetName.java @@ -0,0 +1,16 @@ +import com.intellij.codeInspection.QuickFix; + +class MyQuickFix implements QuickFix { + + String someField; + + public String getName() { + return someField; + }; + + public String getFamilyName() { + return getName() + "123"; + }; + + +} \ No newline at end of file diff --git a/plugins/devkit/testData/inspections/getFamilyNameViolation/ViolationByGetName.java b/plugins/devkit/testData/inspections/getFamilyNameViolation/ViolationByPsiElementFieldUsage.java similarity index 72% rename from plugins/devkit/testData/inspections/getFamilyNameViolation/ViolationByGetName.java rename to plugins/devkit/testData/inspections/getFamilyNameViolation/ViolationByPsiElementFieldUsage.java index 35a461b33a20..abb5a55ca0a8 100644 --- a/plugins/devkit/testData/inspections/getFamilyNameViolation/ViolationByGetName.java +++ b/plugins/devkit/testData/inspections/getFamilyNameViolation/ViolationByPsiElementFieldUsage.java @@ -1,16 +1,17 @@ import com.intellij.codeInspection.QuickFix; +import com.intellij.psi.PsiElement; class MyQuickFix implements QuickFix { String someField; + PsiElement myElement; public String getName() { return someField; }; public String getFamilyName() { - return getName() + "123"; + return "error is here: " + String.valueOf(myElement); }; - } \ No newline at end of file diff --git a/plugins/devkit/testSources/inspections/QuickFixGetFamilyNameViolationInspectionTest.java b/plugins/devkit/testSources/inspections/QuickFixGetFamilyNameViolationInspectionTest.java index 10d4d984aa20..e090ff198a7a 100644 --- a/plugins/devkit/testSources/inspections/QuickFixGetFamilyNameViolationInspectionTest.java +++ b/plugins/devkit/testSources/inspections/QuickFixGetFamilyNameViolationInspectionTest.java @@ -39,17 +39,19 @@ public class QuickFixGetFamilyNameViolationInspectionTest extends JavaCodeInsigh " String getName();" + " String getFamilyName();" + "}"); + myFixture.addClass("package com.intellij.psi;" + + "public interface PsiElement {}"); } - public void testViolationByField() { + public void testNotViolatedByField() { myFixture.testHighlighting(getTestName(false) + ".java"); } - public void testViolationByGetName() { + public void testNotViolatedByGetName() { myFixture.testHighlighting(getTestName(false) + ".java"); } - public void testViolationByExternalParameter() { + public void testNotViolatedByExternalParameter() { myFixture.testHighlighting(getTestName(false) + ".java"); } @@ -64,4 +66,8 @@ public class QuickFixGetFamilyNameViolationInspectionTest extends JavaCodeInsigh public void testNotViolatedGetNameMethod() { myFixture.testHighlighting(getTestName(false) + ".java"); } + + public void testViolationByPsiElementFieldUsage() { + myFixture.testHighlighting(getTestName(false) + ".java"); + } } From cee94e59de6d3591e66d4977168c3094fbced3b9 Mon Sep 17 00:00:00 2001 From: Vladimir Krivosheev Date: Tue, 26 Jul 2016 11:07:34 +0200 Subject: [PATCH 3/5] cleanup --- .../intellij/application/options/colors/ColorAndFontOptions.java | 1 - 1 file changed, 1 deletion(-) diff --git a/platform/lang-impl/src/com/intellij/application/options/colors/ColorAndFontOptions.java b/platform/lang-impl/src/com/intellij/application/options/colors/ColorAndFontOptions.java index 09c152e0c16d..3a7af6500921 100644 --- a/platform/lang-impl/src/com/intellij/application/options/colors/ColorAndFontOptions.java +++ b/platform/lang-impl/src/com/intellij/application/options/colors/ColorAndFontOptions.java @@ -27,7 +27,6 @@ import com.intellij.ide.util.PropertiesComponent; import com.intellij.openapi.Disposable; import com.intellij.openapi.application.ApplicationBundle; import com.intellij.openapi.application.ApplicationNamesInfo; -import com.intellij.openapi.diagnostic.Logger; import com.intellij.openapi.editor.colors.ColorKey; import com.intellij.openapi.editor.colors.EditorColorsManager; import com.intellij.openapi.editor.colors.EditorColorsScheme; From abfa3e0bbbd7fed68e10145fff8122de3f09df45 Mon Sep 17 00:00:00 2001 From: Vladimir Krivosheev Date: Tue, 26 Jul 2016 11:08:13 +0200 Subject: [PATCH 4/5] cleanup --- .../intellij/openapi/actionSystem/ex/QuickListsManager.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/platform/platform-impl/src/com/intellij/openapi/actionSystem/ex/QuickListsManager.java b/platform/platform-impl/src/com/intellij/openapi/actionSystem/ex/QuickListsManager.java index 8d3898df0c09..2d523324655a 100644 --- a/platform/platform-impl/src/com/intellij/openapi/actionSystem/ex/QuickListsManager.java +++ b/platform/platform-impl/src/com/intellij/openapi/actionSystem/ex/QuickListsManager.java @@ -25,7 +25,6 @@ import com.intellij.openapi.options.NonLazySchemeProcessor; import com.intellij.openapi.options.SchemeManager; import com.intellij.openapi.options.SchemeManagerFactory; import com.intellij.openapi.project.Project; -import com.intellij.util.ThrowableConvertor; import gnu.trove.THashSet; import org.jdom.Element; import org.jetbrains.annotations.NotNull; @@ -94,7 +93,7 @@ public class QuickListsManager implements ExportableApplicationComponent { public void initComponent() { for (BundledQuickListsProvider provider : BundledQuickListsProvider.EP_NAME.getExtensions()) { for (final String path : provider.getBundledListsRelativePaths()) { - mySchemeManager.loadBundledScheme(path, provider, element -> createItem(element)); + mySchemeManager.loadBundledScheme(path, provider, QuickListsManager::createItem); } } mySchemeManager.loadSchemes(); From 804ea52c47f66918c58577b88bfba5719f4e1fd4 Mon Sep 17 00:00:00 2001 From: Vladimir Krivosheev Date: Tue, 26 Jul 2016 11:12:43 +0200 Subject: [PATCH 5/5] do not use Kotlin in the core-api It leads to ugly kotlin code, because we cannot use `override var name: String` anymore, but no other solution --- .../testSrc/SchemeManagerTest.kt | 7 +++++- platform/core-api/core-api.iml | 1 - .../com/intellij/openapi/options/Scheme.java | 23 +++++++++++++++++++ .../projectModel-api/projectModel-api.iml | 4 ++-- .../com/intellij/openapi/options/scheme.kt | 6 +---- .../ExternalizableSchemeAdapter.kt | 8 ++++++- 6 files changed, 39 insertions(+), 10 deletions(-) create mode 100644 platform/core-api/src/com/intellij/openapi/options/Scheme.java rename platform/{core-api => projectModel-api}/src/com/intellij/openapi/options/scheme.kt (97%) diff --git a/platform/configuration-store-impl/testSrc/SchemeManagerTest.kt b/platform/configuration-store-impl/testSrc/SchemeManagerTest.kt index 4d3fb2d8247c..d1545796ccd7 100644 --- a/platform/configuration-store-impl/testSrc/SchemeManagerTest.kt +++ b/platform/configuration-store-impl/testSrc/SchemeManagerTest.kt @@ -363,7 +363,12 @@ private fun checkSchemes(baseDir: Path, expected: String, ignoreDeleted: Boolean } @Tag("scheme") -data class TestScheme(override @field:com.intellij.util.xmlb.annotations.Attribute var name: String = "", @field:com.intellij.util.xmlb.annotations.Attribute var data: String? = null) : ExternalizableScheme { +data class TestScheme(@field:com.intellij.util.xmlb.annotations.Attribute @field:kotlin.jvm.JvmField var name: String = "", @field:com.intellij.util.xmlb.annotations.Attribute var data: String? = null) : ExternalizableScheme { + override fun getName() = name + + override fun setName(value: String) { + name = value + } } open class TestSchemesProcessor : BaseSchemeProcessor() { diff --git a/platform/core-api/core-api.iml b/platform/core-api/core-api.iml index 9c4912acf281..0877129ba99f 100644 --- a/platform/core-api/core-api.iml +++ b/platform/core-api/core-api.iml @@ -24,6 +24,5 @@ - \ No newline at end of file diff --git a/platform/core-api/src/com/intellij/openapi/options/Scheme.java b/platform/core-api/src/com/intellij/openapi/options/Scheme.java new file mode 100644 index 000000000000..5ec1dda694c2 --- /dev/null +++ b/platform/core-api/src/com/intellij/openapi/options/Scheme.java @@ -0,0 +1,23 @@ +/* + * Copyright 2000-2016 JetBrains s.r.o. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.intellij.openapi.options; + +import org.jetbrains.annotations.NotNull; + +public interface Scheme { + @NotNull + String getName(); +} diff --git a/platform/projectModel-api/projectModel-api.iml b/platform/projectModel-api/projectModel-api.iml index c0265a8dfada..9ae795864733 100644 --- a/platform/projectModel-api/projectModel-api.iml +++ b/platform/projectModel-api/projectModel-api.iml @@ -9,6 +9,6 @@ + - - + \ No newline at end of file diff --git a/platform/core-api/src/com/intellij/openapi/options/scheme.kt b/platform/projectModel-api/src/com/intellij/openapi/options/scheme.kt similarity index 97% rename from platform/core-api/src/com/intellij/openapi/options/scheme.kt rename to platform/projectModel-api/src/com/intellij/openapi/options/scheme.kt index e26317c0fa42..301293907c2b 100644 --- a/platform/core-api/src/com/intellij/openapi/options/scheme.kt +++ b/platform/projectModel-api/src/com/intellij/openapi/options/scheme.kt @@ -21,12 +21,8 @@ import com.intellij.openapi.project.Project import com.intellij.openapi.util.WriteExternalException import org.jdom.Parent -interface Scheme { - val name: String -} - interface ExternalizableScheme : Scheme { - override var name: String + fun setName(value: String) } abstract class SchemeManagerFactory { diff --git a/platform/projectModel-impl/src/com/intellij/configurationStore/ExternalizableSchemeAdapter.kt b/platform/projectModel-impl/src/com/intellij/configurationStore/ExternalizableSchemeAdapter.kt index 9f1b21e2a548..2cde224b76db 100644 --- a/platform/projectModel-impl/src/com/intellij/configurationStore/ExternalizableSchemeAdapter.kt +++ b/platform/projectModel-impl/src/com/intellij/configurationStore/ExternalizableSchemeAdapter.kt @@ -20,7 +20,13 @@ import org.jdom.Element import kotlin.properties.Delegates abstract class ExternalizableSchemeAdapter : ExternalizableScheme { - override var name: String by Delegates.notNull() + private var myName: String by Delegates.notNull() + + override fun getName() = myName + + override fun setName(value: String) { + myName = value + } override fun toString() = name }