Merge pull request #19164 from yoff/ruby/refine-deadstore

ruby: remove some FPs from `rb/useless-assignment-to-local`

Author: yoff

Diff for: ruby/ql/src/change-notes/2025-04-04-refine-deadstore.md

@@ -0,0 +1,4 @@
+---
+category: majorAnalysis
+---
+* The query `rb/useless-assignment-to-local` now comes with query help and has been tweaked to produce fewer false positives.

Diff for: ruby/ql/src/queries/variables/DeadStoreOfLocal.qhelp

@@ -0,0 +1,49 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+A value is assigned to a local variable, but either that variable is never read
+later on, or its value is always overwritten before being read. This means
+that the original assignment has no effect, and could indicate a logic error or
+incomplete code.
+</p>
+
+</overview>
+<recommendation>
+
+<p>
+Ensure that you check the control and data flow in the method carefully.
+If a value is really not needed, consider omitting the assignment. Be careful,
+though: if the right-hand side has a side-effect (like performing a method call),
+it is important to keep this to preserve the overall behavior.
+</p>
+
+</recommendation>
+<example>
+
+<p>
+In the following example, the return value of the call to <code>send</code> on line 2
+is assigned to the local variable <code>result</code>, but then never used.
+</p>
+
+<sample src="examples/DeadStoreOfLocal.rb" />
+
+<p>
+Assuming that <code>send</code> returns a status code indicating whether the operation
+succeeded or not, the value of <code>result</code> should be checked, perhaps like this:
+</p>
+
+<sample src="examples/DeadStoreOfLocalGood.rb" />
+
+</example>
+<references>
+
+
+<li>Wikipedia: <a href="http://en.wikipedia.org/wiki/Dead_store">Dead store</a>.</li>
+
+
+
+</references>
+</qhelp>

Diff for: ruby/ql/src/queries/variables/DeadStoreOfLocal.ql

@@ -7,16 +7,25 @@
  * @id rb/useless-assignment-to-local
  * @tags maintainability
  *       external/cwe/cwe-563
- * @precision low
+ * @precision medium
  */
 
 import codeql.ruby.AST
 import codeql.ruby.dataflow.SSA
+import codeql.ruby.ApiGraphs
 
 class RelevantLocalVariableWriteAccess extends LocalVariableWriteAccess {
   RelevantLocalVariableWriteAccess() {
     not this.getVariable().getName().charAt(0) = "_" and
-    not this = any(Parameter p).getAVariable().getDefiningAccess()
+    not this = any(Parameter p).getAVariable().getDefiningAccess() and
+    not API::getTopLevelMember("ERB").getInstance().getAMethodCall("result").asExpr().getScope() =
+      this.getCfgScope() and
+    not exists(RetryStmt r | r.getCfgScope() = this.getCfgScope()) and
+    not exists(MethodCall c |
+      c.getReceiver() instanceof SelfVariableAccess and
+      c.getMethodName() = "binding" and
+      c.getCfgScope() = this.getCfgScope()
+    )
   }
 }
 

Diff for: ruby/ql/src/queries/variables/examples/DeadStoreOfLocal.rb

@@ -0,0 +1,5 @@
+def f(x)
+  result = send(x)
+  waitForResponse
+  return getResponse
+end

Diff for: ruby/ql/src/queries/variables/examples/DeadStoreOfLocalGood.rb

@@ -0,0 +1,9 @@
+def f(x)
+  result = send(x)
+	# check for error
+  if (result == -1)
+    raise "Unable to send, check network."
+  end
+  waitForResponse
+  return getResponse
+end

Diff for: ruby/ql/test/query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.expected

@@ -0,0 +1 @@
+| DeadStoreOfLocal.rb:2:5:2:5 | y | This assignment to $@ is useless, since its value is never read. | DeadStoreOfLocal.rb:2:5:2:5 | y | y |

Diff for: ruby/ql/test/query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.qlref

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

Diff for: ruby/ql/test/query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.rb

@@ -0,0 +1,36 @@
+def test_basic(x)
+    y = x  #$ Alert
+    y = x + 2
+    return y
+end
+
+def test_retry
+    x = 0
+    begin
+        if x == 0
+            raise "error"
+        end
+    rescue
+        x = 2  # OK - the retry will allow a later read
+        retry
+    end
+    return 42
+end
+
+def test_binding
+    x = 4  # OK - the binding collects the value of x
+    return binding
+end
+
+class Sup
+    def m(x)
+        print(x + 1)
+    end
+end
+  
+class Sub < Sup
+    def m(y)
+        y = 3  # OK - the call to `super` sees the value of `y``
+        super
+    end
+end

Diff for: ruby/ql/test/query-tests/variables/DeadStoreOfLocal/TestTemplate.rb

@@ -0,0 +1,10 @@
+# -*- coding: utf-8 -*-
+require 'erb'
+
+template = ERB.new <<EOF
+<%#-*- coding: Big5 -*-%>
+  \_\_ENCODING\_\_ is <%= \_\_ENCODING\_\_ %>.
+  x is <%= x %>.
+EOF
+x = 5  # OK - the template can see the value of x
+puts template.result