You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix: prevent panic on type switch bindings in anonymous functions - #534
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 header n has a nil Defs entry, and each case clause declares its own implicit variable.
A variable now belongs to the literal when its scope chain reaches the literal's scope (isDeclaredIn). The old scope.Lookup check treated the case clause variables as captured, which caused a second panic.
A second commit fixes the 7 self-check findings (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/tools nilness, only the wording of crypto/x509 diagnostics changes, plus html/template_test, where a dependency that used to panic is now analyzed.
Testing: new cases in the assertion/anonymousfunc testdata and in testdata/src/anonymousfunction. TestClosureCollection and TestAnonymousFunction fail 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.
Review was skipped as selected files did not have any reviewable changes.
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: fb9840da-e5eb-4d53-8a2e-25a0d17f44a8
Commits
Reviewing files that changed from the base of the PR and between ab2f3cf and 9cb1edf.
You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.
Use the checkbox below for a quick retry:
Trigger review
No actionable comments were generated in the recent review.
Summary by CodeRabbit
Bug Fixes
Improved analysis of variables captured by anonymous functions, including declarations in nested scopes and type-switch bindings. Diagnostics involving these variables are now handled more accurately, including cases with shadowing and nested function literals.
Avoided generating comparisons when the actual expression is unavailable.
Tests
Expanded coverage for anonymous functions, variadic functions, nested local variables, type switches, and nil-flow scenarios, including diagnostic handling for captured variables and nested scopes.
Walkthrough
Closure analysis now treats declarations in nested scopes as local to a function literal and skips type-switch header identifiers without definition entries. Added tests cover closure collection and anonymous-function diagnostics. Other changes centralize affiliation trigger appending, use preorder traversal in two function analyses, skip comparator generation when the actual expression is missing, and use slices.Reverse for field-chain segments.
The change fixes a panic in anonymous-function analysis of type-switch bindings and adds tests. No actionable merge-blocking risk was found in the supplied context.
Pre-merge checks: 4
Check name
Status
Explanation
Title check
The title names the main change: a panic fix for type switch bindings inside anonymous functions. The raw summary and objectives confirm closure collection changes that skip the type-switch header ide…
Description check
The description states the problem (a panic from a type switch binding in a function literal under -experimental-anonymous-function), links issue #531, lists the code changes (skipping the header iden…
Linked Issues check
The only active direct target is #531. Its coding requirement is that -experimental-anonymous-function must not panic on a type-switch binding inside a function literal. The reviewed change addresse…
Out of Scope Changes check
The closure changes, the traversal updates in structfieldeffects (ast.Preorder), the nil guard in hook/split_blocks_on.go, the slices.Reverse change in util/asthelper, the `TestSplitFieldCha…
Full details: Title check
Explanation
The title names the main change: a panic fix for type switch bindings inside anonymous functions. The raw summary and objectives confirm closure collection changes that skip the type-switch header identifier and use scope-chain checks for declarations, which matches the title's subject.
Full details: Description check
Explanation
The description states the problem (a panic from a type switch binding in a function literal under -experimental-anonymous-function), links issue #531, lists the code changes (skipping the header identifier, isDeclaredIn scope check, nilaway self-check fixes), names the testdata added, and notes a known open panic path. It directly describes the changeset.
Full details: Linked Issues check
Explanation
The only active direct target is #531. Its coding requirement is that -experimental-anonymous-function must not panic on a type-switch binding inside a function literal. The reviewed change addresses this in assertion/anonymousfunc/closure.go. collectClosure skips the type-switch header identifier when its TypesInfo.Defs entry is nil, so the failing .(*types.Var) assertion is no longer reached. isDeclaredIn checks the scope chain instead of scope.Lookup, which removes the second panic on case-clause variables. The new fixture testdata/src/anonymousfunction/type_switch.go covers the issue reproducer. TestClosureCollection now asserts that no recovered panic is returned. The second commit contains self-check fixes and does not change the #531 behaviour. The separate outer-binding panic (GetDeclaringIdent) and the diagnostic-position problem in #533 are not #531 requirements. The evidence is the PR summary and the change summary, not a source diff read, so the result depends on those summaries being accurate.
Full details: Out of Scope Changes check
Explanation
The closure changes, the traversal updates in structfieldeffects (ast.Preorder), the nil guard in hook/split_blocks_on.go, the slices.Reverse change in util/asthelper, the TestSplitFieldChain assertion, and the affiliation.go helper refactor are described as self-check fixes. The PR states that these fixes come from make nilaway-lint findings exposed by this change, and that they do not change behaviour. The affiliation.go refactor and the structfieldeffects traversal changes have a weaker link to closure collection. The evidence does not show that they were required for #531, but it also does not show that they are unrelated. The change summary reports no public API changes. No unrelated feature is shown. The behaviour change for variables in nested blocks of a literal is a direct consequence of the isDeclaredIn rule and is covered by the new testNestedScopes and variadicFuncLitWithNestedLocal tests.
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.
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: f010161b-95ba-4f2a-baad-912e277449bd
Commits
Reviewing files that changed from the base of the PR and between b960d0d and ab2f3cf.
Files selected for processing: 1
hook/split_blocks_on.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.
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You have signed the CLA already but the status is still pending? Let us recheck it.
Hi @Thanatos9404! The problem now is that since the 'if' is split you're going to be missing coverage. Could you keep it where it was before and just add a comment?
Makes sense, done in ab2f3cf: the check is back in the single if, and the comment above it now explains that the actualExpr == nil part is only there for the self-check.
⚠️9 benchmarks with a significant change ran on different hardware
Their base and head results were measured on machines with a different CPU, core count or memory, so the changes might not come from the code. Run base and head on the same runner type.
⚠️9 benchmarks with a significant change ran in a different software environment
Their base and head results were measured with a different OS or toolchain, such as compiler, runtime or library versions, so the changes might come from this difference rather than from the code. Open an affected benchmark to see what changed.
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.
Fixesuber-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.
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.
Please note that you need to sign the CLA before the changes can be merged. See the CLAassistant bot comment.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ccuetoh
left a comment
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.