Java: Improve concatenation in loops

java/ql/src/Performance/ConcatenationInLoops.ql

@@ -40,19 +40,26 @@
   )
 }
 
-predicate declaredInLoop(LocalVariableDecl v, LoopStmt loop) {
-  exists(LocalVariableDeclExpr e |
-    e.getVariable() = v and
-    e.getEnclosingStmt().getEnclosingStmt*() = loop.getBody()
-  )
+/**
+ * Holds if `e` is executed often in loop `loop`.
+ */
+predicate executedOften(Assignment a) {
+  a.getDest().getType() instanceof TypeString and
+  exists(ControlFlowNode n | a.getControlFlowNode() = n | getADeepSuccessor(n) = n)
+}
+
+/** Gets a sucessor of `n`, also following function calls. */
+ControlFlowNode getADeepSuccessor(ControlFlowNode n) {
+  result = n.getASuccessor+()
   or
-  exists(EnhancedForStmt for | for = loop | for.getVariable().getVariable() = v)
+  exists(Call c, ControlFlowNode callee | c.(Expr).getControlFlowNode() = n.getASuccessor+() |
+    callee = c.getCallee().getBody().getControlFlowNode() and
+    result = getADeepSuccessor(callee)
+  )
 }
 
 from Assignment a, Variable v
 where
   useAndDef(a, v) and
-  exists(LoopStmt loop | a.getEnclosingStmt().getEnclosingStmt*() = loop |
-    not declaredInLoop(v, loop)
-  )
+  executedOften(a)
 select a, "The string " + v.getName() + " is built-up in a loop: use string buffer."

java/ql/test/query-tests/ConcatenationInLoops/ConcatenationInLoops.expected

@@ -0,0 +1,3 @@
+| ConcatenationInLoops.java:9:4:9:10 | ...+=... | The string s is built-up in a loop: use string buffer. |
+| ConcatenationInLoops.java:32:6:32:12 | ...+=... | The string s is built-up in a loop: use string buffer. |
+| ConcatenationInLoops.java:39:4:39:12 | ...+=... | The string cs is built-up in a loop: use string buffer. |

java/ql/test/query-tests/ConcatenationInLoops/ConcatenationInLoops.java

@@ -0,0 +1,43 @@
+public class ConcatenationInLoops {
+	private String cs = "";
+
+	public static void main(String[] args) {
+		// Random r = 42;
+		long start = System.currentTimeMillis();
+		String s = "";
+		for (int i = 0; i < 65536; i++)
+			s += 42;  // $ loopConcat=s 
+		System.out.println(System.currentTimeMillis() - start);  // This prints roughly 4500.
+
+		// r = 42;
+		start = System.currentTimeMillis();
+		StringBuilder sb = new StringBuilder();
+		for (int i = 0; i < 65536; i++)
+			sb.append(42);//r.nextInt(2));
+		s = sb.toString();
+		System.out.println(System.currentTimeMillis() - start);  // This prints 5.
+
+
+		String s2 = "";
+		for (int i = 0; i < 65536; i++)
+			if (i > 10) {
+				s += 42; // Will only be executed once.
+				break;
+			}
+
+		String s3 = "";
+		for (int i = 0; i < 65536; i++)
+			for (int j = 0; i < 65536; i++)
+				if (j > 10) {
+					s += 42; // $ loopConcat=s
+					break;
+				}
+	}
+
+	public void test(int bound) {
+		if (bound > 0) {
+			cs += "a";   // $ loopConcat=cs
+			test(bound - 1);
+		} 
+	}
+}

java/ql/test/query-tests/ConcatenationInLoops/ConcatenationInLoops.qlref

@@ -0,0 +1 @@
+Performance/ConcatenationInLoops.ql

java/ql/test/query-tests/ConcatenationInLoops/test.expected

@@ -0,0 +1,2 @@
+testFailures
+failures

java/ql/test/query-tests/ConcatenationInLoops/test.ql

@@ -0,0 +1,82 @@
+import java
+import semmle.code.java.dataflow.DataFlow
+import TestUtilities.InlineExpectationsTest
+
+module TheQuery {
+  import semmle.code.java.Type
+  import semmle.code.java.Expr
+  import semmle.code.java.Statement
+  import semmle.code.java.JDK
+
+  /**
+   * An assignment of the form
+   *
+   * ```
+   *   v = ... + ... v ...
+   * ```
+   * or
+   *
+   * ```
+   *   v += ...
+   * ```
+   * where `v` is a simple variable (and not, for example,
+   * an array element).
+   */
+  predicate useAndDef(Assignment a, Variable v) {
+    a.getDest() = v.getAnAccess() and
+    v.getType() instanceof TypeString and
+    (
+      a instanceof AssignAddExpr
+      or
+      exists(VarAccess use | use.getVariable() = v | use.getParent*() = a.getSource()) and
+      a.getSource() instanceof AddExpr
+    )
+  }
+
+  /**
+   * Holds if `e` is executed often in loop `loop`.
+   */
+  predicate executedOften(Assignment a) {
+    a.getDest().getType() instanceof TypeString and
+    exists(ControlFlowNode n | a.getControlFlowNode() = n | getADeepSuccessor(n) = n)
+  }
+
+  /** Gets a sucessor of `n`, also following function calls. */
+  ControlFlowNode getADeepSuccessor(ControlFlowNode n) {
+    result = n.getASuccessor+()
+    or
+    exists(Call c, ControlFlowNode callee | c.(Expr).getControlFlowNode() = n.getASuccessor+() |
+      callee = c.getCallee().getBody().getControlFlowNode() and
+      result = getADeepSuccessor(callee)
+    )
+  }
+
+  predicate queryResult(Assignment a) {
+    exists(Variable v |
+      useAndDef(a, v) and
+      executedOften(a)
+    )
+  }
+}
+
+// module Config implements DataFlow::ConfigSig {
+//   predicate isSource(DataFlow::Node n) { n.asExpr().(MethodCall).getMethod().hasName("source") }
+//   predicate isSink(DataFlow::Node n) {
+//     exists(MethodCall ma | ma.getMethod().hasName("sink") | n.asExpr() = ma.getAnArgument())
+//   }
+// }
+// module Flow = DataFlow::Global<Config>;
+module HasFlowTest implements TestSig {
+  string getARelevantTag() { result = "loopConcat" }
+
+  predicate hasActualResult(Location location, string element, string tag, string value) {
+    tag = "loopConcat" and
+    exists(Assignment a | TheQuery::queryResult(a) |
+      location = a.getLocation() and
+      element = a.toString() and
+      value = a.getDest().toString()
+    )
+  }
+}
+
+import MakeTest<HasFlowTest>