ruby: refine query for uninitialised local variables

- there are places where uninitialised reads are intentional
- there are also some places where they are impossible

ruby/ql/src/queries/variables/UninitializedLocal.qhelp

@@ -0,0 +1,36 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+In Ruby, raw identifiers like <code>x</code> can be both local variable accesses and method calls. It is a local variable access iff it is syntactically preceded by something that binds it (like an assignment).
+Consider the following example:
+</p>
+
+<sample src="examples/UninitializedLocal.rb" />
+
+<p>
+This will generate an alert on the last access to <code>m</code>, where it is not clear that the programmer intended to read from the local variable.
+</p>
+
+</overview>
+<recommendation>
+
+<p>
+Ensure that you check the control and data flow in the method carefully.
+Check that the variable reference is spelled correctly, perhaps the variable has been renamed and the reference needs to be updated.
+Another possibility is that an exception may be raised before the variable is assigned, in which case the read should be protected by a check for <code>nil</code>.
+</p>
+
+</recommendation>
+
+<references>
+
+
+<li>Wikipedia: <a href="http://en.wikipedia.org/wiki/Dead_store">Dead store</a>.</li>
+
+
+
+</references>
+</qhelp>

ruby/ql/src/queries/variables/UninitializedLocal.ql

@@ -5,20 +5,71 @@
  * @kind problem
  * @problem.severity error
  * @id rb/uninitialized-local-variable
- * @tags reliability
+ * @tags quality
+ *       reliability
  *       correctness
- * @precision low
+ * @precision high
  */
 
 import codeql.ruby.AST
 import codeql.ruby.dataflow.SSA
+private import codeql.ruby.dataflow.internal.DataFlowPublic
+import codeql.ruby.controlflow.internal.Guards as Guards
+import codeql.ruby.controlflow.CfgNodes
+
+predicate isInBooleanContext(Expr e) {
+  e = any(ConditionalExpr c).getCondition()
+  or
+  e = any(ConditionalLoop l).getCondition()
+  or
+  e = any(LogicalAndExpr n).getAnOperand()
+  or
+  e = any(LogicalOrExpr n).getAnOperand()
+  or
+  e = any(NotExpr n).getOperand()
+}
+
+predicate isGuarded(LocalVariableReadAccess read) {
+  exists(AstCfgNode guard, boolean branch |
+    Guards::guardControlsBlock(guard, read.getAControlFlowNode().getBasicBlock(), branch)
+  |
+    // guard is `var`
+    guard.getAstNode() = read.getVariable().getAnAccess() and
+    branch = true
+    or
+    // guard is `!var`
+    guard.getAstNode().(NotExpr).getOperand() = read.getVariable().getAnAccess() and
+    branch = false
+    or
+    // guard is `var.nil?`
+    exists(MethodCall c | guard.getAstNode() = c |
+      c.getReceiver() = read.getVariable().getAnAccess() and
+      c.getMethodName() = "nil?"
+    ) and
+    branch = false
+    or
+    // guard is `!var.nil?`
+    exists(MethodCall c | guard.getAstNode().(NotExpr).getOperand() = c |
+      c.getReceiver() = read.getVariable().getAnAccess() and
+      c.getMethodName() = "nil?"
+    ) and
+    branch = true
+  )
+}
+
+predicate isNilChecked(LocalVariableReadAccess read) {
+  exists(MethodCall c | c.getReceiver() = read |
+    c.getMethodName() = "nil?"
+    or
+    c.isSafeNavigation()
+  )
+}
 
 class RelevantLocalVariableReadAccess extends LocalVariableReadAccess {
   RelevantLocalVariableReadAccess() {
-    not exists(MethodCall c |
-      c.getReceiver() = this and
-      c.getMethodName() = "nil?"
-    )
+    not isInBooleanContext(this) and
+    not isNilChecked(this) and
+    not isGuarded(this)
   }
 }
 

ruby/ql/src/queries/variables/examples/UninitializedLocal.rb

@@ -0,0 +1,14 @@
+def m
+  puts "m"
+end
+
+def foo
+  m # calls m above
+  if false
+      m = 0
+      m # reads local variable m
+  else
+  end
+  m # reads uninitialized local variable m, `nil`
+  m2 # undefined local variable or method 'm2' for main (NameError)
+end