frequently used super class inspection: delete selected methods in quick fix; update description; set enabled by default

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<PsiClass> 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<PsiMethod> ownMethods = ((PsiExtensibleClass)aClass).getOwnMethods();
+    // first is own method, second is parent
+    List<Pair<PsiMethod, Set<PsiMethod>>> 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<PsiMethod> memberInfos = oldOverridenMethods.stream().filter(m -> {
+      Set<PsiMethod> 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<PsiMethod> 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<PsiMethod> getOverridenMethodsToDelete(Stream<PsiMethod> candidates, String newClassName) {
+    DialogBuilder dlg = new DialogBuilder();
+    MemberSelectionPanel panel = new MemberSelectionPanel("<html>Choose members to delete since they are already defined in <b>" + newClassName + "</b>",
+                                                          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();
+  }
 }

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<LocalQuickFix> 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;

java/java-tests/testData/inspection/smartInheritance/FixClassAndClass.java

@@ -0,0 +1,46 @@
+public abstract class FixClassAndClass implem<caret>ents 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() {
+
+  }
+}

java/java-tests/testData/inspection/smartInheritance/FixClassAndClass2.java

@@ -0,0 +1,48 @@
+public abstract class FixClassAndClass2 implem<caret>ents 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() {
+
+  }
+}

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() {
+
+  }
+}

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() {
+
+  }
+}

java/java-tests/testData/inspection/smartInheritance/FixClassAndInterface.java

@@ -0,0 +1,48 @@
+public abstract class FixClassAndInterface extend<caret>s 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() {
+
+  }
+}

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() {
+
+  }
+}

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<String, Integer>... 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<IntentionAction> 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);
+  }
 }

resources-en/src/inspectionDescriptions/FrequentlyUsedInheritorInspection.html

@@ -1,7 +1,7 @@
 <html>
 <body>
-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.
 <!-- tooltip end -->
-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.
 </body>
 </html>

resources/src/META-INF/IdeaPlugin.xml

@@ -817,7 +817,7 @@
                      implementationClass="com.intellij.codeInspection.magicConstant.MagicConstantInspection" />
     <localInspection groupPath="Java" language="JAVA" shortName="FrequentlyUsedInheritorInspection"
                      groupBundle="messages.InspectionsBundle"
-                     groupKey="group.names.inheritance.issues" enabledByDefault="false" level="INFORMATION"
+                     groupKey="group.names.inheritance.issues" enabledByDefault="true" level="INFORMATION"
                      implementationClass="com.intellij.compiler.inspection.FrequentlyUsedInheritorInspection"
                      displayName="Class may extend a commonly used base class"/>
     <localInspection language="UAST" shortName="ImplicitSubclassInspection"