Address review comments

Author: owen-mc

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

@@ -717,7 +717,7 @@ class IfStmt extends @ifstmt, Stmt, ScopeNode {
   Expr getCondition() { result = this.getChildExpr(1) }
 
   /** Gets the "then" branch of this `if` statement. */
-  BlockStmt getThen() { result = this.getChildStmt(2) }
+  Stmt getThen() { result = this.getChildStmt(2) }
 
   /** Gets the "else" branch of this `if` statement, if any. */
   Stmt getElse() { result = this.getChildStmt(3) }
@@ -1085,7 +1085,7 @@ class SelectStmt extends @selectstmt, Stmt {
  */
 class LoopStmt extends @loopstmt, Stmt, ScopeNode {
   /** Gets the body of this loop. */
-  BlockStmt getBody() { none() }
+  Stmt getBody() { none() }
 }
 
 /**

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

@@ -50,11 +50,7 @@ module ControlFlow {
     predicate isJoin() { strictcount(this.getAPredecessor()) > 1 }
 
     /** Holds if this is the first control-flow node in `subtree`. */
-    predicate isFirstNodeOf(AstNode subtree) {
-      this.isBefore(subtree)
-      or
-      this.injects(subtree)
-    }
+    predicate isFirstNodeOf(AstNode subtree) { this.isBefore(subtree) }
 
     /** Holds if this node is the unique entry node of a file or function. */
     predicate isEntryNode() { this instanceof CfgImpl::ControlFlow::EntryNode }

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

@@ -164,8 +164,6 @@ module CfgImpl {
         not this = any(Go::SelectStmt sel).getBody()
       }
 
-      override Stmt getStmt(int n) { result = Go::BlockStmt.super.getStmt(n) }
-
       Stmt getLastStmt() {
         exists(int last | result = this.getStmt(last) and not exists(this.getStmt(last + 1)))
       }
@@ -183,23 +181,11 @@ module CfgImpl {
       Expr getExpr() { result = Go::ExprStmt.super.getExpr() }
     }
 
-    class IfStmt extends Stmt {
-      IfStmt() { this instanceof Go::IfStmt }
-
-      Expr getCondition() { result = this.(Go::IfStmt).getCond() }
-
-      Stmt getThen() { result = this.(Go::IfStmt).getThen() }
-
-      Stmt getElse() { result = this.(Go::IfStmt).getElse() }
-    }
+    class IfStmt = Go::IfStmt;
 
     AstNode getIfInit(IfStmt ifstmt) { result = ifstmt.(Go::IfStmt).getInit() }
 
-    class LoopStmt extends Stmt {
-      LoopStmt() { this instanceof Go::LoopStmt }
-
-      Stmt getBody() { result = this.(Go::LoopStmt).getBody() }
-    }
+    class LoopStmt = Go::LoopStmt;
 
     class WhileStmt extends LoopStmt {
       WhileStmt() { none() }
@@ -219,19 +205,15 @@ module CfgImpl {
       Expr getCondition() { none() }
     }
 
-    class ForStmt extends LoopStmt {
-      ForStmt() { this instanceof Go::ForStmt }
-
+    class ForStmt extends LoopStmt instanceof Go::ForStmt {
       AstNode getInit(int index) { index = 0 and result = this.(Go::ForStmt).getInit() }
 
       Expr getCondition() { result = this.(Go::ForStmt).getCond() }
 
       AstNode getUpdate(int index) { index = 0 and result = this.(Go::ForStmt).getPost() }
     }
 
-    class ForeachStmt extends LoopStmt {
-      ForeachStmt() { this instanceof Go::RangeStmt }
-
+    class ForeachStmt extends LoopStmt instanceof Go::RangeStmt {
       // Go's `range` statement binds its key and value by destructuring the
       // current element. The extractor synthesizes a single "range element"
       // node grouping the key and value (see `Go::RangeElementExpr`), which we
@@ -249,9 +231,7 @@ module CfgImpl {
 
     class GotoStmt = Go::GotoStmt;
 
-    class ReturnStmt extends Go::ReturnStmt {
-      override Expr getExpr() { result = Go::ReturnStmt.super.getExpr() }
-    }
+    class ReturnStmt = Go::ReturnStmt;
 
     class Throw extends AstNode {
       Throw() { none() }
@@ -281,9 +261,7 @@ module CfgImpl {
       Stmt getBody() { none() }
     }
 
-    class Switch extends AstNode {
-      Switch() { this instanceof Go::SwitchStmt }
-
+    class Switch extends AstNode instanceof Go::SwitchStmt {
       Expr getExpr() {
         result = this.(Go::ExpressionSwitchStmt).getExpr()
         or
@@ -837,7 +815,7 @@ module CfgImpl {
       PreControlFlowNode source, PreControlFlowNode target, AbruptCompletion completion
     ) {
       completion.getSuccessorType() instanceof ExceptionSuccessor and
-      isExceptionalExitNode(target) and
+      target instanceof ExceptionalExitNodeImpl and
       exists(PreControlFlowNode nextDefer |
         deferExitStep(source, nextDefer, _) and deferInvoke(nextDefer, _)
       )
@@ -862,14 +840,8 @@ module CfgImpl {
      * Holds if `n` is the registration node of `defer` statement `s` (the
      * post-order node of the statement, reached once its call's arguments have
      * been evaluated).
-     *
-     * This uses the reachability-free `isInOrderNode` rather than `n.isIn(s)`
-     * because it is referenced under negation by `succBeforeNextDeferRegistration`, and must
-     * therefore not depend on `reachable`.
      */
-    private predicate deferRegistration(PreControlFlowNode n, Go::DeferStmt s) {
-      isInOrderNode(n, s)
-    }
+    private predicate deferRegistration(PreControlFlowNode n, Go::DeferStmt s) { n.isIn(s) }
 
     /**
      * Holds if `n` is the deferred-invocation node for `defer` statement `s`,

shared/controlflow/codeql/controlflow/ControlFlowGraph.qll

@@ -1028,27 +1028,6 @@ module Make0<LocationSig Location, AstSig<Location> Ast> {
      */
     final class PreControlFlowNode = NodeImpl;
 
-    /**
-     * Holds if `n` is the in-order or post-order control flow node for `ast`.
-     *
-     * Unlike the `PreControlFlowNode.isIn` member predicate, this is computed
-     * structurally (directly from the underlying node representation) and so
-     * does not give rise to a dependency on node reachability. It is intended
-     * for languages implementing `Input2::deferExitStep`, whose definition must
-     * not depend on `reachable` (see `succIgnoringDeferExit`); such languages
-     * can use this to identify nodes inside a negation without introducing a
-     * non-monotonic cycle.
-     */
-    predicate isInOrderNode(PreControlFlowNode n, AstNode ast) { n = TAstNode(ast) }
-
-    /**
-     * Holds if `n` is the exceptional exit node for a callable.
-     *
-     * This is computed structurally so that it can be used while constructing
-     * the CFG without introducing a dependency on node reachability.
-     */
-    predicate isExceptionalExitNode(PreControlFlowNode n) { n = TAnnotatedExitNode(_, false) }
-
     private class BeforeNode extends NodeImpl, TBeforeNode {
       private AstNode n;
 
@@ -1164,7 +1143,7 @@ module Make0<LocationSig Location, AstSig<Location> Ast> {
     }
 
     /** A control flow node indicating exceptional termination of a callable. */
-    final private class ExceptionalExitNodeImpl extends AnnotatedExitNodeImpl {
+    final class ExceptionalExitNodeImpl extends AnnotatedExitNodeImpl {
       ExceptionalExitNodeImpl() { this = TAnnotatedExitNode(_, false) }
     }
 
@@ -1760,26 +1739,6 @@ module Make0<LocationSig Location, AstSig<Location> Ast> {
           n2.isAfterValue(pme, any(BooleanSuccessor s | s.getValue() = true))
         )
         or
-        exists(PatternMatchExpr pme |
-          n1.isBefore(pme) and
-          n2.isBefore(pme.getExpr())
-          or
-          n1.isAfter(pme.getExpr()) and
-          n2.isIn(pme)
-          or
-          n1.isIn(pme) and
-          n2.isAfterValue(pme, any(BooleanSuccessor s | s.getValue() = false))
-          or
-          n1.isIn(pme) and
-          n2.isAdditional(pme, patternMatchTrueTag())
-          or
-          n1.isAdditional(pme, patternMatchTrueTag()) and
-          n2.isBefore(pme.getPattern())
-          or
-          n1.isAfter(pme.getPattern()) and
-          n2.isAfterValue(pme, any(BooleanSuccessor s | s.getValue() = true))
-        )
-        or
         exists(IfStmt ifstmt |
           n1.isBefore(ifstmt) and
           (