adding metadata and messages for public visibility and public constru…

…ctor visibility detectors
heuermh · Feb 8, 2012 · e43d33d · e43d33d
1 parent bb497cb
commit e43d33d
Show file tree

Hide file tree

Showing 4 changed files with 78 additions and 6 deletions.
diff --git a/...java/uk/me/tom_fitzhenry/findbugs/guice/PublicImplementationClassConstructorDetector.java b/...java/uk/me/tom_fitzhenry/findbugs/guice/PublicImplementationClassConstructorDetector.java
@@ -37,11 +37,12 @@ public void sawOpcode(final int seen) {
                 if (isCallingTo()) {
                     OpcodeStack.Item stackItem = stack.getStackItem(0);
                     try {
-                        String implementationClassName = (String) stackItem.getConstant();
+                        String slashedClassName = (String) stackItem.getConstant();
-                        Class<?> implementationClass = Class.forName(implementationClassName.replace("/", "."));
+                        String dottedClassName = slashedClassName.replace("/", ".");
+                        Class<?> implementationClass = Class.forName(dottedClassName);
                         for (Constructor<?> constructor : implementationClass.getDeclaredConstructors()) {
                             if (Modifier.isPublic(constructor.getModifiers())) {
-                                bugReporter.reportBug(new BugInstance(this, "GUICE_PUBLIC_IMPLEMENTATION_CLASS_CONSTRUCTOR", NORMAL_PRIORITY).addClassAndMethod(this));
+                                bugReporter.reportBug(new BugInstance(this, "GUICE_PUBLIC_IMPLEMENTATION_CLASS_CONSTRUCTOR", NORMAL_PRIORITY).addClassAndMethod(this).addTypeOfNamedClass(dottedClassName));
                             }
                         }
                     }

diff --git a/src/main/java/uk/me/tom_fitzhenry/findbugs/guice/PublicImplementationClassDetector.java b/src/main/java/uk/me/tom_fitzhenry/findbugs/guice/PublicImplementationClassDetector.java
@@ -36,10 +36,11 @@ public void sawOpcode(final int seen) {
                 if (isCallingTo()) {
                     OpcodeStack.Item stackItem = stack.getStackItem(0);
                     try {
-                        String implementationClassName = (String) stackItem.getConstant();
+                        String slashedClassName = (String) stackItem.getConstant();
-                        Class<?> implementationClass = Class.forName(implementationClassName.replace("/", "."));
+                        String dottedClassName = slashedClassName.replace("/", ".");
+                        Class<?> implementationClass = Class.forName(dottedClassName);
                         if (Modifier.isPublic(implementationClass.getModifiers())) {
-                            bugReporter.reportBug(new BugInstance(this, "GUICE_PUBLIC_IMPLEMENTATION_CLASS", NORMAL_PRIORITY).addClassAndMethod(this));
+                            bugReporter.reportBug(new BugInstance(this, "GUICE_PUBLIC_IMPLEMENTATION_CLASS", NORMAL_PRIORITY).addClassAndMethod(this).addTypeOfNamedClass(dottedClassName));
                         }
                     }
                     catch (Exception e) {

diff --git a/src/main/resources/findbugs.xml b/src/main/resources/findbugs.xml
@@ -24,6 +24,16 @@
                   speed="fast"
                   reports="GUICE_FINAL_FIELD_INJECTION"
         />
+
+        <Detector class="uk.me.tom_fitzhenry.findbugs.guice.PublicImplementationClassDetector"
+                  speed="fast"
+                  reports="GUICE_PUBLIC_IMPLEMENTATION_CLASS"
+        />
+
+        <Detector class="uk.me.tom_fitzhenry.findbugs.guice.PublicImplementationClassConstructorDetector"
+                  speed="fast"
+                  reports="GUICE_PUBLIC_IMPLEMENTATION_CLASS_CONSTRUCTOR"
+        />
 
         <BugPattern type="GUICE_SCOPE_ON_INTERFACE"
                     category="CORRECTNESS"
@@ -44,5 +54,15 @@
                     category="CORRECTNESS"
                     abbrev="GUICE"
         />
+
+        <BugPattern type="GUICE_PUBLIC_IMPLEMENTATION_CLASS"
+                    category="CORRECTNESS"
+                    abbrev="GUICE"
+        />
+
+        <BugPattern type="GUICE_PUBLIC_IMPLEMENTATION_CLASS_CONSTRUCTOR"
+                    category="CORRECTNESS"
+                    abbrev="GUICE"
+        />
 
 </FindbugsPlugin>
diff --git a/src/main/resources/messages.xml b/src/main/resources/messages.xml
@@ -31,6 +31,18 @@
 	    </Details>
 	</Detector>
 
+	<Detector class="uk.me.tom_fitzhenry.findbugs.guice.PublicImplementationClassDetector">
+	    <Details>
+	    	Finds bindings to implementation classes with public visibility.
+	    </Details>
+	</Detector>
+
+	<Detector class="uk.me.tom_fitzhenry.findbugs.guice.PublicImplementationClassConstructorDetector">
+	    <Details>
+	    	Finds bindings to implementation classes with public constructor visibility.
+	    </Details>
+	</Detector>
+
 	<BugPattern type="GUICE_SCOPE_ON_INTERFACE">
 		<ShortDescription>Scope annotation on interfaces</ShortDescription>
 		<LongDescription>Interface {0} is annotated with a scope, {1}</LongDescription>
@@ -135,6 +147,44 @@
 
 		</Details>
 	</BugPattern>
+
+	<BugPattern type="GUICE_PUBLIC_IMPLEMENTATION_CLASS">
+		<ShortDescription>Binding to implementation class with public visibility</ShortDescription>
+		<LongDescription>Module {0} binds to an implementation class {3} with public visibility.</LongDescription>
+		<Details>
+			<![CDATA[
+			Guice recommends keeping constructors on Guice-instantiated classes as hidden as possible.
+
+			<p><a href="http://code.google.com/p/google-guice/wiki/KeepConstructorsHidden">Guice best practices : KeepConstructorsHidden</a></p>
+			
+			<p>Recommended correction:<br/>
+			Limit the visibility of both your implementation classes and their constructors. Typically package private is preferred for both, as this facilitates:
+			<ul>
+			    <li>binding the class within a Module in the same package</li>
+			    <li>unit testing the class through means of direct instantiation</li>
+			</ul></p>
+			]]>
+		</Details>
+	</BugPattern>
+
+	<BugPattern type="GUICE_PUBLIC_IMPLEMENTATION_CLASS_CONSTRUCTOR">
+		<ShortDescription>Binding to implementation class with public constructor visibility</ShortDescription>
+		<LongDescription>Module {0} binds to an implementation class {3} that has one or more constructor(s) with public visibility.</LongDescription>
+		<Details>
+			<![CDATA[
+			Guice recommends keeping constructors on Guice-instantiated classes as hidden as possible.
+
+			<p><a href="http://code.google.com/p/google-guice/wiki/KeepConstructorsHidden">Guice best practices : KeepConstructorsHidden</a></p>
+			
+			<p>Recommended correction:<br/>
+			Limit the visibility of both your implementation classes and their constructors. Typically package private is preferred for both, as this facilitates:
+			<ul>
+			    <li>binding the class within a Module in the same package</li>
+			    <li>unit testing the class through means of direct instantiation</li>
+			</ul></p>
+			]]>
+		</Details>
+	</BugPattern>
 
 	<BugCode abbrev="GUICE">Guice bugcode abbreviation</BugCode>