From 56a9ebcc20c8a3487a9bf7328bde39a62468a50b Mon Sep 17 00:00:00 2001 From: Bas Leijdekkers Date: Wed, 5 Sep 2012 12:14:05 +0200 Subject: [PATCH] test and expand functionality of "Synchronization on an object initialized with a literal" inspection --- .../siyeh/InspectionGadgetsBundle.properties | 5 +- ...SynchronizedOnLiteralObjectInspection.java | 103 +++++++++++------- .../SynchronizedOnLiteralObject.html | 4 +- .../SynchronizedOnLiteralObject.java | 14 +++ .../expected.xml | 30 +++++ ...hronizedOnLiteralObjectInspectionTest.java | 10 ++ 6 files changed, 122 insertions(+), 44 deletions(-) create mode 100644 plugins/InspectionGadgets/test/com/siyeh/igtest/threading/synchronized_on_literal_object/SynchronizedOnLiteralObject.java create mode 100644 plugins/InspectionGadgets/test/com/siyeh/igtest/threading/synchronized_on_literal_object/expected.xml create mode 100644 plugins/InspectionGadgets/testsrc/com/siyeh/ig/threading/SynchronizedOnLiteralObjectInspectionTest.java diff --git a/plugins/InspectionGadgets/src/com/siyeh/InspectionGadgetsBundle.properties b/plugins/InspectionGadgets/src/com/siyeh/InspectionGadgetsBundle.properties index dfcef2f582c9..4d8a80d7e687 100644 --- a/plugins/InspectionGadgets/src/com/siyeh/InspectionGadgetsBundle.properties +++ b/plugins/InspectionGadgets/src/com/siyeh/InspectionGadgetsBundle.properties @@ -1026,8 +1026,11 @@ notify.not.in.synchronized.context.problem.descriptor=Call to #ref()#ref() should probably be replaced with 'start()' #loc thread.start.in.construction.problem.descriptor=Call to #ref during object construction #loc synchronize.on.lock.problem.descriptor=Synchronization on a ''{0}'' object is unlikely to be intentional #loc -synchronized.on.literal.object.descriptor=Synchronization on {0} #ref which is initialized by a literal #loc +synchronized.on.literal.object.problem.descriptor=Synchronization on {0} #ref which is initialized by a literal #loc +synchronized.on.direct.literal.object.problem.descriptor=Synchronization on {0} literal #ref #loc +synchronized.on.possibly.literal.object.problem.descriptor=Synchronization on {0} #ref #loc synchronize.on.non.final.field.problem.descriptor=Synchronization on a non-final field #ref #loc +synchronized.on.literal.object.warn.on.all.option=Warn on all possible literals synchronize.on.this.problem.descriptor=Lock operations on 'this' may have unforeseen side-effects #loc nested.synchronized.statement.problem.descriptor=Nested #ref statement #loc empty.synchronized.statement.problem.descriptor=Empty #ref statement #loc diff --git a/plugins/InspectionGadgets/src/com/siyeh/ig/threading/SynchronizedOnLiteralObjectInspection.java b/plugins/InspectionGadgets/src/com/siyeh/ig/threading/SynchronizedOnLiteralObjectInspection.java index a58540159e5a..844f29528810 100644 --- a/plugins/InspectionGadgets/src/com/siyeh/ig/threading/SynchronizedOnLiteralObjectInspection.java +++ b/plugins/InspectionGadgets/src/com/siyeh/ig/threading/SynchronizedOnLiteralObjectInspection.java @@ -1,5 +1,5 @@ /* - * Copyright 2007-2010 Bas Leijdekkers + * Copyright 2007-2012 Bas Leijdekkers * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -15,29 +15,50 @@ */ package com.siyeh.ig.threading; -import com.intellij.openapi.project.Project; +import com.intellij.codeInspection.ui.SingleCheckboxOptionsPanel; import com.intellij.psi.*; import com.siyeh.InspectionGadgetsBundle; import com.siyeh.ig.BaseInspection; import com.siyeh.ig.BaseInspectionVisitor; +import com.siyeh.ig.psiutils.ExpressionUtils; +import com.siyeh.ig.psiutils.TypeUtils; import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; + +import javax.swing.*; public class SynchronizedOnLiteralObjectInspection extends BaseInspection { + @SuppressWarnings("PublicField") public boolean warnOnAllPossiblyLiterals = false; + @Override @NotNull public String getDisplayName() { - return InspectionGadgetsBundle.message( - "synchronized.on.literal.object.name"); + return InspectionGadgetsBundle.message("synchronized.on.literal.object.name"); } @Override @NotNull protected String buildErrorString(Object... infos) { - final PsiType type = (PsiType)infos[0]; - return InspectionGadgetsBundle.message( - "synchronized.on.literal.object.descriptor", - type.getPresentableText()); + final String typeText = ((PsiType)infos[0]).getPresentableText(); + final int message = ((Integer)infos[1]).intValue(); + switch (message) { + case 1: + return InspectionGadgetsBundle.message("synchronized.on.literal.object.problem.descriptor", typeText); + case 2: + return InspectionGadgetsBundle.message("synchronized.on.direct.literal.object.problem.descriptor", typeText); + case 3: + return InspectionGadgetsBundle.message("synchronized.on.possibly.literal.object.problem.descriptor", typeText); + default: + throw new AssertionError(); + } + } + + @Nullable + @Override + public JComponent createOptionsPanel() { + return new SingleCheckboxOptionsPanel(InspectionGadgetsBundle.message("synchronized.on.literal.object.warn.on.all.option"), + this, "warnOnAllPossiblyLiterals"); } @Override @@ -45,55 +66,53 @@ public class SynchronizedOnLiteralObjectInspection extends BaseInspection { return new SynchronizeOnLiteralVisitor(); } - private static class SynchronizeOnLiteralVisitor - extends BaseInspectionVisitor { + private class SynchronizeOnLiteralVisitor extends BaseInspectionVisitor { @Override - public void visitSynchronizedStatement( - @NotNull PsiSynchronizedStatement statement) { + public void visitSynchronizedStatement(@NotNull PsiSynchronizedStatement statement) { super.visitSynchronizedStatement(statement); final PsiExpression lockExpression = statement.getLockExpression(); + if (lockExpression == null) { + return; + } + final PsiType type = lockExpression.getType(); + if (type == null) { + return; + } + if (!type.equalsToText(CommonClassNames.JAVA_LANG_STRING) && + !type.equalsToText(CommonClassNames.JAVA_LANG_BOOLEAN) && + !type.equalsToText(CommonClassNames.JAVA_LANG_CHARACTER)) { + final PsiClassType javaLangNumberType = TypeUtils.getType(CommonClassNames.JAVA_LANG_NUMBER, statement); + if (!javaLangNumberType.isAssignableFrom(type)) { + return; + } + } if (!(lockExpression instanceof PsiReferenceExpression)) { + if (ExpressionUtils.isLiteral(lockExpression)) { + registerError(lockExpression, type, Integer.valueOf(2)); + } + else if (warnOnAllPossiblyLiterals) { + registerError(lockExpression, type, Integer.valueOf(3)); + } return; } - if (!isNumberOrStringType(lockExpression)) { - return; - } - final PsiReferenceExpression referenceExpression = - (PsiReferenceExpression)lockExpression; + final PsiReferenceExpression referenceExpression = (PsiReferenceExpression)lockExpression; final PsiElement target = referenceExpression.resolve(); if (!(target instanceof PsiVariable)) { + if (warnOnAllPossiblyLiterals) { + registerError(lockExpression, type, Integer.valueOf(3)); + } return; } final PsiVariable variable = (PsiVariable)target; final PsiExpression initializer = variable.getInitializer(); - if (!(initializer instanceof PsiLiteralExpression)) { + if (!ExpressionUtils.isLiteral(initializer)) { + if (warnOnAllPossiblyLiterals) { + registerError(lockExpression, type, Integer.valueOf(3)); + } return; } - registerError(lockExpression, lockExpression.getType()); - } - - public static boolean isNumberOrStringType(PsiExpression expression) { - final PsiType type = expression.getType(); - if (type == null) { - return false; - } - final Project project = expression.getProject(); - final JavaPsiFacade psiFacade = JavaPsiFacade.getInstance(project); - final PsiClass javaLangNumberClass = - psiFacade.findClass(CommonClassNames.JAVA_LANG_NUMBER, - expression.getResolveScope()); - if (javaLangNumberClass == null) { - return false; - } - final PsiElementFactory elementFactory = - psiFacade.getElementFactory(); - final PsiClassType javaLangNumberType = - elementFactory.createType(javaLangNumberClass); - return type.equalsToText(CommonClassNames.JAVA_LANG_STRING) || - type.equalsToText(CommonClassNames.JAVA_LANG_BOOLEAN) || - type.equalsToText(CommonClassNames.JAVA_LANG_CHARACTER) || - javaLangNumberType.isAssignableFrom(type); + registerError(lockExpression, type, Integer.valueOf(1)); } } } \ No newline at end of file diff --git a/plugins/InspectionGadgets/src/inspectionDescriptions/SynchronizedOnLiteralObject.html b/plugins/InspectionGadgets/src/inspectionDescriptions/SynchronizedOnLiteralObject.html index 36c0ea7aaf3f..05819e11701e 100644 --- a/plugins/InspectionGadgets/src/inspectionDescriptions/SynchronizedOnLiteralObject.html +++ b/plugins/InspectionGadgets/src/inspectionDescriptions/SynchronizedOnLiteralObject.html @@ -2,13 +2,15 @@ This inspection reports any synchronized block which locks on an object which is initialized with a literal. -String literals are interned and Number literals can be allocated from a cache. +String literals are interned and Character, Boolean and Number literals can be allocated from a cache. Because of this, it is possible that some other part of the system which uses an object initialized with the same literal, is actually holding a reference to the exact same object. This can create unexpected dead-lock situations, if the lock object was thought to be private.

