cl: preserve diagnostics for invalid method receivers - #2417
Open
cpunion wants to merge 2 commits into
Open
Conversation
cpunion
added
go-test-compat
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review: nonlocal method receiver diagnostic
Clean, well-scoped refactor. Converting recvTypeName and astFuncName from panic-on-invalid to returning an ok boolean is the right call — it lets the type checker surface the real diagnostic (e.g. func (b *bufio.Reader) Buffered()) instead of the compiler dying with an internal panic. Pointer handling is correctly consolidated into recvTypeName, pointer-to-pointer is properly rejected, and all three call sites (initFiles, ParsePkgSyntaxWithOptions, compileFuncDecl) check ok and skip cleanly.
Reviewed across four dimensions:
- Correctness/quality — recursion covers
Ident/IndexExpr/IndexListExpr/ParenExpr/StarExpr; nested*(*T)correctly rejected. Good. - Performance — no regression; runs once per decl, bounded recursion, no extra allocations.
- Security — no unbounded-recursion/DoS surface; ASTs come from
go/parserwhich enforces its own nesting limit. Change is a net panic-surface improvement (fail-closed). - Documentation — new doc comments accurately describe the ok-boolean behavior; removal of the
// TODO: support generic typeis justified since the new code handlesIndexExpr/IndexListExpr.
Two minor, non-blocking notes inline. No blocking issues found.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Compared with |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Behavior
Before this change, compiling fixedbugs/issue5089.go panics in cl.recvTypeName while preloading the *bufio.Reader method declaration. After this change, metadata collection skips that invalid declaration and the compiler reports:
This fixes 1 Go 1.26 GOROOT compatibility case. It also addresses the receiver-AST crash behind the issue5089 xfail proposed in #2265; if #2265 lands first, that entry should be removed when this PR is rebased.
Validation
Xfail history
This PR intentionally has no
test/goroot/xfail.yamldeletion forfixedbugs/issue5089.go: the entry was added by3f3090f71and then removed from main by4da2efa8a, so it is absent from this PR base (6fe8babd4). Commit15c415828restores the entry on the separate, unmerged #2265 line of work and is not an ancestor of this PR. If that commit is integrated before this fix, its stale issue5089 entry must be dropped while rebasing. The Go 1.26 errorcheck case also passes with an empty xfail configuration after this fix.