CapturingCleanerInspection: fix review issues

java/java-impl/src/META-INF/JavaPlugin.xml

@@ -622,10 +622,10 @@
                      groupKey="group.names.performance.issues" enabledByDefault="true" level="WARNING"
                      implementationClass="com.intellij.codeInspection.CollectionAddAllCanBeReplacedWithConstructorInspection"
                      displayName="Redundant 'Collection.addAll()' call"/>
-    <localInspection groupPath="Java" language="JAVA" shortName="CleanerCapturingThis"
+    <localInspection groupPath="Java" language="JAVA" shortName="CapturingCleaner"
                      groupBundle="messages.InspectionsBundle"
                      groupKey="group.names.probable.bugs" enabledByDefault="true" level="WARNING"
-                     implementationClass="com.intellij.codeInspection.CleanerCapturingThisInspection"
+                     implementationClass="com.intellij.codeInspection.CapturingCleanerInspection"
                      bundle="messages.InspectionsBundle"
                      key="inspection.cleaner.capturing.this.description"/>
     <localInspection groupPath="Java" language="JAVA" shortName="OverwrittenKey"

java/java-impl/src/com/intellij/codeInspection/CapturingCleanerInspection.java

@@ -13,9 +13,11 @@ import org.jetbrains.annotations.Contract;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 
+import java.util.Optional;
+
 import static com.intellij.util.ObjectUtils.tryCast;
 
