Swift: fix IfConfigDecl in QL libraries

redsun82 · redsun82 · commit 69dfdf5a3435 · 2022-09-21T12:12:55.000+02:00
This fixes `IfConfigDecl` for both the AST printer and control flow
libraries.

It turns out that the active lements of an `IfConfigDecl` are already
listed in the enclosing scope (like a `BraceStmt`), so they should not
be listed as children, and `IfConfigDecl` can be jsut a leaf in the
control flow.
diff --git a/csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraphImplShared.qll b/csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraphImplShared.qll
@@ -52,7 +52,7 @@ predicate last(ControlFlowTree cft, ControlFlowElement last, Completion c) {
  */
 pragma[nomagic]
 predicate succ(ControlFlowElement pred, ControlFlowElement succ, Completion c) {
-  any(ControlFlowTree cft).succ(pred, succ, c)
+  exists(ControlFlowTree cft | cft.succ(pred, succ, c))
 }
 
 /** An element that is executed in pre-order. */
diff --git a/ruby/ql/lib/codeql/ruby/controlflow/internal/ControlFlowGraphImplShared.qll b/ruby/ql/lib/codeql/ruby/controlflow/internal/ControlFlowGraphImplShared.qll
@@ -52,7 +52,7 @@ predicate last(ControlFlowTree cft, ControlFlowElement last, Completion c) {
  */
 pragma[nomagic]
 predicate succ(ControlFlowElement pred, ControlFlowElement succ, Completion c) {
-  any(ControlFlowTree cft).succ(pred, succ, c)
+  exists(ControlFlowTree cft | cft.succ(pred, succ, c))
 }
 
 /** An element that is executed in pre-order. */
diff --git a/swift/codegen/schema.yml b/swift/codegen/schema.yml
@@ -303,8 +303,7 @@ EnumCaseDecl:
 
 IfConfigDecl:
   _extends: Decl
-  _children:
-    active_elements: AstNode*
+  active_elements: AstNode*
 
 ImportDecl:
   _extends: Decl
diff --git a/swift/ql/lib/codeql/swift/controlflow/internal/ControlFlowGraphImpl.qll b/swift/ql/lib/codeql/swift/controlflow/internal/ControlFlowGraphImpl.qll
@@ -1002,6 +1002,15 @@ module Decls {
       i = ast.getNumberOfParams()
     }
   }
+
+  /**
+   * The control-flow of an #if block.
+   * The active elements are already listed in the containing scope, so we can just flow through
+   * this as a leaf.
+   */
+  class IfConfigDeclTree extends AstLeafTree {
+    override IfConfigDecl ast;
+  }
 }
 
 module Exprs {
diff --git a/swift/ql/lib/codeql/swift/controlflow/internal/ControlFlowGraphImplShared.qll b/swift/ql/lib/codeql/swift/controlflow/internal/ControlFlowGraphImplShared.qll
@@ -52,7 +52,7 @@ predicate last(ControlFlowTree cft, ControlFlowElement last, Completion c) {
  */
 pragma[nomagic]
 predicate succ(ControlFlowElement pred, ControlFlowElement succ, Completion c) {
-  any(ControlFlowTree cft).succ(pred, succ, c)
+  exists(ControlFlowTree cft | cft.succ(pred, succ, c))
 }
 
 /** An element that is executed in pre-order. */
diff --git a/swift/ql/lib/codeql/swift/generated/ParentChild.qll b/swift/ql/lib/codeql/swift/generated/ParentChild.qll
@@ -1760,18 +1760,14 @@ private module Impl {
   private Element getImmediateChildOfIfConfigDecl(
     IfConfigDecl e, int index, string partialPredicateCall
   ) {
-    exists(int b, int bDecl, int n, int nActiveElement |
+    exists(int b, int bDecl, int n |
       b = 0 and
       bDecl = b + 1 + max(int i | i = -1 or exists(getImmediateChildOfDecl(e, i, _)) | i) and
       n = bDecl and
-      nActiveElement = n + 1 + max(int i | i = -1 or exists(e.getImmediateActiveElement(i)) | i) and
       (
         none()
         or
         result = getImmediateChildOfDecl(e, index - b, partialPredicateCall)
-        or
-        result = e.getImmediateActiveElement(index - n) and
-        partialPredicateCall = "ActiveElement(" + (index - n).toString() + ")"
       )
     )
   }
diff --git a/swift/ql/test/extractor-tests/declarations/all.expected b/swift/ql/test/extractor-tests/declarations/all.expected
@@ -263,3 +263,7 @@
 | declarations.swift:155:1:155:28 | var ... = ... |
 | declarations.swift:155:1:155:28 | { ... } |
 | declarations.swift:155:5:155:5 | d |
+| declarations.swift:157:1:180:1 | ifConfig() |
+| declarations.swift:158:3:166:3 | #if ... |
+| declarations.swift:168:3:171:3 | #if ... |
+| declarations.swift:173:3:179:3 | #if ... |
diff --git a/swift/ql/test/extractor-tests/declarations/declarations.swift b/swift/ql/test/extractor-tests/declarations/declarations.swift
@@ -153,3 +153,28 @@ class Derived : Baz {}
 
 // multiple conversions
 var d: Baz? = Derived() as Baz
+
+func ifConfig() {
+  #if FOO
+  1
+  2
+  3
+  #else
+  4
+  5
+  6
+  #endif
+
+  #if BAR
+  7
+  8
+  #endif
+
+  #if true
+  9
+  10
+  #else
+  11
+  12
+  #endif
+}
diff --git a/swift/ql/test/library-tests/ast/PrintAst.expected b/swift/ql/test/library-tests/ast/PrintAst.expected
@@ -706,6 +706,17 @@ declarations.swift:
 #  155|         getTypeRepr(): [TypeRepr] Baz?
 #  155| [ConcreteVarDecl] d
 #  155|     Type = Baz?
+#  157| [ConcreteFuncDecl] ifConfig()
+#  157|     InterfaceType = () -> ()
+#  157|   getBody(): [BraceStmt] { ... }
+#  158|     getElement(0): [IfConfigDecl] #if ...
+#  163|     getElement(1): [IntegerLiteralExpr] 4
+#  164|     getElement(2): [IntegerLiteralExpr] 5
+#  165|     getElement(3): [IntegerLiteralExpr] 6
+#  168|     getElement(4): [IfConfigDecl] #if ...
+#  173|     getElement(5): [IfConfigDecl] #if ...
+#  174|     getElement(6): [IntegerLiteralExpr] 9
+#  175|     getElement(7): [IntegerLiteralExpr] 10
 expressions.swift:
 #    1| [TopLevelCodeDecl] { ... }
 #    1|   getBody(): [BraceStmt] { ... }
diff --git a/swift/ql/test/library-tests/ast/declarations.swift b/swift/ql/test/library-tests/ast/declarations.swift
@@ -153,3 +153,28 @@ class Derived : Baz {}
 
 // multiple conversions
 var d: Baz? = Derived() as Baz
+
+func ifConfig() {
+  #if FOO
+  1
+  2
+  3
+  #else
+  4
+  5
+  6
+  #endif
+
+  #if BAR
+  7
+  8
+  #endif
+
+  #if true
+  9
+  10
+  #else
+  11
+  12
+  #endif
+}
diff --git a/swift/ql/test/library-tests/controlflow/graph/Cfg.expected b/swift/ql/test/library-tests/controlflow/graph/Cfg.expected
@@ -6295,3 +6295,44 @@ cfg.swift:
 
 #  464| kpGet_mayB_x
 #-----|  -> (KeyPath<A, Int?>) ...
+
+#  467| enter testIfConfig()
+#-----|  -> testIfConfig()
+
+#  467| exit testIfConfig()
+
+#  467| exit testIfConfig() (normal)
+#-----|  -> exit testIfConfig()
+
+#  467| testIfConfig()
+#-----|  -> #if ...
+
+#  468| #if ...
+#-----|  -> 3
+
+#  472| 3
+#-----|  -> 4
+
+#  473| 4
+#-----|  -> 5
+
+#  476| 5
+#-----|  -> #if ...
+
+#  478| #if ...
+#-----|  -> 8
+
+#  483| 8
+#-----|  -> #if ...
+
+#  485| #if ...
+#-----|  -> 11
+
+#  489| 11
+#-----|  -> 12
+
+#  490| 12
+#-----|  -> 13
+
+#  493| 13
+#-----|  -> exit testIfConfig() (normal)
diff --git a/swift/ql/test/library-tests/controlflow/graph/cfg.swift b/swift/ql/test/library-tests/controlflow/graph/cfg.swift
@@ -88,7 +88,7 @@ func testInOut() -> Int {
   func addOptional(a: inout Int?) {
     a = nil
   }
-  
+
   add(a:&temp)
   var tempOptional : Int? = 10
   addOptional(a:&tempOptional)
@@ -463,3 +463,32 @@ func test(a : A) {
   var apply_kpGet_mayB_force_x = a[keyPath: kpGet_mayB_force_x]
   var apply_kpGet_mayB_x = a[keyPath: kpGet_mayB_x]
 }
+
+func testIfConfig() {
+#if FOO
+  1
+  2
+#else
+  3
+  4
+#endif
+
+  5
+
+#if BAR
+  6
+  7
+#endif
+
+  8
+
+#if FOO
+  9
+  10
+#elseif true
+  11
+  12
+#endif
+
+  13
+}

Original file line number	Diff line number	Diff line change
`@@ -52,7 +52,7 @@ predicate last(ControlFlowTree cft, ControlFlowElement last, Completion c) {`
`52`	`52`	`*/`
`53`	`53`	`pragma[nomagic]`
`54`	`54`	`predicate succ(ControlFlowElement pred, ControlFlowElement succ, Completion c) {`
`55`		`- any(ControlFlowTree cft).succ(pred, succ, c)`
	`55`	`+ exists(ControlFlowTree cft \| cft.succ(pred, succ, c))`
`56`	`56`	`}`
`57`	`57`
`58`	`58`	`/** An element that is executed in pre-order. */`
Original file line number	Diff line number	Diff line change
`@@ -1002,6 +1002,15 @@ module Decls {`
`1002`	`1002`	`i = ast.getNumberOfParams()`
`1003`	`1003`	`}`
`1004`	`1004`	`}`
	`1005`	`+`
	`1006`	`+ /**`
	`1007`	`+ * The control-flow of an #if block.`
	`1008`	`+ * The active elements are already listed in the containing scope, so we can just flow through`
	`1009`	`+ * this as a leaf.`
	`1010`	`+ */`
	`1011`	`+ class IfConfigDeclTree extends AstLeafTree {`
	`1012`	`+ override IfConfigDecl ast;`
	`1013`	`+ }`
`1005`	`1014`	`}`
`1006`	`1015`
`1007`	`1016`	`module Exprs {`
Original file line number	Diff line number	Diff line change
`@@ -1760,18 +1760,14 @@ private module Impl {`
`1760`	`1760`	`private Element getImmediateChildOfIfConfigDecl(`
`1761`	`1761`	`IfConfigDecl e, int index, string partialPredicateCall`
`1762`	`1762`	`) {`
`1763`		`- exists(int b, int bDecl, int n, int nActiveElement \|`
	`1763`	`+ exists(int b, int bDecl, int n \|`
`1764`	`1764`	`b = 0 and`
`1765`	`1765`	`bDecl = b + 1 + max(int i \| i = -1 or exists(getImmediateChildOfDecl(e, i, _)) \| i) and`
`1766`	`1766`	`n = bDecl and`
`1767`		`- nActiveElement = n + 1 + max(int i \| i = -1 or exists(e.getImmediateActiveElement(i)) \| i) and`
`1768`	`1767`	`(`
`1769`	`1768`	`none()`
`1770`	`1769`	`or`
`1771`	`1770`	`result = getImmediateChildOfDecl(e, index - b, partialPredicateCall)`
`1772`		`- or`
`1773`		`- result = e.getImmediateActiveElement(index - n) and`
`1774`		`- partialPredicateCall = "ActiveElement(" + (index - n).toString() + ")"`
`1775`	`1771`	`)`
`1776`	`1772`	`)`
`1777`	`1773`	`}`