Merge pull request #19205 from yoff/ruby/refine-uninitialised-local

ruby: refine `rb/uninitialized-local-variable`

Author: yoff

ruby/ql/src/change-notes/2025-04-02-adjust-uninitialized-local-alert-message.md

@@ -0,0 +1,4 @@
+---
+category: majorAnalysis
+---
+* The query `rb/uninitialized-local-variable` now only produces alerts when the variable is the receiver of a method call and should produce very few false positives. It also now comes with a help file.

ruby/ql/src/codeql-suites/ruby-code-quality.qls

@@ -2,4 +2,5 @@
 - include:
     id:
       - rb/database-query-in-loop
-      - rb/useless-assignment-to-local
+      - rb/useless-assignment-to-local
+      - rb/uninitialized-local-variable

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

@@ -0,0 +1,41 @@
+# Method call on `nil`
+
+## Description
+In Ruby, it is not necessary to explicitly initialize variables.
+If a local variable has not been explicitly initialized, it will have the value `nil`. If this happens unintended, though, the variable will not represent an object with the expected methods, and a method call on the variable will raise a `NoMethodError`.
+
+## Recommendation
+
+Ensure that the variable cannot be `nil` at the point hightligted by the alert.
+This can be achieved by using a safe navigation or adding a check for `nil`.
+
+Note: You do not need to explicitly initialize the variable, if you can make the program deal with the possible `nil` value. In particular, initializing the variable to `nil` will have no effect, as this is already the value of the variable. If `nil` is the only possibly default value, you need to handle the `nil` value instead of initializing the variable.
+
+## Examples
+
+In the following code, the call to `create_file` may fail and then the call `f.close` will raise a `NoMethodError` since `f` will be `nil` at that point.
+
+```ruby
+def dump(x)
+  f = create_file
+  f.puts(x)
+ensure
+  f.close
+end
+```
+
+We can fix this by using safe navigation:
+```ruby
+def dump(x)
+  f = create_file
+  f.puts(x)
+ensure
+  f&.close
+end
+```
+
+## References
+
+- https://www.rubyguides.com/: [Nil](https://www.rubyguides.com/2018/01/ruby-nil/)
+- https://ruby-doc.org/: [NoMethodError](https://ruby-doc.org/core-2.6.5/NoMethodError.html)
+

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

@@ -5,20 +5,91 @@
  * @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
+import codeql.ruby.controlflow.internal.Guards as Guards
+import codeql.ruby.controlflow.CfgNodes
+import codeql.ruby.ast.internal.Variable
 
-class RelevantLocalVariableReadAccess extends LocalVariableReadAccess {
-  RelevantLocalVariableReadAccess() {
-    not exists(MethodCall c |
-      c.getReceiver() = this and
+private predicate isInBooleanContext(AstNode n) {
+  exists(ConditionalExpr i |
+    n = i.getCondition()
+    or
+    isInBooleanContext(i) and
+    n = i.getBranch(_)
+  )
+  or
+  n = any(ConditionalLoop parent).getCondition()
+  or
+  n = any(InClause parent).getCondition()
+  or
+  n = any(LogicalAndExpr op).getAnOperand()
+  or
+  n = any(LogicalOrExpr op).getAnOperand()
+  or
+  n = any(NotExpr op).getOperand()
+  or
+  n = any(StmtSequence parent | isInBooleanContext(parent)).getLastStmt()
+  or
+  exists(CaseExpr c, WhenClause w |
+    not exists(c.getValue()) and
+    c.getABranch() = w
+  |
+    w.getPattern(_) = n
+    or
+    w = n
+  )
+}
+
+private 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.nil?`
+    exists(MethodCall c | guard.getAstNode() = c |
+      c.getReceiver() = read.getVariable().getAnAccess() and
       c.getMethodName() = "nil?"
-    )
+    ) and
+    branch = false
+  )
+}
+
+private predicate isNilChecked(LocalVariableReadAccess read) {
+  exists(MethodCall c | c.getReceiver() = read |
+    c.getMethodName() = "nil?"
+    or
+    c.isSafeNavigation()
+  )
+}
+
+/**
+ * Holds if `name` is the name of a method defined on `nil`.
+ * See https://ruby-doc.org/core-2.5.8/NilClass.html
+ */
+private predicate isNilMethodName(string name) {
+  name in [
+      "inspect", "instance_of?", "is_a?", "kind_of?", "method", "nil?", "rationalize", "to_a",
+      "to_c", "to_f", "to_h", "to_i", "to_r", "to_s"
+    ]
+}
+
+class RelevantLocalVariableReadAccess extends LocalVariableReadAccess instanceof TVariableAccessReal
+{
+  RelevantLocalVariableReadAccess() {
+    not isInBooleanContext(this) and
+    not isNilChecked(this) and
+    not isGuarded(this) and
+    this = any(MethodCall m | not isNilMethodName(m.getMethodName())).getReceiver()
   }
 }
 

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.strip # reads uninitialized local variable m, `nil`, and crashes
+  m2 # undefined local variable or method 'm2' for main (NameError)
+end

ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.expected

@@ -0,0 +1,4 @@
+| UninitializedLocal.rb:12:3:12:3 | m | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:8:7:8:7 | m | m |
+| UninitializedLocal.rb:34:5:34:5 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:9:27:9 | b | b |
+| UninitializedLocal.rb:34:23:34:23 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:9:27:9 | b | b |
+| UninitializedLocal.rb:76:5:76:5 | i | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:73:9:73:9 | i | i |

ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.qlref

@@ -0,0 +1,2 @@
+query: queries/variables/UninitializedLocal.ql
+postprocess: utils/test/InlineExpectationsTestQuery.ql

ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.rb

@@ -0,0 +1,77 @@
+def m
+  puts "m"
+end
+
+def foo
+  m # calls m above
+  if false
+      m = "0"
+      m # reads local variable m
+  else
+  end
+  m.strip  #$ Alert
+  m2 # undefined local variable or method 'm2' for main (NameError)
+end
+
+def test_guards
+    if (a = "3" && a)  # OK - a is in a Boolean context
+        a.strip
+    end
+    if (a = "3") && a  # OK - a is assigned in the previous conjunct
+        a.strip
+    end
+    if !(a = "3") or a  # OK - a is assigned in the previous conjunct
+        a.strip
+    end
+    if false
+        b = "0"
+    end
+    b.nil?
+    b || 0  # OK
+    b&.strip  # OK - safe navigation
+    b.strip if b  # OK
+    b.close if b && !b.closed # OK
+    b.blowup if b || !b.blownup #$ Alert
+
+    if false
+        c = "0"
+    end
+    unless c
+        return
+    end
+    c.strip  # OK - given above unless
+
+    if false
+        d = "0"
+    end
+    if (d.nil?)
+        return
+    end
+    d.strip # OK - given above check
+
+    if false
+        e = "0"
+    end
+    unless (!e.nil?)
+        return
+    end
+    e.strip  # OK - given above unless
+end
+
+def test_loop
+    begin
+        if false
+            a = 0
+        else
+            set_a
+        end   
+    end until a  # OK
+    a.strip  # OK - given previous until
+end
+
+def test_for
+    for i in ["foo", "bar"]  # OK - since 0..10 cannot raise
+        puts i.strip
+    end
+    i.strip  #$ SPURIOUS: Alert
+end