test(scanner): confirm --strict-types closes #16, transitive factory wrapping#29
Conversation
…wrapping 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 <krishansharma0327@gmail.com>
|
Warning Review limit reached
Next review available in: 44 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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 <krishansharma0327@gmail.com>
2 participants
Summary
Closes #16. This is a test-only PR — no new resolution logic needed.
Issue #16: a cross-package factory function (
wrapper.NewClientWrapper) whose declared return type isn't*ld.LDClientdirectly 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), sow.Inner.BoolVariation(...)goes undetected even thoughw.Innerreally is a client.Turns out no new resolution logic was needed — found while building this regression test, not before.
resolveByStaticType(added in the--strict-typesPhase 2a work that closed #15, PR #28) checks any expression's real static type against the SDK client type, not just an assignment's RHS. A field selector likew.Inneris exactly such an expression:go/typesreports 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.What changed
testdata/strict/transitive_factory, cross-package:wrapper+consumer, same local-replace-stub-SDK pattern asinterface_satisfaction) reproducing issue scanner: factory-function resolution is one hop only — a factory returning a wrapper type isn't traced #16's exact repro.ScanStrictfinds it correctly.Verification
--strict-typeswarnings.