feat(scanner): --strict-types forwarding-call detection, closing #26

internal/scanner/strict.go

@@ -2,6 +2,7 @@ package scanner
 
 import (
 	"go/token"
+	gotypes "go/types"
 	"path/filepath"
 	"sort"
 	"strconv"
@@ -55,9 +56,57 @@ func ScanStrict(root string, cfg config.Config) (types.ScanResult, error) {
 	}
 
 	mergeStrictTypesUsages(&result, strictTypesUsages(pkgs, absRoot))
+	mergeStrictTypesUsages(&result, forwardingCallUsages(pkgs, absRoot))
 	return result, nil
 }
 
+// forwardingCallUsages is issue #26's detection entry point (ADR 006,
+// extended — see forwarding.go's package doc comment for why): it builds
+// the whole-scan indices (accessor methods, factory-constructor field
+// sources, package-level flag-descriptor var literals, and the fixed-
+// point "which functions evaluate a flag through a forwarded/pass-through
+// callback" index) across every loaded package, then walks every loaded
+// package's syntax for call sites matching that shape. Kept separate from
+// strictTypesUsages/runWholeScanAnalysis, since none of this needs
+// scope-tracked bindings at all (resolveByStaticType queries go/types
+// directly for whatever expression it's given), unlike every other
+// Phase 1/2a detection mechanism.
+func forwardingCallUsages(pkgs []*packages.Package, absRoot string) []types.FlagUsage {
+	var fset *token.FileSet
+	accessors := map[accessorKey]string{}
+	factoryFields := map[*gotypes.Func]map[string]int{}
+	for _, pkg := range pkgs {
+		if pkg.Fset == nil || pkg.TypesInfo == nil {
+			continue
+		}
+		if fset == nil {
+			fset = pkg.Fset
+		}
+		for k, v := range accessorFields(pkg.Syntax, pkg.PkgPath) {
+			accessors[k] = v
+		}
+		for k, v := range factoryFieldParams(pkg.Syntax, pkg.TypesInfo) {
+			factoryFields[k] = v
+		}
+	}
+	if fset == nil {
+		return nil
+	}
+
+	literalVars := map[*gotypes.Var]map[string]string{}
+	for _, pkg := range pkgs {
+		if pkg.TypesInfo == nil {
+			continue
+		}
+		for k, v := range flagDescriptorLiterals(pkg.Syntax, pkg.TypesInfo, factoryFields) {
+			literalVars[k] = v
+		}
+	}
+
+	summaries := findEvalSummaries(pkgs)
+	return detectForwardingCallUsages(fset, pkgs, absRoot, summaries, accessors, literalVars, factoryFields)
+}
+
 // strictTypesUsages re-runs the whole-scan analysis over go/packages-loaded
 // ASTs, which carry real go/types information (Scan's own ASTs, parsed
 // independently via go/parser, never do) — this is what lets

internal/scanner/strict_test.go

@@ -4,6 +4,7 @@ import (
 	"testing"
 
 	"github.com/flaglint/flaglint-go/internal/config"
+	"github.com/flaglint/flaglint-go/internal/types"
 )
 
 func TestScanStrict_positiveInterfaceSatisfaction(t *testing.T) {
@@ -136,6 +137,118 @@ func TestScanStrict_positiveTransitiveFactoryWrapping(t *testing.T) {
 	}
 }
 
