add IMC_IMMATURE_CLASS_COLLECTION_RETURN detector

mebigfatguy · Dec 17, 2024 · 02e31c4 · 02e31c4
1 parent d2c4245
commit 02e31c4
Show file tree

Hide file tree

Showing 6 changed files with 72 additions and 9 deletions.
diff --git a/etc/bugrank.txt b/etc/bugrank.txt
@@ -88,6 +88,7 @@
 +0 BugPattern IMC_IMMATURE_CLASS_UPPER_PACKAGE
 +0 BugPattern IMC_IMMATURE_CLASS_VAR_NAME
 +0 BugPattern IMC_IMMATURE_CLASS_WRONG_FIELD_ORDER
++0 BugPattern IMC_IMMATURE_CLASS_COLLECTION_RETURN
 +0 BugPattern IOI_COPY_WITH_READER
 +0 BugPattern IOI_DOUBLE_BUFFER_COPY
 +0 BugPattern IOI_UNENDED_ZLIB_OBJECT

diff --git a/etc/findbugs.xml b/etc/findbugs.xml
@@ -286,7 +286,7 @@
 
 	<Detector class="com.mebigfatguy.fbcontrib.detect.StringifiedTypes" speed="fast" reports="STT_STRING_PARSING_A_FIELD,STT_TOSTRING_STORED_IN_FIELD,STT_TOSTRING_MAP_KEYING" />
 
-	<Detector class="com.mebigfatguy.fbcontrib.detect.ImmatureClass" speed="moderate" reports="IMC_IMMATURE_CLASS_NO_EQUALS,IMC_IMMATURE_CLASS_NO_HASHCODE,IMC_IMMATURE_CLASS_NO_PACKAGE,IMC_IMMATURE_CLASS_NO_TOSTRING,IMC_IMMATURE_CLASS_IDE_GENERATED_PARAMETER_NAMES,IMC_IMMATURE_CLASS_PRINTSTACKTRACE,IMC_IMMATURE_CLASS_WRONG_FIELD_ORDER,IMC_IMMATURE_CLASS_UPPER_PACKAGE,IMC_IMMATURE_CLASS_LOWER_CLASS,IMC_IMMATURE_CLASS_BAD_SERIALVERSIONUID,IMC_IMMATURE_CLASS_VAR_NAME" />	
+	<Detector class="com.mebigfatguy.fbcontrib.detect.ImmatureClass" speed="moderate" reports="IMC_IMMATURE_CLASS_NO_EQUALS,IMC_IMMATURE_CLASS_NO_HASHCODE,IMC_IMMATURE_CLASS_NO_PACKAGE,IMC_IMMATURE_CLASS_NO_TOSTRING,IMC_IMMATURE_CLASS_IDE_GENERATED_PARAMETER_NAMES,IMC_IMMATURE_CLASS_PRINTSTACKTRACE,IMC_IMMATURE_CLASS_WRONG_FIELD_ORDER,IMC_IMMATURE_CLASS_UPPER_PACKAGE,IMC_IMMATURE_CLASS_LOWER_CLASS,IMC_IMMATURE_CLASS_BAD_SERIALVERSIONUID,IMC_IMMATURE_CLASS_VAR_NAME,IMC_IMMATURE_CLASS_COLLECTION_RETURN" />	
 
 	<Detector class="com.mebigfatguy.fbcontrib.detect.JAXRSIssues" speed="fast" reports="JXI_GET_ENDPOINT_CONSUMES_CONTENT,JXI_INVALID_CONTEXT_PARAMETER_TYPE,JXI_PARM_PARAM_NOT_FOUND_IN_PATH,JXI_UNDEFINED_PARAMETER_SOURCE_IN_ENDPOINT" />
 
@@ -601,6 +601,7 @@
 	<BugPattern abbrev="IMC" type="IMC_IMMATURE_CLASS_LOWER_CLASS" category="STYLE" />
 	<BugPattern abbrev="IMC" type="IMC_IMMATURE_CLASS_BAD_SERIALVERSIONUID" category="CORRECTNESS" />
 	<BugPattern abbrev="IMC" type="IMC_IMMATURE_CLASS_VAR_NAME" category="CORRECTNESS" />
+	<BugPattern abbrev="IMC" type="IMC_IMMATURE_CLASS_COLLECTION_RETURN" category="CORRECTNESS" />
 	<BugPattern abbrev="JXI" type="JXI_GET_ENDPOINT_CONSUMES_CONTENT" category="CORRECTNESS" />
 	<BugPattern abbrev="JXI" type="JXI_INVALID_CONTEXT_PARAMETER_TYPE" category="CORRECTNESS" />
 	<BugPattern abbrev="JXI" type="JXI_PARM_PARAM_NOT_FOUND_IN_PATH" category="CORRECTNESS" />

diff --git a/etc/messages.xml b/etc/messages.xml
@@ -5737,7 +5737,20 @@ for (Person p : people) {
     		<p>A field or variable is named 'var' which will conflict with the built in Java 10 feature using 'var' as a keyword.</p>
     		]]>
     	</Details>
-    </BugPattern>       
+    </BugPattern> 
+
+    <BugPattern type="IMC_IMMATURE_CLASS_COLLECTION_RETURN">
+        <ShortDescription>Method returns java.util.Collection</ShortDescription>
+        <LongDescription>Method {1} returns java.util.Collection</LongDescription>
+        <Details>
+            <![CDATA[
+            <p>This method declares that it returns a Collection, when in fact it has created and returned
+            a more specific interface. Since the caller is free to cast any return value to Collection, it is
+            better to return the specific interface created so that it gives the caller the most
+            power. With methods, always expect the least with parameters, and provide the most with return types.</p>
+            ]]>
+        </Details>
+    </BugPattern>      
 
     <BugPattern type="JXI_GET_ENDPOINT_CONSUMES_CONTENT">
     	<ShortDescription>JAX-RS Method implements a GET request but consumes input</ShortDescription>

