Kotlin: Exclude captured variables from constant loop condition check

Author: tamasvajk

java/ql/lib/semmle/code/java/Variable.qll

@@ -61,6 +61,18 @@ class LocalVariableDecl extends @localvar, LocalScopeVariable {
   override Expr getInitializer() { result = this.getDeclExpr().getInit() }
 
   override string getAPrimaryQlClass() { result = "LocalVariableDecl" }
+
+  /** Holds if this non-final variable is captured by a nested callable. */
+  predicate isCaptured() {
+    not this.(Modifiable).isFinal() and exists(this.getACapturingCallable())
+  }
+
+  /** Gets a callable that captures this non-final variable, if any. */
+  Callable getACapturingCallable() {
+    not this.(Modifiable).isFinal() and
+    result = this.getAnAccess().getEnclosingCallable() and
+    result != this.getCallable()
+  }
 }
 
 /** A formal parameter of a callable. */

java/ql/src/Likely Bugs/Termination/ConstantLoopCondition.ql

@@ -84,6 +84,9 @@ where
     not fa.getField().isFinal() and
     fa.getParent*() = cond
   ) and
-  not exists(ArrayAccess aa | aa.getParent*() = cond)
+  not exists(ArrayAccess aa | aa.getParent*() = cond) and
+  not exists(RValue use |
+    use.getParent*() = cond and use.getVariable().(LocalVariableDecl).isCaptured()
+  )
 select cond, "$@ might not terminate, as this loop condition is constant within the loop.", loop,
   "Loop"

java/ql/test/kotlin/library-tests/variables/variableAccesses.expected

@@ -14,6 +14,11 @@ varAcc
 | variables.kt:30:9:30:9 | this |
 | variables.kt:32:9:32:12 | this |
 | variables.kt:33:9:33:12 | this |
+| variables.kt:52:31:52:31 | f |
+| variables.kt:58:17:58:17 | c |
+| variables.kt:59:17:59:17 | d |
+| variables.kt:65:17:65:17 | e |
+| variables.kt:66:17:66:17 | f |
 extensionReceiverAcc
 | variables.kt:29:9:29:9 | this |
 | variables.kt:30:9:30:9 | this |

java/ql/test/kotlin/library-tests/variables/variables.expected

@@ -2,12 +2,29 @@ isFinal
 | variables.kt:6:9:6:26 | int local1 | final |
 | variables.kt:8:9:8:26 | int local2 | non-final |
 | variables.kt:10:9:10:26 | int local3 | final |
+| variables.kt:55:5:55:16 | boolean c | non-final |
+| variables.kt:56:5:56:16 | boolean d | final |
+| variables.kt:62:5:62:16 | boolean e | non-final |
+| variables.kt:63:5:63:16 | boolean f | final |
 compileTimeConstant
 | variables.kt:3:5:3:21 | prop |
 | variables.kt:3:5:3:21 | this.prop |
 | variables.kt:7:17:7:22 | local1 |
 | variables.kt:15:1:15:21 | VariablesKt.topLevel |
 | variables.kt:15:1:15:21 | VariablesKt.topLevel |
+| variables.kt:59:17:59:17 | d |
+| variables.kt:66:17:66:17 | f |
+isCaptured
+| variables.kt:6:9:6:26 | int local1 | not captured |
+| variables.kt:8:9:8:26 | int local2 | not captured |
+| variables.kt:10:9:10:26 | int local3 | not captured |
+| variables.kt:55:5:55:16 | boolean c | captured |
+| variables.kt:56:5:56:16 | boolean d | not captured |
+| variables.kt:62:5:62:16 | boolean e | captured |
+| variables.kt:63:5:63:16 | boolean f | not captured |
+varCaptured
+| variables.kt:55:5:55:16 | boolean c | variables.kt:57:9:60:5 | invoke |
+| variables.kt:62:5:62:16 | boolean e | variables.kt:64:5:67:5 | fn2 |
 #select
 | variables.kt:3:5:3:21 | prop | int | variables.kt:3:21:3:21 | 1 |
 | variables.kt:5:20:5:29 | param | int | file://:0:0:0:0 | <none> |
@@ -18,3 +35,8 @@ compileTimeConstant
 | variables.kt:21:11:21:18 | o | C1 | file://:0:0:0:0 | <none> |
 | variables.kt:21:11:21:18 | o | C1 | variables.kt:21:11:21:18 | o |
 | variables.kt:28:9:28:10 | <this> | C1 | file://:0:0:0:0 | <none> |
+| variables.kt:52:9:52:26 | f | Function0<Unit> | file://:0:0:0:0 | <none> |
+| variables.kt:55:5:55:16 | boolean c | boolean | variables.kt:55:13:55:16 | true |
+| variables.kt:56:5:56:16 | boolean d | boolean | variables.kt:56:13:56:16 | true |
+| variables.kt:62:5:62:16 | boolean e | boolean | variables.kt:62:13:62:16 | true |
+| variables.kt:63:5:63:16 | boolean f | boolean | variables.kt:63:13:63:16 | true |

java/ql/test/kotlin/library-tests/variables/variables.kt

@@ -47,4 +47,22 @@ class C3 {
             this@C3.f0()
         }
     }
-}
+}
+
+fun fn0(f: Function0<Unit>) = f()
+
+fun fn1() {
+    var c = true
+    val d = true
+    fn0 {
+        println(c)
+        println(d)
+    }
+
+    var e = true
+    val f = true
+    fun fn2() {
+        println(e)
+        println(f)
+    }
+}

java/ql/test/kotlin/library-tests/variables/variables.ql

@@ -43,3 +43,9 @@ query predicate isFinal(LocalVariableDecl v, string isFinal) {
 query predicate compileTimeConstant(CompileTimeConstantExpr e) {
   exists(Variable v | v.getAnAccess() = e)
 }
+
+query predicate isCaptured(LocalVariableDecl v, string captured) {
+  if v.isCaptured() then captured = "captured" else captured = "not captured"
+}
+
+query predicate varCaptured(LocalVariableDecl v, Callable c) { v.getACapturingCallable() = c }

java/ql/test/kotlin/query-tests/ConstantLoopCondition/A.kt

@@ -7,4 +7,18 @@ fun fn1() {
             c = false
         }
     }
+
+    var d = true
+    while (d) { // FALSE NEGATIVE
+        fn0 {
+            println(d)
+        }
+    }
+
+    val e = true
+    while (e) {
+        fn0 {
+            println(e)
+        }
+    }
 }

java/ql/test/kotlin/query-tests/ConstantLoopCondition/ConstantLoopCondition.expected

@@ -1 +1 @@
-| A.kt:5:12:5:12 | c | $@ might not terminate, as this loop condition is constant within the loop. | A.kt:5:5:9:5 | while (...) | Loop |
+| A.kt:19:12:19:12 | e | $@ might not terminate, as this loop condition is constant within the loop. | A.kt:19:5:23:5 | while (...) | Loop |