+func TestScanStrict_positiveForwardingCall(t *testing.T) {
+	// Issue #26 (ADR 006): a method value captured in one function,
+	// passed as an argument, and invoked from inside a different callee
+	// ("forwarding function") — the harder remainder after #6 fixed the
+	// same-scope case. Covers: a non-generic forwarding function, a
+	// generic one matching the issue's exact repro (proving
+	// findEvalSummaries resolves the pre-instantiation *types.Func, not a
+	// per-call-site instantiated wrapper), a one-hop "pass-through"
+	// function (fixes the SDK method internally, forwards only its own
+	// key parameter — the simplified version of the real e2b-dev/infra
+	// shape), a false-positive guard (unrelated same-shaped method
+	// value), and a genuinely dynamic (non-literal) key guard.
+	dir := "testdata/strict/forwarding_call"
+	cfg := config.Config{
+		Include:  []string{"**/*.go"},
+		Exclude:  []string{"**/vendor/**", "**/.git/**"},
+		Provider: "launchdarkly",
+	}
+
+	phase1, err := Scan(dir, cfg)
+	if err != nil {
+		t.Fatalf("Scan error = %v", err)
+	}
+	if len(phase1.Usages) != 0 {
+		t.Fatalf("Scan (Phase 1) found %d usage(s), want 0 — a method value crossing a function boundary is exactly what Phase 1 cannot see: %+v", len(phase1.Usages), phase1.Usages)
+	}
+
+	strict, err := ScanStrict(dir, cfg)
+	if err != nil {
+		t.Fatalf("ScanStrict error = %v", err)
+	}
+	if len(strict.Warnings) != 0 {
+		t.Fatalf("ScanStrict warnings = %+v, want none — the fixture module builds cleanly", strict.Warnings)
+	}
+	if len(strict.Usages) != 3 {
+		t.Fatalf("ScanStrict found %d usage(s), want exactly 3 (should-not-be-detected and the genuinely dynamic-key call must both be absent): %+v", len(strict.Usages), strict.Usages)
+	}
+
+	wantFlagKeys := map[string]bool{"forwarding-direct-flag": false, "forwarding-generic-flag": false, "pass-through-flag": false}
+	for _, got := range strict.Usages {
+		if _, ok := wantFlagKeys[got.FlagKey]; !ok {
+			t.Errorf("unexpected usage %+v", got)
+			continue
+		}
+		wantFlagKeys[got.FlagKey] = true
+		if got.DetectedBy != "strict-types" {
+			t.Errorf("usage %q DetectedBy = %q, want strict-types", got.FlagKey, got.DetectedBy)
+		}
+		if got.IsDynamic {
+			t.Errorf("usage %q IsDynamic = true, want false", got.FlagKey)
+		}
+		if got.SDK != "go-server-sdk-v7" {
+			t.Errorf("usage %q SDK = %q, want go-server-sdk-v7", got.FlagKey, got.SDK)
+		}
+	}
+	for k, found := range wantFlagKeys {
+		if !found {
+			t.Errorf("missing expected usage for flag key %q", k)
+		}
+	}
+}
+
+func TestScanStrict_positiveFlagDescriptorChain(t *testing.T) {
+	// The real e2b-dev/infra shape found during --strict-types
+	// verification against that repo (issue #26's actual motivating
+	// case) — a package-level var (SnapshotFeatureFlag) built by a
+	// factory (NewBoolFlag) that stores a literal into a struct field,
+	// read back via a trivial accessor (Key), forwarded through TWO
+	// levels of function wrapping (Client.BoolFlag, a pass-through, then
+	// getFlag, a direct forwarding function) before reaching the SDK
+	// call. See testdata/strict/flag_descriptor/main.go and
+	// forwarding.go's package doc comment.
+	dir := "testdata/strict/flag_descriptor"
+	cfg := config.Config{
+		Include:  []string{"**/*.go"},
+		Exclude:  []string{"**/vendor/**", "**/.git/**"},
+		Provider: "launchdarkly",
+	}
+
+	phase1, err := Scan(dir, cfg)
+	if err != nil {
+		t.Fatalf("Scan error = %v", err)
+	}
+	if len(phase1.Usages) != 0 {
+		t.Fatalf("Scan (Phase 1) found %d usage(s), want 0: %+v", len(phase1.Usages), phase1.Usages)
+	}
+
+	strict, err := ScanStrict(dir, cfg)
+	if err != nil {
+		t.Fatalf("ScanStrict error = %v", err)
+	}
+	if len(strict.Warnings) != 0 {
+		t.Fatalf("ScanStrict warnings = %+v, want none — the fixture module builds cleanly", strict.Warnings)
+	}
+	if len(strict.Usages) != 1 {
+		t.Fatalf("ScanStrict found %d usage(s), want 1: %+v", len(strict.Usages), strict.Usages)
+	}
+	got := strict.Usages[0]
+	if got.FlagKey != "use-nfs-for-snapshots" {
+		t.Errorf("usages[0].FlagKey = %q, want use-nfs-for-snapshots", got.FlagKey)
+	}
+	if got.CallType != types.CallType("BoolVariationCtx") {
+		t.Errorf("usages[0].CallType = %q, want BoolVariationCtx", got.CallType)
+	}
+	if got.DetectedBy != "strict-types" {
+		t.Errorf("usages[0].DetectedBy = %q, want strict-types", got.DetectedBy)
+	}
+	if got.SDK != "go-server-sdk-v7" {
+		t.Errorf("usages[0].SDK = %q, want go-server-sdk-v7", got.SDK)
+	}
+}
+
 func TestScanStrict_failedLoadFallsBackToPhase1(t *testing.T) {
 	// A directory with no go.mod at all (or above it) fails to load as a
 	// module — ScanStrict must still return Phase 1's result plus a

internal/scanner/testdata/strict/flag_descriptor/.gitignore

@@ -0,0 +1 @@
+/flaglint-strict-fixture-flag-descriptor

internal/scanner/testdata/strict/flag_descriptor/go.mod

@@ -0,0 +1,7 @@
+module flaglint-strict-fixture-flag-descriptor
+
+go 1.22
+
+require github.com/launchdarkly/go-server-sdk/v7 v7.0.0
+
+replace github.com/launchdarkly/go-server-sdk/v7 => ./stubsdk

internal/scanner/testdata/strict/flag_descriptor/main.go

@@ -0,0 +1,92 @@
+package main
+
+import (
+	"context"
+	"time"
+
+	ld "github.com/launchdarkly/go-server-sdk/v7"
+)
+
+// This fixture mirrors the real e2b-dev/infra shape that motivated issue
+// #26 (found during --strict-types verification against that repo,
+// beyond ADR 006's original "direct forwarding function" scope):
+//
+//   - BoolFlag is a "flag descriptor" struct — a factory constructor
+//     (NewBoolFlag) stores a literal name into its "name" field, read
+//     back through a trivial accessor method (Key).
+//   - A package-level var (SnapshotFeatureFlag) is constructed via that
+//     factory with a literal flag name.
+//   - getFlag is a "direct forwarding" function: it calls its own
+//     function-typed parameter, using flag.Key() (a method call on
+//     another of its own parameters) as the key argument — critically,
+//     ctx (ALSO one of getFlag's own parameters) is passed at an EARLIER
+//     argument position than flag.Key(), exactly like the real code. The
+//     first implementation of this fixture put the flag descriptor
+//     first and had no ctx parameter at all, which accidentally passed
+//     even though the underlying bug (assuming the *first* argument
+//     referencing one of the function's own parameters is always the
+//     key) was real — only caught by re-verifying against e2b-dev/infra
+//     itself, where ctx genuinely does come first.
+//   - Client.BoolFlag is a "pass-through" function: it doesn't call a
+//     callback parameter of its own at all — it calls getFlag with an
+//     already-resolved, concrete client method value, forwarding only
+//     its own "flag" parameter into getFlag's flag-descriptor position.
+//
+// Closing the loop requires chaining accessor-method recognition,
+// factory-constructor field tracking, package-var literal resolution,
+// and pass-through discovery — see forwarding.go's package doc comment.
+type typedFlag[T any] interface {
+	Key() string
+	Fallback() T
+}
+
+type BoolFlag struct {
+	name     string
+	fallback bool
+}
+
+func (f BoolFlag) Key() string    { return f.name }
+func (f BoolFlag) Fallback() bool { return f.fallback }
+func (f BoolFlag) String() string { return f.name }
+
+func NewBoolFlag(name string, fallback bool) BoolFlag {
+	flag := BoolFlag{name: name, fallback: fallback}
+	return flag
+}
+
+var SnapshotFeatureFlag = NewBoolFlag("use-nfs-for-snapshots", false)
+
+type Client struct {
+	ld *ld.LDClient
+}
+
+func NewClient() (*Client, error) {
+	ldClient, err := ld.MakeClient("sdk-key", 5*time.Second)
+	if err != nil {
+		return nil, err
+	}
+	return &Client{ld: ldClient}, nil
+}
+
+func getFlag[T any](
+	ctx context.Context,
+	ldClient *ld.LDClient,
+	getFromLaunchDarkly func(ctx context.Context, key string, defaultVal T) (T, error),
+	flag typedFlag[T],
+) T {
+	value, _ := getFromLaunchDarkly(ctx, flag.Key(), flag.Fallback())
+	return value
+}
+
+func (c *Client) BoolFlag(ctx context.Context, flag BoolFlag) bool {
+	return getFlag(ctx, c.ld, c.ld.BoolVariationCtx, flag)
+}
+
+func run() {
+	client, _ := NewClient()
+	_ = client.BoolFlag(context.Background(), SnapshotFeatureFlag)
+}
+
+func main() {
+	run()
+}

internal/scanner/testdata/strict/flag_descriptor/stubsdk/client.go

@@ -0,0 +1,19 @@
+// Package ldclient is a minimal local stand-in for the real
+// github.com/launchdarkly/go-server-sdk/v7 — see the sibling
+// interface_satisfaction fixture's stubsdk for why.
+package ldclient
+
+import (
+	"context"
+	"time"
+)
+
+type LDClient struct{}
+
+func MakeClient(sdkKey string, waitFor time.Duration) (*LDClient, error) {
+	return &LDClient{}, nil
+}
+
+func (c *LDClient) BoolVariationCtx(ctx context.Context, key string, defaultVal bool) (bool, error) {
+	return defaultVal, nil
+}

internal/scanner/testdata/strict/flag_descriptor/stubsdk/go.mod

@@ -0,0 +1,3 @@
+module github.com/launchdarkly/go-server-sdk/v7
+
+go 1.22

internal/scanner/testdata/strict/flag_descriptor/unrelated/unrelated.go

@@ -0,0 +1,25 @@
+// Package unrelated deliberately declares a type named exactly "BoolFlag"
+// with a method also named "Key" — same names as the real flag
+// descriptor in the parent package, but reading a *different* field
+// ("identifier", not "name"). Proves accessorKey's package-qualification
+// (found via independent review of an earlier, unqualified version of
+// this mechanism) actually keeps the two apart, rather than one
+// package's entry silently overwriting the other's in the shared
+// cross-package accessorFields index — which would otherwise make
+// TestScanStrict_positiveFlagDescriptorChain's result depend on Go's
+// randomized map iteration order.
+package unrelated
+
+type BoolFlag struct {
+	identifier string
+	value      bool
+}
+
+func (f BoolFlag) Key() string    { return f.identifier }
+func (f BoolFlag) Fallback() bool { return f.value }
+
+func NewBoolFlag(identifier string, value bool) BoolFlag {
+	return BoolFlag{identifier: identifier, value: value}
+}
+
+var UnrelatedConfigFlag = NewBoolFlag("unrelated-config-flag", false)

internal/scanner/testdata/strict/forwarding_call/.gitignore

@@ -0,0 +1 @@
+/flaglint-strict-fixture-forwarding-call

internal/scanner/testdata/strict/forwarding_call/go.mod

@@ -0,0 +1,7 @@
+module flaglint-strict-fixture-forwarding-call
+
+go 1.22
+
+require github.com/launchdarkly/go-server-sdk/v7 v7.0.0
+
+replace github.com/launchdarkly/go-server-sdk/v7 => ./stubsdk