Draft: Inefficient Appendable Append fix for spotbugs/spotbugs#704

etc/findbugs.xml

@@ -331,6 +331,7 @@
     <Detector class="com.mebigfatguy.fbcontrib.detect.SetUsageIssues" speed="fast" reports="SUI_CONTAINS_BEFORE_ADD,SUI_CONTAINS_BEFORE_REMOVE"/>
 
     <Detector class="com.mebigfatguy.fbcontrib.detect.ClassEnvy" speed="slow" reports="CE_CLASS_ENVY" />
+	<Detector class="com.mebigfatguy.fbcontrib.detect.InefficientAppendableAppend" speed="fast" reports="IAA_INEFFICIENT_APPENDABLE_APPEND" />
 
     <!-- COMMENT OUT FOR POINT RELEASE -->
 
@@ -341,7 +342,7 @@
     <!-- COMMENT OUT FOR POINT RELEASE -->
 
 	<!-- BugPattern -->
-
+	<BugPattern abbrev="IAA" type="IAA_INEFFICIENT_APPENDABLE_APPEND" category="PERFORMANCE" />
 	<BugPattern abbrev="ISB" type="ISB_INEFFICIENT_STRING_BUFFERING" category="PERFORMANCE" />
 	<BugPattern abbrev="ISB" type="ISB_EMPTY_STRING_APPENDING" category="PERFORMANCE" />
 	<BugPattern abbrev="ISB" type="ISB_TOSTRING_APPENDING" category="CORRECTNESS" />

etc/messages.xml

@@ -56,6 +56,28 @@
 			]]>
 		</Details>
 	</Detector>
+
+		<Detector class="com.mebigfatguy.fbcontrib.detect.InefficientAppendableAppend">
+		<Details>
+			<![CDATA[
+			<p>Looks for appending CharSequence.toString inside of calls to Appendable append.</p>
+			<pre>
+				StringBuilder sb = new StringBuilder();
+				sb.append(a + b);
+				return sb.toString();
+			</pre>
+			You should use the .append method to append values
+			<pre>
+				sb.append(a).append(b);
+			</pre>
+			<p>It is a fast detector.</p>
+			]]>
+		</Details>
+	</Detector>
+
+
+
+
 
 	<Detector class="com.mebigfatguy.fbcontrib.detect.SyncCollectionIterators">
 		<Details>
@@ -6513,6 +6535,22 @@ if (shouldCalcHalting && (calculateHaltingProbability() &gt; 0) { }
             ]]>
         </Details> 
     </BugPattern>
+
+
+	<BugPattern type="IAA_INEFFICIENT_APPENDABLE_APPEND">
+		<ShortDescription>Method concatenates the result of a toString() call</ShortDescription>
+		<LongDescription>Method {1} concatenates the result of a toString() call</LongDescription>
+		<Details>
+			<![CDATA[
+			<p>This method concatenates the output of a <code>toString()</code> call on a <code>CharSequence</code> into a <code>Appendable</code>.
+			It is simpler just to pass the <code>CharSequence</code>object you want to append to the append call as that gives better performance.</p>
+				<p>
+
+			</p>
+			]]>
+		</Details>
+	</BugPattern>
+
 
     <BugPattern type="ENMI_EQUALS_ON_ENUM">
         <ShortDescription>Method calls equals on an enum instance</ShortDescription>
