diff --git a/java/compiler/impl/src/com/intellij/compiler/inspection/ChangeSuperClassFix.java b/java/compiler/impl/src/com/intellij/compiler/inspection/ChangeSuperClassFix.java index c1b207a92fa1..73575544a2e5 100644 --- a/java/compiler/impl/src/com/intellij/compiler/inspection/ChangeSuperClassFix.java +++ b/java/compiler/impl/src/com/intellij/compiler/inspection/ChangeSuperClassFix.java @@ -15,18 +15,32 @@ */ package com.intellij.compiler.inspection; +import com.intellij.codeInsight.FileModificationService; import com.intellij.codeInsight.daemon.GroupNames; import com.intellij.codeInsight.intention.HighPriorityAction; import com.intellij.codeInspection.LocalQuickFix; import com.intellij.codeInspection.ProblemDescriptor; +import com.intellij.openapi.application.WriteAction; import com.intellij.openapi.project.Project; +import com.intellij.openapi.ui.DialogBuilder; +import com.intellij.openapi.util.Pair; import com.intellij.psi.*; import com.intellij.psi.codeStyle.JavaCodeStyleManager; +import com.intellij.psi.impl.source.PsiExtensibleClass; +import com.intellij.refactoring.ui.MemberSelectionPanel; +import com.intellij.refactoring.util.classMembers.MemberInfo; import com.intellij.util.ArrayUtil; import com.intellij.util.ObjectUtils; +import com.intellij.util.containers.ContainerUtil; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.TestOnly; +import java.util.Collections; +import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; + public class ChangeSuperClassFix implements LocalQuickFix, HighPriorityAction { @NotNull private final SmartPsiElementPointer myNewSuperClass; @@ -35,15 +49,18 @@ public class ChangeSuperClassFix implements LocalQuickFix, HighPriorityAction { private final int myInheritorCount; @NotNull private final String myNewSuperName; - private final boolean myNewSuperIsInterface; + private final boolean myImplements; - public ChangeSuperClassFix(@NotNull final PsiClass newSuperClass, final int percent, @NotNull final PsiClass oldSuperClass) { + public ChangeSuperClassFix(@NotNull final PsiClass newSuperClass, + @NotNull final PsiClass oldSuperClass, + final int percent, + final boolean isImplements) { final SmartPointerManager smartPointerManager = SmartPointerManager.getInstance(newSuperClass.getProject()); myNewSuperName = ObjectUtils.notNull(newSuperClass.getQualifiedName()); - myNewSuperIsInterface = newSuperClass.isInterface(); myNewSuperClass = smartPointerManager.createSmartPsiElementPointer(newSuperClass); myOldSuperClass = smartPointerManager.createSmartPsiElementPointer(oldSuperClass); myInheritorCount = percent; + myImplements = isImplements; } @NotNull @@ -60,7 +77,7 @@ public class ChangeSuperClassFix implements LocalQuickFix, HighPriorityAction { @NotNull @Override public String getName() { - return String.format("Make " + (myNewSuperIsInterface ? "implements" : "extends") + " '%s'", myNewSuperName); + return String.format("Make " + (myImplements ? "implements" : "extends") + " '%s'", myNewSuperName); } @NotNull @@ -69,12 +86,20 @@ public class ChangeSuperClassFix implements LocalQuickFix, HighPriorityAction { return GroupNames.INHERITANCE_GROUP_NAME; } + @Override + public boolean startInWriteAction() { + return false; + } + @Override public void applyFix(@NotNull final Project project, @NotNull final ProblemDescriptor problemDescriptor) { final PsiClass oldSuperClass = myOldSuperClass.getElement(); final PsiClass newSuperClass = myNewSuperClass.getElement(); if (oldSuperClass == null || newSuperClass == null) return; - changeSuperClass((PsiClass)problemDescriptor.getPsiElement(), oldSuperClass, newSuperClass); + PsiElement element = problemDescriptor.getPsiElement(); + if (!(element instanceof PsiClass) || !FileModificationService.getInstance().preparePsiElementsForWrite(element)) return; + PsiClass aClass = (PsiClass)element; + changeSuperClass(aClass, oldSuperClass, newSuperClass); } /** @@ -86,38 +111,85 @@ public class ChangeSuperClassFix implements LocalQuickFix, HighPriorityAction { private static void changeSuperClass(@NotNull final PsiClass aClass, @NotNull final PsiClass oldSuperClass, @NotNull final PsiClass newSuperClass) { + List ownMethods = ((PsiExtensibleClass)aClass).getOwnMethods(); + // first is own method, second is parent + List>> oldOverridenMethods = + ownMethods.stream().map(m -> { + if (m.isConstructor()) return null; + PsiMethod[] supers = m.findSuperMethods(oldSuperClass); + if (supers.length == 0) return null; + return Pair.create(m, ContainerUtil.set(supers)); + }).collect(Collectors.toList()); + JavaPsiFacade psiFacade = JavaPsiFacade.getInstance(aClass.getProject()); PsiElementFactory factory = psiFacade.getElementFactory(); - if (aClass instanceof PsiAnonymousClass) { - ((PsiAnonymousClass)aClass).getBaseClassReference().replace(factory.createClassReferenceElement(newSuperClass)); + WriteAction.run(() -> { + PsiElement ref; + if (aClass instanceof PsiAnonymousClass) { + ref = ((PsiAnonymousClass)aClass).getBaseClassReference().replace(factory.createClassReferenceElement(newSuperClass)); + } else { + PsiReferenceList extendsList = ObjectUtils.notNull(aClass.getExtendsList()); + PsiJavaCodeReferenceElement[] refElements = + ArrayUtil.mergeArrays(getReferences(extendsList), getReferences(aClass.getImplementsList())); + for (PsiJavaCodeReferenceElement refElement : refElements) { + if (refElement.isReferenceTo(oldSuperClass)) { + refElement.delete(); + } + } + + PsiReferenceList list; + if (newSuperClass.isInterface() && !aClass.isInterface()) { + list = aClass.getImplementsList(); + } + else { + list = extendsList; + PsiJavaCodeReferenceElement[] elements = list.getReferenceElements(); + if (elements.length == 1 && + elements[0].isReferenceTo(psiFacade.findClass(CommonClassNames.JAVA_LANG_OBJECT, aClass.getResolveScope()))) { + elements[0].delete(); + } + } + assert list != null; + ref = list.add(factory.createClassReferenceElement(newSuperClass)); + } + JavaCodeStyleManager.getInstance(aClass.getProject()).shortenClassReferences(ref); + }); + + if (ownMethods.isEmpty()) { + // should not override methods from a new super class return; } - PsiReferenceList extendsList = ObjectUtils.notNull(aClass.getExtendsList()); - PsiJavaCodeReferenceElement[] refElements = - ArrayUtil.mergeArrays(getReferences(extendsList), getReferences(aClass.getImplementsList())); - for (PsiJavaCodeReferenceElement refElement : refElements) { - if (refElement.isReferenceTo(oldSuperClass)) { - refElement.delete(); - } - } + Stream memberInfos = oldOverridenMethods.stream().filter(m -> { + Set newSupers = ContainerUtil.set(m.getFirst().findSuperMethods(newSuperClass)); + return !newSupers.equals(m.getSecond()); + }).map(m -> m.getFirst()); - PsiReferenceList list; - if (newSuperClass.isInterface()) { - list = aClass.getImplementsList(); - } - else { - list = extendsList; - PsiJavaCodeReferenceElement[] elements = list.getReferenceElements(); - if (elements.length == 1 && - elements[0].isReferenceTo(psiFacade.findClass(CommonClassNames.JAVA_LANG_OBJECT, aClass.getResolveScope()))) { - elements[0].delete(); + List toDelete = getOverridenMethodsToDelete(memberInfos, newSuperClass.getName()); + WriteAction.run(() -> { + for (PsiMethod method : toDelete) { + method.delete(); } - } - PsiElement ref = list.add(factory.createClassReferenceElement(newSuperClass)); - JavaCodeStyleManager.getInstance(aClass.getProject()).shortenClassReferences(ref); + }); } + @NotNull private static PsiJavaCodeReferenceElement[] getReferences(PsiReferenceList list) { return list == null ? PsiJavaCodeReferenceElement.EMPTY_ARRAY : list.getReferenceElements(); } + + @NotNull + private static List getOverridenMethodsToDelete(Stream candidates, String newClassName) { + DialogBuilder dlg = new DialogBuilder(); + MemberSelectionPanel panel = new MemberSelectionPanel("Choose members to delete since they are already defined in " + newClassName + "", + candidates.map(m -> { + MemberInfo info = new MemberInfo(m); + info.setChecked(true); + return info; + }).collect(Collectors.toList()), null); + dlg.setCenterPanel(panel); + dlg.setTitle("Choose Members"); + return dlg.showAndGet() + ? panel.getTable().getSelectedMemberInfos().stream().map(info -> (PsiMethod)info.getMember()).collect(Collectors.toList()) + : Collections.emptyList(); + } } diff --git a/java/compiler/impl/src/com/intellij/compiler/inspection/FrequentlyUsedInheritorInspection.java b/java/compiler/impl/src/com/intellij/compiler/inspection/FrequentlyUsedInheritorInspection.java index 3fb3f83c4c32..50fd67e0da3c 100644 --- a/java/compiler/impl/src/com/intellij/compiler/inspection/FrequentlyUsedInheritorInspection.java +++ b/java/compiler/impl/src/com/intellij/compiler/inspection/FrequentlyUsedInheritorInspection.java @@ -27,6 +27,7 @@ import com.intellij.openapi.vfs.VirtualFile; import com.intellij.psi.*; import com.intellij.psi.impl.java.stubs.index.JavaFullClassNameIndex; import com.intellij.psi.search.GlobalSearchScope; +import com.intellij.util.SystemProperties; import one.util.streamex.MoreCollectors; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -40,14 +41,14 @@ public class FrequentlyUsedInheritorInspection extends BaseJavaLocalInspectionTo private static final Logger LOG = Logger.getInstance(FrequentlyUsedInheritorInspection.class); public static final byte MAX_RESULT = 3; - private static final int PERCENT_THRESHOLD = 20; + private static final int PERCENT_THRESHOLD = SystemProperties.getIntProperty("FrequentlyUsedInheritorInspection.percent.threshold", 20); @Nullable @Override public ProblemDescriptor[] checkClass(@NotNull final PsiClass aClass, @NotNull final InspectionManager manager, final boolean isOnTheFly) { - if (aClass.isInterface() || aClass instanceof PsiTypeParameter) { + if (aClass instanceof PsiTypeParameter) { return null; } @@ -63,7 +64,8 @@ public class FrequentlyUsedInheritorInspection extends BaseJavaLocalInspectionTo final Collection topInheritorsQuickFix = new ArrayList<>(topInheritors.size()); for (final ClassAndInheritorCount searchResult : topInheritors) { - final LocalQuickFix quickFix = new ChangeSuperClassFix(searchResult.psi, searchResult.number, superClass); + final LocalQuickFix quickFix = new ChangeSuperClassFix(searchResult.psi, superClass, searchResult.number, + searchResult.psi.isInterface() && !aClass.isInterface()); topInheritorsQuickFix.add(quickFix); if (topInheritorsQuickFix.size() >= MAX_RESULT) { break; @@ -128,7 +130,7 @@ public class FrequentlyUsedInheritorInspection extends BaseJavaLocalInspectionTo .filter(inheritor -> !(inheritor instanceof LightRef.LightAnonymousClassDef)) .map(inheritor -> { int count = compilerRefService.getInheritorCount(inheritor); - if (count * 100 > finalHierarchyCardinality * PERCENT_THRESHOLD) { + if (count != 1 && count * 100 > finalHierarchyCardinality * PERCENT_THRESHOLD) { return new Object() { final LightRef.LightClassHierarchyElementDef myDef = inheritor; final int inheritorCount = count; diff --git a/java/java-tests/testData/inspection/smartInheritance/FixClassAndClass.java b/java/java-tests/testData/inspection/smartInheritance/FixClassAndClass.java new file mode 100644 index 000000000000..6aee47e911a9 --- /dev/null +++ b/java/java-tests/testData/inspection/smartInheritance/FixClassAndClass.java @@ -0,0 +1,46 @@ +public abstract class FixClassAndClass implements A { + @Override + public void m1() {} +} + +interface A { + void m1(); + + void m2(); +} + +abstract class B implements A { + @Override + public void m1() {} +} + +class C1 extends B { + @Override + public void m2() { + + } +} +class C2 extends B { + @Override + public void m2() { + + } +} +class C3 extends B { + @Override + public void m2() { + + } +} +class C4 extends B { + @Override + public void m2() { + + } +} +class C5 extends B { + @Override + public void m2() { + + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/smartInheritance/FixClassAndClass2.java b/java/java-tests/testData/inspection/smartInheritance/FixClassAndClass2.java new file mode 100644 index 000000000000..5f96f654c2e2 --- /dev/null +++ b/java/java-tests/testData/inspection/smartInheritance/FixClassAndClass2.java @@ -0,0 +1,48 @@ +public abstract class FixClassAndClass2 implements A { +@Override +public void m1() {} +@Override +public void m2() {} +} + +interface A { + void m1(); + + void m2(); +} + +abstract class B implements A { + @Override + public void m1() {} +} + +class C1 extends B { + @Override + public void m2() { + + } +} +class C2 extends B { + @Override + public void m2() { + + } +} +class C3 extends B { + @Override + public void m2() { + + } +} +class C4 extends B { + @Override + public void m2() { + + } +} +class C5 extends B { + @Override + public void m2() { + + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/smartInheritance/FixClassAndClass2_after.java b/java/java-tests/testData/inspection/smartInheritance/FixClassAndClass2_after.java new file mode 100644 index 000000000000..9dbc9bbb20fd --- /dev/null +++ b/java/java-tests/testData/inspection/smartInheritance/FixClassAndClass2_after.java @@ -0,0 +1,46 @@ +public abstract class FixClassAndClass2 extends B { + @Override +public void m2() {} +} + +interface A { + void m1(); + + void m2(); +} + +abstract class B implements A { + @Override + public void m1() {} +} + +class C1 extends B { + @Override + public void m2() { + + } +} +class C2 extends B { + @Override + public void m2() { + + } +} +class C3 extends B { + @Override + public void m2() { + + } +} +class C4 extends B { + @Override + public void m2() { + + } +} +class C5 extends B { + @Override + public void m2() { + + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/smartInheritance/FixClassAndClass_after.java b/java/java-tests/testData/inspection/smartInheritance/FixClassAndClass_after.java new file mode 100644 index 000000000000..41946f78851d --- /dev/null +++ b/java/java-tests/testData/inspection/smartInheritance/FixClassAndClass_after.java @@ -0,0 +1,44 @@ +public abstract class FixClassAndClass extends B { +} + +interface A { + void m1(); + + void m2(); +} + +abstract class B implements A { + @Override + public void m1() {} +} + +class C1 extends B { + @Override + public void m2() { + + } +} +class C2 extends B { + @Override + public void m2() { + + } +} +class C3 extends B { + @Override + public void m2() { + + } +} +class C4 extends B { + @Override + public void m2() { + + } +} +class C5 extends B { + @Override + public void m2() { + + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/smartInheritance/FixClassAndInterface.java b/java/java-tests/testData/inspection/smartInheritance/FixClassAndInterface.java new file mode 100644 index 000000000000..54cc4e051282 --- /dev/null +++ b/java/java-tests/testData/inspection/smartInheritance/FixClassAndInterface.java @@ -0,0 +1,48 @@ +public abstract class FixClassAndInterface extends A { +@Override +public void m1() {} + } + +abstract class A { + abstract void m1(); + + abstract void m2(); +} + +abstract class B extends A { + void m1() { + + }; + +} + +class C1 extends B { + @Override + public void m2() { + + } +} +class C2 extends B { + @Override + public void m2() { + + } +} +class C3 extends B { + @Override + public void m2() { + + } +} +class C4 extends B { + @Override + public void m2() { + + } +} +class C5 extends B { + @Override + public void m2() { + + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/smartInheritance/FixClassAndInterface_after.java b/java/java-tests/testData/inspection/smartInheritance/FixClassAndInterface_after.java new file mode 100644 index 000000000000..2e3681a33026 --- /dev/null +++ b/java/java-tests/testData/inspection/smartInheritance/FixClassAndInterface_after.java @@ -0,0 +1,46 @@ +public abstract class FixClassAndInterface extends B { +} + +abstract class A { + abstract void m1(); + + abstract void m2(); +} + +abstract class B extends A { + void m1() { + + }; + +} + +class C1 extends B { + @Override + public void m2() { + + } +} +class C2 extends B { + @Override + public void m2() { + + } +} +class C3 extends B { + @Override + public void m2() { + + } +} +class C4 extends B { + @Override + public void m2() { + + } +} +class C5 extends B { + @Override + public void m2() { + + } +} \ No newline at end of file diff --git a/java/java-tests/testSrc/com/intellij/codeInspection/FrequentlyUsedInheritorInspectionTest.java b/java/java-tests/testSrc/com/intellij/codeInspection/FrequentlyUsedInheritorInspectionTest.java index b1967020d2ec..e91ec9538ad3 100644 --- a/java/java-tests/testSrc/com/intellij/codeInspection/FrequentlyUsedInheritorInspectionTest.java +++ b/java/java-tests/testSrc/com/intellij/codeInspection/FrequentlyUsedInheritorInspectionTest.java @@ -8,7 +8,9 @@ import com.intellij.compiler.CompilerReferencesTestBase; import com.intellij.compiler.inspection.ChangeSuperClassFix; import com.intellij.compiler.inspection.FrequentlyUsedInheritorInspection; import com.intellij.openapi.util.Pair; +import com.intellij.pom.java.LanguageLevel; import com.intellij.testFramework.SkipSlowTestLocally; +import com.intellij.testFramework.builders.JavaModuleFixtureBuilder; import com.intellij.util.containers.ContainerUtil; import com.intellij.util.containers.HashSet; import org.jetbrains.annotations.Nullable; @@ -31,6 +33,8 @@ public class FrequentlyUsedInheritorInspectionTest extends CompilerReferencesTes return JavaTestUtil.getJavaTestDataPath() + "/inspection/smartInheritance/"; } + //test inspection + public void testRelevantClassShowed() { doTest(Pair.create("B", 12)); } @@ -59,6 +63,20 @@ public class FrequentlyUsedInheritorInspectionTest extends CompilerReferencesTes doTest(); } + //test fixes + + public void testFixClassAndClass() { + doTestQuickFix("extends 'B'"); + } + + public void testFixClassAndClass2() { + doTestQuickFix("extends 'B'"); + } + + public void testFixClassAndInterface() { + doTestQuickFix("extends 'B'"); + } + private void doTest(final Pair... expectedResults) { myFixture.configureByFile(getTestName(false) + ".java"); rebuildProject(); @@ -88,6 +106,16 @@ public class FrequentlyUsedInheritorInspectionTest extends CompilerReferencesTes assertEquals(expectedSize, actions.size()); } + private void doTestQuickFix(String hintSuffix) { + myFixture.configureByFile(getTestName(false) + ".java"); + rebuildProject(); + + List fixes = myFixture.filterAvailableIntentions("Make " + hintSuffix); + IntentionAction fix = assertOneElement(fixes); + myFixture.launchAction(fix); + myFixture.checkResultByFile(getTestName(false) + "_after.java"); + } + @Nullable private static ChangeSuperClassFix getQuickFixFromWrapper(final QuickFixWrapper quickFixWrapper) { final LocalQuickFix quickFix = quickFixWrapper.getFix(); @@ -96,4 +124,9 @@ public class FrequentlyUsedInheritorInspectionTest extends CompilerReferencesTes } return null; } + + @Override + protected void tuneFixture(JavaModuleFixtureBuilder moduleBuilder) throws Exception { + moduleBuilder.setLanguageLevel(LanguageLevel.JDK_1_8); + } } diff --git a/resources-en/src/inspectionDescriptions/FrequentlyUsedInheritorInspection.html b/resources-en/src/inspectionDescriptions/FrequentlyUsedInheritorInspection.html index 51e2e99314a5..0cb923eb642c 100644 --- a/resources-en/src/inspectionDescriptions/FrequentlyUsedInheritorInspection.html +++ b/resources-en/src/inspectionDescriptions/FrequentlyUsedInheritorInspection.html @@ -1,7 +1,7 @@ -This inspection finds commonly used base class that could be extended instead of implementing interface or extending abstract class. +The inspection finds commonly used class/interface that could be extended/implemented instead of extending too broad interface or class. -The inspection works only if a project is built using IntelliJ IDEA build system and super class is located inside source files. +The inspection works only if a project is built using IntelliJ IDEA build system and a super class is located inside project source files. \ No newline at end of file diff --git a/resources/src/META-INF/IdeaPlugin.xml b/resources/src/META-INF/IdeaPlugin.xml index 7b059d1e6316..4c98597353b7 100644 --- a/resources/src/META-INF/IdeaPlugin.xml +++ b/resources/src/META-INF/IdeaPlugin.xml @@ -817,7 +817,7 @@ implementationClass="com.intellij.codeInspection.magicConstant.MagicConstantInspection" />