ruby: refine rb/uninitialized-local-variable

[yoff]

In preparation for shipping as a quality query, this PR removes some false positives for rb/uninitialized-local-variable:

• there are places where uninitialized reads are intentional (say in 'nil'-checks)
• there are also some places where they are impossible (e.g. places guarded by 'nil'-checks)

In addition, the query will now only alert if the uninitialized read is also the receiver of a method call.
This is to account for the common practice of producing 'nil' values and deal with them later.
The query now no longer tries to highlight that a 'nil' value may have been produced, but instead tries to highlight when it may not have been correctly dealt with.

The difference in alert counts can be seen here (once that run completes).

The first three rows are

Repository	Before	After	diff
github-github_no-test-dir	3915
artichoke__artichoke	2612
opal__opal	2328

Given the small number of alerts (along with local triage) the precision has been upped to 'high'.

[Copilot]

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

Files not reviewed (1)

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

Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more

[github-actions]

QHelp previews:

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

Potentially uninitialized local variable

In Ruby, raw identifiers like x 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:

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

This will generate an alert on the last access to m, where it is not clear that the programmer intended to read from the local variable. In fact, the last access to m is a method call, and the value of the local variable is nil, so this will raise a NoMethodError.

Recommendation

Ensure that you check the control and data flow in the method carefully. Add a check for nil before the read, or rewrite the code to ensure that the variable is always initialized before being read.

References

• https://www.rubyguides.com/: Nil.
• https://ruby-doc.org/: NoMethodError.

[hvitved]

The logic for Boolean operators seems rather complex to me. I wonder if what we are after is allowing for uninitialized accesses, when they happen in a Boolean context, and then also filter out accesses that are guarded by nil checks, such as the implicit nil check of a in a && a.safe.

[hvitved]

This actually parses as a = (3 && a), so the alert is correct. If you change it to (a = 3) && a the alert disappears.

[yoff]

Ah, that is right, the alert is correct.

[hvitved]

So the reason that this is OK is because nil is treated as false in a Boolean context?

[yoff]

Yes, this loop will break once a is not nil.

[yoff]

The logic for Boolean operators seems rather complex to me. I wonder if what we are after is allowing for uninitialized accesses, when they happen in a Boolean context, and then also filter out accesses that are guarded by nil checks, such as the implicit nil check of a in a && a.safe.

Yes, I think that is a good characterization. I will attempt to express it as two simple predicates in this vein.

[yoff]

Rewrote and rebased. Thanks for the push, it became much nicer.

[hvitved]

Thanks a lot for rewriting the query, I agree that it makes it much nicer.

[hvitved]

I assume this file should be deleted?

[hvitved]

This disjunct can be removed; the CFG automatically handles negation.

[yoff]

Ah, nice!

[hvitved]

This disjunct can be removed; the CFG automatically handles negation.

[yoff]

Again, nice! (That does deserve repeating...)

[hvitved]

This predicate has a lot of overlap with the inBooleanContext predicate from Completion.qll. However, that predicate cannot be used directly, but it highlights some missing cases. I suggest

private predicate inBooleanContext(AstNode n) {
  exists(ConditionalExpr i |
    n = i.getCondition()
    or
    inBooleanContext(i) and
    n = i.getBranch(_)
  )
  or
  n = any(ConditionalLoop parent).getCondition()
  or
  n = any(InClause parent).getCondition()
  or
  n = any(LogicalAndExpr n).getAnOperand()
  or
  n = any(LogicalOrExpr n).getAnOperand()
  or
  n = any(NotExpr n).getOperand()
  or
  n = any(StmtSequence parent | inBooleanContext(parent)).getLastStmt()
  or
  exists(CaseExpr c, WhenClause w |
    not exists(c.getValue()) and
    c.getABranch() = w
  |
    w.getPattern(_) = n
    or
    w = n
  )
}

[yoff]

Started experiment on suite of projects that actually contain alerts (generated with MRVA).

Requested changes have been addressed