Skip to content

Conversation

@aofei
Copy link
Member

@aofei aofei commented Dec 19, 2025

  • 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

Copilot AI review requested due to automatic review settings December 19, 2025 08:28
@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @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

  • Unused Variable Detection: Implemented a new static analysis feature to identify variables declared but not used within function bodies.
  • Scope Tracking: Introduced a usedVars map within funcBodyCtx to accurately track variable usage across different scopes.
  • Usage Marking: Variables are now explicitly marked as used in key operations such as Val(), IncDec(), AssignOp(), and when taking the address with UnaryOp(&).
  • Closure Propagation: Enhanced the system to correctly propagate variable usage from nested closures to their parent functions, ensuring comprehensive detection.
  • Exclusions: Receiver, parameters, results, and auto-generated variables are intelligently skipped from unused variable checks to avoid false positives.
  • Comprehensive Testing: Added extensive test cases covering various scenarios for both unused variable detection and valid usage across different control flow statements and language constructs.

🧠 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 Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@codecov
Copy link

codecov bot commented Dec 19, 2025

Codecov Report

❌ Patch coverage is 89.85507% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.94%. Comparing base (03c926d) to head (f1a6904).

Files with missing lines Patch % Lines
codebuild.go 90.90% 3 Missing and 3 partials ⚠️
stmt.go 66.66% 0 Missing and 1 partial ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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.

@xgopilot
Copy link

xgopilot bot commented Dec 19, 2025

Code Review Summary

Excellent implementation of unused variable detection! The code is well-structured and handles edge cases correctly.

Key Strengths:

  • Proper handling of nested closures and scopes
  • Comprehensive test coverage
  • Clean integration with existing codebase

Noteworthy Suggestions:

  1. Performance: isParamOrResult() has O(V×(P+R)) complexity - consider pre-computing the skip set
  2. Documentation: Several methods lack documentation (see inline comments)
  3. Code quality: Replace magic string "func " with a constant

The implementation is production-ready; suggestions are optional improvements.

Copy link

Copilot AI left a 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 usedVars map to funcBodyCtx to track which variables have been used
  • Implemented variable usage tracking in Val(), IncDec(), AssignOp(), and UnaryOp(&) 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.

@aofei aofei force-pushed the unusedVar branch 2 times, most recently from 783a6d3 to cf6404f Compare December 19, 2025 08:43
aofei added a commit to aofei/fork.goplus.xgolsw that referenced this pull request Dec 19, 2025
- 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]>
@aofei aofei force-pushed the unusedVar branch 2 times, most recently from ce60dec to 110133c Compare December 19, 2025 13:06
@aofei

This comment was marked as resolved.

@xgopilot
Copy link

xgopilot bot commented Dec 19, 2025

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 Assessment

Excellent implementation! The unused variable detection feature is well-structured, handles complex edge cases correctly, and is production-ready.

Key Strengths:

  • ✅ Proper handling of nested closures and multi-level scope propagation
  • ✅ Comprehensive test coverage (239 new test lines)
  • ✅ Clean integration with existing codebase architecture
  • ✅ Correct filtering of parameters, results, receivers, and auto-generated variables

Review Findings

I've posted 4 inline comments on the code with specific recommendations:

1. Performance Optimization (codebuild.go:213)

  • isParamOrResult() has O(V×(P+R)) complexity
  • Suggested pre-computing parameter/result set once per function
  • Would reduce ~68% of operations for typical functions

2. Code Quality (codebuild.go:189)

  • Replace magic string "func " with a named constant
  • Improves maintainability

3. Documentation (codebuild.go:164)

  • checkUnusedVarsInScope() lacks documentation
  • Complex recursive logic should be documented

4. Documentation (codebuild.go:125)

  • usedVars field should have a doc comment
  • Clarifies purpose and lifecycle

Additional Observations

Security: ✅ No security concerns identified

  • Safe nil handling for maps
  • Proper recursive termination
  • No unsafe operations

Test Coverage: 📊 89.85% patch coverage

  • Comprehensive test cases for various scenarios
  • Covers nested scopes, closures, control flow statements
  • Missing coverage on 7 lines (per Codecov)

Note: Another reviewer (copilot) suggested using strings.HasPrefix() instead of the length check at line 189, which aligns with my recommendation to use a constant.


Recommendation

Approved 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 /review in the comments! For more usage examples, visit CodeAgent GitHub Repository.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants