Repository navigation
fix: prevent panic on type switch bindings in anonymous functions - #534
Thanatos9404 wants to merge 3 commits into
Conversation
With -experimental-anonymous-function, a type switch with a binding inside a function literal (`switch n := v.(type)`) made the closure collection panic, which also hid every other diagnostic in the package. The binding in the switch header does not denote a variable by itself: go/types records a nil object for it in Defs and declares a separate implicit variable in each case clause instead. Skip the header identifier, and decide whether a variable belongs to the function literal by walking its scope chain instead of looking it up in the function scope only. Otherwise the implicit variables of the case clauses, which live in nested scopes, are collected as closure variables and passed as extra arguments at the call site, which then panics in the function analysis. As a result, variables declared in other nested scopes of a function literal (for example in an `if` or `for` statement) are no longer collected as closure variables either. This changes some results for the better: a nil flow into such a variable is now reported from the variable itself instead of through a fake parameter, and a variadic function literal with such a variable no longer hits the panic from issue uber-go#532. Fixes uber-go#531
The type switch panic fixed in the previous commit also hit several NilAway packages in `make nilaway-lint`, and the resulting INTERNAL ERROR was filtered out. Now that those packages are analyzed again, the self-check reports 7 findings. None of the changes below alter NilAway's behaviour: * Call `record` and `markUnstable` from `ast.Preorder` loops instead of `ast.Inspect` callbacks (the callbacks always returned true), and turn `appendTypeToTypeTriggers` into a method. The anonymous function analysis does not pass the variables captured by a function literal through another function literal that calls it, so it reported them as unassigned. * Use `slices.Reverse` in `SplitFieldChain`. * Return early in `requireComparators` if `actualExpr` is nil, which cannot happen once `expectedExprValue` is known. * Check that the base is not nil in `TestSplitFieldChain` before using it.
|
No actionable comments were generated in the recent review.
 |
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly and concisely identifies the main change: preventing a panic caused by type-switch bindings in anonymous functions. |
| Description check | |
The description directly explains the panic, the cause, the implemented fixes, testing, behavior changes, and remaining scope. |
| Linked Issues check | |
The implementation satisfies the coding requirement in [#531]. collectClosure skips a type-switch header identifier when TypesInfo.Defs contains a nil entry. isDeclaredIn recognizes variables de… |
| Out of Scope Changes check | |
The changes remain within the scope of [#531]. Closure-analysis updates and anonymous-function fixtures implement and verify the fix. The self-check changes, traversal updates, nil guard, `slices.Reve… |
Full details: Linked Issues check
-
Explanation
The implementation satisfies the coding requirement in [
#531].collectClosureskips a type-switch header identifier whenTypesInfo.Defscontains a nil entry.isDeclaredInrecognizes variables declared in the function literal and nested scopes. The focused test asserts that closure analysis returns no recovered panic, and the new type-switch fixture covers the issue reproducer. The separate outer-binding panic is not required by [#531].
Full details: Out of Scope Changes check
-
Explanation
The changes remain within the scope of [
#531]. Closure-analysis updates and anonymous-function fixtures implement and verify the fix. The self-check changes, traversal updates, nil guard,slices.Reversechange, and test assertion support failures exposed by this work. No unrelated feature or public API change is shown in the supplied change summary.
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 @coderabbitai help to get the list of available commands.
|
|
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. I have only one small nit, everything else looks all right!
Please note that you need to sign the CLA before the changes can be merged. See the CLAassistant bot comment.
|
|
||
| // likely represents a case that we don't support. | ||
| if expectedExprValue == _unknown { | ||
| if expectedExprValue == _unknown || actualExpr == nil { |
There was a problem hiding this comment.
actualExpr here cannot be nil or am I missing something?
There was a problem hiding this comment.
You're right, it can't be nil there: every branch that sets expectedExprValue also sets actualExpr. The check is only for make nilaway-lint. This literal has a type switch with a binding, so before the fix the self-check panicked on it and that error was filtered out; now it gets analyzed, and since NilAway doesn't relate the two variables it reports actualExpr as possibly nil once it reaches hook.go:128. Happy to add a short comment saying that, or handle it differently if you prefer.
There was a problem hiding this comment.
Ah, understood. Let's please add a comment here since this is non-functional code
There was a problem hiding this comment.
Done in 6ff1a0e: I moved the actualExpr nil check into its own if and added a comment saying it is only there for the self-check.
Split the actualExpr nil check from the unsupported-case check and add a comment: actualExpr is always set together with expectedExprValue, so the check only exists for NilAway's self-check, which does not relate the two variables.
Fixes #531
With
-experimental-anonymous-function, a type switch binding inside a function literal (switch n := v.(type)) makes the closure collection panic, which hides every other diagnostic in the package. The headernhas a nilDefsentry, and each case clause declares its own implicit variable.Changes:
collectClosureskips the header identifier, next to the blank identifier check from fix: prevent panic with blank identifiers in anonymous functions (cleaned-up replacement PR) #510.isDeclaredIn). The oldscope.Lookupcheck treated the case clause variables as captured, which caused a second panic.make nilaway-lint) that this fix exposes, without changing behaviour.Behaviour change: variables declared in nested blocks of a literal (
if,for) no longer become fake parameters. On 16 standard library packages and x/toolsnilness, only the wording ofcrypto/x509diagnostics changes, plushtml/template_test, where a dependency that used to panic is now analyzed.Testing: new cases in the
assertion/anonymousfunctestdata and intestdata/src/anonymousfunction.TestClosureCollectionandTestAnonymousFunctionfail on main and pass with this change.Still open: a binding declared outside a literal and used inside it panics on a separate path (
GetDeclaringIdent); I can follow up.