sqf: Inspector reduce false positives inside loops #1169
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR reduces false positives in the SQF inspector for variables that are initialized within loop iterations. The main approach is to run loop bodies twice: first with error suppression enabled to allow variables to be assigned, then a second time with normal error checking to catch genuine issues.
Changes:
- Added lifetime parameter to
Inspectorstruct and storedDatabaseas a field to simplify passing it through method calls - Implemented loop detection and double-execution logic with error suppression for the first run
- Added test cases demonstrating safe vs unsafe usage patterns in loops
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| libs/sqf/src/analyze/inspector/mod.rs | Added errors_suppressed tracking to ScriptScope, stored Database as field, modified stack_push to accept test_run parameter, implemented error_insert method with suppression logic |
| libs/sqf/src/analyze/inspector/commands.rs | Added is_loop parameter to loop-related commands, implemented double-execution logic in cmd_generic_call and cmd_b_do, removed database parameters |
| libs/sqf/src/analyze/inspector/external_functions.rs | Removed database parameters from method signatures, updated to use self.database |
| libs/sqf/tests/inspector/test_iteration.sqf | Added test cases for safe variable usage within loops (lines 28-62) |
| libs/sqf/tests/snapshots/inspector__tests__iteration.snap | Updated snapshot to reflect new test case with expected error |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub fn error_insert(&mut self, issue: Issue) { | ||
| // skip if errors are suppressed in any parent scope | ||
| if self | ||
| .active_scope() | ||
| .errors_suppressed | ||
| .iter() | ||
| .rev() | ||
| .skip(1) | ||
| .all(|s| !*s) | ||
| { | ||
| self.errors.insert(issue); | ||
| } | ||
| } |
There was a problem hiding this comment.
The skip(1) in the error suppression check skips the current stack frame, which means errors won't be suppressed in the immediate loop body where test_run is true. The logic should check all frames including the current one. Remove .skip(1) so that if self.active_scope().errors_suppressed.iter().rev().all(|s| !*s) checks all frames.
| if is_loop { | ||
| // repeat the loop, but with error suppression off | ||
| return self.cmd_generic_call(code_possibilities, magic_opt, false); | ||
| } |
There was a problem hiding this comment.
The recursive call to cmd_generic_call will re-execute the loop body, but the variable assignments from the first run remain in the stack. This means variables assigned in the first run (like _oldLoc becoming _locNew) will still be set during the second run, which defeats the purpose of detecting uninitialized variable errors. The variable state should be restored or the stack should be reset between runs.
| if is_loop { | ||
| // repeat the loop, but with error suppression off | ||
| return self.cmd_b_do(lhs, rhs, false); | ||
| } |
There was a problem hiding this comment.
Similar to the issue in cmd_generic_call, this recursive call to cmd_b_do will re-execute the loop body with variable assignments from the first run still present in the stack, which may hide errors that should be detected. The variable state should be managed between the two runs.
| pub fn stack_push( | ||
| &mut self, | ||
| expression_opt: Option<&Expression>, | ||
| test_run: bool, |
There was a problem hiding this comment.
The parameter name test_run is ambiguous. Consider renaming it to suppress_errors or is_test_iteration to more clearly indicate its purpose of suppressing error reporting during the first iteration of loop analysis.
ref #1168
Databasearound