-
Notifications
You must be signed in to change notification settings - Fork 32
feat: add unused variable detection for func bodies #551
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @aofei, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the static analysis capabilities by introducing robust detection for unused variables within function bodies. The primary goal is to improve code quality by identifying and flagging variables that are declared but never utilized, thereby reducing dead code, improving readability, and potentially optimizing compilation. The implementation involves a detailed tracking mechanism for variable usage across different scopes, including complex scenarios like nested functions and closures, ensuring accurate reporting while avoiding false positives for essential language constructs. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #551 +/- ##
==========================================
- Coverage 96.00% 95.94% -0.06%
==========================================
Files 23 23
Lines 6482 6541 +59
==========================================
+ Hits 6223 6276 +53
- Misses 193 196 +3
- Partials 66 69 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a valuable feature: detection of unused variables within function bodies. The implementation is robust, correctly handling various scopes including nested blocks and closures. The logic for propagating variable usage from closures to parent functions is well-designed. The new feature is supported by a comprehensive set of unit tests that cover a wide range of scenarios, ensuring its correctness and reliability. The necessary updates to existing tests to accommodate this new check have also been handled properly. Overall, this is an excellent contribution that improves code quality by enforcing better coding practices.
Code Review SummaryExcellent implementation of unused variable detection! The code is well-structured and handles edge cases correctly. Key Strengths:
Noteworthy Suggestions:
The implementation is production-ready; suggestions are optional improvements. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request adds unused variable detection for function bodies to catch "declared and not used" errors at code generation time. The implementation tracks variable usage within function scopes and reports errors for variables that are declared but never referenced.
Key Changes
- Added
usedVarsmap tofuncBodyCtxto track which variables have been used - Implemented variable usage tracking in
Val(),IncDec(),AssignOp(), andUnaryOp(&)operations - Added recursive checking of nested scopes with proper closure usage propagation to parent functions
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| codebuild.go | Core implementation of unused variable detection with tracking, checking, and propagation logic |
| xgo_test.go | Updated tests to use variables to avoid triggering new unused variable errors |
| unsafe_test.go | Updated tests to use variables to avoid triggering new unused variable errors |
| typeparams_test.go | Updated tests to use variables to avoid triggering new unused variable errors |
| package_test.go | Updated tests to use variables and fixed some test cases to be consistent with unused var detection |
| error_msg_test.go | Added comprehensive test coverage for unused variable detection in various scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
783a6d3 to
cf6404f
Compare
- Bump gogen to include unused variable detection[^1], enabling "unused variable" errors to be reported in the editor rather than only at runtime - Update test cases to use blank identifiers for unused variables [^1]: goplus/gogen#551 Signed-off-by: Aofei Sheng <[email protected]>
ce60dec to
110133c
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Code Review Complete ✅I've completed a comprehensive review of PR #551 using specialized analysis agents covering code quality, performance, documentation, and security aspects. Overall AssessmentExcellent implementation! The unused variable detection feature is well-structured, handles complex edge cases correctly, and is production-ready. Key Strengths:
Review FindingsI've posted 4 inline comments on the code with specific recommendations: 1. Performance Optimization (codebuild.go:213)
2. Code Quality (codebuild.go:189)
3. Documentation (codebuild.go:164)
4. Documentation (codebuild.go:125)
Additional ObservationsSecurity: ✅ No security concerns identified
Test Coverage: 📊 89.85% patch coverage
Note: Another reviewer (copilot) suggested using RecommendationApproved for merge ✅ The suggested improvements are optional refinements that would enhance code quality and performance, but the current implementation is solid and ready for production use. 👉 Want me to re-run this review? Just type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a robust mechanism for detecting unused variables within function bodies, including support for nested scopes and closures. The implementation is thorough, touching upon variable declaration and usage points to track variable liveness. The changes are well-tested with a comprehensive suite of new tests and updates to existing ones to accommodate the new checks. My only suggestion is a minor improvement in scope lookup to enhance accuracy in variable shadowing scenarios.
- Track variable usage within func bodies and report "declared and not used" errors - Add `usedVars` map to `funcBodyCtx` for tracking used variables - Mark variables as used in `Val()`, `IncDec()`, `AssignOp()`, and `UnaryOp(&)` - Recursively check nested scopes (if blocks, for loops, etc.) - Propagate usage from nested closures to parent funcs - Skip receiver, parameters, results, and auto-generated variables Signed-off-by: Aofei Sheng <[email protected]>
usedVarsmap tofuncBodyCtxfor tracking used variablesVal(),IncDec(),AssignOp(), andUnaryOp(&)