Repository navigation
fix: prevent panic on variadic anonymous functions with closure variables - #535
Thanatos9404 wants to merge 3 commits into
Conversation
…bles
With -experimental-anonymous-function, the fake func decl created for a
function literal gets one extra parameter per closure variable, appended
after the literal's own parameters, while the fake signature keeps the
literal's variadic flag. For a variadic literal, the last parameter is
then a closure variable instead of the variadic slice, so
types.NewSignatureType panics ("got *T, want variadic parameter of slice
or string type"), and the panic hides every other diagnostic in the
package. A captured slice or string does not panic, but is then treated
as the variadic parameter.
Insert the fake parameters right before the variadic parameter instead,
in both the AST field list and the signature, so that the fake func
stays variadic, and pass the closure variables at the same position at
the call sites: after the regular arguments, before the variadic ones.
The argument unpacked with `...` is now recognized by identity rather
than by its index, since the closure variables come before it. This
also covers method expressions such as T.m(t, xs...), whose receiver is
not passed to the method as an argument.
Only making the fake signature non-variadic is not enough: calls with
more than one variadic argument then panic with "no such parameter
number", and the arguments flow to the wrong parameters.
Fixes uber-go#532
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes.
 |
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
The changes satisfy #532. createFakeFuncDecl places captured closure parameters before the variadic parameter in the fake declaration and signature. Call argument construction uses the same order. T… |
| Out of Scope Changes check | |
The changes stay within the implementation and test scope of #532. The call-site update is required to keep arguments aligned with the new fake parameter order. The unpacked-argument check and method-… |
| Title check | |
The title clearly and concisely identifies the main fix: preventing panics when variadic anonymous functions capture closure variables. |
| Description check | |
The description directly explains the panic, the implementation changes, testing, and known limitations. It is fully related to the changeset. |
Full details: Linked Issues check
-
Explanation
The changes satisfy
#532.createFakeFuncDeclplaces captured closure parameters before the variadic parameter in the fake declaration and signature. Call argument construction uses the same order. The variadic parameter therefore remains last and preserves variadic behavior. The added regression test covers the reported receiver capture and verifies that analysis reports the variadic-parameter indexing diagnostic instead of panicking. Additional tests cover nil closure flow and captured slices. The reported test validation also supports the fix.
Full details: Out of Scope Changes check
-
Explanation
The changes stay within the implementation and test scope of
#532. The call-site update is required to keep arguments aligned with the new fake parameter order. The unpacked-argument check and method-expression test cover the shared call-path behavior affected by that fix. The declaration tests and variadic test fixture provide regression coverage. No unrelated product behavior or unrelated files are identified.
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.
ℹ️ Recent review info
-
⚙️ Run configuration
-
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
ce54d1bc-3db7-4687-8455-aea8a613a405
📒 Files selected for processing: 5
-
assertion/anonymousfunc/analyzer.goassertion/anonymousfunc/analyzer_test.goassertion/anonymousfunc/testdata/src/go.uber.org/anonymousfunc/variadic.goassertion/function/assertiontree/root_assertion_node.gotestdata/src/anonymousfunction/variadic.go
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.
ℹ️ Recent review info
-
⚙️ Run configuration
-
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
ce54d1bc-3db7-4687-8455-aea8a613a405
📒 Files selected for processing: 5
-
assertion/anonymousfunc/analyzer.goassertion/anonymousfunc/analyzer_test.goassertion/anonymousfunc/testdata/src/go.uber.org/anonymousfunc/variadic.goassertion/function/assertiontree/root_assertion_node.gotestdata/src/anonymousfunction/variadic.go
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.
-
|
|
ccuetoh
left a comment
ccuetoh
left a comment
There was a problem hiding this comment.
Hi @Thanatos9404! Thank you very much for your contribution and for taking this fix. The changes look good to me!
Please note that you need to sign the CLA before the changes can be merged. See the CLAassistant bot comment.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #535 +/- ##
==========================================
- Coverage 93.20% 92.92% -0.28%
==========================================
Files 82 82
Lines 8515 9949 +1434
==========================================
+ Hits 7936 9245 +1309
- Misses 579 694 +115
- Partials 0 10 +10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@Thanatos9404, changes re-approved pending CLA signing |
Fixes #532
With
-experimental-anonymous-function, a variadic function literal that captures a variable makes the analyzer panic (want variadic parameter of slice or string type), which hides every other diagnostic in the package.createFakeFuncDeclappends the closure variables after the literal's parameters but keeps the variadic flag, so the last parameter is no longer the variadic one. A captured slice doesn't panic, but it gets treated as the variadic parameter.Changes:
createFakeFuncDeclinserts the closure variables right before the variadic parameter, in both the AST and the signature. Non-variadic literals are unchanged.funcArgsFromCallExprpasses them at the same position, before the variadic arguments.xs...argument is now detected by comparing it with the call's last argument instead of by index. As a side effect, a method expression call likeT.m(t, xs...)is now handled liket.m(xs...).Testing: new cases in
testdata/src/anonymousfunction/variadic.goand theassertion/anonymousfunctestdata.TestAnonymousFunctionandTestClosureCollectionfail on main and pass with this change.This is independent of #534 and the two merge cleanly. A literal with closure variables called as
f(g())still panics, as it already does on main for non-variadic literals.