-public class CleanerCapturingThisInspection extends AbstractBaseJavaLocalInspectionTool {
+public class CapturingCleanerInspection extends AbstractBaseJavaLocalInspectionTool {
 
   private static final CallMatcher CLEANER_REGISTER = CallMatcher.instanceCall(
     "java.lang.ref.Cleaner", "register"
@@ -33,62 +35,78 @@ public class CleanerCapturingThisInspection extends AbstractBaseJavaLocalInspect
         if (!CLEANER_REGISTER.test(call)) return;
         PsiExpression[] expressions = call.getArgumentList().getExpressions();
         PsiExpression trackedObject = ExpressionUtils.resolveExpression(expressions[0]);
-        PsiExpression runnableExpr = ExpressionUtils.resolveExpression(expressions[1]);
-        if (trackedObject == null || runnableExpr == null) return;
+        PsiExpression runnableExpression = ExpressionUtils.resolveExpression(expressions[1]);
+        if (trackedObject == null || runnableExpression == null) return;
 
         final String referenceName;
+        final PsiElement highlightingElement;
         if (trackedObject instanceof PsiThisExpression) {
           PsiClassType classType = tryCast(trackedObject.getType(), PsiClassType.class);
           if (classType == null) return;
           PsiClass trackedClass = classType.resolve();
           if (trackedClass == null) return;
-          if (!capturesThis(runnableExpr, trackedClass)) return;
+          PsiElement elementCapturingThis = getElementCapturingThis(runnableExpression, trackedClass);
+          if (elementCapturingThis == null) return;
           referenceName = "this";
+          highlightingElement = elementCapturingThis;
         }
         else if (trackedObject instanceof PsiReferenceExpression) {
           PsiVariable variable = tryCast(((PsiReferenceExpression)trackedObject).resolve(), PsiVariable.class);
           if (variable == null) return;
           if (variable instanceof PsiField) return;
-          if (!VariableAccessUtils.variableIsUsed(variable, runnableExpr)) return;
+          if (!VariableAccessUtils.variableIsUsed(variable, runnableExpression)) return;
+          Optional<PsiElement> referenceExpression = StreamEx.ofTree(((PsiElement)runnableExpression), el -> StreamEx.of(el.getChildren()))
+            .findAny(el -> el instanceof PsiReferenceExpression &&
+                           tryCast(((PsiReferenceExpression)el).resolve(), PsiVariable.class) == variable);
+          if (!referenceExpression.isPresent()) return;
           String variableName = variable.getName();
           if (variableName == null) return;
           referenceName = variableName;
+          highlightingElement = referenceExpression.get();
         }
         else {
           return;
         }
-        holder.registerProblem(runnableExpr, InspectionsBundle.message("inspection.cleaner.capturing.this", referenceName));
+        holder.registerProblem(highlightingElement, InspectionsBundle.message("inspection.cleaner.capturing.this", referenceName));
       }
 
-      private boolean capturesThis(PsiExpression runnableExpr, PsiClass trackedClass) {
+      private PsiElement getElementCapturingThis(PsiExpression runnableExpr, PsiClass trackedClass) {
         if (runnableExpr instanceof PsiMethodReferenceExpression) {
           PsiMethodReferenceExpression methodReference = (PsiMethodReferenceExpression)runnableExpr;
-          if (PsiMethodReferenceUtil.isStaticallyReferenced(methodReference)) return false;
+          if (PsiMethodReferenceUtil.isStaticallyReferenced(methodReference)) return null;
 
-          PsiThisExpression thisExpression = tryCast(methodReference.getQualifier(), PsiThisExpression.class);
-          if (thisExpression == null) return false;
-          PsiClass thisClass = resolveThis(thisExpression);
-          if (thisClass != trackedClass) return false;
-        }
-        else if (runnableExpr instanceof PsiLambdaExpression) {
-          PsiLambdaExpression lambda = (PsiLambdaExpression)runnableExpr;
-          if (lambda.getParameterList().getParametersCount() != 0) return false;
-          PsiElement lambdaBody = lambda.getBody();
-          if (lambdaBody == null) return false;
-          if (!lambdaCapturesThis(lambdaBody, trackedClass)) return false;
-        }
-        else if (runnableExpr instanceof PsiNewExpression) {
-          PsiNewExpression newExpression = (PsiNewExpression)runnableExpr;
-          if (newExpression.getAnonymousClass() == null) {
-            PsiJavaCodeReferenceElement classReference = newExpression.getClassReference();
-            if (classReference == null) return false;
-            PsiClass aClass = tryCast(classReference.resolve(), PsiClass.class);
-            if (aClass == null) return false;
-            if (aClass.getContainingClass() != trackedClass) return false;
-            if (aClass.hasModifier(JvmModifier.STATIC)) return false;
+          PsiElement qualifier = methodReference.getQualifier();
+          if (qualifier instanceof PsiThisExpression) {
+            PsiClass thisClass = resolveThis((PsiThisExpression)qualifier);
+            if (thisClass != trackedClass) return null;
+            return qualifier;
           }
+          //if (qualifier instanceof PsiReferenceExpression) {
+          //  PsiField field = tryCast(((PsiReferenceExpression)qualifier).resolve(), PsiField.class);
+          //  if (!memberBringsThisRef(trackedClass, field)) return null;
+          //  return qualifier;
+          //}
+          return null;
         }
-        return true;
+        if (runnableExpr instanceof PsiLambdaExpression) {
+          PsiLambdaExpression lambda = (PsiLambdaExpression)runnableExpr;
+          if (lambda.getParameterList().getParametersCount() != 0) return null;
+          PsiElement lambdaBody = lambda.getBody();
+          if (lambdaBody == null) return null;
+          return getElementLambdaCapturingThis(lambdaBody, trackedClass).orElse(null);
+        }
+        if (runnableExpr instanceof PsiNewExpression) {
+          PsiNewExpression newExpression = (PsiNewExpression)runnableExpr;
+          if (newExpression.getAnonymousClass() != null) return newExpression;
+          PsiJavaCodeReferenceElement classReference = newExpression.getClassReference();
+          if (classReference == null) return null;
+          PsiClass aClass = tryCast(classReference.resolve(), PsiClass.class);
+          if (aClass == null) return null;
+          if (aClass.getContainingClass() != trackedClass) return null;
+          if (aClass.hasModifier(JvmModifier.STATIC)) return null;
+          return classReference;
+        }
+        return null;
       }
     };
   }
@@ -102,9 +120,9 @@ public class CleanerCapturingThisInspection extends AbstractBaseJavaLocalInspect
     return classType.resolve();
   }
 
-  private static boolean lambdaCapturesThis(@NotNull PsiElement lambdaBody, @NotNull PsiClass containingClass) {
+  private static Optional<PsiElement> getElementLambdaCapturingThis(@NotNull PsiElement lambdaBody, @NotNull PsiClass containingClass) {
     return StreamEx.ofTree(lambdaBody, el -> StreamEx.of(el.getChildren()))
-      .anyMatch(element -> isThisCapturingElement(containingClass, element));
+      .findAny(element -> isThisCapturingElement(containingClass, element));
   }
 
   private static boolean isThisCapturingElement(@NotNull PsiClass containingClass, PsiElement element) {
@@ -113,18 +131,23 @@ public class CleanerCapturingThisInspection extends AbstractBaseJavaLocalInspect
     }
     else if (element instanceof PsiReferenceExpression) {
       PsiMember member = tryCast(((PsiReferenceExpression)element).resolve(), PsiMember.class);
-      if (member == null) return false;
-      PsiClass memberContainingClass = member.getContainingClass();
-      if (memberContainingClass == null) return false;
-      if (!InheritanceUtil.isInheritorOrSelf(containingClass, memberContainingClass, true) &&
-          !isInnerClassOf(containingClass, memberContainingClass)) {
-        return false;
-      }
-      return !member.hasModifierProperty(PsiModifier.STATIC);
+      return memberBringsThisRef(containingClass, member);
     }
     return false;
   }
 
+  @Contract("_, null -> false")
+  private static boolean memberBringsThisRef(@NotNull PsiClass containingClass, PsiMember member) {
+    if (member == null) return false;
+    PsiClass memberContainingClass = member.getContainingClass();
+    if (memberContainingClass == null) return false;
+    if (!InheritanceUtil.isInheritorOrSelf(containingClass, memberContainingClass, true) &&
+        !isInnerClassOf(containingClass, memberContainingClass)) {
+      return false;
+    }
+    return !member.hasModifierProperty(PsiModifier.STATIC);
+  }
+
   @Contract("_, null -> false")
   private static boolean isInnerClassOf(@Nullable PsiClass inner, @Nullable PsiClass outer) {
     if (outer == null) return false;

java/java-tests/testData/inspection/cleanerCapturingThis/CapturingCleanerInspection.java

@@ -5,7 +5,7 @@ class Anonymous {
 
   static void free(int descriptor) {}
 
-  Cleaner.Cleanable cleanable = Cleaner.create().register(this, <warning descr="Runnable passed to register() captures 'this' reference that leads to memory leak">new Runnable() {
+  Cleaner.Cleanable cleanable = Cleaner.create().register(this, <warning descr="Runnable passed to Cleaner.register() captures 'this' reference that leads to memory leak">new Runnable() {
     @Override
     public void run() {
       System.out.println("adsad");
@@ -18,7 +18,7 @@ class Inner {
 
   static void free(int descriptor) {}
 
-  Cleaner.Cleanable cleanable = Cleaner.create().register(this, <warning descr="Runnable passed to register() captures 'this' reference that leads to memory leak">new MyRunnable()</warning>);
+  Cleaner.Cleanable cleanable = Cleaner.create().register(this, new <warning descr="Runnable passed to Cleaner.register() captures 'this' reference that leads to memory leak">MyRunnable</warning>());
 
   private class MyRunnable implements Runnable {
     @Override
@@ -33,7 +33,7 @@ class InstanceMethodReference {
 
   static void free(int descriptor) {}
 
-  Cleaner.Cleanable cleanable = Cleaner.create().register(this, <warning descr="Runnable passed to register() captures 'this' reference that leads to memory leak">this::run</warning>);
+  Cleaner.Cleanable cleanable = Cleaner.create().register(this, <warning descr="Runnable passed to Cleaner.register() captures 'this' reference that leads to memory leak">this</warning>::run);
 
   private void run() {
     System.out.println("adsad");
@@ -46,7 +46,7 @@ class LambdaExprBodyInstanceMethod {
 
   static void free(int descriptor) {}
 
-  Cleaner.Cleanable cleanable = Cleaner.create().register(this, <warning descr="Runnable passed to register() captures 'this' reference that leads to memory leak">() -> run()</warning>);
+  Cleaner.Cleanable cleanable = Cleaner.create().register(this, () -> <warning descr="Runnable passed to Cleaner.register() captures 'this' reference that leads to memory leak">run</warning>());
 
   private void run() {
     System.out.println("adsad");
@@ -57,10 +57,10 @@ class LambdaExprBodyInstanceMethod {
 class LambdaInstanceField {
   int fileDescriptor;
 
-  Cleaner.Cleanable cleanable = Cleaner.create().register(this, <warning descr="Runnable passed to register() captures 'this' reference that leads to memory leak">() -> {
+  Cleaner.Cleanable cleanable = Cleaner.create().register(this, () -> {
     System.out.println("adsad");
-    fileDescriptor = 0;
-  }</warning>);
+    <warning descr="Runnable passed to Cleaner.register() captures 'this' reference that leads to memory leak">fileDescriptor</warning> = 0;
+  });
 }
 
 class LambdaInstanceMethod {
@@ -68,10 +68,10 @@ class LambdaInstanceMethod {
 
   static void free(int descriptor) {}
 
-  Cleaner.Cleanable cleanable = Cleaner.create().register(this, <warning descr="Runnable passed to register() captures 'this' reference that leads to memory leak">() -> {
+  Cleaner.Cleanable cleanable = Cleaner.create().register(this, () -> {
     System.out.println("adsad");
-    free(fileDescriptor);
-  }</warning>);
+    free(<warning descr="Runnable passed to Cleaner.register() captures 'this' reference that leads to memory leak">fileDescriptor</warning>);
+  });
 }
 
 class Base {
@@ -79,10 +79,10 @@ class Base {
 }
 
 class LambdaInstanceSuperField extends Base {
-  Cleaner.Cleanable cleanable = Cleaner.create().register(this, <warning descr="Runnable passed to register() captures 'this' reference that leads to memory leak">() -> {
+  Cleaner.Cleanable cleanable = Cleaner.create().register(this, () -> {
     System.out.println("adsad");
-    fileDescriptor = 0;
-  }</warning>);
+    <warning descr="Runnable passed to Cleaner.register() captures 'this' reference that leads to memory leak">fileDescriptor</warning> = 0;
+  });
 }
 
 class LambdaThis {
@@ -90,9 +90,9 @@ class LambdaThis {
 
   static void free(int descriptor) {}
 
-  Cleaner.Cleanable cleanable = Cleaner.create().register(this, <warning descr="Runnable passed to register() captures 'this' reference that leads to memory leak">() -> {
-    LambdaThis o = this;
-  }</warning>);
+  Cleaner.Cleanable cleanable = Cleaner.create().register(this, () -> {
+    LambdaThis o = <warning descr="Runnable passed to Cleaner.register() captures 'this' reference that leads to memory leak">this</warning>;
+  });
 }
 
 class StaticMethodReference {
@@ -122,6 +122,16 @@ class SafeInstanceMethodReference {
   }
 }
 
+
+class FieldMethodReference {
+  Cleaner cleaner = Cleaner.create();
+  ResourceHolder resource = new ResourceHolder();
+
+  FieldMethodReference() {
+    cleaner.register(this, resource::free);
+  }
+}
+
 // Reference as tracking target
 
 class StaticMethodFactory {
@@ -129,7 +139,7 @@ class StaticMethodFactory {
 
   static ResourceHolder create() {
     ResourceHolder holder = new ResourceHolder();
-    cleaner.register(holder, <warning descr="Runnable passed to register() captures 'holder' reference that leads to memory leak">holder::free</warning>);
+    cleaner.register(holder, <warning descr="Runnable passed to Cleaner.register() captures 'holder' reference that leads to memory leak">holder</warning>::free);
     return holder;
   }
 }
@@ -144,7 +154,7 @@ class ConstructorDelegatesToStaticMethod {
   }
 
   static void register(ConstructorDelegatesToStaticMethod holder) {
-    cleaner.register(holder, <warning descr="Runnable passed to register() captures 'holder' reference that leads to memory leak">() -> free(holder.resource)</warning>);
+    cleaner.register(holder, () -> free(<warning descr="Runnable passed to Cleaner.register() captures 'holder' reference that leads to memory leak">holder</warning>.resource));
   }
 
   static void free(int resource){}
@@ -158,7 +168,7 @@ class InnerAccesInstanceOuterMembers {
     Cleaner cleaner = Cleaner.create();
 
     public Inner() {
-      cleaner.register(this, <warning descr="Runnable passed to register() captures 'this' reference that leads to memory leak">() -> resource = -1</warning>);
+      cleaner.register(this, () -> <warning descr="Runnable passed to Cleaner.register() captures 'this' reference that leads to memory leak">resource</warning> = -1);
     }
   }
 }

java/java-tests/testSrc/com/intellij/java/codeInspection/CapturingCleanerInspectionTest.java

@@ -3,20 +3,20 @@
 package com.intellij.java.codeInspection;
 
 import com.intellij.JavaTestUtil;
-import com.intellij.codeInspection.CleanerCapturingThisInspection;
+import com.intellij.codeInspection.CapturingCleanerInspection;
 import com.intellij.testFramework.LightProjectDescriptor;
 import com.intellij.testFramework.fixtures.LightCodeInsightFixtureTestCase;
 import org.jetbrains.annotations.NotNull;
 
-public class CleanerCapturingThisInspectionTest extends LightCodeInsightFixtureTestCase {
+public class CapturingCleanerInspectionTest extends LightCodeInsightFixtureTestCase {
 
-  public void testCleanerCapturingThisInspection() {doTest();}
+  public void testCapturingCleanerInspection() {doTest();}
 
   @Override
   protected void setUp() throws Exception {
     super.setUp();
 
-    myFixture.enableInspections(new CleanerCapturingThisInspection());
+    myFixture.enableInspections(new CapturingCleanerInspection());
   }
 
   private void doTest() {

platform/platform-resources-en/src/messages/InspectionsBundle.properties

@@ -940,5 +940,5 @@ inspection.conditional.break.in.infinite.loop.description=Conditional break insi
 
 inspection.endless.stream.description=Non-short-circuit operation consumes the infinite stream
 
-inspection.cleaner.capturing.this=Runnable passed to register() captures ''{0}'' reference that leads to memory leak
+inspection.cleaner.capturing.this=Runnable passed to Cleaner.register() captures ''{0}'' reference that leads to memory leak
 inspection.cleaner.capturing.this.description=Runnable passed to Cleaner.register() captures reference that leads to memory leak