From d3e6727712ccb98e6c6e01df718c722f404aef50 Mon Sep 17 00:00:00 2001 From: Yvonne Devlin Date: Fri, 17 Jul 2026 08:17:30 +0000 Subject: [PATCH] refactor: decompose mapping.go's three highest-complexity functions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract 9 helpers from the three highest-complexity functions in internal/quality/mapping.go: - matchContainerUnwrap (50 → 8): collectTrackedVars, traceForwardDataFlow, matchTrackedInExpr - isTransformationCall (26 → 5): isByteLikeParam, isPointerDestParam, resolveCallSignature, findParamIndex - matchAssertionToEffect (25 → 5): matchDirect, matchIndirectRoot Combined complexity reduced from 101 to 18. +25 new unit tests using synthetic AST via parseAndTypeCheck. Filled 3 test gaps in isTransformationCall (io.Reader, empty interface, mixed parameter ordering). TestSC003_MappingAccuracy ratchet (85.0%) passes unchanged — no behavioral changes. Refactored parseAndTypeCheck to delegate to parseAndTypeCheckWithFset to eliminate DRY violation. Added comment documenting intentionally ignored type-check errors in test helper. Phase 2b of issue #166. Assisted-by: OpenCode (claude-opus-4-6) Signed-off-by: Yvonne Devlin --- ...pose-pr2b-20260716T100551-yvonne-devlin.md | 10 + ...pose-pr2b-20260716T100559-yvonne-devlin.md | 10 + AGENTS.md | 1 + .../quality/container_unwrap_internal_test.go | 653 +++++++++++++++++- internal/quality/mapping.go | 408 ++++++----- .../crapload-decompose-pr2b/.openspec.yaml | 2 + .../changes/crapload-decompose-pr2b/design.md | 193 ++++++ .../crapload-decompose-pr2b/proposal.md | 111 +++ .../specs/decompose-mapping.md | 230 ++++++ .../changes/crapload-decompose-pr2b/tasks.md | 182 +++++ 10 files changed, 1620 insertions(+), 180 deletions(-) create mode 100644 .uf/dewey/learnings/crapload-decompose-pr2b-20260716T100551-yvonne-devlin.md create mode 100644 .uf/dewey/learnings/crapload-decompose-pr2b-20260716T100559-yvonne-devlin.md create mode 100644 openspec/changes/crapload-decompose-pr2b/.openspec.yaml create mode 100644 openspec/changes/crapload-decompose-pr2b/design.md create mode 100644 openspec/changes/crapload-decompose-pr2b/proposal.md create mode 100644 openspec/changes/crapload-decompose-pr2b/specs/decompose-mapping.md create mode 100644 openspec/changes/crapload-decompose-pr2b/tasks.md diff --git a/.uf/dewey/learnings/crapload-decompose-pr2b-20260716T100551-yvonne-devlin.md b/.uf/dewey/learnings/crapload-decompose-pr2b-20260716T100551-yvonne-devlin.md new file mode 100644 index 0000000..0874bf3 --- /dev/null +++ b/.uf/dewey/learnings/crapload-decompose-pr2b-20260716T100551-yvonne-devlin.md @@ -0,0 +1,10 @@ +--- +tag: crapload-decompose-pr2b +author: yvonne-devlin +category: pattern +created_at: 2026-07-16T10:05:51Z +identity: crapload-decompose-pr2b-20260716T100551-yvonne-devlin +tier: draft +--- + +When decomposing complex AST-walking functions in Go (like matchContainerUnwrap at complexity 50), the key insight is identifying the phases: setup (collect initial state), process (iterate and transform), and match (check result). Each phase can be extracted as a separate function. However, the process phase often retains high complexity even after extraction because nested ast.Inspect closures, multi-iteration convergence loops, and transformation call detection involve inherent branching. traceForwardDataFlow landed at complexity 32 despite the design estimating ~12. The parent function (matchContainerUnwrap) still dropped from 50 to 8 because the complexity was moved, not eliminated. For future decomposition work on AST-heavy functions, expect the extracted "core loop" to retain ~60-70% of the original complexity — the wins come from isolating concerns, not from reducing total branch count. diff --git a/.uf/dewey/learnings/crapload-decompose-pr2b-20260716T100559-yvonne-devlin.md b/.uf/dewey/learnings/crapload-decompose-pr2b-20260716T100559-yvonne-devlin.md new file mode 100644 index 0000000..ed021e0 --- /dev/null +++ b/.uf/dewey/learnings/crapload-decompose-pr2b-20260716T100559-yvonne-devlin.md @@ -0,0 +1,10 @@ +--- +tag: crapload-decompose-pr2b +author: yvonne-devlin +category: pattern +created_at: 2026-07-16T10:05:59Z +identity: crapload-decompose-pr2b-20260716T100559-yvonne-devlin +tier: draft +--- + +The parseAndTypeCheck test helper pattern in internal/quality/container_unwrap_internal_test.go is highly effective for testing Go AST manipulation functions. It takes a Go source string, parses it, and type-checks it with Importer:nil (intentionally ignoring import resolution errors). This enables testing functions that operate on types.Object, types.Info, and ast.File without loading real packages. For functions that also need *packages.Package (like traceForwardDataFlow which needs Syntax for file iteration), construct a minimal struct: packages.Package{Syntax: []*ast.File{file}, TypesInfo: info, Fset: fset}. The parseAndTypeCheckWithFset variant was added in Phase 2b to return the fset needed for this pattern. One gotcha: for isByteLikeParam and isPointerDestParam tests, it's simpler to construct types.Type values directly using the go/types constructors (types.NewSlice, types.NewPointer, types.NewInterfaceType) rather than parsing Go source — the classifiers only need type metadata, not full AST context. diff --git a/AGENTS.md b/AGENTS.md index dce0e9c..dd15dd9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -484,6 +484,7 @@ Formatters: gofmt, goimports. ## Recent Changes +- crapload-decompose-pr2b: Decomposed three highest-complexity functions in `internal/quality/mapping.go`: `matchContainerUnwrap` (50→8), `isTransformationCall` (26→5), `matchAssertionToEffect` (25→5). Extracted 7+ helpers: `isByteLikeParam`, `isPointerDestParam`, `resolveCallSignature`, `findParamIndex` (from isTransformationCall); `matchDirect`, `matchIndirectRoot` (from matchAssertionToEffect); `collectTrackedVars`, `traceForwardDataFlow`, `matchTrackedInExpr` (from matchContainerUnwrap). Added 25 new unit tests using synthetic AST via `parseAndTypeCheck`. Filled 3 test gaps in `isTransformationCall` (io.Reader, empty interface, mixed ordering). `TestSC003_MappingAccuracy` ratchet (85.0%) passes unchanged. Phase 2b of issue #166. - extract-violation-helper: Extracted `isNewFunctionViolation` helper in `internal/crap/compare.go` to centralize the new-function violation check (CRAP threshold OR GazeCRAP threshold exceeded) that was duplicated at three sites: `buildComparisonSummary` (counting), `WriteComparisonJSON` (JSON status), and `writeComparisonNewFunctions` (text output). All three call sites now use the helper. Added `TestIsNewFunctionViolation` table-driven test with 6 cases covering the full truth table. No behavioral change — pure DRY extraction. Closes #179. - universal-taxonomy: Generalized the side effect taxonomy for multi-language support (Issue #96). Added 10 new universal `SideEffectType` constants for language-neutral effects: `ErrorSignal` (P0), `GeneratorYield`, `ContainerMutation`, `StreamOutput` (P1), `AsyncGeneratorYield`, `MetaprogrammingMutation`, `DescriptorEffect`, `ResourceManagement`, `ImportSideEffect`, `MonkeyPatch` (P2). Added 11 language-neutral aliases for Go-specific types (e.g., `AsyncTaskSpawn = GoroutineSpawn`, `FFICall = CgoCall`). Added `Detail map[string]any` field to `taxonomy.SideEffect` for opaque language-specific metadata passthrough from external analyzers. Added `ValidTypes` set and `IsKnownType` function to `internal/taxonomy/types.go` for programmatic type validation (replaces hardcoded `isKnownP4` switch in adapter). Added `Detail` field to `protocol.AnalyzedSideEffect` and wired passthrough in adapter's `convertAnalysisResults`. Updated JSON Schema enum with all new types and `detail` property. Updated `docs/protocol.md` with comprehensive 48-type reference table, bumped protocol version to 1.1.0. Updated `docs/porting/` guides with expanded type counts and tier tables. Total canonical types: 48 (38 existing + 10 new). - analyze-multi-package: Added multi-package support to `gaze analyze` and `gaze quality`. Both commands now accept multiple package patterns including `./...` wildcards. Changed Cobra constraints from `ExactArgs(1)` to `MinimumNArgs(1)`, added `loader.ResolvePackagePaths` and `loader.IsMainPkg` as exported utilities in `internal/loader/loader.go`, consolidated 3 duplicate copies each of `resolvePackagePaths`/`resolvePatterns` and `isMainPkg`/`isMainPackage` from `internal/crap/contract.go`, `internal/aireport/runner_steps.go`, and `cmd/gaze/main.go`. Updated `runAnalyze` and `runQuality` to resolve patterns then iterate per package. Added defense-in-depth warning to `loader.Load` when `len(pkgs) > 1`. Updated `runClassify` to accept pre-loaded module packages (avoids repeated `LoadModule` calls). Added `mergeSummaries` for quality report aggregation. Removed "Single package loading" known limitation from README. Closes #107. diff --git a/internal/quality/container_unwrap_internal_test.go b/internal/quality/container_unwrap_internal_test.go index 0a22b5e..802c5d5 100644 --- a/internal/quality/container_unwrap_internal_test.go +++ b/internal/quality/container_unwrap_internal_test.go @@ -6,6 +6,10 @@ import ( "go/token" "go/types" "testing" + + "golang.org/x/tools/go/packages" + + "github.com/unbound-force/gaze/internal/taxonomy" ) // parseAndTypeCheck parses Go source code and type-checks it, @@ -13,18 +17,7 @@ import ( // tests for the container unwrap helper functions. func parseAndTypeCheck(t *testing.T, src string) (*ast.File, *types.Info) { t.Helper() - fset := token.NewFileSet() - file, err := parser.ParseFile(fset, "test.go", src, 0) - if err != nil { - t.Fatalf("parse: %v", err) - } - info := &types.Info{ - Types: make(map[ast.Expr]types.TypeAndValue), - Defs: make(map[*ast.Ident]types.Object), - Uses: make(map[*ast.Ident]types.Object), - } - conf := types.Config{Importer: nil} - _, _ = conf.Check("p", fset, []*ast.File{file}, info) + file, info, _ := parseAndTypeCheckWithFset(t, src) return file, info } @@ -392,3 +385,639 @@ func TestIsTransformationCall_NilInputs(t *testing.T) { t.Error("isTransformationCall(nil, nil) should return ok=false") } } + +// TestIsTransformationCall_IoReaderAndPointer verifies that a function +// with an interface parameter that has a Read method (io.Reader-like) +// and a pointer parameter is detected as a transformation call. +func TestIsTransformationCall_IoReaderAndPointer(t *testing.T) { + // Define a local Reader interface with a Read method to avoid + // depending on the io package importer (parseAndTypeCheck uses + // Importer: nil). + src := `package p + +type Reader interface { Read(p []byte) (n int, err error) } +func decode(r Reader, dst *int) {} +func f() { var x int; decode(nil, &x) }` + file, info := parseAndTypeCheck(t, src) + call := extractCallFromFunc(t, file, 2, 1) // f(), stmt 1: decode(nil, &x) + + byteIdx, ptrIdx, ok := isTransformationCall(call, info) + if !ok { + t.Fatal("isTransformationCall should match func(io.Reader, *int)") + } + if byteIdx != 0 { + t.Errorf("byteArgIdx = %d, want 0", byteIdx) + } + if ptrIdx != 1 { + t.Errorf("ptrDestIdx = %d, want 1", ptrIdx) + } +} + +// TestIsTransformationCall_EmptyInterfaceAsPointerDest verifies that a +// function with []byte and interface{} parameters is detected as a +// transformation call, where the empty interface serves as the pointer +// destination (e.g., json.Unmarshal). +func TestIsTransformationCall_EmptyInterfaceAsPointerDest(t *testing.T) { + src := `package p + +func unmarshal(data []byte, v interface{}) {} +func f() { unmarshal(nil, nil) }` + file, info := parseAndTypeCheck(t, src) + call := extractCallFromFunc(t, file, 1, 0) // f(), stmt 0: unmarshal(nil, nil) + + byteIdx, ptrIdx, ok := isTransformationCall(call, info) + if !ok { + t.Fatal("isTransformationCall should match func([]byte, interface{})") + } + if byteIdx != 0 { + t.Errorf("byteArgIdx = %d, want 0", byteIdx) + } + if ptrIdx != 1 { + t.Errorf("ptrDestIdx = %d, want 1", ptrIdx) + } +} + +// TestIsTransformationCall_PointerBeforeByteSlice verifies that a +// function with *T before []byte is correctly detected — parameter +// ordering does not affect detection, and indices are correct. +func TestIsTransformationCall_PointerBeforeByteSlice(t *testing.T) { + src := `package p + +func decode(dst *int, data []byte) {} +func f() { var x int; decode(&x, nil) }` + file, info := parseAndTypeCheck(t, src) + call := extractCallFromFunc(t, file, 1, 1) // f(), stmt 1: decode(&x, nil) + + byteIdx, ptrIdx, ok := isTransformationCall(call, info) + if !ok { + t.Fatal("isTransformationCall should match func(*int, []byte) regardless of parameter order") + } + if ptrIdx != 0 { + t.Errorf("ptrDestIdx = %d, want 0 (pointer is first param)", ptrIdx) + } + if byteIdx != 1 { + t.Errorf("byteArgIdx = %d, want 1 ([]byte is second param)", byteIdx) + } +} + +// --- isByteLikeParam helper unit tests --- + +// TestIsByteLikeParam_ByteSlice verifies that []byte is recognized +// as a byte-like parameter type. +func TestIsByteLikeParam_ByteSlice(t *testing.T) { + typ := types.NewSlice(types.Typ[types.Byte]) + if !isByteLikeParam(typ) { + t.Error("isByteLikeParam should return true for []byte") + } +} + +// TestIsByteLikeParam_String verifies that string is recognized +// as a byte-like parameter type. +func TestIsByteLikeParam_String(t *testing.T) { + if !isByteLikeParam(types.Typ[types.String]) { + t.Error("isByteLikeParam should return true for string") + } +} + +// TestIsByteLikeParam_IoReader verifies that an interface with a +// Read method is recognized as a byte-like parameter type. +func TestIsByteLikeParam_IoReader(t *testing.T) { + // Construct an interface with a Read([]byte) (int, error) method + // to simulate io.Reader. + readSig := types.NewSignatureType( + nil, nil, nil, + types.NewTuple(types.NewVar(token.NoPos, nil, "p", types.NewSlice(types.Typ[types.Byte]))), + types.NewTuple( + types.NewVar(token.NoPos, nil, "n", types.Typ[types.Int]), + types.NewVar(token.NoPos, nil, "err", types.Universe.Lookup("error").Type()), + ), + false, + ) + readMethod := types.NewFunc(token.NoPos, nil, "Read", readSig) + iface := types.NewInterfaceType([]*types.Func{readMethod}, nil) + iface.Complete() + + if !isByteLikeParam(iface) { + t.Error("isByteLikeParam should return true for an interface with a Read method") + } +} + +// TestIsByteLikeParam_NonReadInterface verifies that an interface +// without a Read method is not recognized as a byte-like parameter. +func TestIsByteLikeParam_NonReadInterface(t *testing.T) { + writeSig := types.NewSignatureType( + nil, nil, nil, + types.NewTuple(types.NewVar(token.NoPos, nil, "p", types.NewSlice(types.Typ[types.Byte]))), + types.NewTuple( + types.NewVar(token.NoPos, nil, "n", types.Typ[types.Int]), + types.NewVar(token.NoPos, nil, "err", types.Universe.Lookup("error").Type()), + ), + false, + ) + writeMethod := types.NewFunc(token.NoPos, nil, "Write", writeSig) + iface := types.NewInterfaceType([]*types.Func{writeMethod}, nil) + iface.Complete() + + if isByteLikeParam(iface) { + t.Error("isByteLikeParam should return false for an interface with only a Write method (no Read)") + } +} + +// TestIsByteLikeParam_Int verifies that int is not recognized as a +// byte-like parameter type. +func TestIsByteLikeParam_Int(t *testing.T) { + if isByteLikeParam(types.Typ[types.Int]) { + t.Error("isByteLikeParam should return false for int") + } +} + +// --- isPointerDestParam helper unit tests --- + +// TestIsPointerDestParam_Pointer verifies that *int is recognized +// as a pointer destination parameter type. +func TestIsPointerDestParam_Pointer(t *testing.T) { + typ := types.NewPointer(types.Typ[types.Int]) + if !isPointerDestParam(typ) { + t.Error("isPointerDestParam should return true for *int") + } +} + +// TestIsPointerDestParam_EmptyInterface verifies that interface{} +// is recognized as a pointer destination parameter type. +func TestIsPointerDestParam_EmptyInterface(t *testing.T) { + iface := types.NewInterfaceType(nil, nil) + iface.Complete() + if !isPointerDestParam(iface) { + t.Error("isPointerDestParam should return true for interface{}") + } +} + +// TestIsPointerDestParam_Int verifies that int is not recognized +// as a pointer destination parameter type. +func TestIsPointerDestParam_Int(t *testing.T) { + if isPointerDestParam(types.Typ[types.Int]) { + t.Error("isPointerDestParam should return false for int") + } +} + +// --- matchDirect helper unit tests --- + +// TestMatchDirect_IdentityMatch verifies that matchDirect returns a +// mapping with confidence 75 when the assertion expression contains +// an identifier whose types.Object is in objToEffectID. +func TestMatchDirect_IdentityMatch(t *testing.T) { + src := `package p +func f() { + result := 42 + _ = result + 1 +}` + file, info := parseAndTypeCheck(t, src) + + // result is defined at stmt 0, used in stmt 1's RHS. + resultObj := extractVarObj(t, file, info, 0, 0) + rhs := extractRHS(t, file, 0, 1) + + effectID := "effect-return-1" + objToEffectID := map[types.Object]string{resultObj: effectID} + effectMap := map[string]*taxonomy.SideEffect{ + effectID: {ID: effectID, Type: taxonomy.ReturnValue}, + } + + site := AssertionSite{ + Location: "test.go:4", + Kind: AssertionKindStdlibComparison, + Expr: rhs, + } + + m := matchDirect(site, objToEffectID, effectMap, info, nil) + if m == nil { + t.Fatal("matchDirect should return a mapping for identity match") + } + if m.Confidence != 75 { + t.Errorf("confidence = %d, want 75", m.Confidence) + } + if m.SideEffectID != effectID { + t.Errorf("SideEffectID = %q, want %q", m.SideEffectID, effectID) + } +} + +// TestMatchDirect_HelperBridgeMatch verifies that matchDirect returns +// a mapping with confidence 70 when an identifier maps through +// helperBridge to a key in objToEffectID. +func TestMatchDirect_HelperBridgeMatch(t *testing.T) { + src := `package p +func f() { + got := 42 + want := 99 + _ = got + want +}` + file, info := parseAndTypeCheck(t, src) + + gotObj := extractVarObj(t, file, info, 0, 0) // got := 42 + wantObj := extractVarObj(t, file, info, 0, 1) // want := 99 + rhs := extractRHS(t, file, 0, 2) // got + want + + // Simulate: gotObj is a helper parameter, callerObj is the real + // test variable mapped to an effect. The helperBridge maps + // gotObj → wantObj, and wantObj is in objToEffectID. + effectID := "effect-return-1" + objToEffectID := map[types.Object]string{wantObj: effectID} + effectMap := map[string]*taxonomy.SideEffect{ + effectID: {ID: effectID, Type: taxonomy.ReturnValue}, + } + helperBridge := map[types.Object]types.Object{gotObj: wantObj} + + site := AssertionSite{ + Location: "test.go:5", + Kind: AssertionKindStdlibComparison, + Expr: rhs, + } + + m := matchDirect(site, objToEffectID, effectMap, info, helperBridge) + if m == nil { + t.Fatal("matchDirect should return a mapping for helper bridge match") + } + if m.Confidence != 70 { + t.Errorf("confidence = %d, want 70", m.Confidence) + } + if m.SideEffectID != effectID { + t.Errorf("SideEffectID = %q, want %q", m.SideEffectID, effectID) + } +} + +// TestMatchDirect_NoMatch verifies that matchDirect returns nil when +// no identifiers in the expression match objToEffectID or helperBridge. +func TestMatchDirect_NoMatch(t *testing.T) { + src := `package p +func f() { + x := 1 + y := 2 + _ = y + 3 + _ = x +}` + file, info := parseAndTypeCheck(t, src) + + xObj := extractVarObj(t, file, info, 0, 0) // x := 1 + rhs := extractRHS(t, file, 0, 2) // y + 3 + + // Map x but not y — the expression only contains y. + effectID := "effect-return-1" + objToEffectID := map[types.Object]string{xObj: effectID} + effectMap := map[string]*taxonomy.SideEffect{ + effectID: {ID: effectID, Type: taxonomy.ReturnValue}, + } + + site := AssertionSite{ + Location: "test.go:5", + Kind: AssertionKindStdlibComparison, + Expr: rhs, + } + + m := matchDirect(site, objToEffectID, effectMap, info, nil) + if m != nil { + t.Error("matchDirect should return nil when no identifiers match") + } +} + +// TestMatchDirect_NilExpr verifies that matchDirect returns nil when +// the assertion site has a nil expression. +func TestMatchDirect_NilExpr(t *testing.T) { + site := AssertionSite{ + Location: "test.go:1", + Kind: AssertionKindStdlibComparison, + Expr: nil, + } + + m := matchDirect(site, nil, nil, nil, nil) + if m != nil { + t.Error("matchDirect should return nil for nil expression") + } +} + +// --- matchIndirectRoot helper unit tests --- + +// TestMatchIndirectRoot_SelectorMatch verifies that matchIndirectRoot +// returns a mapping with confidence 65 when a SelectorExpr's root +// identifier (via resolveExprRoot) is in objToEffectID. +func TestMatchIndirectRoot_SelectorMatch(t *testing.T) { + src := `package p + +type Result struct { Name string } + +func f() { + result := Result{Name: "test"} + _ = result.Name +}` + file, info := parseAndTypeCheck(t, src) + + resultObj := extractVarObj(t, file, info, 1, 0) // result := Result{...} + rhs := extractRHS(t, file, 1, 1) // result.Name + + effectID := "effect-return-1" + objToEffectID := map[types.Object]string{resultObj: effectID} + effectMap := map[string]*taxonomy.SideEffect{ + effectID: {ID: effectID, Type: taxonomy.ReturnValue}, + } + + site := AssertionSite{ + Location: "test.go:7", + Kind: AssertionKindStdlibComparison, + Expr: rhs, + } + + m := matchIndirectRoot(site, objToEffectID, effectMap, info) + if m == nil { + t.Fatal("matchIndirectRoot should return a mapping for selector match") + } + if m.Confidence != 65 { + t.Errorf("confidence = %d, want 65", m.Confidence) + } + if m.SideEffectID != effectID { + t.Errorf("SideEffectID = %q, want %q", m.SideEffectID, effectID) + } +} + +// TestMatchIndirectRoot_NoComposite verifies that matchIndirectRoot +// returns nil when the expression contains only simple identifiers +// (no SelectorExpr, IndexExpr, or CallExpr nodes). +func TestMatchIndirectRoot_NoComposite(t *testing.T) { + src := `package p +func f() { + x := 1 + _ = x + 2 +}` + file, info := parseAndTypeCheck(t, src) + + xObj := extractVarObj(t, file, info, 0, 0) // x := 1 + rhs := extractRHS(t, file, 0, 1) // x + 2 + + effectID := "effect-return-1" + objToEffectID := map[types.Object]string{xObj: effectID} + effectMap := map[string]*taxonomy.SideEffect{ + effectID: {ID: effectID, Type: taxonomy.ReturnValue}, + } + + site := AssertionSite{ + Location: "test.go:4", + Kind: AssertionKindStdlibComparison, + Expr: rhs, + } + + m := matchIndirectRoot(site, objToEffectID, effectMap, info) + if m != nil { + t.Error("matchIndirectRoot should return nil when expression has only simple identifiers") + } +} + +// parseAndTypeCheckWithFset parses Go source code and type-checks it, +// returning the AST file, populated types.Info, and the file set. This +// variant is needed for constructing packages.Package instances in +// traceForwardDataFlow tests. +func parseAndTypeCheckWithFset(t *testing.T, src string) (*ast.File, *types.Info, *token.FileSet) { + t.Helper() + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, "test.go", src, 0) + if err != nil { + t.Fatalf("parse: %v", err) + } + info := &types.Info{ + Types: make(map[ast.Expr]types.TypeAndValue), + Defs: make(map[*ast.Ident]types.Object), + Uses: make(map[*ast.Ident]types.Object), + } + // Type-check errors are expected and intentionally ignored because + // Importer is nil — the synthetic test code only needs local + // definitions to be resolved, not imported packages. + conf := types.Config{Importer: nil} + _, _ = conf.Check("p", fset, []*ast.File{file}, info) + return file, info, fset +} + +// --- collectTrackedVars helper unit tests --- + +// TestCollectTrackedVars_MultipleMatches verifies that collectTrackedVars +// returns only the types.Object keys whose effect ID matches the target +// returnEffectID when multiple entries exist. +func TestCollectTrackedVars_MultipleMatches(t *testing.T) { + src := `package p +func f() { + a := 1 + b := 2 + c := 3 + _ = a + b + c +}` + file, info := parseAndTypeCheck(t, src) + + aObj := extractVarObj(t, file, info, 0, 0) // a := 1 + bObj := extractVarObj(t, file, info, 0, 1) // b := 2 + cObj := extractVarObj(t, file, info, 0, 2) // c := 3 + + targetID := "effect-return-1" + objToEffectID := map[types.Object]string{ + aObj: targetID, + bObj: "effect-other", + cObj: targetID, + } + + tracked := collectTrackedVars(objToEffectID, targetID) + if len(tracked) != 2 { + t.Fatalf("len(tracked) = %d, want 2", len(tracked)) + } + if !tracked[aObj] { + t.Error("tracked set should contain aObj") + } + if !tracked[cObj] { + t.Error("tracked set should contain cObj") + } + if tracked[bObj] { + t.Error("tracked set should NOT contain bObj (different effect ID)") + } +} + +// TestCollectTrackedVars_NoMatches verifies that collectTrackedVars +// returns an empty map when no entries match the returnEffectID. +func TestCollectTrackedVars_NoMatches(t *testing.T) { + src := `package p +func f() { + a := 1 + b := 2 + _ = a + b +}` + file, info := parseAndTypeCheck(t, src) + + aObj := extractVarObj(t, file, info, 0, 0) // a := 1 + bObj := extractVarObj(t, file, info, 0, 1) // b := 2 + + objToEffectID := map[types.Object]string{ + aObj: "effect-other-1", + bObj: "effect-other-2", + } + + tracked := collectTrackedVars(objToEffectID, "effect-return-1") + if len(tracked) != 0 { + t.Errorf("len(tracked) = %d, want 0", len(tracked)) + } +} + +// --- traceForwardDataFlow helper unit tests --- + +// TestTraceForwardDataFlow_SimpleChain verifies that traceForwardDataFlow +// traces through a simple field-access assignment: if x is tracked, +// then y := x.Field causes y to be added to the tracked set. +func TestTraceForwardDataFlow_SimpleChain(t *testing.T) { + src := `package p + +type S struct { Field int } + +func f() { + x := S{Field: 42} + y := x.Field + _ = y +}` + file, info, fset := parseAndTypeCheckWithFset(t, src) + + xObj := extractVarObj(t, file, info, 1, 0) // x := S{...} + yObj := extractVarObj(t, file, info, 1, 1) // y := x.Field + + tracked := map[types.Object]bool{xObj: true} + pkg := &packages.Package{ + Syntax: []*ast.File{file}, + TypesInfo: info, + Fset: fset, + } + + result := traceForwardDataFlow(tracked, pkg) + if !result[yObj] { + t.Error("traceForwardDataFlow should add y (from y := x.Field) to tracked set") + } + if !result[xObj] { + t.Error("traceForwardDataFlow should preserve original tracked variable x") + } +} + +// TestTraceForwardDataFlow_EmptyTracked verifies that traceForwardDataFlow +// returns the empty tracked set immediately when given no initial variables. +func TestTraceForwardDataFlow_EmptyTracked(t *testing.T) { + src := `package p +func f() { + x := 1 + _ = x +}` + file, info, fset := parseAndTypeCheckWithFset(t, src) + + tracked := map[types.Object]bool{} + pkg := &packages.Package{ + Syntax: []*ast.File{file}, + TypesInfo: info, + Fset: fset, + } + + result := traceForwardDataFlow(tracked, pkg) + if len(result) != 0 { + t.Errorf("traceForwardDataFlow with empty tracked should return empty, got %d entries", len(result)) + } +} + +// TestTraceForwardDataFlow_NonDataExtraction verifies that traceForwardDataFlow +// does NOT add the LHS variable when the RHS is a method call (not a +// data-extraction expression). Method calls like s.Get("key") are gated +// by isDataExtraction to prevent false positives. +func TestTraceForwardDataFlow_NonDataExtraction(t *testing.T) { + src := `package p + +type Store struct{} +func (Store) Get(key string) int { return 0 } + +func f() { + s := Store{} + got := s.Get("key") + _ = got +}` + file, info, fset := parseAndTypeCheckWithFset(t, src) + + // s is at funcIdx=2 (after type decl and method decl), stmtIdx=0 + sObj := extractVarObj(t, file, info, 2, 0) // s := Store{} + gotObj := extractVarObj(t, file, info, 2, 1) // got := s.Get("key") + + tracked := map[types.Object]bool{sObj: true} + pkg := &packages.Package{ + Syntax: []*ast.File{file}, + TypesInfo: info, + Fset: fset, + } + + result := traceForwardDataFlow(tracked, pkg) + if result[gotObj] { + t.Error("traceForwardDataFlow should NOT add got from s.Get(\"key\") — method call is not a data extraction") + } + if !result[sObj] { + t.Error("traceForwardDataFlow should preserve original tracked variable s") + } +} + +// --- matchTrackedInExpr helper unit tests --- + +// TestMatchTrackedInExpr_DirectMatch verifies that matchTrackedInExpr +// returns true when the expression contains an identifier whose +// types.Object is directly in the tracked set. +func TestMatchTrackedInExpr_DirectMatch(t *testing.T) { + src := `package p +func f() { + x := 42 + _ = x + 1 +}` + file, info := parseAndTypeCheck(t, src) + + xObj := extractVarObj(t, file, info, 0, 0) // x := 42 + rhs := extractRHS(t, file, 0, 1) // x + 1 + + tracked := map[types.Object]bool{xObj: true} + if !matchTrackedInExpr(rhs, tracked, info) { + t.Error("matchTrackedInExpr should return true when expression contains tracked identifier") + } +} + +// TestMatchTrackedInExpr_RootResolution verifies that matchTrackedInExpr +// returns true for composite expressions like tracked.Field where the +// root identifier resolves to a tracked variable via resolveExprRoot. +func TestMatchTrackedInExpr_RootResolution(t *testing.T) { + src := `package p + +type S struct { Field int } + +func f() { + tracked := S{Field: 1} + _ = tracked.Field +}` + file, info := parseAndTypeCheck(t, src) + + trackedObj := extractVarObj(t, file, info, 1, 0) // tracked := S{...} + rhs := extractRHS(t, file, 1, 1) // tracked.Field + + trackedSet := map[types.Object]bool{trackedObj: true} + if !matchTrackedInExpr(rhs, trackedSet, info) { + t.Error("matchTrackedInExpr should return true for tracked.Field via root resolution") + } +} + +// TestMatchTrackedInExpr_NoMatch verifies that matchTrackedInExpr +// returns false when the expression contains no identifiers in the +// tracked set. +func TestMatchTrackedInExpr_NoMatch(t *testing.T) { + src := `package p +func f() { + x := 1 + y := 2 + _ = y + 3 + _ = x +}` + file, info := parseAndTypeCheck(t, src) + + xObj := extractVarObj(t, file, info, 0, 0) // x := 1 + rhs := extractRHS(t, file, 0, 2) // y + 3 + + tracked := map[types.Object]bool{xObj: true} + if matchTrackedInExpr(rhs, tracked, info) { + t.Error("matchTrackedInExpr should return false when no identifiers match tracked set") + } +} diff --git a/internal/quality/mapping.go b/internal/quality/mapping.go index 1993bf0..ee751ac 100644 --- a/internal/quality/mapping.go +++ b/internal/quality/mapping.go @@ -734,6 +734,86 @@ const containerUnwrapConfidence = 55 // assert) with margin for more complex chains. const maxContainerChainDepth = 6 +// isByteLikeParam reports whether typ represents a byte-like input type +// suitable for transformation call detection. It recognizes three patterns: +// - []byte: a slice with a byte element type +// - string: a basic type with String kind +// - io.Reader: an interface whose method set includes a Read method +func isByteLikeParam(typ types.Type) bool { + // Check for []byte: *types.Slice with byte element. + if sl, ok := typ.(*types.Slice); ok { + if basic, ok := sl.Elem().(*types.Basic); ok && basic.Kind() == types.Byte { + return true + } + } + + // Check for string: *types.Basic with Kind() == types.String. + if basic, ok := typ.(*types.Basic); ok && basic.Kind() == types.String { + return true + } + + // Check for io.Reader: interface with Read method. + if iface, ok := typ.Underlying().(*types.Interface); ok { + ms := types.NewMethodSet(iface) + for i := 0; i < ms.Len(); i++ { + if ms.At(i).Obj().Name() == "Read" { + return true + } + } + } + + return false +} + +// isPointerDestParam reports whether typ represents a pointer destination +// type suitable for transformation call detection. It recognizes two patterns: +// - *T: a pointer type +// - interface{}/any: an empty interface (zero methods), commonly used as +// the destination parameter in unmarshal functions +func isPointerDestParam(typ types.Type) bool { + // Check for pointer type: *types.Pointer. + if _, ok := typ.(*types.Pointer); ok { + return true + } + + // Check for empty interface (any/interface{}) via Underlying + // to handle named types. + if iface, ok := typ.Underlying().(*types.Interface); ok { + return iface.NumMethods() == 0 + } + + return false +} + +// resolveCallSignature extracts the function signature from a call +// expression using type information. Returns nil if the call or info +// is nil, or if the function type cannot be resolved to a signature. +func resolveCallSignature(call *ast.CallExpr, info *types.Info) *types.Signature { + if call == nil || info == nil { + return nil + } + tv, exists := info.Types[call.Fun] + if !exists { + return nil + } + sig, ok := tv.Type.(*types.Signature) + if !ok { + return nil + } + return sig +} + +// findParamIndex returns the index of the first parameter matching the +// predicate, or -1 if none match. +func findParamIndex(params *types.Tuple, predicate func(types.Type) bool) int { + for i := 0; i < params.Len(); i++ { + if predicate(params.At(i).Type()) { + return i + } + } + return -1 +} + // isTransformationCall checks whether a call expression matches the // structural signature pattern of a transformation function: a function // that accepts a byte-like input ([]byte, string, or io.Reader) AND a @@ -743,85 +823,18 @@ const maxContainerChainDepth = 6 // This enables structural detection of unmarshal-like functions without // hardcoding specific function names (FR-001, FR-008). func isTransformationCall(call *ast.CallExpr, info *types.Info) (byteArgIdx int, ptrDestIdx int, ok bool) { - if call == nil || info == nil { - return 0, 0, false - } - - tv, exists := info.Types[call.Fun] - if !exists { - return 0, 0, false - } - - sig, isSig := tv.Type.(*types.Signature) - if !isSig { + sig := resolveCallSignature(call, info) + if sig == nil { return 0, 0, false } params := sig.Params() - if params == nil || params.Len() == 0 { + if params == nil { return 0, 0, false } - byteArgIdx = -1 - ptrDestIdx = -1 - - for i := 0; i < params.Len(); i++ { - paramType := params.At(i).Type() - - // Check for []byte: *types.Slice with byte element. - if sl, isSl := paramType.(*types.Slice); isSl { - if basic, isBasic := sl.Elem().(*types.Basic); isBasic && basic.Kind() == types.Byte { - if byteArgIdx == -1 { - byteArgIdx = i - } - continue - } - } - - // Check for string: *types.Basic with Kind() == types.String. - if basic, isBasic := paramType.(*types.Basic); isBasic && basic.Kind() == types.String { - if byteArgIdx == -1 { - byteArgIdx = i - } - continue - } - - // Check for pointer type: *types.Pointer. - if _, isPtr := paramType.(*types.Pointer); isPtr { - if ptrDestIdx == -1 { - ptrDestIdx = i - } - continue - } - - // Check for interface types: io.Reader (byte-like input) or - // interface{}/any (pointer destination for unmarshal functions). - if iface, isIface := paramType.Underlying().(*types.Interface); isIface { - // io.Reader: interface with Read method → byte-like input. - hasRead := false - ms := types.NewMethodSet(iface) - for j := 0; j < ms.Len(); j++ { - if ms.At(j).Obj().Name() == "Read" { - hasRead = true - break - } - } - if hasRead { - if byteArgIdx == -1 { - byteArgIdx = i - } - continue - } - // Empty interface (any/interface{}) — commonly used as - // pointer destination in unmarshal functions (e.g., - // json.Unmarshal takes any as second param, callers pass &data). - if iface.NumMethods() == 0 { - if ptrDestIdx == -1 { - ptrDestIdx = i - } - } - } - } + byteArgIdx = findParamIndex(params, isByteLikeParam) + ptrDestIdx = findParamIndex(params, isPointerDestParam) if byteArgIdx >= 0 && ptrDestIdx >= 0 { return byteArgIdx, ptrDestIdx, true @@ -939,51 +952,38 @@ func isDataExtraction(expr ast.Expr) bool { } } -// matchContainerUnwrap traces data flow forward from the return value -// variable through intermediate assignments and transformation calls -// to the assertion expression. This handles the container-unwrap-assert -// pattern where a test assigns a function's return value, accesses a -// field, passes it through a transformation (like JSON unmarshal), and -// asserts on the result. -// -// The algorithm: -// 1. Collect all types.Object keys from objToEffectID that map to a -// ReturnValue effect as the initial tracked variable set. -// 2. For up to maxContainerChainDepth iterations, walk the test -// package AST looking for assignment statements where the RHS -// references a tracked variable. For transformation calls, extract -// the pointer destination as the new tracked variable. -// 3. Check if the assertion site's expression contains any tracked -// variable. -// 4. If matched, return an AssertionMapping with confidence 55. -func matchContainerUnwrap( - site AssertionSite, - objToEffectID map[types.Object]string, - effectMap map[string]*taxonomy.SideEffect, - testPkg *packages.Package, - returnEffectID string, -) *taxonomy.AssertionMapping { - if site.Expr == nil || testPkg == nil || testPkg.TypesInfo == nil || returnEffectID == "" { - return nil - } - - info := testPkg.TypesInfo - - // Step 1: Collect initial tracked variables — those mapped to - // the ReturnValue effect. +// collectTrackedVars filters objToEffectID entries whose value matches +// returnEffectID and returns a set of the matching types.Object keys. +// This is the initial "seed" set for forward data-flow tracing — +// variables that hold the return value of interest. +func collectTrackedVars(objToEffectID map[types.Object]string, returnEffectID string) map[types.Object]bool { tracked := make(map[types.Object]bool) for obj, effectID := range objToEffectID { if effectID == returnEffectID { tracked[obj] = true } } + return tracked +} + +// traceForwardDataFlow performs multi-iteration forward data-flow +// tracing through assignment statements in the test package AST. +// Starting from the tracked set of variables, it walks assignments +// looking for RHS expressions that reference tracked variables. For +// transformation calls (detected via isTransformationCall), it +// extracts the pointer destination as a new tracked variable. For +// non-transformation assignments, it gates on isDataExtraction to +// prevent false positives from method calls like s.Get("key"). +// Iterations continue until convergence (no new variables discovered) +// or maxContainerChainDepth is reached. The input tracked map is +// mutated in place and returned with any newly discovered entries. +func traceForwardDataFlow(tracked map[types.Object]bool, testPkg *packages.Package) map[types.Object]bool { if len(tracked) == 0 { - return nil + return tracked } - // Step 2: Forward-trace through assignments for up to - // maxContainerChainDepth iterations. Each iteration may - // discover new tracked variables derived from existing ones. + info := testPkg.TypesInfo + for iter := 0; iter < maxContainerChainDepth; iter++ { newTracked := make(map[types.Object]bool) @@ -1103,16 +1103,17 @@ func matchContainerUnwrap( } } - // Step 3: Check if the assertion expression contains any - // tracked variable. - effect := effectMap[returnEffectID] - if effect == nil { - return nil - } + return tracked +} - // Direct identity check via ast.Inspect. +// matchTrackedInExpr walks the expression tree via ast.Inspect and +// returns true if any identifier's types.Object is in the tracked set. +// It checks both direct identity (via info.Uses and info.Defs) and +// resolveExprRoot fallback for composite expressions like data["key"] +// where the root ident "data" is the tracked variable. +func matchTrackedInExpr(expr ast.Expr, tracked map[types.Object]bool, info *types.Info) bool { var matched bool - ast.Inspect(site.Expr, func(n ast.Node) bool { + ast.Inspect(expr, func(n ast.Node) bool { if matched { return false } @@ -1134,7 +1135,7 @@ func matchContainerUnwrap( // Also check via resolveExprRoot for compound expressions // like data["key"] where the root ident is "data". if !matched { - root := resolveExprRoot(site.Expr, info) + root := resolveExprRoot(expr, info) if root != nil { rootObj := info.Uses[root] if rootObj == nil { @@ -1146,68 +1147,79 @@ func matchContainerUnwrap( } } - if !matched { - return nil - } - - return &taxonomy.AssertionMapping{ - AssertionLocation: site.Location, - AssertionType: mapKindToType(site.Kind), - SideEffectID: returnEffectID, - Confidence: containerUnwrapConfidence, - } + return matched } -// matchAssertionToEffect attempts to match an assertion site to a -// traced side effect value using types.Object identity. -// -// It uses a two-pass matching strategy: -// -// Pass 1 (direct): Walk the expression tree with ast.Inspect looking -// for *ast.Ident nodes whose types.Object is directly in objToEffectID. -// This is the original behavior. Matches produce confidence 75. -// Because ast.Inspect visits all descendant nodes, this handles -// simple selector expressions (e.g., result.Name) — the root ident -// "result" is visited as a child of the SelectorExpr and matched -// directly at confidence 75. +// matchContainerUnwrap traces data flow forward from the return value +// variable through intermediate assignments and transformation calls +// to the assertion expression. This handles the container-unwrap-assert +// pattern where a test assigns a function's return value, accesses a +// field, passes it through a transformation (like JSON unmarshal), and +// asserts on the result. // -// Pass 2 (indirect): If Pass 1 found no match, walk the expression -// tree again. For each SelectorExpr, IndexExpr, or CallExpr node, -// call resolveExprRoot to unwind to the root identifier. If the -// root's types.Object is in objToEffectID, produce a match at -// confidence 65. This handles cases where the root ident is not -// directly reachable by ast.Inspect as a bare *ast.Ident — e.g., -// index expressions (results[0]) or nested composites where the -// root is buried inside a complex expression structure. +// The algorithm delegates to three helpers: +// 1. collectTrackedVars — seeds the tracked set from objToEffectID. +// 2. traceForwardDataFlow — expands the set through AST assignments. +// 3. matchTrackedInExpr — checks the assertion expression for matches. // -// Pass 1 always executes first so direct identity matches are never -// degraded by indirect resolution. -func matchAssertionToEffect( +// If matched, returns an AssertionMapping with confidence 55. +func matchContainerUnwrap( site AssertionSite, objToEffectID map[types.Object]string, effectMap map[string]*taxonomy.SideEffect, testPkg *packages.Package, + returnEffectID string, ) *taxonomy.AssertionMapping { - if site.Expr == nil { + if site.Expr == nil || testPkg == nil || testPkg.TypesInfo == nil || returnEffectID == "" { return nil } - var info *types.Info - if testPkg != nil { - info = testPkg.TypesInfo + info := testPkg.TypesInfo + + tracked := collectTrackedVars(objToEffectID, returnEffectID) + if len(tracked) == 0 { + return nil } - if info == nil { + + tracked = traceForwardDataFlow(tracked, testPkg) + + effect := effectMap[returnEffectID] + if effect == nil { return nil } - // Build supplemental param→arg bridging for helper assertions. - // When a test calls assertEqual(t, got, 12), the assertion - // expression inside assertEqual references the helper's - // parameter objects. We bridge these back to the caller's - // argument objects to find matches in objToEffectID. - helperBridge := buildHelperBridge(site, objToEffectID, info) + if matchTrackedInExpr(site.Expr, tracked, info) { + return &taxonomy.AssertionMapping{ + AssertionLocation: site.Location, + AssertionType: mapKindToType(site.Kind), + SideEffectID: returnEffectID, + Confidence: containerUnwrapConfidence, + } + } + + return nil +} + +// matchDirect walks the assertion expression tree via ast.Inspect looking +// for *ast.Ident nodes whose types.Object is directly in objToEffectID +// (confidence 75) or reachable through helperBridge → objToEffectID +// (confidence 70). It skips nil/true/false literal identifiers and +// returns the first match found, or nil if no match exists. +// +// Because ast.Inspect visits all descendant nodes, this handles simple +// selector expressions (e.g., result.Name) — the root ident "result" +// is visited as a child of the SelectorExpr and matched directly. +func matchDirect( + site AssertionSite, + objToEffectID map[types.Object]string, + effectMap map[string]*taxonomy.SideEffect, + info *types.Info, + helperBridge map[types.Object]types.Object, +) *taxonomy.AssertionMapping { + if site.Expr == nil { + return nil + } - // Pass 1: Direct identity matching (confidence 75). var matched *taxonomy.AssertionMapping ast.Inspect(site.Expr, func(n ast.Node) bool { if matched != nil { @@ -1268,14 +1280,31 @@ func matchAssertionToEffect( return true }) - if matched != nil { - return matched + return matched +} + +// matchIndirectRoot walks the assertion expression tree looking for +// composite expression nodes (SelectorExpr, IndexExpr, CallExpr) and +// resolves each to its root identifier via resolveExprRoot. If the +// root's types.Object is in objToEffectID, it returns a match at +// confidence 65. Returns nil if no composite expression resolves to +// a tracked object. +// +// This handles cases where the root ident is not directly reachable by +// ast.Inspect as a bare *ast.Ident — e.g., index expressions +// (results[0]) or nested composites where the root is buried inside +// a complex expression structure. +func matchIndirectRoot( + site AssertionSite, + objToEffectID map[types.Object]string, + effectMap map[string]*taxonomy.SideEffect, + info *types.Info, +) *taxonomy.AssertionMapping { + if site.Expr == nil { + return nil } - // Pass 2: Indirect root resolution (confidence 65). - // For each composite expression node (SelectorExpr, IndexExpr, - // CallExpr), resolve to the root identifier and check against - // the traced object map. + var matched *taxonomy.AssertionMapping ast.Inspect(site.Expr, func(n ast.Node) bool { if matched != nil { return false @@ -1326,6 +1355,49 @@ func matchAssertionToEffect( return matched } +// matchAssertionToEffect attempts to match an assertion site to a +// traced side effect value using types.Object identity. +// +// It uses a two-pass matching strategy: +// +// Pass 1 (direct via matchDirect): Walk the expression tree looking +// for *ast.Ident nodes whose types.Object is directly in objToEffectID +// (confidence 75) or reachable through the helper bridge (confidence 70). +// +// Pass 2 (indirect via matchIndirectRoot): If Pass 1 found no match, +// walk the expression tree for composite nodes (SelectorExpr, IndexExpr, +// CallExpr), resolve to root identifiers, and check against objToEffectID +// (confidence 65). +// +// Pass 1 always executes first so direct identity matches are never +// degraded by indirect resolution. +func matchAssertionToEffect( + site AssertionSite, + objToEffectID map[types.Object]string, + effectMap map[string]*taxonomy.SideEffect, + testPkg *packages.Package, +) *taxonomy.AssertionMapping { + if site.Expr == nil { + return nil + } + + if testPkg == nil { + return nil + } + info := testPkg.TypesInfo + if info == nil { + return nil + } + + // Build supplemental param→arg bridging for helper assertions. + helperBridge := buildHelperBridge(site, objToEffectID, info) + + if m := matchDirect(site, objToEffectID, effectMap, info, helperBridge); m != nil { + return m + } + return matchIndirectRoot(site, objToEffectID, effectMap, info) +} + // matchInlineCall checks if the assertion expression contains a direct // call to the target function (e.g., `if c.Value() != 5`). When the // return value is used inline without assignment, the normal tracing diff --git a/openspec/changes/crapload-decompose-pr2b/.openspec.yaml b/openspec/changes/crapload-decompose-pr2b/.openspec.yaml new file mode 100644 index 0000000..071d09f --- /dev/null +++ b/openspec/changes/crapload-decompose-pr2b/.openspec.yaml @@ -0,0 +1,2 @@ +schema: unbound-force +created: 2026-07-14 diff --git a/openspec/changes/crapload-decompose-pr2b/design.md b/openspec/changes/crapload-decompose-pr2b/design.md new file mode 100644 index 0000000..3e98feb --- /dev/null +++ b/openspec/changes/crapload-decompose-pr2b/design.md @@ -0,0 +1,193 @@ +## Context + +`internal/quality/mapping.go` (1575 lines, 32 functions) is the assertion +mapping engine — it maps test assertion expressions to side effects via AST +and SSA analysis. Three functions dominate the complexity budget: + +| Function | Complexity | Lines | Direct unit tests | +|----------|-----------|-------|-------------------| +| `matchContainerUnwrap` | 50 | 201 (959-1159) | 0 (4 integration) | +| `isTransformationCall` | 26 | 86 (745-830) | 5 (3 gaps) | +| `matchAssertionToEffect` | 25 | 143 (1185-1327) | 0 | + +These functions already accept explicit parameters (AST nodes, type info, +maps) — no DI structs are needed. The existing `parseAndTypeCheck` helper +in `container_unwrap_internal_test.go` provides synthetic AST construction +for unit testing. + +### Matching cascade (call order in `mapAssertionsToEffectsImpl`) + +```text +mapAssertionsToEffectsImpl + ├─ matchAssertionToEffect (primary, conf 75/70/65) + ├─ matchInlineCall (inline calls, conf 60) — out of scope + ├─ matchContainerUnwrap (container chains, conf 55) + └─ aiMapperFn (AI fallback, conf 50) — out of scope +``` + +## Goals / Non-Goals + +### Goals +- Reduce combined complexity from 101 to ≤30 across the three functions +- Make each interleaved concern independently testable +- Fill `isTransformationCall` test gaps (io.Reader, empty interface, ordering) +- Add direct unit tests for `matchAssertionToEffect` (currently zero) +- Add unit tests for `matchContainerUnwrap` helpers (currently integration only) +- All new tests run without `testing.Short()` guard + +### Non-Goals +- Decomposing `matchInlineCall` (complexity 17, deferred to Phase 2c) +- Decomposing `findHelperCallInFuncDepth` (complexity 15, deferred) +- Decomposing `mapAssertionsToEffectsImpl` (complexity 14, deferred) +- Changing confidence values (75, 70, 65, 55) or matching behavior +- Modifying the `TestSC003_MappingAccuracy` ratchet floor (85.0%) + +## Decisions + +### D1: Extract `isByteLikeParam` and `isPointerDestParam` from `isTransformationCall` + +**Decision**: Split the parameter-classification logic in the loop body into +two predicate functions that each return `bool`. + +**Rationale**: The current loop body interleaves 5 type-checking patterns +([]byte, string, io.Reader, *T, empty interface) with index tracking in a +single iteration. Extracting two classifiers makes each pattern independently +testable and reduces `isTransformationCall` to: validate → loop → classify → +return indices. + +**Signatures**: +```go +func isByteLikeParam(typ types.Type) bool +func isPointerDestParam(typ types.Type) bool +``` + +### D2: Extract `matchDirect` and `matchIndirectRoot` from `matchAssertionToEffect` + +**Decision**: Extract the two `ast.Inspect` passes as separate functions. + +**Rationale**: The current function has two distinct matching strategies +interleaved with nil guards and helper bridge construction. Each pass +produces an `*AssertionMapping` independently. Separating them allows +testing each matching strategy in isolation. + +Both helpers accept `*types.Info` directly instead of `*packages.Package` +because they only use `TypesInfo`; the parent function extracts and passes it. + +**Signatures**: +```go +func matchDirect( + site AssertionSite, + objToEffectID map[types.Object]string, + effectMap map[string]*taxonomy.SideEffect, + info *types.Info, + helperBridge map[types.Object]types.Object, +) *taxonomy.AssertionMapping + +func matchIndirectRoot( + site AssertionSite, + objToEffectID map[types.Object]string, + effectMap map[string]*taxonomy.SideEffect, + info *types.Info, +) *taxonomy.AssertionMapping +``` + +### D3: Extract `collectTrackedVars`, `traceForwardDataFlow`, and `matchTrackedInExpr` from `matchContainerUnwrap` + +**Decision**: Decompose the 201-line function into three phases: setup, +trace, and match. + +**Rationale**: The function's complexity comes from interleaving forward +data-flow tracing (multi-iteration AST walking with transformation call +detection) with assertion matching. These are fundamentally different +operations that happen to share a `tracked` set. Separating them makes +each phase testable with controlled inputs. + +**Signatures**: +```go +func collectTrackedVars( + objToEffectID map[types.Object]string, + returnEffectID string, +) map[types.Object]bool + +func traceForwardDataFlow( + tracked map[types.Object]bool, + testPkg *packages.Package, +) map[types.Object]bool + +func matchTrackedInExpr( + expr ast.Expr, + tracked map[types.Object]bool, + info *types.Info, +) bool +``` + +`traceForwardDataFlow` takes `*packages.Package` (for `Syntax` file +iteration) and extracts `info := testPkg.TypesInfo` internally for +type resolution. This differs from `matchTrackedInExpr` which takes +`*types.Info` directly — the asymmetry exists because `traceForwardDataFlow` +needs both file access (`Syntax`) and type info, while `matchTrackedInExpr` +only needs type info. The function mutates and returns the input `tracked` +map (same map, expanded with new entries). + +`traceForwardDataFlow` absorbs the multi-iteration loop, transformation +call detection via `isTransformationCall`, and data-extraction gating via +`isDataExtraction`. Even extracted, this function will have inherent +complexity (~12) from the nested AST walking — hence the ≤12 target for +`matchContainerUnwrap` overall. + +### D4: No DI needed — functions already take explicit parameters + +**Decision**: Do not add DI structs or function parameters for testability. + +**Rationale**: Unlike Phase 2a's `BuildContractCoverageFunc` (which needed +DI to avoid real package loading), these functions accept `*ast.File`, +`*types.Info`, and `*packages.Package` directly. The `parseAndTypeCheck` +test helper creates synthetic instances of these types from Go source +strings. No external I/O is involved. + +### D5: Task ordering — `isTransformationCall` first + +**Decision**: Decompose in order: `isTransformationCall` → +`matchAssertionToEffect` → `matchContainerUnwrap`. + +**Rationale**: `matchContainerUnwrap` calls `isTransformationCall` internally +(line 1037). Decomposing `isTransformationCall` first means the cleaner +helper is in place when `matchContainerUnwrap` is refactored. +`matchAssertionToEffect` is independent and can go second. + +## Risks / Trade-offs + +### Risk: `traceForwardDataFlow` remains complex + +Even after extraction, the forward data-flow tracing involves nested +`ast.Inspect` closures, multi-iteration convergence, transformation call +detection, and data-extraction gating. Target complexity is ≤12 for this +function specifically. If it exceeds 12, a second decomposition pass may +be needed in Phase 2c. + +### Risk: Test fixture complexity for `matchContainerUnwrap` helpers + +The `parseAndTypeCheck` helper only provides AST and type info — it does +not create `packages.Package` instances with full dependency resolution. +`traceForwardDataFlow` takes `*packages.Package` (for file access). +Tests will need to construct minimal `packages.Package` structs with +synthetic `Syntax` (parsed files) and `TypesInfo`. This is feasible +(the existing `isTransformationCall` tests show the pattern) but requires +care. + +### Trade-off: More functions in mapping.go + +Seven new unexported helpers increase the function count from 32 to ~39. +This is acceptable because each helper has a clear single responsibility +and reduces the cognitive load of the three parent functions. The file +is already large (1575 lines) — decomposition makes it more navigable, +not less. + +### Trade-off: `matchAssertionToEffect` → `matchDirect` passes `helperBridge` explicitly + +The helper bridge map is computed inside `matchAssertionToEffect` and +consumed by `matchDirect`. After extraction, `matchAssertionToEffect` +computes the bridge and passes it to `matchDirect`. This exposes an +internal detail in the function signature. Acceptable because both +functions are unexported and the bridge map is a well-typed value +(`map[types.Object]types.Object`). diff --git a/openspec/changes/crapload-decompose-pr2b/proposal.md b/openspec/changes/crapload-decompose-pr2b/proposal.md new file mode 100644 index 0000000..32146e7 --- /dev/null +++ b/openspec/changes/crapload-decompose-pr2b/proposal.md @@ -0,0 +1,111 @@ +## Why + +Three functions in `internal/quality/mapping.go` have the highest cyclomatic +complexities in the codebase: `matchContainerUnwrap` (50), `isTransformationCall` +(26), and `matchAssertionToEffect` (25). Together they represent 101 complexity +points — the top 3 in a 1575-line file with 32 functions. + +`matchContainerUnwrap` at complexity 50 is the single most complex function in +the entire project. It interleaves 5-6 distinct concerns (tracked variable +collection, forward data flow tracing, transformation call detection, +data-extraction gating, assertion expression matching, and mapping construction) +in a 201-line function with nested `ast.Inspect` closures. + +Direct unit test coverage is minimal: `isTransformationCall` has 5 unit tests +(with gaps for `io.Reader` and empty interface patterns), `matchAssertionToEffect` +has zero direct tests, and `matchContainerUnwrap` has 4 integration tests but +no synthetic unit tests. + +This is Phase 2b of issue #166 (CRAPload fragility reduction). Phase 2a (#192) +decomposed `BuildContractCoverageFunc` and reduced CRAPload from 30 to ~29. +This phase addresses the three highest-complexity functions remaining. + +## What Changes + +### Decomposition + +Extract concern-specific helpers from each function: + +1. **`isTransformationCall` (26 → ≤8)**: Extract `isByteLikeParam` and + `isPointerDestParam` as type-classification helpers. The main function + becomes a simple parameter loop calling two classifiers. + +2. **`matchAssertionToEffect` (25 → ≤10)**: Extract `matchDirect` (Pass 1: + identity matching at confidence 75 + helper bridge at 70) and + `matchIndirectRoot` (Pass 2: root resolution at confidence 65). + +3. **`matchContainerUnwrap` (50 → ≤12)**: Extract `collectTrackedVars`, + `traceForwardDataFlow`, and `matchTrackedInExpr` as data-flow tracing + helpers. + +### Test gap coverage + +Add ~20 new unit tests using the existing `parseAndTypeCheck` synthetic +AST helper, plus fill 3 gaps in `isTransformationCall` coverage (`io.Reader`, +empty interface, mixed parameter ordering). + +## Capabilities + +### New Capabilities +- `isByteLikeParam`: Detects `[]byte`, `string`, and `io.Reader` parameter types +- `isPointerDestParam`: Detects `*T` and empty interface parameter types +- `matchDirect`: Direct `types.Object` identity matching with helper bridge +- `matchIndirectRoot`: Indirect root resolution matching via `resolveExprRoot` +- `collectTrackedVars`: Collects initial tracked variables from effect ID map +- `traceForwardDataFlow`: Multi-iteration forward data flow tracing through AST +- `matchTrackedInExpr`: Checks if any tracked variable appears in an expression + +### Modified Capabilities +- `isTransformationCall`: Same behavior, reduced complexity (26 → ≤8) +- `matchAssertionToEffect`: Same behavior, reduced complexity (25 → ≤10) +- `matchContainerUnwrap`: Same behavior, reduced complexity (50 → ≤12) + +### Removed Capabilities +- None + +## Impact + +- **Files modified**: `internal/quality/mapping.go`, + `internal/quality/container_unwrap_internal_test.go` +- **No API surface changes**: All three functions are unexported. All extracted + helpers are also unexported. +- **No behavior changes**: Pure decomposition. The `TestSC003_MappingAccuracy` + ratchet (85.0% floor) must pass unchanged. +- **Total complexity reduction**: 101 → ≤30 across the three functions. + +## Constitution Alignment + +Assessed against the Gaze project constitution (v1.3.0). + +### I. Accuracy + +**Assessment**: PASS + +No analysis or mapping logic changes. The `TestSC003_MappingAccuracy` ratchet +(85.0% floor across 66 assertion sites) serves as the regression gate. All +4 existing `matchContainerUnwrap` integration tests and 5 existing +`isTransformationCall` unit tests remain unchanged. + +### II. Minimal Assumptions + +**Assessment**: N/A + +No changes to assumptions about host projects, test frameworks, or coding +styles. + +### III. Actionable Output + +**Assessment**: N/A + +No changes to output formats, report content, or metric computation. +Confidence values (75, 70, 65, 55) are preserved identically. + +### IV. Testability + +**Assessment**: PASS + +Directly improves testability. Seven new helpers are each independently +testable with synthetic AST data via the existing `parseAndTypeCheck` helper. +No `testing.Short()` guards needed — tests use synthetic Go source strings, +not real package loading. Coverage strategy: unit tests on each helper +covering all branches. diff --git a/openspec/changes/crapload-decompose-pr2b/specs/decompose-mapping.md b/openspec/changes/crapload-decompose-pr2b/specs/decompose-mapping.md new file mode 100644 index 0000000..26e11ec --- /dev/null +++ b/openspec/changes/crapload-decompose-pr2b/specs/decompose-mapping.md @@ -0,0 +1,230 @@ +## ADDED Requirements + +### Requirement: isByteLikeParam helper + +`isByteLikeParam` MUST accept a `types.Type` and return `true` if the type +is `[]byte`, `string`, or implements `io.Reader` (has a method named `Read` +in its method set). + +#### Scenario: []byte parameter + +- **GIVEN** a `types.Type` representing `[]byte` (Slice with byte element) +- **WHEN** `isByteLikeParam` is called +- **THEN** it SHALL return `true` + +#### Scenario: string parameter + +- **GIVEN** a `types.Type` representing `string` (Basic with String kind) +- **WHEN** `isByteLikeParam` is called +- **THEN** it SHALL return `true` + +#### Scenario: io.Reader parameter + +- **GIVEN** a `types.Type` representing an interface with a `Read` method +- **WHEN** `isByteLikeParam` is called +- **THEN** it SHALL return `true` + +#### Scenario: non-byte-like parameter + +- **GIVEN** a `types.Type` representing `int` +- **WHEN** `isByteLikeParam` is called +- **THEN** it SHALL return `false` + +### Requirement: isPointerDestParam helper + +`isPointerDestParam` MUST accept a `types.Type` and return `true` if the type +is a pointer (`*T`) or an empty interface (`interface{}` / `any`). + +#### Scenario: pointer parameter + +- **GIVEN** a `types.Type` representing `*int` +- **WHEN** `isPointerDestParam` is called +- **THEN** it SHALL return `true` + +#### Scenario: empty interface parameter + +- **GIVEN** a `types.Type` representing `interface{}` (empty method set) +- **WHEN** `isPointerDestParam` is called +- **THEN** it SHALL return `true` + +#### Scenario: non-pointer non-interface parameter + +- **GIVEN** a `types.Type` representing `int` +- **WHEN** `isPointerDestParam` is called +- **THEN** it SHALL return `false` + +### Requirement: matchDirect helper + +`matchDirect` MUST walk the assertion expression tree via `ast.Inspect`, +look up each `*ast.Ident` in `types.Info.Uses` (falling back to `.Defs`), +and check for a match in `objToEffectID` (confidence 75) or `helperBridge` +→ `objToEffectID` (confidence 70). It MUST return the first match found, +or `nil` if no match exists. + +#### Scenario: Direct identity match + +- **GIVEN** an assertion expression containing an identifier whose + `types.Object` is a key in `objToEffectID` +- **WHEN** `matchDirect` is called +- **THEN** it SHALL return an `AssertionMapping` with confidence 75 + +#### Scenario: Helper bridge match + +- **GIVEN** an assertion expression containing an identifier whose + `types.Object` maps through `helperBridge` to a key in `objToEffectID` +- **WHEN** `matchDirect` is called +- **THEN** it SHALL return an `AssertionMapping` with confidence 70 + +#### Scenario: No match + +- **GIVEN** an assertion expression with no identifiers in `objToEffectID` + or `helperBridge` +- **WHEN** `matchDirect` is called +- **THEN** it SHALL return `nil` + +#### Scenario: Nil expression + +- **GIVEN** a nil assertion expression +- **WHEN** `matchDirect` is called +- **THEN** it SHALL return `nil` + +### Requirement: matchIndirectRoot helper + +`matchIndirectRoot` MUST walk the assertion expression tree, identify +composite expressions (`SelectorExpr`, `IndexExpr`, `CallExpr`), resolve +them to root identifiers via `resolveExprRoot`, and check the root's +`types.Object` against `objToEffectID`. Matches SHALL have confidence 65. + +#### Scenario: Selector expression match + +- **GIVEN** an assertion expression `result.Name` where `result` is in + `objToEffectID` +- **WHEN** `matchIndirectRoot` is called +- **THEN** it SHALL return an `AssertionMapping` with confidence 65 + +#### Scenario: No composite expressions + +- **GIVEN** an assertion expression with only simple identifiers (no + SelectorExpr, IndexExpr, or CallExpr) +- **WHEN** `matchIndirectRoot` is called +- **THEN** it SHALL return `nil` + +### Requirement: collectTrackedVars helper + +`collectTrackedVars` MUST iterate over `objToEffectID` and return all +`types.Object` keys whose effect ID matches `returnEffectID`. + +#### Scenario: Multiple objects map to return effect + +- **GIVEN** an `objToEffectID` map with 3 entries, 2 mapping to + `returnEffectID` +- **WHEN** `collectTrackedVars` is called +- **THEN** the returned map SHALL contain exactly 2 entries + +#### Scenario: No objects map to return effect + +- **GIVEN** an `objToEffectID` map with entries but none matching + `returnEffectID` +- **WHEN** `collectTrackedVars` is called +- **THEN** the returned map SHALL be empty + +### Requirement: traceForwardDataFlow helper + +`traceForwardDataFlow` MUST iterate over AST files in the test package, +walking assignment statements to trace tracked variables forward through +assignments, transformation calls (via `isTransformationCall`), and data +extractions (via `isDataExtraction`). It MUST run multiple iterations +until no new tracked variables are discovered or `maxContainerChainDepth` +is reached. + +#### Scenario: Simple assignment chain + +- **GIVEN** source code `x := target(); y := x.Field` with `x` in the + tracked set +- **WHEN** `traceForwardDataFlow` is called +- **THEN** `y` SHALL be added to the tracked set + +#### Scenario: Transformation call chain + +- **GIVEN** source code with `json.Unmarshal(data, &result)` where `data` + is tracked +- **WHEN** `traceForwardDataFlow` is called +- **THEN** `result` SHALL be added to the tracked set (via pointer + destination extraction) + +#### Scenario: Empty tracked set + +- **GIVEN** an empty tracked set +- **WHEN** `traceForwardDataFlow` is called +- **THEN** it SHALL return an empty tracked set + +#### Scenario: Convergence before max depth + +- **GIVEN** source code where the tracked set stops growing after 2 + iterations (no new assignments reference tracked variables) +- **WHEN** `traceForwardDataFlow` is called with `maxContainerChainDepth` = 6 +- **THEN** it SHALL terminate after 2 iterations, not 6 + +#### Scenario: Non-data-extraction gating + +- **GIVEN** an assignment `got := s.Get("key")` where `s` is tracked but + `Get` is a method call (not a field access, index, or type assertion) +- **WHEN** `traceForwardDataFlow` processes the assignment +- **THEN** `got` SHALL NOT be added to the tracked set (gated by + `isDataExtraction`) + +### Requirement: matchTrackedInExpr helper + +`matchTrackedInExpr` MUST walk the assertion expression tree and return +`true` if any identifier's `types.Object` is in the tracked set. It MUST +check both direct identity and `resolveExprRoot` fallback for composite +expressions. + +#### Scenario: Direct match + +- **GIVEN** an expression containing an identifier in the tracked set +- **WHEN** `matchTrackedInExpr` is called +- **THEN** it SHALL return `true` + +#### Scenario: Root resolution match + +- **GIVEN** an expression `tracked.Field` where `tracked` is in the set +- **WHEN** `matchTrackedInExpr` is called +- **THEN** it SHALL return `true` (via `resolveExprRoot`) + +#### Scenario: No match + +- **GIVEN** an expression with no identifiers in the tracked set +- **WHEN** `matchTrackedInExpr` is called +- **THEN** it SHALL return `false` + +## MODIFIED Requirements + +### Requirement: isTransformationCall complexity + +`isTransformationCall` MUST delegate byte-like detection to `isByteLikeParam` +and pointer destination detection to `isPointerDestParam`. The function's +cyclomatic complexity MUST be 8 or less. + +Previously: All 5 type-checking patterns were inline in a single loop body +with complexity 26. + +### Requirement: matchAssertionToEffect complexity + +`matchAssertionToEffect` MUST delegate Pass 1 to `matchDirect` and Pass 2 +to `matchIndirectRoot`. The function's cyclomatic complexity MUST be 10 or +less. + +Previously: Both passes were inline with complexity 25. + +### Requirement: matchContainerUnwrap complexity + +`matchContainerUnwrap` MUST delegate to `collectTrackedVars`, +`traceForwardDataFlow`, and `matchTrackedInExpr`. The function's cyclomatic +complexity MUST be 12 or less. + +Previously: All concerns were interleaved with complexity 50. + +## REMOVED Requirements + +None — pure decomposition, no functionality removed. diff --git a/openspec/changes/crapload-decompose-pr2b/tasks.md b/openspec/changes/crapload-decompose-pr2b/tasks.md new file mode 100644 index 0000000..618b1ce --- /dev/null +++ b/openspec/changes/crapload-decompose-pr2b/tasks.md @@ -0,0 +1,182 @@ + + +## 1. Decompose `isTransformationCall` (26 → ≤8) + +All tasks modify the same files and MUST run sequentially. + +- [x] 1.1 Extract `isByteLikeParam` + `isPointerDestParam` helpers + - Extract `isByteLikeParam(typ types.Type) bool` from the loop body + in `isTransformationCall` (lines 772-814). Consolidates 3 patterns: + - `[]byte`: `types.Slice` with `types.Basic` byte element + - `string`: `types.Basic` with `types.String` kind + - `io.Reader`: interface with `Read` method via `types.NewMethodSet` + - Extract `isPointerDestParam(typ types.Type) bool` from the loop body + (lines 790-822). Consolidates 2 patterns: + - `*T`: `types.Pointer` + - Empty interface: `types.Interface` with zero methods + - Replace inline logic in `isTransformationCall` with calls to + `isByteLikeParam` and `isPointerDestParam` + - Add GoDoc comments on both helpers + - **Files**: `internal/quality/mapping.go` + +- [x] 1.2 Add gap tests for `isTransformationCall` + - Add 3 tests to `internal/quality/container_unwrap_internal_test.go`: + - `TestIsTransformationCall_IoReaderAndPointer` — function with + `io.Reader` + `*int` parameters → positive, correct indices + - `TestIsTransformationCall_EmptyInterfaceAsPointerDest` — function + with `[]byte` + `interface{}` parameters → positive + - `TestIsTransformationCall_PointerBeforeByteSlice` — function with + `*int` before `[]byte` → positive, correct indices (verifies + parameter ordering doesn't matter) + - Add 7 tests for the extracted helpers: + - `TestIsByteLikeParam_ByteSlice` — `[]byte` → true + - `TestIsByteLikeParam_String` — `string` → true + - `TestIsByteLikeParam_IoReader` — interface with `Read` method → true + - `TestIsByteLikeParam_NonReadInterface` — interface without `Read` → false + - `TestIsByteLikeParam_Int` — `int` → false + - `TestIsPointerDestParam_Pointer` — `*int` → true + - `TestIsPointerDestParam_EmptyInterface` — `interface{}` → true + - `TestIsPointerDestParam_Int` — `int` → false + - Tests MUST NOT use `testing.Short()` guard + - Use the existing `parseAndTypeCheck` helper for synthetic AST + - **Files**: `internal/quality/container_unwrap_internal_test.go` + +## 2. Decompose `matchAssertionToEffect` (25 → ≤10) + +- [x] 2.1 Extract `matchDirect` + `matchIndirectRoot` helpers + - Extract `matchDirect(site, objToEffectID, effectMap, info, + helperBridge) *taxonomy.AssertionMapping` from the first + `ast.Inspect` closure (lines 1212-1269). Handles: + - Direct identity matching at confidence 75 + - Helper bridge matching at confidence 70 + - Skips nil/true/false literal identifiers + - Extract `matchIndirectRoot(site, objToEffectID, effectMap, + info) *taxonomy.AssertionMapping` from the second `ast.Inspect` + closure (lines 1279-1324). Handles: + - SelectorExpr/IndexExpr/CallExpr detection + - `resolveExprRoot` unwinding + - Indirect matching at confidence 65 + - Replace inline closures in `matchAssertionToEffect` with: + ``` + if m := matchDirect(...); m != nil { return m } + return matchIndirectRoot(...) + ``` + - Add GoDoc comments on both helpers + - **Files**: `internal/quality/mapping.go` + +- [x] 2.2 Add unit tests for `matchAssertionToEffect` helpers + - Add tests to `internal/quality/container_unwrap_internal_test.go` + using `parseAndTypeCheck`: + - `TestMatchDirect_IdentityMatch` — expression contains identifier + in `objToEffectID` → mapping with confidence 75 + - `TestMatchDirect_HelperBridgeMatch` — identifier maps through + `helperBridge` → mapping with confidence 70 + - `TestMatchDirect_NoMatch` — no identifiers in maps → nil + - `TestMatchDirect_NilExpr` — nil expression → nil + - `TestMatchIndirectRoot_SelectorMatch` — `result.Name` where + `result` is in `objToEffectID` → mapping with confidence 65 + - `TestMatchIndirectRoot_NoComposite` — simple identifier only → nil + - Tests MUST NOT use `testing.Short()` guard + - **Files**: `internal/quality/container_unwrap_internal_test.go` + +## 3. Decompose `matchContainerUnwrap` (50 → ≤12) + +- [x] 3.1 Extract `collectTrackedVars` + `matchTrackedInExpr` helpers + - Extract `collectTrackedVars(objToEffectID, returnEffectID) + map[types.Object]bool` from lines 974-982. + Pure function: filters `objToEffectID` entries by `returnEffectID`. + - Extract `matchTrackedInExpr(expr, tracked, info) bool` from + lines 1106-1151. Walks expression tree via `ast.Inspect`, checks + both direct identity and `resolveExprRoot` fallback. + - Replace inline logic in `matchContainerUnwrap` with calls to + these helpers + - Add GoDoc comments + - **Files**: `internal/quality/mapping.go` + +- [x] 3.2 Extract `traceForwardDataFlow` helper + - Extract `traceForwardDataFlow(tracked, testPkg) + map[types.Object]bool` from lines 987-1104. + Multi-iteration forward data-flow tracer: + - Walks AST assignment statements + - Checks if RHS references tracked variables (via `containsObject` + and `resolveExprRoot`) + - Detects transformation calls via `isTransformationCall` + - Extracts pointer destinations via `extractPointerDest` + - Gates non-transformation assignments via `isDataExtraction` + - Iterates until convergence or `maxContainerChainDepth` + - Replace inline logic in `matchContainerUnwrap` with: + ``` + tracked = traceForwardDataFlow(tracked, testPkg) + ``` + - Add GoDoc comment + - **Files**: `internal/quality/mapping.go` + +- [x] 3.3 Add unit tests for `matchContainerUnwrap` helpers + - Add tests to `internal/quality/container_unwrap_internal_test.go`: + - `TestCollectTrackedVars_MultipleMatches` — 3 entries, 2 match + returnEffectID → 2 in result + - `TestCollectTrackedVars_NoMatches` — no entries match → empty map + - `TestTraceForwardDataFlow_SimpleChain` — assignment `y := x.Field` + with `x` tracked → `y` added. Construct `packages.Package` with + `Syntax: []*ast.File{file}` and `TypesInfo: info` from + `parseAndTypeCheck` + - `TestTraceForwardDataFlow_EmptyTracked` — empty tracked set → + returns empty + - `TestTraceForwardDataFlow_NonDataExtraction` — method call + `got := s.Get("key")` where `s` is tracked → `got` NOT added + (gated by `isDataExtraction`) + - `TestMatchTrackedInExpr_DirectMatch` — identifier in tracked → true + - `TestMatchTrackedInExpr_RootResolution` — `tracked.Field` → true + - `TestMatchTrackedInExpr_NoMatch` — unrelated identifier → false + - Tests MUST NOT use `testing.Short()` guard + - **Files**: `internal/quality/container_unwrap_internal_test.go` + +## 4. Verification + +- [x] 4.1 Build and test + - Run `go build ./...` — MUST pass + - Run `go test -race -count=1 -short ./...` — MUST pass + - Run `golangci-lint run` — MUST pass with 0 issues + - **Verify** complexity targets: + - `isTransformationCall` ≤ 8 + - `matchAssertionToEffect` ≤ 10 + - `matchContainerUnwrap` ≤ 12 + (check via `gocyclo -over 12 internal/quality/mapping.go`) + - **Verify** mapping accuracy ratchet: + `go test -race -count=1 -run TestSC003_MappingAccuracy ./internal/quality/...` + — MUST pass at 85.0% floor + - **Verify** GoDoc comments on all new helpers start with function name + - **Verify** net test delta: ~20 new tests added, 0 removed + +- [x] 4.2 Constitution alignment verification + - **Accuracy (I)**: Confirm `TestSC003_MappingAccuracy` passes at + 85.0% — mapping behavior unchanged + - **Testability (IV)**: Confirm all new tests run without + `testing.Short()` guard — they contribute to CRAPload + - **Minimal Assumptions (II)**: N/A — no analysis behavior changed + - **Actionable Output (III)**: N/A — no output format changed + +## 5. Documentation + +- [x] 5.1 Update AGENTS.md Recent Changes + - Add entry for `crapload-decompose-pr2b` describing the decomposition + - **Files**: `AGENTS.md` + +- [x] 5.2 Confirm no README/website updates needed + - This is internal-only refactoring with no user-facing changes + - No new CLI commands, flags, or output formats + - No website issue required + + + +