+Use the checkbox below to warn on any synchronization on String, Character, Boolean and Number objects. +

Powered by InspectionGadgets \ No newline at end of file diff --git a/plugins/InspectionGadgets/test/com/siyeh/igtest/threading/synchronized_on_literal_object/SynchronizedOnLiteralObject.java b/plugins/InspectionGadgets/test/com/siyeh/igtest/threading/synchronized_on_literal_object/SynchronizedOnLiteralObject.java new file mode 100644 index 000000000000..2125ad639a66 --- /dev/null +++ b/plugins/InspectionGadgets/test/com/siyeh/igtest/threading/synchronized_on_literal_object/SynchronizedOnLiteralObject.java @@ -0,0 +1,14 @@ +package com.siyeh.igtest.threading.synchronized_on_literal_object; + +class SynchronizedOnLiteralObject { + + private Integer myInteger = (Integer)1; + private Character c = 'a'; + + void foo() { + synchronized (myInteger) {} + synchronized ("asdf") {} + synchronized ((Boolean) true) {} + synchronized (c) {} + } +} \ No newline at end of file diff --git a/plugins/InspectionGadgets/test/com/siyeh/igtest/threading/synchronized_on_literal_object/expected.xml b/plugins/InspectionGadgets/test/com/siyeh/igtest/threading/synchronized_on_literal_object/expected.xml new file mode 100644 index 000000000000..c4cfac836961 --- /dev/null +++ b/plugins/InspectionGadgets/test/com/siyeh/igtest/threading/synchronized_on_literal_object/expected.xml @@ -0,0 +1,30 @@ + + + + SynchronizedOnLiteralObject.java + 9 + Synchronization on an object initialized with a literal + Synchronization on Integer <code>myInteger</code> which is initialized by a literal #loc + + + + SynchronizedOnLiteralObject.java + 10 + Synchronization on an object initialized with a literal + Synchronization on String <code>"asdf"</code> which is initialized by a literal #loc + + + + SynchronizedOnLiteralObject.java + 11 + Synchronization on an object initialized with a literal + Synchronization on Boolean <code>(Boolean) true</code> which is initialized by a literal #loc + + + + SynchronizedOnLiteralObject.java + 12 + Synchronization on an object initialized with a literal + Synchronization on Character <code>c</code> which is initialized by a literal #loc + + \ No newline at end of file diff --git a/plugins/InspectionGadgets/testsrc/com/siyeh/ig/threading/SynchronizedOnLiteralObjectInspectionTest.java b/plugins/InspectionGadgets/testsrc/com/siyeh/ig/threading/SynchronizedOnLiteralObjectInspectionTest.java new file mode 100644 index 000000000000..2013fcdc8449 --- /dev/null +++ b/plugins/InspectionGadgets/testsrc/com/siyeh/ig/threading/SynchronizedOnLiteralObjectInspectionTest.java @@ -0,0 +1,10 @@ +package com.siyeh.ig.threading; + +import com.siyeh.ig.IGInspectionTestCase; + +public class SynchronizedOnLiteralObjectInspectionTest extends IGInspectionTestCase { + + public void test() throws Exception { + doTest("com/siyeh/igtest/threading/synchronized_on_literal_object", new SynchronizedOnLiteralObjectInspection()); + } +} \ No newline at end of file