From f37e3e8b2fde680f64c0efc2f2c5c7df2f8d257b Mon Sep 17 00:00:00 2001 From: Krishan Kant Sharma Date: Tue, 7 Jul 2026 14:10:33 -0500 Subject: [PATCH 1/2] test(scanner): confirm --strict-types closes #16, transitive factory wrapping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #16: a cross-package factory function (wrapper.NewClientWrapper) whose declared return type isn't *ld.LDClient directly but a wrapper struct with a client field — Phase 1 doesn't register it as a factory at all (its return type doesn't match), so w.Inner.BoolVariation(...) goes undetected even though w.Inner really is a client. Turns out no new resolution logic was needed — found while building this regression test, not before. resolveByStaticType (identity.go, added in the --strict-types Phase 2a work that closed #15) checks *any* expression's real static type against the SDK client type, not just an assignment's RHS. A field selector like w.Inner is exactly such an expression: go/types reports its type the same way regardless of how the struct it's read from was constructed, so the interface-satisfaction fix already resolves this transitive-factory case as a natural consequence. Added a real, buildable fixture (testdata/strict/transitive_factory, cross-package: wrapper + consumer, same local-replace-stub-SDK pattern as the interface_satisfaction fixture) reproducing issue #16's exact repro, and a regression test proving Phase 1 misses it while ScanStrict finds it correctly. Re-verified against weaviate: same 4 real usages, no regressions, no --strict-types warnings. Fixes #16 Signed-off-by: Krishan Kant Sharma --- internal/scanner/strict_test.go | 51 +++++++++++++++++++ .../transitive_factory/consumer/.gitignore | 1 + .../transitive_factory/consumer/main.go | 19 +++++++ .../testdata/strict/transitive_factory/go.mod | 7 +++ .../transitive_factory/stubsdk/client.go | 16 ++++++ .../strict/transitive_factory/stubsdk/go.mod | 3 ++ .../transitive_factory/wrapper/wrapper.go | 23 +++++++++ 7 files changed, 120 insertions(+) create mode 100644 internal/scanner/testdata/strict/transitive_factory/consumer/.gitignore create mode 100644 internal/scanner/testdata/strict/transitive_factory/consumer/main.go create mode 100644 internal/scanner/testdata/strict/transitive_factory/go.mod create mode 100644 internal/scanner/testdata/strict/transitive_factory/stubsdk/client.go create mode 100644 internal/scanner/testdata/strict/transitive_factory/stubsdk/go.mod create mode 100644 internal/scanner/testdata/strict/transitive_factory/wrapper/wrapper.go diff --git a/internal/scanner/strict_test.go b/internal/scanner/strict_test.go index 4202630..3c043e4 100644 --- a/internal/scanner/strict_test.go +++ b/internal/scanner/strict_test.go @@ -71,6 +71,57 @@ func TestScanStrict_positiveInterfaceSatisfaction(t *testing.T) { } } +func TestScanStrict_positiveTransitiveFactoryWrapping(t *testing.T) { + // Issue #16's exact repro: a cross-package factory function + // (wrapper.NewClientWrapper) whose declared return type isn't + // *ld.LDClient directly but a wrapper struct with a client field, + // called from a different package than the one that constructs it. + // Turns out this needed no new resolution logic at all — found while + // building this regression test, not before: resolveByStaticType + // (identity.go, added for issue #15's interface-satisfaction fix) + // checks *any* expression's real static type against the SDK client + // type, not just an assignment's RHS — so `w.Inner` in + // `w.Inner.BoolVariation(...)` resolves correctly as a byproduct, + // since go/types reports a field selector's type exactly like any + // other expression's, regardless of how the struct it's read from was + // itself constructed. + dir := "testdata/strict/transitive_factory" + 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 — transitive factory wrapping 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) != 1 { + t.Fatalf("ScanStrict found %d usage(s), want 1: %+v", len(strict.Usages), strict.Usages) + } + got := strict.Usages[0] + if got.FlagKey != "transitive-factory-flag" { + t.Errorf("usages[0].FlagKey = %q, want transitive-factory-flag", got.FlagKey) + } + 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 diff --git a/internal/scanner/testdata/strict/transitive_factory/consumer/.gitignore b/internal/scanner/testdata/strict/transitive_factory/consumer/.gitignore new file mode 100644 index 0000000..abf2d2a --- /dev/null +++ b/internal/scanner/testdata/strict/transitive_factory/consumer/.gitignore @@ -0,0 +1 @@ +/consumer diff --git a/internal/scanner/testdata/strict/transitive_factory/consumer/main.go b/internal/scanner/testdata/strict/transitive_factory/consumer/main.go new file mode 100644 index 0000000..95c287b --- /dev/null +++ b/internal/scanner/testdata/strict/transitive_factory/consumer/main.go @@ -0,0 +1,19 @@ +package main + +import "flaglint-strict-fixture-transitive-factory/wrapper" + +// Reproduces issue #16 exactly: w.Inner.BoolVariation(...) is not +// detected by Phase 1, since wrapper.NewClientWrapper isn't a registered +// factory function (its return type is *ClientWrapper, not *ld.LDClient +// directly) — the composite-literal binding establishing +// ClientWrapper.Inner also lives in a different package than this call +// site, so Phase 1's same-package composite-literal resolution doesn't +// reach it either. +func run() { + w := wrapper.NewClientWrapper() + _, _ = w.Inner.BoolVariation("transitive-factory-flag", nil, false) +} + +func main() { + run() +} diff --git a/internal/scanner/testdata/strict/transitive_factory/go.mod b/internal/scanner/testdata/strict/transitive_factory/go.mod new file mode 100644 index 0000000..8095b46 --- /dev/null +++ b/internal/scanner/testdata/strict/transitive_factory/go.mod @@ -0,0 +1,7 @@ +module flaglint-strict-fixture-transitive-factory + +go 1.22 + +require github.com/launchdarkly/go-server-sdk/v7 v7.0.0 + +replace github.com/launchdarkly/go-server-sdk/v7 => ./stubsdk diff --git a/internal/scanner/testdata/strict/transitive_factory/stubsdk/client.go b/internal/scanner/testdata/strict/transitive_factory/stubsdk/client.go new file mode 100644 index 0000000..b72309a --- /dev/null +++ b/internal/scanner/testdata/strict/transitive_factory/stubsdk/client.go @@ -0,0 +1,16 @@ +// 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 "time" + +type LDClient struct{} + +func MakeClient(sdkKey string, waitFor time.Duration) (*LDClient, error) { + return &LDClient{}, nil +} + +func (c *LDClient) BoolVariation(key string, ctx interface{}, defaultVal bool) (bool, error) { + return defaultVal, nil +} diff --git a/internal/scanner/testdata/strict/transitive_factory/stubsdk/go.mod b/internal/scanner/testdata/strict/transitive_factory/stubsdk/go.mod new file mode 100644 index 0000000..032528e --- /dev/null +++ b/internal/scanner/testdata/strict/transitive_factory/stubsdk/go.mod @@ -0,0 +1,3 @@ +module github.com/launchdarkly/go-server-sdk/v7 + +go 1.22 diff --git a/internal/scanner/testdata/strict/transitive_factory/wrapper/wrapper.go b/internal/scanner/testdata/strict/transitive_factory/wrapper/wrapper.go new file mode 100644 index 0000000..a496d52 --- /dev/null +++ b/internal/scanner/testdata/strict/transitive_factory/wrapper/wrapper.go @@ -0,0 +1,23 @@ +package wrapper + +import ( + "time" + + ld "github.com/launchdarkly/go-server-sdk/v7" +) + +// ClientWrapper is issue #16's exact repro shape: a factory function +// (NewClientWrapper) whose *declared return type* is not *ld.LDClient +// directly, but a wrapper struct that itself has a client field. +type ClientWrapper struct { + Inner *ld.LDClient +} + +// NewClientWrapper is NOT a registered Phase 1 factory function (its +// return type is *ClientWrapper, not *ld.LDClient) — closing this +// generally requires real go/types information (--strict-types), per +// issue #16 and ADR 005. +func NewClientWrapper() *ClientWrapper { + client, _ := ld.MakeClient("sdk-key", 5*time.Second) + return &ClientWrapper{Inner: client} +} From 830e5641260d7affdf8b35fe6e1c96c33f859218 Mon Sep 17 00:00:00 2001 From: Krishan Kant Sharma Date: Tue, 7 Jul 2026 14:13:51 -0500 Subject: [PATCH 2/2] test(scanner): lock in two-hop generalization for #16 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Suggested by independent review of the previous commit: the one-hop fixture (w.Inner.BoolVariation) proves resolveByStaticType resolves a single field-selector hop, but doesn't explicitly prove the mechanism generalizes to a deeper chain rather than happening to work by coincidence for exactly one hop. Added OuterWrapper (wrapper.go) — Middle *ClientWrapper, two hops removed from the SDK client — and runTwoHop (consumer/main.go), evaluating o.Middle.Inner.BoolVariation(...). Confirmed: resolves correctly with no new code, exactly as expected, since resolveByStaticType queries go/types.Info.TypeOf for the whole expression's real type directly rather than manually walking one hop at a time (that manual walking is Phase 1's syntax-only structFieldTypes chain resolution, never invoked once typesInfo is populated). Phase 1 still correctly misses both call sites (confirmed by the test, not assumed). Signed-off-by: Krishan Kant Sharma --- internal/scanner/strict_test.go | 36 +++++++++++++------ .../transitive_factory/consumer/main.go | 8 +++++ .../transitive_factory/wrapper/wrapper.go | 14 ++++++++ 3 files changed, 47 insertions(+), 11 deletions(-) diff --git a/internal/scanner/strict_test.go b/internal/scanner/strict_test.go index 3c043e4..c6fc518 100644 --- a/internal/scanner/strict_test.go +++ b/internal/scanner/strict_test.go @@ -107,18 +107,32 @@ func TestScanStrict_positiveTransitiveFactoryWrapping(t *testing.T) { 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 != "transitive-factory-flag" { - t.Errorf("usages[0].FlagKey = %q, want transitive-factory-flag", got.FlagKey) - } - if got.DetectedBy != "strict-types" { - t.Errorf("usages[0].DetectedBy = %q, want strict-types", got.DetectedBy) + // 2: the one-hop repro (w.Inner.BoolVariation) and a two-hop variant + // (o.Middle.Inner.BoolVariation) — proving resolveByStaticType + // generalizes past a single field-selector hop for free, since it + // queries go/types for the whole expression's real type directly + // rather than manually walking one hop at a time. + if len(strict.Usages) != 2 { + t.Fatalf("ScanStrict found %d usage(s), want 2: %+v", len(strict.Usages), strict.Usages) + } + wantFlagKeys := map[string]bool{"transitive-factory-flag": false, "two-hop-transitive-factory-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.SDK != "go-server-sdk-v7" { + t.Errorf("usage %q SDK = %q, want go-server-sdk-v7", got.FlagKey, got.SDK) + } } - if got.SDK != "go-server-sdk-v7" { - t.Errorf("usages[0].SDK = %q, want go-server-sdk-v7", got.SDK) + for k, found := range wantFlagKeys { + if !found { + t.Errorf("missing expected usage for flag key %q", k) + } } } diff --git a/internal/scanner/testdata/strict/transitive_factory/consumer/main.go b/internal/scanner/testdata/strict/transitive_factory/consumer/main.go index 95c287b..10d522b 100644 --- a/internal/scanner/testdata/strict/transitive_factory/consumer/main.go +++ b/internal/scanner/testdata/strict/transitive_factory/consumer/main.go @@ -14,6 +14,14 @@ func run() { _, _ = w.Inner.BoolVariation("transitive-factory-flag", nil, false) } +// runTwoHop proves the resolution generalizes past a single field-selector +// hop — o.Middle.Inner, not just w.Inner. +func runTwoHop() { + o := wrapper.NewOuterWrapper() + _, _ = o.Middle.Inner.BoolVariation("two-hop-transitive-factory-flag", nil, false) +} + func main() { run() + runTwoHop() } diff --git a/internal/scanner/testdata/strict/transitive_factory/wrapper/wrapper.go b/internal/scanner/testdata/strict/transitive_factory/wrapper/wrapper.go index a496d52..64b4473 100644 --- a/internal/scanner/testdata/strict/transitive_factory/wrapper/wrapper.go +++ b/internal/scanner/testdata/strict/transitive_factory/wrapper/wrapper.go @@ -21,3 +21,17 @@ func NewClientWrapper() *ClientWrapper { client, _ := ld.MakeClient("sdk-key", 5*time.Second) return &ClientWrapper{Inner: client} } + +// OuterWrapper adds a second hop (Middle.Inner, not just Inner) — proving +// resolveByStaticType's go/types.Info.TypeOf(rhs) generalizes to an +// arbitrarily deep field-selector chain for free, since it queries the +// expression's real type directly rather than manually walking one hop at +// a time the way Phase 1's syntax-only structFieldTypes chain resolution +// does. +type OuterWrapper struct { + Middle *ClientWrapper +} + +func NewOuterWrapper() *OuterWrapper { + return &OuterWrapper{Middle: NewClientWrapper()} +}