ruby: test rb/uninitialized-local-variable
#19247
Conversation
It finds none of the right things and all of the wrong things...
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new Ruby test file aimed at verifying the behavior of uninitialized local variable detection. Key changes include the addition of multiple test cases with alert annotations to check for both missing and spurious alerts in various contexts (basic usage, conditional constructs, assignments, and rescue/ensure blocks).
Files not reviewed (2)
- ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.expected: Language not supported
- ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.qlref: Language not supported
| @@ -0,0 +1,49 @@ | |||
| def test_basic | |||
| puts x #$ MISSING: Alert | |||
There was a problem hiding this comment.
Uninitialized variable 'x' is used without prior assignment, yet the expected alert is not triggered. Please verify that the analyzer correctly flags such cases.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
| end | ||
|
|
||
| def test_nested_condition | ||
| if (x = 4 || x) #$ SPURIOUS: Alert |
There was a problem hiding this comment.
The alert on this conditional assignment appears to be spurious since assignment within the condition should not be flagged. Consider updating the analyzer to ignore such valid patterns.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
| end | ||
|
|
||
| def test_conditional_assignment | ||
| if false |
There was a problem hiding this comment.
The use of variable 'i' in the assignment should trigger an alert for uninitialized usage, but no alert is generated. Verify that the analyzer processes assignments in unreachable branches correctly.
| if false | |
| if false | |
| i = 0 |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
| if false | ||
| status = i #$ MISSING: Alert | ||
| end | ||
| puts status #$ SPURIOUS: Alert |
There was a problem hiding this comment.
The alert on accessing 'status' appears to be spurious as it is derived from a branch where it was assigned (even if conditionally). Review the analyzer's logic for conditional assignments.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
| status = i #$ MISSING: Alert | ||
| end | ||
| puts status #$ SPURIOUS: Alert | ||
| puts i #$ MISSING: Alert |
There was a problem hiding this comment.
Uninitialized variable 'i' is used but the expected alert is missing. Ensure that the analyzer flags such usage consistently.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
| rescue SyntaxError | ||
| puts "rescue" | ||
| puts a #$ SPURIOUS: Alert | ||
| puts b #$ SPURIOUS: Alert |
There was a problem hiding this comment.
The alert on variable 'b' within the rescue block seems incorrect because its assignment might already be handled; review the analyzer's handling of variable scope in rescue clauses.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
| puts "rescue" | ||
| puts a #$ SPURIOUS: Alert | ||
| puts b #$ SPURIOUS: Alert | ||
| puts c #$ MISSING: Alert |
There was a problem hiding this comment.
Uninitialized variable 'c' is used in the rescue block but the alert is missing. Ensure that the analyzer reports such variables in all contexts consistently.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
| puts "rescue end" | ||
| ensure | ||
| puts "ensure" | ||
| puts a #$ SPURIOUS: Alert |
There was a problem hiding this comment.
In the ensure block, the alert on variable 'a' appears spurious since it remains available from earlier assignments. Please re-check the analyzer's behavior regarding variable availability in ensure blocks.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
| ensure | ||
| puts "ensure" | ||
| puts a #$ SPURIOUS: Alert | ||
| puts b #$ SPURIOUS: Alert |
There was a problem hiding this comment.
The alert triggered on 'b' in the ensure block seems unwarranted given its context; consider revising the conditions that lead to this warning.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
| puts "ensure" | ||
| puts a #$ SPURIOUS: Alert | ||
| puts b #$ SPURIOUS: Alert | ||
| puts c #$ MISSING: Alert |
There was a problem hiding this comment.
The uninitialized variable 'c' in the ensure block is not flagged as expected. Verify that the analyzer consistently reports uninitialized variables across rescue and ensure blocks.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
The intention behind this query probably deserves a bit of explanation. In Ruby, raw identifiers like 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)
endThe query is meant to find instances of the last access to |
|
Thanks for the explanation! In that case I will proceed with my branch where I filter out a number of reads that the programmer clearly expect could be nil. |
Indeed, I tried to write that query. While it solved the test cases perfectly, it was extremely noisy on MRVA. |
It finds none of the right things and all of the wrong things...