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

@@ -33,6 +33,21 @@ module GoCfg {
     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 constRoot(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;
@@ -81,6 +96,10 @@ module GoCfg {
       // 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.
+      constRoot(e.(Go::Expr).getParent+())
     }
 
     AstNode getChild(AstNode n, int index) {
@@ -346,17 +365,29 @@ module GoCfg {
 
     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;
@@ -474,6 +505,13 @@ module GoCfg {
       // 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`).
+      constRoot(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

@@ -222,12 +222,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/dataflow/GlobalValueNumbering/GlobalValueNumber.expected

@@ -34,10 +34,8 @@
 | main.go:38:2:38:9 | SSA def(res) | main.go:38:9:38:9 | 9 |
 | main.go:38:9:38:9 | 9 | main.go:38:9:38:9 | 9 |
 | regressions.go:5:11:5:31 | call to Sizeof | regressions.go:5:11:5:31 | call to Sizeof |
-| regressions.go:5:25:5:30 | call to test | regressions.go:5:25:5:30 | call to test |
 | regressions.go:7:11:7:15 | false | regressions.go:7:11:7:15 | false |
-| regressions.go:9:11:9:12 | After !... | regressions.go:11:11:11:14 | true |
-| regressions.go:9:12:9:12 | d | regressions.go:7:11:7:15 | false |
+| regressions.go:9:11:9:12 | !... | regressions.go:11:11:11:14 | true |
 | regressions.go:11:11:11:14 | true | regressions.go:11:11:11:14 | true |
 | regressions.go:30:9:30:22 | call to getPayload | regressions.go:30:9:30:22 | call to getPayload |
 | regressions.go:30:26:30:39 | call to getPayload | regressions.go:30:26:30:39 | call to getPayload |