[Fb-contrib-commit] SF.net SVN: fb-contrib: [949] trunk/fb-contrib

[<dbr...@us...>]

Revision: 949
          http://fb-contrib.svn.sourceforge.net/fb-contrib/?rev=949&view=rev
Author:   dbrosius
Date:     2007-11-01 22:00:22 -0700 (Thu, 01 Nov 2007)

Log Message:
-----------
initial checkin CFS detector, barely working

Modified Paths:
--------------
    trunk/fb-contrib/etc/findbugs.xml
    trunk/fb-contrib/etc/messages.xml

Added Paths:
-----------
    trunk/fb-contrib/samples/CFS_Sample.java
    trunk/fb-contrib/src/com/mebigfatguy/fbcontrib/detect/ConfusingFunctionSemantics.java

Modified: trunk/fb-contrib/etc/findbugs.xml
===================================================================
--- trunk/fb-contrib/etc/findbugs.xml	2007-11-02 03:30:37 UTC (rev 948)
+++ trunk/fb-contrib/etc/findbugs.xml	2007-11-02 05:00:22 UTC (rev 949)
@@ -300,6 +300,10 @@
 			speed="moderate"
 			reports="EXS_EXCEPTION_SOFTENING_NO_CONSTRAINTS,EXS_EXCEPTION_SOFTENING_HAS_CHECKED,EXS_EXCEPTION_SOFTENING_NO_CHECKED" />
 			
+	<Detector class="com.mebigfatguy.fbcontrib.detect.ConfusingFunctionSemantics"
+			speed="fast"
+			reports="CFS_CONFUSING_FUNCTION_SEMANTICS" />
+			
    	<!-- BugPattern -->
 	
 	<BugPattern abbrev="ISB" type="ISB_INEFFICIENT_STRING_BUFFERING" category="PERFORMANCE" />
@@ -396,4 +400,5 @@
 	<BugPattern abbrev="EXS" type="EXS_EXCEPTION_SOFTENING_NO_CONSTRAINTS" category="STYLE" experimental="true" />
 	<BugPattern abbrev="EXS" type="EXS_EXCEPTION_SOFTENING_HAS_CHECKED" category="STYLE" experimental="true" />
 	<BugPattern abbrev="EXS" type="EXS_EXCEPTION_SOFTENING_NO_CHECKED" category="STYLE" experimental="true" />
+	<BugPattern abbrev="CFS" type="CFS_CONFUSING_FUNCTION_SEMANTICS" category="STYLE" experimental="true" />
 </FindbugsPlugin>
\ No newline at end of file

Modified: trunk/fb-contrib/etc/messages.xml
===================================================================
--- trunk/fb-contrib/etc/messages.xml	2007-11-02 03:30:37 UTC (rev 948)
+++ trunk/fb-contrib/etc/messages.xml	2007-11-02 05:00:22 UTC (rev 949)
@@ -584,7 +584,7 @@
 		<Details>
 			<![CDATA[
 			<p>looks for if/else blocks where a series of them use instanceof on the same 
-			variable to determine what do to. If these classes are related by inheritance,
+			variable to determine what to do. If these classes are related by inheritance,
 			this often is better handled through calling a single overridden method.</p>
 			<p>It is a moderately fast detector</p>
 			]]>
@@ -816,6 +816,21 @@
 		</Details>
 	</Detector>
 	
+	<Detector class="com.mebigfatguy.fbcontrib.detect.ConfusingFunctionSemantics">
+		<Details>
+			<![CDATA[
+			<p>looks for methods that return a parameter after modifying that parameter.
+			Doing this will confuse the user of this method, as it will be assumed that the 
+			passed in argument is different than the output, or at least won't be changed.
+			If the purpose of this method is just to modify the parameter, this method should
+			probably be changed to have a void return type. If you must return a variable, perhaps
+			a clone of the parameter should be returned.
+			</p>
+			<p>It is a fast detector</p>
+			]]>
+		</Details>
+	</Detector>
+	
 	<!-- BugPattern -->	
 	                
 	<BugPattern type="ISB_INEFFICIENT_STRING_BUFFERING">
@@ -2055,6 +2070,21 @@
 		</Details>
 	</BugPattern>
 	
+	<BugPattern type="CFS_CONFUSING_FUNCTION_SEMANTICS">
+		<ShortDescription>method returns modified parameter</ShortDescription>
+		<LongDescription>method {1} returns modified parameter</LongDescription>
+		<Details>
+			<![CDATA[
+			<p>This method appears to modify a parameter, and then return this parameter as the 
+			methods return value. This will be confusing to callers of this method, as it won't be
+			apparent that the 'original' passed in parameter will be changed as well. If the purpose
+			of this method is to change the parameter, it would be more clear to change the method to 
+			a have a void return value. If a return type is required due to interface or superclass contract,
+			perhaps a clone of the parameter should be made.</p>
+			]]>
+		</Details>
+	</BugPattern>
+	
 	<!-- BugCode -->
 	
 	<BugCode abbrev="ISB">Inefficient String Buffering</BugCode>
