Merge pull request #9153 from tamasvajk/kotlin-simplify-loop-breaks-1

Kotlin: Unify loop `break`/`continue` statement handling between java and kotlin

java/kotlin-extractor/src/main/kotlin/KotlinFileExtractor.kt

@@ -2197,8 +2197,6 @@ open class KotlinFileExtractor(
         }
     }
 
-    private val loopIdMap: MutableMap<IrLoop, Label<out DbKtloopstmt>> = mutableMapOf()
-
     // todo: calculating the enclosing ref type could be done through this, instead of walking up the declaration parent chain
     private val declarationStack: Stack<IrDeclaration> = Stack()
 
@@ -2446,32 +2444,10 @@ open class KotlinFileExtractor(
                     }
                 }
                 is IrWhileLoop -> {
-                    val stmtParent = parent.stmt(e, callable)
-                    val id = tw.getFreshIdLabel<DbWhilestmt>()
-                    loopIdMap[e] = id
-                    val locId = tw.getLocation(e)
-                    tw.writeStmts_whilestmt(id, stmtParent.parent, stmtParent.idx, callable)
-                    tw.writeHasLocation(id, locId)
-                    extractExpressionExpr(e.condition, callable, id, 0, id)
-                    val body = e.body
-                    if(body != null) {
-                        extractExpressionStmt(body, callable, id, 1)
-                    }
-                    loopIdMap.remove(e)
+                    extractLoop(e, parent, callable)
                 }
                 is IrDoWhileLoop -> {
-                    val stmtParent = parent.stmt(e, callable)
-                    val id = tw.getFreshIdLabel<DbDostmt>()
-                    loopIdMap[e] = id
-                    val locId = tw.getLocation(e)
-                    tw.writeStmts_dostmt(id, stmtParent.parent, stmtParent.idx, callable)
-                    tw.writeHasLocation(id, locId)
-                    extractExpressionExpr(e.condition, callable, id, 0, id)
-                    val body = e.body
-                    if(body != null) {
-                        extractExpressionStmt(body, callable, id, 1)
-                    }
-                    loopIdMap.remove(e)
+                    extractLoop(e, parent, callable)
                 }
                 is IrInstanceInitializerCall -> {
                     val irConstructor = declarationStack.peek() as? IrConstructor
@@ -2988,6 +2964,49 @@ open class KotlinFileExtractor(
         }
     }
 