@@ -6551,8 +6589,10 @@ if (shouldCalcHalting && (calculateHaltingProbability() &gt; 0) { }
         </Details> 
     </BugPattern>
 
-	<!-- BugCode -->
+		
 
+	<!-- BugCode -->
+	<BugCode abbrev="IAA">Inefficient Appendable Appending</BugCode>
 	<BugCode abbrev="ISB">Inefficient String Buffering</BugCode>
 	<BugCode abbrev="SCI">Synchronized Collection Iterators</BugCode>
 	<BugCode abbrev="CC">Cyclomatic Complexity</BugCode>

src/main/java/com/mebigfatguy/fbcontrib/detect/InefficientAppendableAppend.java

@@ -0,0 +1,205 @@
+/*
+ * fb-contrib - Auxiliary detectors for Java programs
+ * Copyright (C) 2005-2019 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 javax.annotation.Nullable;
+
+import org.apache.bcel.Const;
+import org.apache.bcel.classfile.Code;
+import org.apache.bcel.classfile.Constant;
+import org.apache.bcel.classfile.ConstantString;
+import org.apache.bcel.classfile.JavaClass;
+
+import com.mebigfatguy.fbcontrib.utils.BugType;
+import com.mebigfatguy.fbcontrib.utils.OpcodeUtils;
+import com.mebigfatguy.fbcontrib.utils.TernaryPatcher;
+import com.mebigfatguy.fbcontrib.utils.Values;
+
+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.OpcodeStack.CustomUserValue;
+import edu.umd.cs.findbugs.ba.ClassContext;
+import edu.umd.cs.findbugs.ba.ch.Subtypes2;
+import edu.umd.cs.findbugs.util.ClassName;
+
+/**
+ * Looks for following the append methods of interface java.lang.Appendable
+ * <code>append(CharSeq csq)</code>
+ * <code>append (CharSeq csq, int start, int end)</code> Reports if
+ * <code>toString</code> is used on the <code>CharSeq</code>
+ */
+@CustomUserValue
+public class InefficientAppendableAppend extends BytecodeScanningDetector {
+
+	private enum AppendType {
+		CLEAR, TOSTRING
+	};
+
+	private BugReporter bugReporter;
+	private OpcodeStack stack;
+	private boolean sawLDCEmpty;
+
+	/**
+	 * constructs a IAA detector given the reporter to report bugs on
+	 *
+	 * @param bugReporter the sync of bug reports
+	 */
+	public InefficientAppendableAppend(final BugReporter bugReporter) {
+		this.bugReporter = bugReporter;
+	}
+
+	/**
+	 * implements the visitor to create and clear the stack
+	 *
+	 * @param classContext the context object of the currently parsed class
+	 */
+	@Override
+	public void visitClassContext(ClassContext classContext) {
+		try {
+			stack = new OpcodeStack();
+			super.visitClassContext(classContext);
+		} finally {
+			stack = null;
+		}
+	}
+
+	/**
+	 * implements the visitor to create and clear the stack
+	 *
+	 * @param obj the context object of the currently parsed code block
+	 */
+	@Override
+	public void visitCode(final Code obj) {
+		if (obj.getCode() != null) {
+			stack.resetForMethodEntry(this);
+			sawLDCEmpty = false;
+			super.visitCode(obj);
+		}
+	}
+
+	@Override
+	public void sawOpcode(final int seen) {
+		AppendType userValue = null;
+
+		try {
+			stack.precomputation(this);
+
+			if (seen == Const.INVOKEVIRTUAL || seen == Const.INVOKEINTERFACE) {
+				userValue = sawInvokeVirtualOrInterface();
+			} else if ((seen == Const.GOTO) || (seen == Const.GOTO_W)) {
+				int depth = stack.getStackDepth();
+				for (int i = 0; i < depth; i++) {
+					OpcodeStack.Item itm = stack.getStackItem(i);
+					itm.setUserValue(null);
+				}
+			} else if ((seen == Const.LDC) || (seen == Const.LDC_W)) {
+				Constant c = getConstantRefOperand();
+				if (c instanceof ConstantString) {
+					String s = ((ConstantString) c).getBytes(getConstantPool());
+					if (s.length() == 0) {
+						sawLDCEmpty = true;
+					}
+				}
+				// ((seen >= Const.ALOAD_0) && (seen <= Const.ALOAD_3))
+			} else if (OpcodeUtils.isALoad(seen)) {
+				userValue = AppendType.CLEAR;
+			}
+		} finally {
+			handleOpcode(seen);
+			if ((userValue != null) && (stack.getStackDepth() > 0)) {
+				OpcodeStack.Item itm = stack.getStackItem(0);
+				itm.setUserValue(userValue);
+			}
+		}
+	}
+
+	private void handleOpcode(final int seen) {
+		TernaryPatcher.pre(stack, seen);
+		stack.sawOpcode(this, seen);
+		TernaryPatcher.post(stack, seen);
+	}
+
+	private AppendType sawInvokeVirtualOrInterface() {
+		AppendType userValue = null;
+		String calledClass = getClassConstantOperand();
+		if (Subtypes2.instanceOf((ClassName.toDottedClassName(calledClass)), "java.lang.CharSequence")
+				&& Values.TOSTRING.equals(getNameConstantOperand())) {
+			userValue = AppendType.TOSTRING;
+			return userValue;
+		} else if (Subtypes2.instanceOf(ClassName.toDottedClassName(calledClass), "java.lang.Appendable")) {
+			String methodName = getNameConstantOperand();
+			if ("append".equals(methodName)) {
+				int numArgs = getNumberArguments(getSigConstantOperand());
+				OpcodeStack.Item itm = getAppendableItemAt(numArgs);
+				if (itm != null) {
+					userValue = (AppendType) itm.getUserValue();
+				}
+
+				if (stack.getStackDepth() > 0) {
+					itm = stack.getStackItem(numArgs - 1);
+					AppendType uv = (AppendType) itm.getUserValue();
+					if (uv != null && uv.equals(AppendType.TOSTRING)) {
+						bugReporter.reportBug(new BugInstance(this, BugType.IAA_INEFFICIENT_APPENDABLE_APPEND.name(),
+								Values.TOSTRING.equals(getMethodName()) ? LOW_PRIORITY : NORMAL_PRIORITY).addClass(this)
+										.addMethod(this).addSourceLine(this));
+					}
+				}
+			}
+		}
+		return userValue;
+	}
+
+	/*
+	 * @Nullable private OpcodeStack.Item getCharSequenceItemAt(int depth) { if
+	 * (stack.getStackDepth() > depth) { OpcodeStack.Item itm =
+	 * stack.getStackItem(depth); try { JavaClass cls = itm.getJavaClass(); if
+	 * (Subtypes2.instanceOf(cls, "java.lang.CharSequence")) return itm; } catch
+	 * (ClassNotFoundException e) { return null; }
+	 * 
+	 * }
+	 * 
+	 * return null; }
+	 */
+
+	@Nullable
+	private OpcodeStack.Item getAppendableItemAt(int depth) {
+		if (stack.getStackDepth() > depth) {
+			OpcodeStack.Item itm = stack.getStackItem(depth);
+			try {
+				// This piece of code only required to avoid duplidate reporting by IAA and ISB
+				// detectors
+				/*
+				 * String signature = itm.getSignature(); if
+				 * (Values.SIG_JAVA_UTIL_STRINGBUFFER.equals(signature) ||
+				 * Values.SIG_JAVA_UTIL_STRINGBUILDER.equals(signature)) { return null; }
+				 */
+				JavaClass cls = itm.getJavaClass();
+				if (Subtypes2.instanceOf(cls, "java.lang.Appendable"))
+					return itm;
+			} catch (ClassNotFoundException e) {
+				return null;
+			}
+
+		}
+
+		return null;
+	}
+}

src/main/java/com/mebigfatguy/fbcontrib/utils/BugType.java

@@ -124,6 +124,7 @@ public enum BugType {
     IPU_IMPROPER_PROPERTIES_USE_SETPROPERTY,
     ISB_EMPTY_STRING_APPENDING,
     ISB_INEFFICIENT_STRING_BUFFERING,
+    IAA_INEFFICIENT_APPENDABLE_APPEND,
     ISB_TOSTRING_APPENDING,
     ITC_INHERITANCE_TYPE_CHECKING,
     ITU_INAPPROPRIATE_TOSTRING_USE,

src/samples/java/ex/IAA_Sample.java

@@ -0,0 +1,103 @@
+package ex;
+
+import java.io.File;
+import java.io.FileNotFoundException;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.io.PrintWriter;
+import java.nio.CharBuffer;
+
+@SuppressWarnings("all")
+public class IAA_Sample {
+
+	public String testIAA1Report() {
+		Appendable apd = new StringBuilder();
+		CharSequence cs = new StringBuilder("foo");
+		try {
+			apd.append(cs.toString());
+		} catch (IOException e) {
+			e.printStackTrace();
+		}
+		return apd.toString();
+	}
+
+	public String testIAH2Report() {
+		Appendable apd = new StringBuilder();
+		StringBuilder sb = new StringBuilder("bar");
+		try {
+			apd.append(sb.toString());
+		} catch (IOException e) { 
+			e.printStackTrace();
+		}
+		return sb.toString();
+	}
+
+	public String testIAH3Report() {
+		PrintWriter pw = null;
+		File temp = null;
+		try {
+			temp = File.createTempFile("myTempFile", ".txt"); 
+			temp.deleteOnExit();
+			pw = new PrintWriter(temp);
+			CharSequence cs = new StringBuilder("foo");
+			pw.append(cs.toString());		
+			return cs.toString();
+		} catch (FileNotFoundException e) {
+			e.printStackTrace();
+			return null;
+		} catch (IOException e) {
+			e.printStackTrace();
+			return null;
+		} finally {
+			if (pw != null)
+				pw.close();			
+		}
+	}
+
+	public String testIAH4Report() {
+		FileWriter fw = null;
+		try {
+			File temp = File.createTempFile("myTempFile", ".txt"); 
+			temp.deleteOnExit();
+			fw = new FileWriter(temp);
+			CharBuffer cb = CharBuffer.allocate(10); 
+			cb.put("foobar");
+			fw.append(cb.toString());
+			fw.close();
+			return cb.toString();
+		} catch (FileNotFoundException e) {
+			e.printStackTrace();
+			return null;
+		} catch (IOException e) {
+			e.printStackTrace();
+			return null;
+		}
+	}
+
+	public String testIAH5Report() {
+		PrintWriter pw = null;
+		try {
+			pw = new PrintWriter("file.txt");
+			StringBuilder cs = new StringBuilder("foobar");
+			pw.append(cs.toString(), 1, 2);
+			pw.close();
+			return cs.toString();
+		} catch (FileNotFoundException e) {
+			e.printStackTrace();
+			return null;
+		}
+
+	}
+
+	public String testIAA6DoNotReport() {
+		Appendable sb = new StringBuilder();
+		int[] intArr = new int[] { 1, 2, 3, 4 }; 
+		try {
+			sb.append(intArr.toString());
+		} catch (IOException e) { 
+			e.printStackTrace();
+		}
+		return sb.toString();
+	}
+
+}