Skip to content

fix: track conditional reassignments for proper context propagation check#22

Merged
mpyw merged 1 commit intomainfrom
fix/conditional-reassignment-false-positive
Dec 26, 2025
Merged

fix: track conditional reassignments for proper context propagation check#22
mpyw merged 1 commit intomainfrom
fix/conditional-reassignment-false-positive

Conversation

@mpyw
Copy link
Owner

@mpyw mpyw commented Dec 26, 2025

Summary

  • Fixed false negative when a variable is conditionally reassigned with closures that don't all capture context
  • Added FuncLitAssignment struct to track whether an assignment occurs inside a control structure (if/for/switch/select)
  • Changed logic: now checks ALL reachable assignments (from the last unconditional assignment onwards)
  • ALL assignments in the checked range must capture context for the check to pass

Problem

Previously, code like this would not trigger a warning:

func bad(ctx context.Context) {
    fn := func() { _ = ctx }
    if condition {
        fn = func() { fmt.Println("no ctx") }  // Reassigned without context!
    }
    go fn()  // No warning - but should warn!
}

The analyzer only looked at the "last" assignment and would see the context-capturing one from the initial assignment if the reassignment wasn't the last in AST order.

Solution

  • Track ALL assignments with conditionality information using inspector.WithStack
  • Find the last unconditional assignment as the starting point
  • Check all assignments from that point onwards (including conditional ones)
  • ALL must capture context for the check to pass

Test plan

  • All existing tests pass
  • Added 3 new test cases for conditional reassignment patterns
    • badConditionalReassignFirstUsesCtx: First uses ctx, conditional doesn't → should warn
    • badConditionalReassignFirstNoCtx: First doesn't use ctx, conditional does → should warn
    • goodConditionalReassignAllUseCtx: All assignments use ctx → should NOT warn

🤖 Generated with Claude Code

…heck

Previously, FuncLitAssignedTo only returned the last assignment, missing
cases where conditional branches assign different closures to the same
variable. Now tracks ALL assignments with conditionality information.

The check now requires ALL reachable assignments (from the last
unconditional assignment onwards) to capture context, ensuring that no
execution path can bypass context propagation.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@codecov
Copy link

codecov bot commented Dec 26, 2025

Codecov Report

❌ Patch coverage is 75.86207% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.96%. Comparing base (4fb020b) to head (5c2a65e).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
internal/probe/assignment.go 54.71% 22 Missing and 2 partials ⚠️
internal/checkers/goroutine.go 95.83% 1 Missing and 1 partial ⚠️
internal/probe/capture.go 86.66% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #22      +/-   ##
==========================================
- Coverage   84.38%   83.96%   -0.43%     
==========================================
  Files          24       24              
  Lines        1710     1808      +98     
==========================================
+ Hits         1443     1518      +75     
- Misses        151      173      +22     
- Partials      116      117       +1     
Flag Coverage Δ
unittests 83.96% <75.86%> (-0.43%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mpyw mpyw merged commit 1069525 into main Dec 26, 2025
12 of 14 checks passed
@mpyw mpyw deleted the fix/conditional-reassignment-false-positive branch December 26, 2025 03:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant