Add handling for ok in hook.ReplaceConditional`
#311
Conversation
WalkthroughThis pull request modifies the Changes
Sequence Diagram(s)sequenceDiagram
participant P as Preprocessor
participant B as Block Nodes
participant A as Assignment Checker
P->>B: Retrieve last node
alt Last node is a call expression
B-->>P: Return call expression
else Last node is not a call expression
P->>B: Retrieve second-to-last node
B-->>P: Return assignment statement
P->>A: Compare LHS of assignment with last node identifier
alt Identifiers match
A-->>P: Extract call expression from RHS
P-->>B: Use extracted call expression
else
A-->>P: No valid call expression found
end
end
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
testdata/src/go.uber.org/trustedfunc/trustedfuncs.go (1)
1004-1034: Comprehensive test cases for short variable declarations.The new test cases effectively validate the implementation for different scenarios:
- Basic short assignment in if statement
- Short assignment with OR condition
- Short assignment with AND condition (both orderings)
These cover the key cases mentioned in the PR objectives, including identifying the limitation with the
dummy && okcase. The comment about the false positive in the last test case aligns with the acknowledged limitation in the PR summary.One minor note: there's a duplicate case name "short assignment in if statement with AND condition" on lines 1019 and 1026. While it doesn't affect functionality, it might be worth renaming one for clarity.
-case "short assignment in if statement with AND condition": +case "short assignment in if statement with reversed AND condition":
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
assertion/function/preprocess/cfg.go(1 hunks)testdata/src/go.uber.org/trustedfunc/trustedfuncs.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test (1.24.x)
- GitHub Check: Test (1.23.x)
- GitHub Check: Run
🔇 Additional comments (1)
assertion/function/preprocess/cfg.go (1)
189-220: Enhanced handling of short variable declarations in conditionals.The implementation now correctly handles cases where a variable is defined in the initialization part of an if statement (e.g.,
if ok := errors.As(...); ok { ... }). The approach is to check if the second-to-last node is an assignment whose left-hand side matches the identifier used in the condition.This change addresses the PR's primary objective and improves error handling scenarios that use this pattern.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #311 +/- ##
==========================================
+ Coverage 88.16% 88.18% +0.01%
==========================================
Files 67 67
Lines 10395 10411 +16
==========================================
+ Hits 9165 9181 +16
Misses 1051 1051
Partials 179 179 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
614479a to
320e87d
Compare
320e87d to
c35f9f6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
assertion/function/preprocess/cfg.go (1)
205-212: Consider prioritizing the SSA-based implementation in a future PR.The current approach has limitations with handling nested conditions like
if dummy && ok {...}whereokis defined in a short assignment. Your comment correctly identifies this limitation, but using SSA would provide a more robust solution.Using the SSA analyzer would allow tracking variable definitions more accurately, regardless of where they appear in the control flow, which would eliminate this class of false positives.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
assertion/function/preprocess/cfg.go(1 hunks)testdata/src/go.uber.org/trustedfunc/trustedfuncs.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test (1.24.x)
- GitHub Check: Test (1.23.x)
- GitHub Check: Run
🔇 Additional comments (6)
assertion/function/preprocess/cfg.go (3)
191-228: Good implementation for handling short assignments in if conditions.The implementation correctly handles scenarios like
if ok := errors.As(...); ok { ... }by checking if the last node is an identifier and then looking at the second-to-last node for an assignment statement. The detailed comments explain the approach and its limitations clearly.
213-226: Thorough validation of the assignment structure.The code correctly validates various aspects of the assignment statement: ensuring it's an actual assignment, has exactly one left-hand and right-hand expression, confirms the left side is an identifier, and verifies the identifier name matches the one in the condition.
230-230: Good null check placement.Moving the
call == nilcheck to after the switch statement is appropriate since the call could now be extracted from either a direct call expression or from an assignment.testdata/src/go.uber.org/trustedfunc/trustedfuncs.go (3)
1004-1010: Great test case for basic short assignment in if statement.This test case verifies the core functionality being added in this PR - handling
okvariable that's defined in a short assignment within an if statement.
1011-1017: Good test coverage for OR conditions.Testing the scenario with an OR condition is important as it confirms the expected behavior where the dereference is flagged as unsafe when using an OR with the result of errors.As.
1018-1024: Good test coverage for AND conditions with ok first.This test case correctly verifies that when
okis the first operand in an AND condition, the execution guarantees thatokis true before evaluating the second operand, which means variables guarded byerrors.Asare properly tracked.
| case "short assignment in if statement with AND condition (dummy && ok)": | ||
| var exitErr *exec.ExitError | ||
| var nilError error | ||
| if ok := errors.As(nilError, &exitErr); dummy && ok { | ||
| // The following is an FP: since the `dummy && ok` is canonicalized to `if dummy { if ok { } }`, | ||
| // our current handling fails to find the `ok := errors.As(...)` call at the end of the block. | ||
| print(*exitErr) //want "unassigned variable `exitErr` dereferenced" | ||
| print(ok) | ||
| } |
There was a problem hiding this comment.
Well-documented false positive case.
This test case clearly documents the current limitation where the implementation fails to track the ok variable when it's the second operand in an AND condition. The comment explaining why this is a false positive is very helpful.
The current implementation doesn't handle the case where ok is the second operand in an AND condition because the canonical form turns it into a nested if structure, making it harder to track the variable definition. This is a known limitation that would be addressed by the planned SSA-based implementation.
In our
hook.ReplaceConditionalwe have support for replacing a conditional with another since we know it has certain implications. E.g.,errors.As(..., &targetErr)actually meanserrors.As(..., &targetErr) && targetErr != nil.This PR adds support for cases where the value is stored in the init part of the if statement, e.g.,
if ok := errors.As(...); ok { ... }.Note that the handling is limited: we simply check the last node within the current block. This would lead to missing cases like
if ok := errors.As(...); dummy && ok { ... }since it would be canonicalized toif dummy { if ok { ... } }, and we won't find the definition ofokwithin the current block anymore (it would work ifokis actually placed at the beginning, e.g.,if ok && dummy {...}.This is weird, but it is the simplest way to implement such support for now (and we assume it would be a common case to place
okas the first component of the expression anyways).A more general handling to get the latest declaration with the help of SSA analyzer will be done in follow-up PR.