From c2484042bc8628ea267797a82d65afadbcf16880 Mon Sep 17 00:00:00 2001 From: Roman Ivanov Date: Tue, 12 Dec 2017 15:29:46 +0700 Subject: [PATCH] CapturingCleanerInspection: fix review issues --- java/java-impl/src/META-INF/JavaPlugin.xml | 2 +- .../CapturingCleanerInspection.java | 28 ++++--------------- ...rInspection.java => CapturingCleaner.java} | 22 +++++++-------- .../CapturingCleanerInspectionTest.java | 2 +- .../src/messages/InspectionsBundle.properties | 4 +-- 5 files changed, 20 insertions(+), 38 deletions(-) rename java/java-tests/testData/inspection/cleanerCapturingThis/{CapturingCleanerInspection.java => CapturingCleaner.java} (66%) diff --git a/java/java-impl/src/META-INF/JavaPlugin.xml b/java/java-impl/src/META-INF/JavaPlugin.xml index 0b31b1733ac7..555a69ebbfaf 100644 --- a/java/java-impl/src/META-INF/JavaPlugin.xml +++ b/java/java-impl/src/META-INF/JavaPlugin.xml @@ -627,7 +627,7 @@ groupKey="group.names.probable.bugs" enabledByDefault="true" level="WARNING" implementationClass="com.intellij.codeInspection.CapturingCleanerInspection" bundle="messages.InspectionsBundle" - key="inspection.cleaner.capturing.this.description"/> + key="inspection.capturing.cleaner.description"/> referenceExpression = StreamEx.ofTree(((PsiElement)runnableExpression), el -> StreamEx.of(el.getChildren())) .findAny(el -> el instanceof PsiReferenceExpression && - tryCast(((PsiReferenceExpression)el).resolve(), PsiVariable.class) == variable); + ((PsiReferenceExpression)el).isReferenceTo(variable)); if (!referenceExpression.isPresent()) return; String variableName = variable.getName(); if (variableName == null) return; - referenceName = variableName; highlightingElement = referenceExpression.get(); } else { return; } - holder.registerProblem(highlightingElement, InspectionsBundle.message("inspection.cleaner.capturing.this", referenceName)); + holder.registerProblem(highlightingElement, InspectionsBundle.message("inspection.capturing.cleaner")); } private PsiElement getElementCapturingThis(PsiExpression runnableExpr, PsiClass trackedClass) { @@ -77,15 +73,10 @@ public class CapturingCleanerInspection extends AbstractBaseJavaLocalInspectionT PsiElement qualifier = methodReference.getQualifier(); if (qualifier instanceof PsiThisExpression) { - PsiClass thisClass = resolveThis((PsiThisExpression)qualifier); + PsiClass thisClass = PsiUtil.resolveClassInType(((PsiThisExpression)qualifier).getType()); 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; } if (runnableExpr instanceof PsiLambdaExpression) { @@ -103,7 +94,7 @@ public class CapturingCleanerInspection extends AbstractBaseJavaLocalInspectionT 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; + if (aClass.hasModifierProperty(PsiModifier.STATIC)) return null; return classReference; } return null; @@ -111,15 +102,6 @@ public class CapturingCleanerInspection extends AbstractBaseJavaLocalInspectionT }; } - @Contract("null -> null") - @Nullable - static PsiClass resolveThis(@Nullable PsiThisExpression thisExpression) { - if (thisExpression == null) return null; - PsiClassType classType = tryCast(thisExpression.getType(), PsiClassType.class); - if (classType == null) return null; - return classType.resolve(); - } - private static Optional getElementLambdaCapturingThis(@NotNull PsiElement lambdaBody, @NotNull PsiClass containingClass) { return StreamEx.ofTree(lambdaBody, el -> StreamEx.of(el.getChildren())) .findAny(element -> isThisCapturingElement(containingClass, element)); @@ -127,7 +109,7 @@ public class CapturingCleanerInspection extends AbstractBaseJavaLocalInspectionT private static boolean isThisCapturingElement(@NotNull PsiClass containingClass, PsiElement element) { if (element instanceof PsiThisExpression) { - return resolveThis((PsiThisExpression)element) == containingClass; + return PsiUtil.resolveClassInType(((PsiThisExpression)element).getType()) == containingClass; } else if (element instanceof PsiReferenceExpression) { PsiMember member = tryCast(((PsiReferenceExpression)element).resolve(), PsiMember.class); diff --git a/java/java-tests/testData/inspection/cleanerCapturingThis/CapturingCleanerInspection.java b/java/java-tests/testData/inspection/cleanerCapturingThis/CapturingCleaner.java similarity index 66% rename from java/java-tests/testData/inspection/cleanerCapturingThis/CapturingCleanerInspection.java rename to java/java-tests/testData/inspection/cleanerCapturingThis/CapturingCleaner.java index a47ef7d82b79..6c80dff4d249 100644 --- a/java/java-tests/testData/inspection/cleanerCapturingThis/CapturingCleanerInspection.java +++ b/java/java-tests/testData/inspection/cleanerCapturingThis/CapturingCleaner.java @@ -5,7 +5,7 @@ class Anonymous { static void free(int descriptor) {} - Cleaner.Cleanable cleanable = Cleaner.create().register(this, new Runnable() { + Cleaner.Cleanable cleanable = Cleaner.create().register(this, 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, new MyRunnable()); + Cleaner.Cleanable cleanable = Cleaner.create().register(this, new MyRunnable()); private class MyRunnable implements Runnable { @Override @@ -33,7 +33,7 @@ class InstanceMethodReference { static void free(int descriptor) {} - Cleaner.Cleanable cleanable = Cleaner.create().register(this, this::run); + Cleaner.Cleanable cleanable = Cleaner.create().register(this, this::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, () -> run()); + Cleaner.Cleanable cleanable = Cleaner.create().register(this, () -> run()); private void run() { System.out.println("adsad"); @@ -59,7 +59,7 @@ class LambdaInstanceField { Cleaner.Cleanable cleanable = Cleaner.create().register(this, () -> { System.out.println("adsad"); - fileDescriptor = 0; + fileDescriptor = 0; }); } @@ -70,7 +70,7 @@ class LambdaInstanceMethod { Cleaner.Cleanable cleanable = Cleaner.create().register(this, () -> { System.out.println("adsad"); - free(fileDescriptor); + free(fileDescriptor); }); } @@ -81,7 +81,7 @@ class Base { class LambdaInstanceSuperField extends Base { Cleaner.Cleanable cleanable = Cleaner.create().register(this, () -> { System.out.println("adsad"); - fileDescriptor = 0; + fileDescriptor = 0; }); } @@ -91,7 +91,7 @@ class LambdaThis { static void free(int descriptor) {} Cleaner.Cleanable cleanable = Cleaner.create().register(this, () -> { - LambdaThis o = this; + LambdaThis o = this; }); } @@ -139,7 +139,7 @@ class StaticMethodFactory { static ResourceHolder create() { ResourceHolder holder = new ResourceHolder(); - cleaner.register(holder, holder::free); + cleaner.register(holder, holder::free); return holder; } } @@ -154,7 +154,7 @@ class ConstructorDelegatesToStaticMethod { } static void register(ConstructorDelegatesToStaticMethod holder) { - cleaner.register(holder, () -> free(holder.resource)); + cleaner.register(holder, () -> free(holder.resource)); } static void free(int resource){} @@ -168,7 +168,7 @@ class InnerAccesInstanceOuterMembers { Cleaner cleaner = Cleaner.create(); public Inner() { - cleaner.register(this, () -> resource = -1); + cleaner.register(this, () -> resource = -1); } } } \ No newline at end of file diff --git a/java/java-tests/testSrc/com/intellij/java/codeInspection/CapturingCleanerInspectionTest.java b/java/java-tests/testSrc/com/intellij/java/codeInspection/CapturingCleanerInspectionTest.java index c365975343e1..2d27c96233d4 100644 --- a/java/java-tests/testSrc/com/intellij/java/codeInspection/CapturingCleanerInspectionTest.java +++ b/java/java-tests/testSrc/com/intellij/java/codeInspection/CapturingCleanerInspectionTest.java @@ -10,7 +10,7 @@ import org.jetbrains.annotations.NotNull; public class CapturingCleanerInspectionTest extends LightCodeInsightFixtureTestCase { - public void testCapturingCleanerInspection() {doTest();} + public void testCapturingCleaner() {doTest();} @Override protected void setUp() throws Exception { diff --git a/platform/platform-resources-en/src/messages/InspectionsBundle.properties b/platform/platform-resources-en/src/messages/InspectionsBundle.properties index 1d6dab93f834..0b4c7bd87506 100644 --- a/platform/platform-resources-en/src/messages/InspectionsBundle.properties +++ b/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 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 \ No newline at end of file +inspection.capturing.cleaner=Cleaner capture object reference +inspection.capturing.cleaner.description=Cleaner capture object reference \ No newline at end of file