fix(instrument): SelectorExpr in IncDecStmt now instrumented (#30)

CHANGELOG.md

@@ -5,6 +5,56 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## [0.8.3] - 2025-12-19
+
+### Fixed
+
+**Issue #30: Struct Field Access via IncDecStmt Not Instrumented**
+
+Critical bug found by @thepudds where struct field access like `s.x++` was not being instrumented:
+
+```go
+func update(s *S) {
+    s.x++  // This was NOT instrumented!
+}
+```
+
+**Root cause:** In `visitSelectorInModifyContext()`, we recorded `Node: expr` (SelectorExpr), but `findParentStatement()` then returned the outermost containing statement (BlockStmt) instead of the IncDecStmt. This caused the instrumentation lookup to fail.
+
+**Changes:**
+- Modified `visitSelectorInModifyContext()` to accept parent statement parameter
+- Use parent statement as `Node` so `findParentStatement()` returns correct statement
+- Added `TestInstrumentFile_SelectorExprIncDec` test case
+- Added `TestInstrumentFile_SelectorExprInGoroutine` for full reproduction
+
+**Before fix:**
+```go
+func update(s *S) {
+    s.x++  // No race detection!
+}
+```
+
+**After fix:**
+```go
+func update(s *S) {
+    race.RaceRead(uintptr(unsafe.Pointer(&s.x)))
+    race.RaceWrite(uintptr(unsafe.Pointer(&s.x)))
+    s.x++
+}
+```
+
+### Known Limitations
+
+**Compiler Directives May Be Misplaced**
+
+When instrumenting files with compiler directives like `//go:noinline`, the directive may be incorrectly moved to the import block. This is a bug in the import injection code that will be addressed in a future release. See Issue #31.
+
+### Acknowledgments
+
+Thanks to @thepudds for finding this critical bug and providing the reproduction case in Issue #27 comments.
+
+---
+
 ## [0.8.2] - 2025-12-19
 
 ### Fixed

ROADMAP.md

@@ -3,7 +3,7 @@
 > **Strategic Advantage**: Proven FastTrack algorithm implementation without CGO dependency!
 > **Approach**: Scientific algorithm + Go best practices - eliminates C++ ThreadSanitizer dependency
 
-**Last Updated**: 2025-12-19 | **Current Version**: v0.8.0 (STABLE) | **Strategy**: MVP → Optimization + Hardening → Advanced Optimizations → Assembly GID → Test Suite → Escape Analysis → Runtime Integration → Go Proposal | **Milestone**: v0.8.0 (Escape Analysis) → v0.9.0 (Polish) → v1.0.0 (Q1 2026)
+**Last Updated**: 2025-12-19 | **Current Version**: v0.8.3 (STABLE) | **Strategy**: MVP → Optimization + Hardening → Advanced Optimizations → Assembly GID → Test Suite → Escape Analysis → Community Bug Fixes → Runtime Integration → Go Proposal | **Milestone**: v0.8.3 (Bug Fixes) → v0.9.0 (Polish) → v1.0.0 (Q1 2026)
 
 ---
 
@@ -58,6 +58,12 @@ v0.7.0-v0.7.2 (Stack Traces & Hotfixes) ✅ RELEASED 2025-12-18/19
          ↓ (Performance optimizations, false positive fixes)
 v0.8.0 (Escape Analysis) ✅ RELEASED 2025-12-19
          ↓ (30-50% fewer false positives, toolexec command!)
+v0.8.1 (Escape Analysis Fixes) ✅ RELEASED 2025-12-19
+         ↓ (Minor fixes, path normalization)
+v0.8.2 (Issue #27: *ptr++ fix) ✅ RELEASED 2025-12-19
+         ↓ (Pointer dereference instrumentation, by @thepudds)
+v0.8.3 (Issue #30: s.x++ fix) ✅ RELEASED 2025-12-19
+         ↓ (Struct field access instrumentation, by @thepudds)
 v0.9.0 (Polish & Stabilization) → Final polish before v1.0
          ↓ (1-2 weeks)
 v1.0.0 LTS → Production-ready with Go community adoption (Q1 2026)
@@ -152,11 +158,11 @@ v1.0.0 LTS → Production-ready with Go community adoption (Q1 2026)
 
 ---
 
-## 📊 Current Status (v0.8.0 Stable)
+## 📊 Current Status (v0.8.3 Stable)
 
-**Phase**: ✅ Escape Analysis Integration Complete!
-**Detector**: Production-grade with 30-50% fewer false positives! ⚡
-**AST Instrumentation**: Complete with escape analysis! ✅
+**Phase**: ✅ Community Bug Fixes Complete!
+**Detector**: Production-grade with critical bug fixes from @thepudds! ⚡
+**AST Instrumentation**: Complete with escape analysis and struct field support! ✅
 
 **What Works**:
 - ✅ `racedetector build` command (drop-in for `go build`)
@@ -247,6 +253,64 @@ Previous Write at 0xc00000a0b8 by goroutine 3:
 
 ---
 
+### **v0.8.2 - Pointer Dereference Fix** (December 2025) [RELEASED! ✅]
+
+**Goal**: Fix Issue #27 - pointer dereferences like `*ptr++` not instrumented
+
+**Duration**: Same day (December 19, 2025)
+
+**Status**: ✅ RELEASED
+
+**Root Cause**: Go parser uses `*ast.StarExpr` for pointer dereference in `IncDecStmt`, but we only checked `*ast.UnaryExpr`.
+
+**Changes**:
+- ✅ Added `*ast.StarExpr` handling in `Visit()` switch
+- ✅ Added `visitStarExpr()` for pointer dereference instrumentation
+- ✅ Added `StarExpr` handling in `extractReads()` and `extractAddress()`
+- ✅ Test cases for Issue #27 reproduction
+
+**Credit**: Thanks to @thepudds for finding this critical bug!
+
+---
+
+### **v0.8.3 - Struct Field Access Fix** (December 2025) [RELEASED! ✅]
+
+**Goal**: Fix Issue #30 - struct field access like `s.x++` not instrumented
+
+**Duration**: Same day (December 19, 2025)
+
+**Status**: ✅ RELEASED
+
+**Root Cause**: `visitSelectorInModifyContext()` recorded `Node: expr` (SelectorExpr), but `findParentStatement()` returned outermost statement (BlockStmt) instead of IncDecStmt.
+
+**Changes**:
+- ✅ Modified `visitSelectorInModifyContext(parentStmt, expr)` to accept parent statement
+- ✅ Use parent statement as `Node` for correct instrumentation lookup
+- ✅ Added `TestInstrumentFile_SelectorExprIncDec` test
+- ✅ Added `TestInstrumentFile_SelectorExprInGoroutine` test
+
+**Before fix**:
+```go
+func update(s *S) {
+    s.x++  // No race detection!
+}
+```
+
+**After fix**:
+```go
+func update(s *S) {
+    race.RaceRead(uintptr(unsafe.Pointer(&s.x)))
+    race.RaceWrite(uintptr(unsafe.Pointer(&s.x)))
+    s.x++
+}
+```
+
+**Known Limitation**: Compiler directives (`//go:noinline`) may be misplaced during instrumentation. See Issue #31.
+
+**Credit**: Thanks to @thepudds for finding this critical bug!
+
+---
+
 ### **v0.9.0 - Polish & Stabilization** (January 2026) [PLANNED]
 
 **Goal**: Final polish before v1.0.0 LTS release
@@ -452,5 +516,5 @@ Previous Write at 0xc00000a0b8 by goroutine 3:
 
 ---
 
-*Version 1.5 (Updated 2025-12-19)*
-*Current: v0.8.0 (STABLE - Escape Analysis) | Next: v0.9.0 (Polish) | Target: v1.0.0 LTS (Q1 2026)*
+*Version 1.6 (Updated 2025-12-19)*
+*Current: v0.8.3 (STABLE - Community Bug Fixes) | Next: v0.9.0 (Polish) | Target: v1.0.0 LTS (Q1 2026)*

cmd/racedetector/instrument/instrument_test.go

@@ -1043,3 +1043,86 @@ func main() {
 	t.Logf("Instrumented output:\n%s", result.Code)
 	t.Logf("Stats: reads=%d, writes=%d (must be > 0 for *ptr++)", result.Stats.ReadsInstrumented, result.Stats.WritesInstrumented)
 }
+
+// TestInstrumentFile_SelectorExprIncDec tests that struct field access via IncDecStmt is instrumented.
+// This is the fix for Issue #30: s.x++ was not instrumented because we skip all SelectorExpr.
+// In IncDecStmt context, we KNOW the selector is addressable (otherwise Go won't compile).
+func TestInstrumentFile_SelectorExprIncDec(t *testing.T) {
+	input := `package main
+
+type S struct {
+	x int
+}
+
+func update(s *S) {
+	s.x++
+}
+`
+	result, err := InstrumentFile("test.go", input)
+	if err != nil {
+		t.Fatalf("InstrumentFile failed: %v", err)
+	}
+
+	// Must have instrumentation for s.x++
+	// s.x++ is both READ and WRITE
+	if result.Stats.ReadsInstrumented < 1 {
+		t.Errorf("Stats.ReadsInstrumented = %d, want >= 1 for s.x++", result.Stats.ReadsInstrumented)
+	}
+	if result.Stats.WritesInstrumented < 1 {
+		t.Errorf("Stats.WritesInstrumented = %d, want >= 1 for s.x++", result.Stats.WritesInstrumented)
+	}
+
+	// Verify the instrumented code contains race calls for the field
+	if !strings.Contains(result.Code, "race.RaceRead") {
+		t.Error("Instrumented code missing race.RaceRead for s.x++")
+	}
+	if !strings.Contains(result.Code, "race.RaceWrite") {
+		t.Error("Instrumented code missing race.RaceWrite for s.x++")
+	}
+
+	t.Logf("Instrumented output:\n%s", result.Code)
+	t.Logf("Stats: reads=%d, writes=%d", result.Stats.ReadsInstrumented, result.Stats.WritesInstrumented)
+}
+
+// TestInstrumentFile_SelectorExprInGoroutine tests @thepudds Issue #30 reproduction case.
+func TestInstrumentFile_SelectorExprInGoroutine(t *testing.T) {
+	input := `package main
+
+import "sync"
+
+type S struct {
+	x int
+}
+
+func update(s *S) {
+	s.x++
+}
+
+func main() {
+	var shared S
+	var wg sync.WaitGroup
+	wg.Add(2)
+	go func() {
+		defer wg.Done()
+		update(&shared)
+	}()
+	go func() {
+		defer wg.Done()
+		update(&shared)
+	}()
+	wg.Wait()
+}
+`
+	result, err := InstrumentFile("test.go", input)
+	if err != nil {
+		t.Fatalf("InstrumentFile failed: %v", err)
+	}
+
+	// Critical: s.x++ must be instrumented
+	if result.Stats.ReadsInstrumented == 0 && result.Stats.WritesInstrumented == 0 {
+		t.Fatalf("CRITICAL: No instrumentation for s.x++! Issue #30 not fixed.")
+	}
+
+	t.Logf("Instrumented output:\n%s", result.Code)
+	t.Logf("Stats: reads=%d, writes=%d", result.Stats.ReadsInstrumented, result.Stats.WritesInstrumented)
+}

cmd/racedetector/instrument/visitor.go

@@ -470,6 +470,16 @@ func (v *instrumentVisitor) visitAssignment(stmt *ast.AssignStmt) {
 // Parameters:
 //   - stmt: IncDecStmt node
 func (v *instrumentVisitor) visitIncDec(stmt *ast.IncDecStmt) {
+	// v0.8.3: Fix for Issue #30 - handle SelectorExpr specially in IncDecStmt
+	// In IncDecStmt context, we KNOW the operand is addressable (otherwise Go won't compile)
+	// So we can safely instrument SelectorExpr here even though shouldInstrument skips it
+	if sel, ok := stmt.X.(*ast.SelectorExpr); ok {
+		// s.x++ - struct field access, must instrument
+		// Pass stmt as parent so findParentStatement works correctly
+		v.visitSelectorInModifyContext(stmt, sel)
+		return
+	}
+
 	// Skip if this expression shouldn't be instrumented
 	if !shouldInstrument(stmt.X) {
 		v.trackSkipped(stmt.X)
@@ -992,6 +1002,54 @@ func (v *instrumentVisitor) visitStarExpr(expr *ast.StarExpr) {
 	v.stats.ReadsInstrumented++
 }
 
+// visitSelectorInModifyContext handles struct field access in modify contexts (IncDecStmt, AssignStmt LHS).
+//
+// v0.8.3: Fix for Issue #30 - in modify contexts we KNOW the selector is addressable
+// (otherwise Go wouldn't compile), so we can safely instrument it.
+//
+// Examples:
+//
+//	s.x++         → RaceRead(&s.x), RaceWrite(&s.x)
+//	s.field = 42  → RaceWrite(&s.field)
+//
+// Parameters:
+//   - parentStmt: The parent statement (IncDecStmt or AssignStmt) - used as Node for instrumentation
+//   - expr: SelectorExpr being modified
+//
+// IMPORTANT: We use parentStmt as Node (not expr) because findParentStatement()
+// needs a Stmt to work correctly. If we pass SelectorExpr, findParentStatement
+// returns the outermost containing statement (BlockStmt), not the IncDecStmt.
+func (v *instrumentVisitor) visitSelectorInModifyContext(parentStmt ast.Stmt, expr *ast.SelectorExpr) {
+	// For s.x, we need to take address of the field: &s.x
+	// Create UnaryExpr with & operator wrapping the SelectorExpr
+	addrExpr := &ast.UnaryExpr{
+		Op: token.AND,
+		X:  expr,
+	}
+
+	// Record READ (value is read before modification in ++ case)
+	// Use parentStmt as Node so findParentStatement returns correct statement
+	v.instrumentationPoints = append(v.instrumentationPoints, InstrumentPoint{
+		Node:       parentStmt,
+		AccessType: AccessRead,
+		Addr:       addrExpr,
+	})
+	v.stats.ReadsInstrumented++
+
+	// Record WRITE (new value is written back)
+	// Create a separate address expression to avoid AST sharing
+	addrExprWrite := &ast.UnaryExpr{
+		Op: token.AND,
+		X:  expr,
+	}
+	v.instrumentationPoints = append(v.instrumentationPoints, InstrumentPoint{
+		Node:       parentStmt,
+		AccessType: AccessWrite,
+		Addr:       addrExprWrite,
+	})
+	v.stats.WritesInstrumented++
+}
+
 // visitIndexAccess handles array/slice accesses: arr[0], slice[i].
 //
 // Similar to dereferences, index accesses can be reads or writes: