Merge pull request #16120 from owen-mc/go/fix/type-switch-control-flow

Go: Fix data flow through variable defined in type switch guard

go/ql/lib/change-notes/2024-04-06-type-switch-implicitly-declared-variables.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Data flow through variables declared in statements of the form `x := y.(type)` at the beginning of type switches has been fixed, which may result in more alerts.

go/ql/lib/semmle/go/Scopes.qll

@@ -122,9 +122,10 @@ class Entity extends @object {
   /**
    * Gets the declaring identifier for this entity, if any.
    *
-   * Note that type switch statements which define a new variable in the guard
+   * Note that type switch statements which declare a new variable in the guard
    * actually have a new variable (of the right type) implicitly declared at
-   * the beginning of each case clause, and these do not have a declaration.
+   * the beginning of each case clause, and these do not have a syntactic
+   * declaration.
    */
   Ident getDeclaration() { result.declares(this) }
 
@@ -137,6 +138,15 @@ class Entity extends @object {
   /** Gets a textual representation of this entity. */
   string toString() { result = this.getName() }
 
+  private predicate hasRealLocationInfo(
+    string filepath, int startline, int startcolumn, int endline, int endcolumn
+  ) {
+    // take the location of the declaration if there is one
+    this.getDeclaration().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn) or
+    any(CaseClause cc | this = cc.getImplicitlyDeclaredVariable())
+        .hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
+  }
+
   /**
    * Holds if this element is at the specified location.
    * The location spans column `startcolumn` of line `startline` to
@@ -148,15 +158,16 @@ class Entity extends @object {
     string filepath, int startline, int startcolumn, int endline, int endcolumn
   ) {
     // take the location of the declaration if there is one
-    this.getDeclaration().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
-    or
-    // otherwise fall back on dummy location
-    not exists(this.getDeclaration()) and
-    filepath = "" and
-    startline = 0 and
-    startcolumn = 0 and
-    endline = 0 and
-    endcolumn = 0
+    if this.hasRealLocationInfo(_, _, _, _, _)
+    then this.hasRealLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
+    else (
+      // otherwise fall back on dummy location
+      filepath = "" and
+      startline = 0 and
+      startcolumn = 0 and
+      endline = 0 and
+      endcolumn = 0
+    )
   }
 }
 

go/ql/lib/semmle/go/Stmt.qll

@@ -777,6 +777,16 @@ class CaseClause extends @caseclause, Stmt, ScopeNode {
   /** Gets the number of statements of this `case` clause. */
   int getNumStmt() { result = this.getNumChildStmt() }
 
+  /**
+   * Gets the implicitly declared variable for this `case` clause, if any.
+   *
+   * This exists for case clauses in type switch statements which declare a
+   * variable in the guard.
+   */
+  LocalVariable getImplicitlyDeclaredVariable() {
+    not exists(result.getDeclaration()) and result.getScope().(LocalScope).getNode() = this
+  }
+
   override predicate mayHaveSideEffects() {
     this.getAnExpr().mayHaveSideEffects() or
     this.getAStmt().mayHaveSideEffects()

go/ql/lib/semmle/go/controlflow/ControlFlowGraphImpl.qll

@@ -306,6 +306,19 @@ newtype TControlFlowNode =
    * the `i`th expression of a case clause `cc`.
    */
   MkCaseCheckNode(CaseClause cc, int i) { exists(cc.getExpr(i)) } or
+  /**
+   * A control-flow node that represents the implicit declaration of the
+   * variable `lv` in case clause `cc` and its assignment of the value
+   * `switchExpr` from the guard. This only occurs in case clauses in a type
+   * switch statement which declares a variable in its guard.
+   */
+  MkTypeSwitchImplicitVariable(CaseClause cc, LocalVariable lv, Expr switchExpr) {
+    exists(TypeSwitchStmt ts, DefineStmt ds | ds = ts.getAssign() |
+      cc = ts.getACase() and
+      lv = cc.getImplicitlyDeclaredVariable() and
+      switchExpr = ds.getRhs().(TypeAssertExpr).getExpr()
+    )
+  } or
   /**
    * A control-flow node that represents the implicit lower bound of a slice expression.
    */
@@ -1099,6 +1112,10 @@ module CFG {
       first = this.getExprStart(0)
       or
       not exists(this.getAnExpr()) and
+      first = MkTypeSwitchImplicitVariable(this, _, _)
+      or
+      not exists(this.getAnExpr()) and
+      not exists(MkTypeSwitchImplicitVariable(this, _, _)) and
       first = this.getBodyStart()
     }
 
@@ -1119,6 +1136,9 @@ module CFG {
     override predicate succ0(ControlFlow::Node pred, ControlFlow::Node succ) {
       ControlFlowTree.super.succ0(pred, succ)
       or
+      pred = MkTypeSwitchImplicitVariable(this, _, _) and
+      succ = this.getBodyStart()
+      or
       exists(int i |
         lastNode(this.getExpr(i), pred, normalCompletion()) and
         succ = MkCaseCheckNode(this, i)
@@ -1137,8 +1157,13 @@ module CFG {
 
     predicate isPassingEdge(int i, ControlFlow::Node pred, ControlFlow::Node succ, Expr testExpr) {
       pred = this.getExprEnd(i, true) and
-      succ = this.getBodyStart() and
-      testExpr = this.getExpr(i)
+      testExpr = this.getExpr(i) and
+      (
+        succ = MkTypeSwitchImplicitVariable(this, _, _)
+        or
+        not exists(MkTypeSwitchImplicitVariable(this, _, _)) and
+        succ = this.getBodyStart()
+      )
     }
 
     override ControlFlowTree getChildTree(int i) { result = this.getStmt(i) }

go/ql/lib/semmle/go/controlflow/IR.qll

@@ -45,6 +45,7 @@ module IR {
       this instanceof MkNextNode or
       this instanceof MkImplicitTrue or
       this instanceof MkCaseCheckNode or
+      this instanceof MkTypeSwitchImplicitVariable or
       this instanceof MkImplicitLowerSliceBound or
       this instanceof MkImplicitUpperSliceBound or
       this instanceof MkImplicitMaxSliceBound or
@@ -167,6 +168,9 @@ module IR {
       or
       this instanceof MkCaseCheckNode and result = "case"
       or
+      this instanceof MkTypeSwitchImplicitVariable and
+      result = "type switch implicit variable declaration"
+      or
       this instanceof MkImplicitLowerSliceBound and result = "implicit lower bound"
       or
       this instanceof MkImplicitUpperSliceBound and result = "implicit upper bound"
@@ -1269,6 +1273,52 @@ module IR {
     }
   }
 
+  /**
+   * An instruction corresponding to the implicit declaration of the variable
+   * `lv` in case clause `cc` and its assignment of the value `switchExpr` from
+   * the guard. This only occurs in case clauses in a type switch statement
+   * which declares a variable in its guard.
+   *
+   * For example, consider this type switch statement:
+   *
+   * ```go
+   * switch y := x.(type) {
+   * case Type1:
+   *   f(y)
+   *   ...
+   * }
+   * ```
+   *
+   * The `y` inside the case clause is actually a local variable with type
+   * `Type1` that is implicitly declared at the top of the case clause. In
+   * default clauses and case clauses which list more than one type, the type
+   * of the implicitly declared variable is the type of `switchExpr`.
+   */
+  class TypeSwitchImplicitVariableInstruction extends Instruction, MkTypeSwitchImplicitVariable {
+    CaseClause cc;
+    LocalVariable lv;
+    Expr switchExpr;
+
+    TypeSwitchImplicitVariableInstruction() {
+      this = MkTypeSwitchImplicitVariable(cc, lv, switchExpr)
+    }
+
+    override predicate writes(ValueEntity v, Instruction rhs) {
+      v = lv and
+      rhs = evalExprInstruction(switchExpr)
+    }
+
+    override ControlFlow::Root getRoot() { result.isRootOf(cc) }
+
+    override string toString() { result = "implicit type switch variable declaration" }
+
+    override predicate hasLocationInfo(
+      string filepath, int startline, int startcolumn, int endline, int endcolumn
+    ) {
+      cc.hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
+    }
+  }
+
   /**
    * An instruction computing the implicit lower slice bound of zero in a slice expression without
    * an explicit lower bound.