No CFG nodes for subexprs of const exprs

In Go, sub-expressions of a constant expression are folded at compile time and never evaluated at runtime, so they shouldn't get evaluation nodes.

Author: owen-mc

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

@@ -30,6 +30,21 @@ module CfgImpl {
     Input::implicitFieldSelection(e, index, implicitField)
   }
 
+  /**
+   * Holds if `root` is a constant root: a constant expression (with any
+   * enclosing parentheses stripped) whose parent expression is not itself
+   * constant. The strict sub-expressions of a constant root are folded at
+   * compile time and are not evaluated at run time, so they get no evaluation
+   * node; the constant root itself is evaluated as a single leaf value.
+   */
+  private predicate constantRoot(Go::Expr root) {
+    exists(Go::Expr c |
+      c.isConst() and
+      not c.getParent().(Go::Expr).isConst() and
+      root = c.stripParens()
+    )
+  }
+
   /** Provides an implementation of the AST signature for Go. */
   private module Ast implements CfgLib::AstSig<Go::Location> {
     class AstNode = Go::AstNode;
@@ -78,6 +93,10 @@ module CfgImpl {
       // the switch expression (see `Switch.getExpr`), so the wrapping
       // statement must not introduce its own assignment or expression nodes.
       e = any(Go::TypeSwitchStmt ts).getTest()
+      or
+      // The strict sub-expressions of a constant expression are not evaluated
+      // at run time, so they must not get their own evaluation nodes.
+      constantRoot(e.(Go::Expr).getParent+())
     }
 
     AstNode getChild(AstNode n, int index) {
@@ -345,17 +364,29 @@ module CfgImpl {
 
     class BinaryExpr = Go::BinaryExpr;
 
-    class LogicalAndExpr = Go::LandExpr;
+    // Constant short-circuiting operators are folded at compile time and their
+    // operands are not evaluated at run time, so they are not treated as
+    // logical operators here (which would give their operands their own
+    // evaluation nodes via `getLeftOperand`/`getRightOperand`/`getOperand`,
+    // bypassing `skipCfg`). Instead they are handled as constant-root leaf
+    // value nodes (see `postOrInOrder`).
+    class LogicalAndExpr extends Go::LandExpr {
+      LogicalAndExpr() { not this.isConst() }
+    }
 
-    class LogicalOrExpr = Go::LorExpr;
+    class LogicalOrExpr extends Go::LorExpr {
+      LogicalOrExpr() { not this.isConst() }
+    }
 
     class NullCoalescingExpr extends BinaryExpr {
       NullCoalescingExpr() { none() }
     }
 
     class UnaryExpr = Go::UnaryExpr;
 
-    class LogicalNotExpr = Go::NotExpr;
+    class LogicalNotExpr extends Go::NotExpr {
+      LogicalNotExpr() { not this.isConst() }
+    }
 
     class BooleanLiteral extends Expr {
       boolean val;
@@ -473,6 +504,13 @@ module CfgImpl {
       // needs an explicit in-order (allocation) node.
       n instanceof Go::CompositeLit
       or
+      // A constant expression is folded at compile time and its sub-expressions
+      // are not evaluated (they are pruned by `skipCfg`), so the constant root
+      // has no CFG children. It therefore needs an explicit in-order node to
+      // remain a single value-producing leaf (e.g. `unsafe.Sizeof(test())`,
+      // `1 << 10`, or `!d` for constant `d`).
+      constantRoot(n)
+      or
       // Statements/declarations that compute a value or perform an operation and
       // are not among the statements the shared library makes post-order by
       // default.

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

@@ -243,12 +243,16 @@ module IR {
       // `defer-invoke` node that models the call at function exit.
       this.isAdditional(e, "defer-invoke")
       or
-      // `NotExpr` and `LogicalBinaryExpr` are not in `postOrInOrder`, so they
-      // don't have an `isIn` node. Only use the after-node when the
-      // expression is not in a conditional context; otherwise the value is
-      // split across `TAfterValueNode`s per branch and should not be exposed
-      // as a single value-producing instruction.
+      // Non-constant `NotExpr` and `LogicalBinaryExpr` are not in
+      // `postOrInOrder`, so they don't have an `isIn` node; their value is
+      // produced by the after-node. (Constant ones are folded and get a leaf
+      // `isIn` node via `constRoot`, handled by the first disjunct above, so
+      // they are excluded here to avoid a duplicate value node.) Only use the
+      // after-node when the expression is not in a conditional context;
+      // otherwise the value is split across `TAfterValueNode`s per branch and
+      // should not be exposed as a single value-producing instruction.
       (e instanceof NotExpr or e instanceof LogicalBinaryExpr) and
+      not e.isConst() and
       not isInBooleanCondContext(e) and
       this.isAfter(e)
     }

go/ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph/CONSISTENCY/CfgConsistency.expected

@@ -1,23 +1,13 @@
 consistencyOverview
 | deadEnd | 1 |
-| multipleSuccessors | 20 |
+| multipleSuccessors | 10 |
 deadEnd
 | stmts.go:61:2:61:10 | select statement |
 multipleSuccessors
-| epilogues.go:61:2:61:38 | call to Println | successor | epilogues.go:60:8:60:33 | defer-invoke call to log |
-| epilogues.go:61:2:61:38 | call to Println | successor | epilogues.go:61:2:61:38 | After call to Println |
-| epilogues.go:70:2:70:20 | call to Println | successor | epilogues.go:69:8:69:25 | defer-invoke call to log |
-| epilogues.go:70:2:70:20 | call to Println | successor | epilogues.go:70:2:70:20 | After call to Println |
-| epilogues.go:81:2:81:23 | call to Println | successor | epilogues.go:78:8:80:15 | defer-invoke function call |
-| epilogues.go:81:2:81:23 | call to Println | successor | epilogues.go:81:2:81:23 | After call to Println |
 | stmts2.go:16:2:26:2 | select statement | successor | stmts2.go:17:2:17:15 | comm clause |
 | stmts2.go:16:2:26:2 | select statement | successor | stmts2.go:18:2:19:10 | comm clause |
 | stmts2.go:16:2:26:2 | select statement | successor | stmts2.go:20:2:24:10 | comm clause |
 | stmts2.go:16:2:26:2 | select statement | successor | stmts2.go:25:2:25:18 | comm clause |
-| stmts7.go:29:2:29:31 | call to Println | successor | stmts7.go:28:8:28:23 | defer-invoke call to fn |
-| stmts7.go:29:2:29:31 | call to Println | successor | stmts7.go:29:2:29:31 | After call to Println |
-| stmts7.go:54:6:54:18 | index expression | successor | stmts7.go:53:8:53:21 | defer-invoke call to recoverPanic |
-| stmts7.go:54:6:54:18 | index expression | successor | stmts7.go:54:6:54:18 | After index expression |
 | stmts.go:50:2:59:2 | select statement | successor | stmts.go:51:2:52:31 | comm clause |
 | stmts.go:50:2:59:2 | select statement | successor | stmts.go:53:2:55:16 | comm clause |
 | stmts.go:50:2:59:2 | select statement | successor | stmts.go:56:2:57:15 | comm clause |