@@ -2124,4 +2154,5 @@
 	<BugCode abbrev="NCS">Needless Custom Serialization</BugCode>
 	<BugCode abbrev="MOM">Misleading Overload Model</BugCode>
 	<BugCode abbrev="EXS">Exception Softening</BugCode>
+	<BugCode abbrev="CFS">Confusing Function Semantics</BugCode>
 </MessageCollection>
\ No newline at end of file

Added: trunk/fb-contrib/samples/CFS_Sample.java
===================================================================
--- trunk/fb-contrib/samples/CFS_Sample.java	                        (rev 0)
+++ trunk/fb-contrib/samples/CFS_Sample.java	2007-11-02 05:00:22 UTC (rev 949)
@@ -0,0 +1,10 @@
+import java.util.Date;
+
+public class CFS_Sample 
+{
+	public Date getNextDate(Date d)
+	{
+		d.setHours(0);
+		return d;
+	}
+}


Property changes on: trunk/fb-contrib/samples/CFS_Sample.java
___________________________________________________________________
Name: svn:mime-type
   + text/plain
Name: svn:eol-style
   + native

Added: trunk/fb-contrib/src/com/mebigfatguy/fbcontrib/detect/ConfusingFunctionSemantics.java
===================================================================
--- trunk/fb-contrib/src/com/mebigfatguy/fbcontrib/detect/ConfusingFunctionSemantics.java	                        (rev 0)
+++ trunk/fb-contrib/src/com/mebigfatguy/fbcontrib/detect/ConfusingFunctionSemantics.java	2007-11-02 05:00:22 UTC (rev 949)
@@ -0,0 +1,199 @@
+/*
+ * fb-contrib - Auxilliary detectors for Java programs
+ * Copyright (C) 2005-2007 Dave Brosius
+ * 
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ * 
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ * 
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+package com.mebigfatguy.fbcontrib.detect;
+
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+import org.apache.bcel.Repository;
+import org.apache.bcel.classfile.Code;
+import org.apache.bcel.classfile.JavaClass;
+import org.apache.bcel.classfile.Method;
+import org.apache.bcel.generic.Type;
+
+import com.mebigfatguy.fbcontrib.utils.Integer14;
+import com.mebigfatguy.fbcontrib.utils.RegisterUtils;
+
+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;
+
+/** looks for methods that return a parameter after making what looks like
+ * modifications to that parameter. This leads to confusing on the user of this
+ * method as it isn't obvious that the 'original' object is modified. If the
+ * point of this method is to modify the parameter, it is probably better just
+ * to have the method be a void method, to avoid confusion.
+ */
+public class ConfusingFunctionSemantics extends BytecodeScanningDetector 
+{
+	private static final Set<String> knownImmutables = new HashSet<String>();
+	static {
+		knownImmutables.add("Ljava/lang/String;");
+		knownImmutables.add("Ljava/lang/Byte;");
+		knownImmutables.add("Ljava/lang/Character;");
+		knownImmutables.add("Ljava/lang/Short;");
+		knownImmutables.add("Ljava/lang/Integer;");
+		knownImmutables.add("Ljava/lang/Long;");
+		knownImmutables.add("Ljava/lang/Float;");
+		knownImmutables.add("Ljava/lang/Double;");
+		knownImmutables.add("Ljava/lang/Boolean;");
+		knownImmutables.add("Ljava/lang/Class;");
+	};
+	
+	private BugReporter bugReporter;
+	private Map<Integer, ParmUsage> possibleParmRegs;
+	private OpcodeStack stack;
+	
+	/**
+     * constructs a CFS detector given the reporter to report bugs on
+     * @param bugReporter the sync of bug reports
+	 */
+	public ConfusingFunctionSemantics(BugReporter bugReporter) {
+		this.bugReporter = bugReporter;
+	}
+	
+	/** implements the visitor to initialize/destroy the possible parameter registers and opcode stack
+	 * 
+	 * @param classContext the context object of the currently parsed class
+	 */
+	@Override
+	public void visitClassContext(ClassContext classContext) {
+		try {
+			stack = new OpcodeStack();
+			possibleParmRegs = new HashMap<Integer, ParmUsage>();
+			super.visitClassContext(classContext);
+		} finally {
+			stack = null;
+			possibleParmRegs = null;
+		}
+	}
+	
+	/** implements the visitor to look for any non-immutable typed parameters are assignable
+	 * to the return type. If found, the method is parsed.
+	 * 
+	 * @param obj the context object of the currently parsed code block
+	 */
+	@Override
+	public void visitCode(Code obj) {
+		try {
+			Method m = getMethod();
+			String methodSignature = m.getSignature();
+			String retSignature = Type.getReturnType(methodSignature).getSignature();
+			JavaClass returnClass = null;
+			int[] parmRegs = null;
+			
+			if ((retSignature.charAt(0) == 'L') && !knownImmutables.contains(retSignature)) {
+				Type[] parmTypes = Type.getArgumentTypes(methodSignature);
+				for (int p = 0; p < parmTypes.length; p++) {
+					String parmSignature = parmTypes[p].getSignature();
+					if ((parmSignature.charAt(0) == 'L') && !knownImmutables.contains(parmSignature)) {
+						if (returnClass == null) {
+							returnClass = Repository.lookupClass(retSignature.substring(1, retSignature.length() - 1));
+							parmRegs = RegisterUtils.getParameterRegisters(m);
+						}
+						
+						JavaClass parmClass = Repository.lookupClass(parmSignature.substring(1, parmSignature.length() - 1));
+						if (parmClass.instanceOf(returnClass)) {
+							possibleParmRegs.put(Integer14.valueOf(parmRegs[p]), new ParmUsage());
+						}
+					}
+				}
+				
+				if (possibleParmRegs.size() > 0) {
+					stack.resetForMethodEntry(this);
+					super.visitCode(obj);
+					for (Map.Entry<Integer, ParmUsage> entry : possibleParmRegs.entrySet()) {
+						Integer reg = entry.getKey();
+						ParmUsage pu = entry.getValue();
+						
+						if (pu.returned && (pu.alteredPC >= 0)) {
+							bugReporter.reportBug(new BugInstance(this, "CFS_CONFUSING_FUNCTION_SEMANTICS", NORMAL_PRIORITY)
+										.addClass(this)
+										.addMethod(this)
+										.addSourceLine(this, pu.alteredPC));
+						}
+					}
+				}
+			}
+		} catch (ClassNotFoundException cnfe) {
+			bugReporter.reportMissingClass(cnfe);
+		}
+	}
+	
+	@Override
+	public void sawOpcode(int seen) {
+		if (possibleParmRegs.size() == 0)
+			return;
+		
+		try {
+			if (seen == ARETURN) {
+				if (stack.getStackDepth() > 0) {
+					OpcodeStack.Item item = stack.getStackItem(0);
+					int reg = item.getRegisterNumber();
+					ParmUsage pu = possibleParmRegs.get(Integer14.valueOf(reg));
+					if (pu != null)
+						pu.setReturned();
+				}
+			} else if (seen == PUTFIELD) {
+				if (stack.getStackDepth() > 1) {
+					OpcodeStack.Item item = stack.getStackItem(1);
+					int reg = item.getRegisterNumber();
+					ParmUsage pu = possibleParmRegs.get(Integer14.valueOf(reg));
+					if (pu != null)
+						pu.setAlteredPC(getPC());
+				}
+			} else if ((seen == INVOKEVIRTUAL) || (seen == INVOKEINTERFACE)) {
+				String calledSig = getSigConstantOperand();
+				String calledRet = Type.getReturnType(calledSig).getSignature();
+				if ("V".equals(calledRet)) {
+					int calledObjOffset = Type.getArgumentTypes(calledSig).length;
+					if (stack.getStackDepth() >= calledObjOffset) {
+						OpcodeStack.Item item = stack.getStackItem(calledObjOffset);
+						int reg = item.getRegisterNumber();
+						ParmUsage pu = possibleParmRegs.get(Integer14.valueOf(reg));
+						if (pu != null)
+							pu.setAlteredPC(getPC());
+					}
+				}
+			}
+			
+		} finally {
+			stack.sawOpcode(this, seen);
+		}
+	}
+	
+	static class ParmUsage
+	{
+		boolean returned = false;
+		int alteredPC = -1;
+		
+		public void setReturned() {
+			returned = true;
+		}
+		
+		public void setAlteredPC(int pc) {
+			if (alteredPC < 0)
+				alteredPC = pc;
+		}
+	}
+}


Property changes on: trunk/fb-contrib/src/com/mebigfatguy/fbcontrib/detect/ConfusingFunctionSemantics.java
___________________________________________________________________
Name: svn:mime-type
   + text/plain
Name: svn:eol-style
   + native


This was sent by the SourceForge.net collaborative development platform, the world's largest Open Source development site.