diff --git a/src/main/java/com/mebigfatguy/fbcontrib/detect/ImmatureClass.java b/src/main/java/com/mebigfatguy/fbcontrib/detect/ImmatureClass.java
@@ -8,6 +8,7 @@
 import org.apache.bcel.Const;
 import org.apache.bcel.Repository;
 import org.apache.bcel.classfile.AnnotationEntry;
+import org.apache.bcel.classfile.Code;
 import org.apache.bcel.classfile.Constant;
 import org.apache.bcel.classfile.ConstantLong;
 import org.apache.bcel.classfile.ConstantValue;
@@ -27,7 +28,9 @@
 import edu.umd.cs.findbugs.BugInstance;
 import edu.umd.cs.findbugs.BugReporter;
 import edu.umd.cs.findbugs.BytecodeScanningDetector;
+import edu.umd.cs.findbugs.OpcodeStack;
 import edu.umd.cs.findbugs.ba.ClassContext;
+import edu.umd.cs.findbugs.ba.SignatureParser;
 
 /**
  * looks for classes that aren't fully flushed out to be easily usable for
@@ -62,8 +65,10 @@ enum FieldStatus {
     }
 
     private BugReporter bugReporter;
+    private OpcodeStack stack;
     private FieldStatus fieldStatus = FieldStatus.NONE;
     private boolean classIsJPAEntity;
+    private String actualReturnType;
 
     public ImmatureClass(BugReporter reporter) {
         bugReporter = reporter;
@@ -177,7 +182,12 @@ public void visitClassContext(ClassContext classContext) {
             }
         }
 
-        super.visitClassContext(classContext);
+        try {
+        	stack = new OpcodeStack();
+        	super.visitClassContext(classContext);
+        } finally {
+        	stack = null;
+        }
     }
 
     @Override
@@ -253,6 +263,22 @@ public void visitMethod(Method m) {
             }
         }
     }
+
+    public void visitCode(Code obj) {
+
+		stack.resetForMethodEntry(this);
+
+    	String declaredReturnType = new SignatureParser(getMethodSig()).getReturnTypeSignature();
+    	actualReturnType = null;
+
+    	super.visitCode(obj);
+
+        if ("Ljava/util/Collection;".equals(declaredReturnType) && !"Ljava/util/Collection;".equals(actualReturnType)) {
+            bugReporter.reportBug(
+                    new BugInstance(this, BugType.IMC_IMMATURE_CLASS_COLLECTION_RETURN.name(), NORMAL_PRIORITY)
+                            .addClass(this).addMethod(this).addSourceLine(this, 0));
+        }
+    }
 
     /**
      * implements the visitor to check for calls to Throwable.printStackTrace()
@@ -261,12 +287,25 @@ public void visitMethod(Method m) {
      */
     @Override
     public void sawOpcode(int seen) {
-        if ((seen == Const.INVOKEVIRTUAL) && "printStackTrace".equals(getNameConstantOperand())
-                && SignatureBuilder.SIG_VOID_TO_VOID.equals(getSigConstantOperand())) {
-            bugReporter
-                    .reportBug(new BugInstance(this, BugType.IMC_IMMATURE_CLASS_PRINTSTACKTRACE.name(), NORMAL_PRIORITY)
-                            .addClass(this).addMethod(this).addSourceLine(this));
-        }
+    	try {
+	        if ((seen == Const.INVOKEVIRTUAL) && "printStackTrace".equals(getNameConstantOperand())
+	                && SignatureBuilder.SIG_VOID_TO_VOID.equals(getSigConstantOperand())) {
+	            bugReporter
+	                    .reportBug(new BugInstance(this, BugType.IMC_IMMATURE_CLASS_PRINTSTACKTRACE.name(), NORMAL_PRIORITY)
+	                            .addClass(this).addMethod(this).addSourceLine(this));
+	        } else if (seen == Const.ARETURN) {
+	        	if (stack.getStackDepth() > 0) {
+	        		OpcodeStack.Item itm = stack.getStackItem(0);
+	        		if (actualReturnType == null) {
+	        			actualReturnType = itm.getSignature();
+	        		} else if (!actualReturnType.equals(itm.getSignature())) {
+	        			actualReturnType = "";
+	        		}
+	        	}
+	        }
+    	} finally {
+    		stack.sawOpcode(this, seen);
+    	}
     }
 
     /**

diff --git a/src/main/java/com/mebigfatguy/fbcontrib/utils/BugType.java b/src/main/java/com/mebigfatguy/fbcontrib/utils/BugType.java
@@ -119,6 +119,7 @@ public enum BugType {
     IMC_IMMATURE_CLASS_LOWER_CLASS,
     IMC_IMMATURE_CLASS_BAD_SERIALVERSIONUID,
     IMC_IMMATURE_CLASS_VAR_NAME,
+    IMC_IMMATURE_CLASS_COLLECTION_RETURN,
     IOI_COPY_WITH_READER,
     IOI_DOUBLE_BUFFER_COPY,
     IOI_UNENDED_ZLIB_OBJECT,

diff --git a/src/samples/java/ex/IMC_Sample.java b/src/samples/java/ex/IMC_Sample.java
@@ -9,6 +9,10 @@
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
 import java.lang.annotation.Target;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Collection;
+import java.util.Collections;
 
 import javax.persistence.Entity;
 
@@ -47,6 +51,10 @@ public void psf(File f) {
     public void hasVar() {
         int var = 0;
     }
+
+    public Collection returnsCollection() {
+    	return Collections.emptySet();
+    }
 }
 
 class NeedsEquals {