+    private fun extractLoop(
+        loop: IrLoop,
+        stmtExprParent: StmtExprParent,
+        callable: Label<out DbCallable>
+    ) {
+        val stmtParent = stmtExprParent.stmt(loop, callable)
+        val locId = tw.getLocation(loop)
+
+        val idx: Int
+        val parent: Label<out DbStmtparent>
+
+        val label = loop.label
+        if (label != null) {
+            val labeledStmt = tw.getFreshIdLabel<DbLabeledstmt>()
+            tw.writeStmts_labeledstmt(labeledStmt, stmtParent.parent, stmtParent.idx, callable)
+            tw.writeHasLocation(labeledStmt, locId)
+
+            tw.writeNamestrings(label, "", labeledStmt)
+            idx = 0
+            parent = labeledStmt
+        } else {
+            idx = stmtParent.idx
+            parent = stmtParent.parent
+        }
+
+        val id = if (loop is IrWhileLoop) {
+            val id = tw.getFreshIdLabel<DbWhilestmt>()
+            tw.writeStmts_whilestmt(id, parent, idx, callable)
+            id
+        } else {
+            val id = tw.getFreshIdLabel<DbDostmt>()
+            tw.writeStmts_dostmt(id, parent, idx, callable)
+            id
+        }
+
+        tw.writeHasLocation(id, locId)
+        extractExpressionExpr(loop.condition, callable, id, 0, id)
+        val body = loop.body
+        if (body != null) {
+            extractExpressionStmt(body, callable, id, 1)
+        }
+    }
+
     private fun IrValueParameter.isExtensionReceiver(): Boolean {
         val parentFun = parent as? IrFunction ?: return false
         return parentFun.extensionReceiverParameter == this
@@ -4261,7 +4280,7 @@ open class KotlinFileExtractor(
 
     private fun extractBreakContinue(
         e: IrBreakContinue,
-        id: Label<out DbBreakcontinuestmt>
+        id: Label<out DbNamedexprorstmt>
     ) {
         with("break/continue", e) {
             val locId = tw.getLocation(e)
@@ -4270,14 +4289,6 @@ open class KotlinFileExtractor(
             if (label != null) {
                 tw.writeNamestrings(label, "", id)
             }
-
-            val loopId = loopIdMap[e.loop]
-            if (loopId == null) {
-                logger.errorElement("Missing break/continue target", e)
-                return
-            }
-
-            tw.writeKtBreakContinueTargets(id, loopId)
         }
     }
 

java/ql/lib/change-notes/2022-06-13-kotlin-break-loops.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Removed Kotlin-specific database and QL structures for loops and `break`/`continue` statements. The Kotlin extractor was changed to reuse the Java structures for these constructs.

java/ql/lib/config/semmlecode.dbscheme

@@ -1165,17 +1165,6 @@ ktCommentOwners(
   int owner: @top ref
 )
 
-@breakcontinuestmt = @breakstmt
-                   | @continuestmt;
-
-@ktloopstmt = @whilestmt
-            | @dostmt;
-
-ktBreakContinueTargets(
-  unique int id: @breakcontinuestmt ref,
-  int target: @ktloopstmt ref
-)
-
 ktExtensionFunctions(
   unique int id: @method ref,
   int typeid: @type ref,

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

@@ -888,27 +888,3 @@ class SuperConstructorInvocationStmt extends Stmt, ConstructorCall, @superconstr
 
   override string getAPrimaryQlClass() { result = "SuperConstructorInvocationStmt" }
 }
-
-/** A Kotlin loop statement. */
-class KtLoopStmt extends Stmt, @ktloopstmt {
-  KtLoopStmt() {
-    this instanceof WhileStmt or
-    this instanceof DoStmt
-  }
-}
-
-/** A Kotlin `break` or `continue` statement. */
-abstract class KtBreakContinueStmt extends Stmt, @breakcontinuestmt {
-  KtLoopStmt loop;
-
-  KtBreakContinueStmt() { ktBreakContinueTargets(this, loop) }
-
-  /** Gets the target loop statement of this `break`. */
-  KtLoopStmt getLoopStmt() { result = loop }
-}
-
-/** A Kotlin `break` statement. */
-class KtBreakStmt extends BreakStmt, KtBreakContinueStmt { }
-
-/** A Kotlin `continue` statement. */
-class KtContinueStmt extends ContinueStmt, KtBreakContinueStmt { }

java/ql/lib/semmle/code/java/security/SecurityFlag.qll

@@ -20,21 +20,34 @@ abstract class FlagKind extends string {
   bindingset[result]
   abstract string getAFlagName();
 
+  private predicate flagFlowStepTC(DataFlow::Node node1, DataFlow::Node node2) {
+    node2 = node1 and
+    isFlagWithName(node1)
+    or
+    exists(DataFlow::Node nodeMid |
+      flagFlowStep(nodeMid, node2) and
+      flagFlowStepTC(node1, nodeMid)
+    )
+  }
+
+  private predicate isFlagWithName(DataFlow::Node flag) {
+    exists(VarAccess v | v.getVariable().getName() = getAFlagName() |
+      flag.asExpr() = v and v.getType() instanceof FlagType
+    )
+    or
+    exists(StringLiteral s | s.getValue() = getAFlagName() | flag.asExpr() = s)
+    or
+    exists(MethodAccess ma | ma.getMethod().getName() = getAFlagName() |
+      flag.asExpr() = ma and
+      ma.getType() instanceof FlagType
+    )
+  }
+
   /** Gets a node representing a (likely) security flag. */
   DataFlow::Node getAFlag() {
     exists(DataFlow::Node flag |
-      exists(VarAccess v | v.getVariable().getName() = getAFlagName() |
-        flag.asExpr() = v and v.getType() instanceof FlagType
-      )
-      or
-      exists(StringLiteral s | s.getValue() = getAFlagName() | flag.asExpr() = s)
-      or
-      exists(MethodAccess ma | ma.getMethod().getName() = getAFlagName() |
-        flag.asExpr() = ma and
-        ma.getType() instanceof FlagType
-      )
-    |
-      flagFlowStep*(flag, result)
+      isFlagWithName(flag) and
+      flagFlowStepTC(flag, result)
     )
   }
 }

java/ql/lib/upgrades/b9225587bc0a643ae484ec215b9a6f19d17d0fc2/upgrade.properties

@@ -0,0 +1,4 @@
+description: Remove unneeded break/continue structures for Kotlin
+compatibility: backwards
+
+ktBreakContinueTargets.rel: delete

java/ql/src/Violations of Best Practice/Naming Conventions/Shadowing.qll

@@ -20,13 +20,21 @@ predicate setterFor(Method m, Field f) {
 predicate shadows(LocalVariableDecl d, Class c, Field f, Callable method) {
   d.getCallable() = method and
   method.getDeclaringType() = c and
-  c.getAField() = f and
-  f.getName() = d.getName() and
-  f.getType() = d.getType() and
-  not d.getCallable().isStatic() and
+  f = getField(c, d.getName(), d.getType()) and
+  not method.isStatic() and
   not f.isStatic()
 }
 
+/**
+ * Gets the field with the given name and type from the given class, if any.
+ */
+pragma[nomagic]
+private Field getField(Class c, string name, Type t) {
+  result.getDeclaringType() = c and
+  result.getName() = name and
+  result.getType() = t
+}
+
 predicate thisAccess(LocalVariableDecl d, Field f) {
   shadows(d, _, f, _) and
   exists(VarAccess va | va.getVariable().(Field).getSourceDeclaration() = f |

java/ql/test/kotlin/library-tests/stmts/loops.expected

@@ -1,7 +1,6 @@
 breakLabel
 | stmts.kt:25:24:25:33 | break | loop |
 continueLabel
-breakTarget
+jumpTarget
 | stmts.kt:25:24:25:33 | break | stmts.kt:23:11:27:5 | while (...) |
-continueTarget
 | stmts.kt:29:9:29:16 | continue | stmts.kt:28:5:29:16 | while (...) |

java/ql/test/kotlin/library-tests/stmts/loops.ql

@@ -4,6 +4,4 @@ query predicate breakLabel(BreakStmt s, string label) { s.getLabel() = label }
 
 query predicate continueLabel(ContinueStmt s, string label) { s.getLabel() = label }
 
-query predicate breakTarget(KtBreakStmt s, KtLoopStmt l) { s.getLoopStmt() = l }
-
-query predicate continueTarget(KtContinueStmt s, KtLoopStmt l) { s.getLoopStmt() = l }
+query predicate jumpTarget(JumpStmt s, StmtParent p) { s.getTarget() = p }

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

@@ -29,7 +29,8 @@ enclosing
 | stmts.kt:18:37:18:37 | <Expr>; | stmts.kt:18:26:18:56 | ... -> ... |
 | stmts.kt:18:52:18:52 | <Expr>; | stmts.kt:18:26:18:56 | ... -> ... |
 | stmts.kt:19:5:19:16 | return ... | stmts.kt:2:41:20:1 | { ... } |
-| stmts.kt:23:11:27:5 | while (...) | stmts.kt:22:27:30:1 | { ... } |
+| stmts.kt:23:11:27:5 | <Label>: ... | stmts.kt:22:27:30:1 | { ... } |
+| stmts.kt:23:11:27:5 | while (...) | stmts.kt:23:11:27:5 | <Label>: ... |
 | stmts.kt:23:27:27:5 | { ... } | stmts.kt:23:11:27:5 | while (...) |
 | stmts.kt:24:9:26:25 | do ... while (...) | stmts.kt:24:9:26:25 | { ... } |
 | stmts.kt:24:9:26:25 | { ... } | stmts.kt:23:27:27:5 | { ... } |
@@ -80,6 +81,7 @@ enclosing
 | stmts.kt:18:52:18:52 | <Expr>; | ExprStmt |
 | stmts.kt:19:5:19:16 | return ... | ReturnStmt |
 | stmts.kt:22:27:30:1 | { ... } | BlockStmt |
+| stmts.kt:23:11:27:5 | <Label>: ... | LabeledStmt |
 | stmts.kt:23:11:27:5 | while (...) | WhileStmt |
 | stmts.kt:23:27:27:5 | { ... } | BlockStmt |
 | stmts.kt:24:9:26:25 | do ... while (...) | DoStmt |