Repository navigation
cl: implement interface-based keyword arguments - #2679
Conversation
Add support for interface-based keyword arguments that desugar into
builder-pattern method chains. When a function's last parameter is a
named interface type and the receiver has a factory method matching
the interface name, keyword arguments are synthesized as chained
method calls.
For example: c.Complete ctx, maxOutputTokens = 1024, system = "hello"
desugars to: c.Complete(ctx, c.CompletionParams().MaxOutputTokens(1024).System("hello"))
Features:
- Case-insensitive keyword-to-method matching
- Optional Set(string, any) fallback for unknown keywords
- Compile-time error for unknown keywords when no Set method exists
Generated with [codeagent](https://github.com/qbox/codeagent)
Co-authored-by: xushiwei <396972+xushiwei@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2679 +/- ##
==========================================
- Coverage 94.11% 94.09% -0.03%
==========================================
Files 32 32
Lines 9969 10036 +67
==========================================
+ Hits 9382 9443 +61
- Misses 419 422 +3
- Partials 168 171 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| func findIfaceMethod(iface *types.Interface, kwName string) string { | ||
| kwLower := strings.ToLower(kwName) | ||
| for i := 0; i < iface.NumMethods(); i++ { | ||
| m := iface.Method(i) | ||
| if m.Name() == "Set" { | ||
| continue | ||
| } | ||
| if strings.ToLower(m.Name()) == kwLower { | ||
| return m.Name() | ||
| } | ||
| } | ||
| return "" | ||
| } |
There was a problem hiding this comment.
Two concerns with findIfaceMethod:
1. Ambiguous case-insensitive matching: Returns the first method whose lowered name matches. If an interface has two methods differing only in casing (e.g., TopP and Topp), the result depends on iteration order with no warning. Consider preferring an exact match first, or detecting and reporting ambiguity.
2. No signature validation: Matches purely by name and the call site always emits exactly one argument. A getter-style method like Temperature() float64 would match keyword temperature = 0.7 and produce chain.Temperature(0.7), which fails downstream with a confusing arity error instead of a clear kwargs-specific message. Consider filtering to methods with at least one parameter that return the interface type (Self).
| kwLower := strings.ToLower(kwName) | ||
| for i := 0; i < iface.NumMethods(); i++ { | ||
| m := iface.Method(i) | ||
| if m.Name() == "Set" { |
There was a problem hiding this comment.
Nit: The Set exclusion here is case-sensitive (m.Name() == "Set"), but keyword-to-method matching below is case-insensitive. This means a keyword set = value would case-insensitively match the Set method here (not skipped since "Set" != "Set" — wait, it would be skipped). Actually the real concern is the reverse: if a user writes set = 42, findIfaceMethod skips Set, returns "", and falls through to the hasSet fallback which calls Set("set", 42). This is probably the desired behavior, but it means Set can never be called as a typed single-arg method via kwargs. Worth a brief comment documenting this intentional reservation of the Set name.
| for _, kwarg := range v.Kwargs { | ||
| methodName := findIfaceMethod(iface, kwarg.Name.Name) | ||
| if methodName != "" { | ||
| chain = &ast.CallExpr{ | ||
| Fun: &ast.SelectorExpr{ | ||
| X: chain, | ||
| Sel: &ast.Ident{NamePos: kwarg.Name.NamePos, Name: methodName}, | ||
| }, | ||
| Args: []ast.Expr{kwarg.Value}, | ||
| } | ||
| } else if hasSet { | ||
| chain = &ast.CallExpr{ | ||
| Fun: &ast.SelectorExpr{ | ||
| X: chain, | ||
| Sel: &ast.Ident{NamePos: kwarg.Name.NamePos, Name: "Set"}, | ||
| }, | ||
| Args: []ast.Expr{ | ||
| &ast.BasicLit{ValuePos: kwarg.Name.NamePos, Kind: token.STRING, Value: strconv.Quote(kwarg.Name.Name)}, | ||
| kwarg.Value, | ||
| }, | ||
| } | ||
| } else { | ||
| panic(ctx.newCodeErrorf(kwarg.Pos(), kwarg.End(), msgIfaceNoMatchKeyword, t, kwarg.Name.Name)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Nit: No duplicate keyword detection — if the same keyword appears twice (e.g., maxTokens = 1024, maxTokens = 2048), both calls are chained silently. The struct kwargs path would be caught by Go's duplicate field check, but the interface path succeeds quietly. This is almost certainly a programmer mistake. Consider detecting and erroring on duplicates.
|
Clean implementation — the builder-pattern desugaring is well-structured, the Test coverage gaps worth addressing:
See inline comments for specific code-level findings. |
Requested by @xushiwei
Closes #2678
Summary
clpackage that desugar into builder-pattern method chainsSet(string, any)fallback for unknown keywordsSetmethod exists on the interfaceDesugaring Example
Desugars to:
Changes
cl/expr.go: ExtendedmergeKwargsto handle*types.Interface, addedmergeInterfaceKwargsand helper functionscl/_testgop/kwargs5/: Test case for basic interface kwargs (typed methods only)cl/_testgop/kwargs6/: Test case for interface kwargs withSetfallbackcl/error_msg_test.go: Error test cases for unknown keywords and missing factory methodsTest plan
TestTestgop/kwargs5— basic interface kwargs with typed methodsTestTestgop/kwargs6— interface kwargs with Set fallback for unknown keysTestErrInterfaceKwargsNoSet— error for unknown keyword without Set methodTestErrInterfaceKwargsNoFactory— error when factory method missing on receivercl/test suite passes