Skip to content

runtime: clean code, impl reflect arrayOf, chanOf, structOf#1515

Merged
xushiwei merged 8 commits intogoplus:mainfrom
visualfc:cleancode
Jan 1, 2026
Merged

runtime: clean code, impl reflect arrayOf, chanOf, structOf#1515
xushiwei merged 8 commits intogoplus:mainfrom
visualfc:cleancode

Conversation

@visualfc
Copy link
Member

@visualfc visualfc commented Dec 29, 2025

  • ssa/abi: abi types.Named check scope indices
    ( Name.N.N... N is scope in parent scope index. eg: func demo() { type T int } T => T.1.0 )
  • ssa/abi: fix abi types.Named str pkgpath.name => pkgname.name
  • ssa: fix abi struct pkgpath by field
  • ssa: fix abi closure funcType
  • reflect: impl ArrayOf, ChanOf, StructOf
  • runtime: clean type func

@gemini-code-assist
Copy link

Summary of Changes

Hello @visualfc, 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 focuses on a significant code cleanup and refactoring effort within the Go runtime's internal reflection mechanisms. The primary objective is to enhance code organization by relocating numerous type-related utility functions and constants to a more semantically appropriate file, z_type.go, from z_face.go. This change also includes a simplification of the logic for retrieving type names in type.go, contributing to a more streamlined and maintainable codebase for runtime type handling.

Highlights

  • Code Refactoring: Moved several type-related constants and utility functions, such as hdrSizeOf, NewNamed, Func, funcStr, hashTuple, and shallowHash, from runtime/internal/runtime/z_face.go to runtime/internal/runtime/z_type.go to centralize type-related logic.
  • API Simplification: Simplified the Name() method in runtime/internal/lib/reflect/type.go by removing special conditional handling for basic types, leading to a more direct and cleaner implementation.
  • Consolidation of Type Utilities: Consolidated various functions responsible for type creation, string representation, and hashing into runtime/internal/runtime/z_type.go, improving modularity and maintainability within the runtime package.

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

Copy link

@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 provides a good cleanup of the runtime code by refactoring several type-related functions into a more appropriate file (z_type.go) and removing what appears to be a significant amount of unused code. These changes improve the overall code structure and maintainability. However, I've identified one critical regression in reflect/type.go where a simplification to the Name() method will cause it to behave incorrectly for basic types. Please see the specific comment for details.

@xgopilot
Copy link
Contributor

xgopilot bot commented Dec 29, 2025

Code Review Summary

This PR performs a clean refactoring by reorganizing type-related functions from z_face.go to z_type.go and removing dead code. The file organization improvement is excellent, separating interface operations from type construction.

Overall Assessment: One critical issue found that needs attention.


✅ Strengths

  • Excellent code organization: Functions are now logically grouped by purpose
  • Safe dead code removal: All removed functions have zero references in the codebase
  • No performance impact: Code movement is performance-neutral
  • No security issues: All unsafe operations remain properly guarded
  • Clean refactoring: No breaking changes to public APIs

⚠️ Critical Issue: Removed Basic Type Handling

The removal of special case handling for basic types in reflect.Name() appears problematic. The original code handled basic types (int, string, bool, etc.) which don't have TFlagNamed set but do have names.

Impact: The current implementation returns empty string for basic types instead of their names (e.g., "int", "string").

Recommendation: Either restore the original logic or add documentation explaining why this behavior change is intentional.

See inline comment for details.

@codecov
Copy link

codecov bot commented Dec 29, 2025

Codecov Report

❌ Patch coverage is 97.29730% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 91.04%. Comparing base (c844c73) to head (efb546d).
⚠️ Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
ssa/abi/abi.go 94.11% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1515      +/-   ##
==========================================
+ Coverage   90.78%   91.04%   +0.26%     
==========================================
  Files          45       45              
  Lines       11970    11996      +26     
==========================================
+ Hits        10867    10922      +55     
+ Misses        929      897      -32     
- Partials      174      177       +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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@visualfc visualfc marked this pull request as draft December 29, 2025 23:30
@visualfc visualfc changed the title runtime: clean code [WIP] runtime: reflect arrayOf, chanOf, structOf Dec 30, 2025
@visualfc visualfc marked this pull request as ready for review December 30, 2025 08:14
@xgopilot
Copy link
Contributor

xgopilot bot commented Dec 30, 2025

Code Review Summary

This PR implements significant refactoring of the runtime reflection system, adding ArrayOf, ChanOf, and StructOf functions. The code demonstrates solid engineering with appropriate defensive checks and caching strategies.

Critical Issue:

  • emitGCMask function is completely disabled (line 1571) but is called for GC mask generation. This needs immediate attention.

Key Concerns:

  • Integer overflow checks should occur before arithmetic operations (lines 1618, 1933)
  • Significant code duplication in embedded field method handling (lines 1831-1911)
  • Documentation inaccuracies in StructOf panic conditions

Positive Aspects:

  • Good use of caching to avoid duplicate type creation
  • Comprehensive input validation with appropriate panics
  • Proper concurrent access patterns with double-checked locking

Please address the critical emitGCMask issue before merging.

@visualfc visualfc changed the title [WIP] runtime: reflect arrayOf, chanOf, structOf [WIP] runtime: impl reflect arrayOf, chanOf, structOf Dec 31, 2025
@visualfc visualfc force-pushed the cleancode branch 2 times, most recently from b15f490 to 806f8b3 Compare December 31, 2025 02:02
@visualfc visualfc changed the title [WIP] runtime: impl reflect arrayOf, chanOf, structOf runtime: impl reflect arrayOf, chanOf, structOf Dec 31, 2025
@visualfc visualfc changed the title runtime: impl reflect arrayOf, chanOf, structOf runtime: clean code, impl reflect arrayOf, chanOf, structOf Dec 31, 2025
@visualfc visualfc changed the title runtime: clean code, impl reflect arrayOf, chanOf, structOf [WIP] runtime: clean code, impl reflect arrayOf, chanOf, structOf Dec 31, 2025
@visualfc visualfc changed the title [WIP] runtime: clean code, impl reflect arrayOf, chanOf, structOf runtime: clean code, impl reflect arrayOf, chanOf, structOf Dec 31, 2025
@xushiwei xushiwei merged commit 3e72be7 into goplus:main Jan 1, 2026
43 checks passed
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