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

[yoff]

In preparation for shipping rb/useless-assignment-to-local as a quality query, this PR removes three classes of FPs:

• a call to super without any parameters is calling the super method with all the parameters. These reads are currently not recognised by our SSA analysis, leading to false positives. This PR crudely filters them out.
• a call to result on an ERB template will grab all the local variables referenced in the template (example here). As an approximation, we do not report useless assignments in the vicinity of such calls.
• a reference to binding (as seen in the above example, but templates can also work without such a reference) will capture all the local variables in the scope. This means such assignments are useful even if not explicitly read. Our SSA analysis currently do not see these implicit reads. As an approximation, we do not report useless assignments in the vicinity of references to binding.
• if there is a retry statement, the assigned variable may be read when control is transferred to the block being rescued. Currently, the control flow graph is missing this edge, so the uses are not seen. As an approximation, we do not report useless assignments in the vicinity of retry statements. (I did implement this missing edge, but it had some performance implications, so I will postpone that solution.)

It may be interesting to make proper improvements to the SSA analysis and the CFG to handle these cases more accurately in the future.

[Copilot]

Copilot wasn't able to review any files in this pull request.

Files not reviewed (1)

• ruby/ql/src/queries/variables/DeadStoreOfLocal.ql: Language not supported

Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more

[hvitved]

• a call to super without any parameters is calling the super method with all the parameters. These reads are currently not recognised by our SSA analysis, leading to false positives. This PR crudely filters them out.

I have attempted to synthesize these arguments here.

[yoff]

• a call to super without any parameters is calling the super method with all the parameters. These reads are currently not recognised by our SSA analysis, leading to false positives. This PR crudely filters them out.

I have attempted to synthesize these arguments here.

Excellent! As soon as that lands, we can remove the filtering from this query.

[hvitved]

I think this PR ought to add some tests for the query.

[yoff]

I think this PR ought to add some tests for the query.

Agreed

[github-actions]

QHelp previews:

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

Useless assignment to local variable

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.

Recommendation

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.

Example

In the following example, the return value of the call to send on line 2 is assigned to the local variable result, but then never used.

def f(x)
  result = send(x)
  waitForResponse
  return getResponse
end

Assuming that send returns a status code indicating whether the operation succeeded or not, the value of result should be checked, perhaps like this:

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

References

• Wikipedia: Dead store.
• Common Weakness Enumeration: CWE-563.

[yoff]

I have rebased for a proper structure: Add tests - make improvements - see test results improve.