reflect: fix Type.Name() returning empty string for basic types#1511
reflect: fix Type.Name() returning empty string for basic types#1511xushiwei merged 2 commits intogoplus:mainfrom
Conversation
Summary of ChangesHello @luoliwoshang, 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 addresses several key areas, including a critical fix for 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. Ignored Files
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
|
Basic types (int, string, bool, etc.) don't have TFlagNamed set in llgo, but they do have valid names stored in Str_ field. This fix adds a special case in Name() to return String() for basic types (Kind <= UnsafePointer). Fixes goplus#1412 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
luoliwoshang
force-pushed
the
fix/issue-1412-basic-type-name
branch
from
5b7f657 to
a362ba0
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request, titled to fix an issue with reflect.Type.Name(), introduces a vast range of significant improvements and refactorings across the codebase. The core fix for reflect.Type.Name() is correct and well-implemented. Beyond that, this PR includes a major refactoring of the SSA backend, particularly for closures, which greatly improves the design and correctness. It also adds a substantial implementation of the standard time package, new development tooling including Docker setup and helper scripts, and a new tool for checking standard library test coverage. While these are all valuable additions, the scope of this PR is very large, and the title is quite misleading. In the future, it would be highly beneficial to split such extensive changes into smaller, logically-grouped pull requests. This would make the review process more manageable and focused. I've found a couple of minor issues, mainly related to the new tooling and test files, which I've commented on.
I am having trouble creating individual review comments. Click here to see my feedback.
cl/_testgo/deferclosure/out.ll (1)
This out.ll file, which seems to be a golden file for the deferclosure test, contains only a semicolon. The corresponding in.go file contains a main function with println calls, so it should generate non-empty LLVM IR. This looks like a mistake. Please verify if this file should be empty or if it should contain the expected LLVM IR output.
chore/check_std_symbols/main.go (231-235)
The approach of parsing the output of go doc to get exported symbols is quite brittle and might break if the go doc output format changes in future Go versions. A more robust approach would be to use the go/packages library to load the package's source code and inspect its scope for exported symbols. This would provide structured access to the package's API without relying on text parsing.
For example, you could do something like this:
import (
"go/token"
"golang.org/x/tools/go/packages"
)
func exportedSymbols(pkgPath string) ([]symbol, error) {
cfg := &packages.Config{Mode: packages.NeedTypes}
pkgs, err := packages.Load(cfg, pkgPath)
if err != nil {
return nil, err
}
if len(pkgs) == 0 {
return nil, fmt.Errorf("package not found: %s", pkgPath)
}
pkg := pkgs[0]
scope := pkg.Types.Scope()
var symbols []symbol
for _, name := range scope.Names() {
if token.IsExported(name) {
obj := scope.Lookup(name)
// ... create symbol from obj ...
}
}
return symbols, nil
}This would make the tool more resilient to changes in the Go toolchain.
Code Review SummaryThis PR successfully fixes the Key Findings:
See inline comments for specific recommendations. |
![xgopilot[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
| if !t.t.HasName() { | ||
| // For basic types (int, string, etc.), String() returns the type name directly. | ||
| // Basic types don't have TFlagNamed set, but they do have names. | ||
| if t.t.Kind() <= abi.UnsafePointer { |
There was a problem hiding this comment.
Documentation: Clarify basic type boundary
The comment mentions "basic types" but doesn't explain why <= abi.UnsafePointer is the boundary. Consider enhancing the comment to explain that this covers all predefined types (Invalid through UnsafePointer = kinds 0-25), while composite types (Array, Chan, Func, etc.) correctly return empty string when unnamed.
Suggested addition:
// For basic types (int, string, etc.), String() returns the type name directly.
// Basic types don't have TFlagNamed set, but they do have names.
// Check if Kind <= abi.UnsafePointer (25), which covers all basic predefined types
// from Invalid(0) through UnsafePointer(25). Types beyond this are composite types.
Additional Feedbackssa/closure_wrap.goDocumentation & Safety (line 90): // closureWrapPtr wraps a raw function pointer by loading it from ctx.
// The ctx parameter is treated as a pointer to a stored function pointer cell.
//
// SAFETY: This function does not perform null checks on ctx. The caller must
// ensure ctx is a valid non-nil pointer. Invalid ctx values indicate a compiler
// or user error and will result in undefined behavior.chore/check_std_symbols/main.goPerformance Optimization (lines 586-599 - Recommendation: Cache absolute path results per unique filename. This could provide 10-100x improvement for large codebases: // In usedSymbols(), create a cache
absPathCache := make(map[string]string)
getAbsPath := func(path string) (string, bool) {
if abs, ok := absPathCache[path]; ok {
return abs, true
}
abs, err := filepath.Abs(path)
if err != nil {
return "", false
}
absPathCache[path] = abs
return abs, true
}Performance - Check Ordering (lines 365, 371): if !strings.HasSuffix(pos.Filename, "_test.go") { // Fast rejection
return false
}
return pathWithinAbs(dirAbs, pos.Filename) // Only for test filesCode Quality - Map Allocation (line 217): var unsafeIntrinsics = map[string]bool{
"Alignof": true, "ArbitraryType": true, "IntegerType": true,
"Offsetof": true, "Sizeof": true, "String": true, "StringData": true,
}Security - Defensive Validation (line 602): func runGoDoc(pkgPath string) ([]byte, error) {
if err := validatePkgPath(pkgPath); err != nil {
return nil, err
}
cmd := exec.Command("go", "doc", "-all", pkgPath)
// ... rest
} |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1511 +/- ##
=======================================
Coverage 90.93% 90.93%
=======================================
Files 44 44
Lines 11528 11528
=======================================
Hits 10483 10483
Misses 883 883
Partials 162 162 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Add demo program to verify reflect.Type.Name() returns correct name for basic types (int, string, etc.) instead of empty string. Relates to goplus#1412 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
2 participants
Summary
reflect.Type.Name()returning empty string for basic types (int, string, bool, etc.)TFlagNamedset in llgo, but they do have valid names stored inStr_fieldName()to returnString()for basic types (Kind <= UnsafePointer)Test
All basic types tested:
Why not just add TFlagNamed to basicFlags()?
Attempted to add
TFlagNamedtobasicFlags()function, but it causes segmentation fault. TheTFlagNamedflag implies the type has additional metadata structures (likeUncommonType), which basic types don't have. Accessing these non-existent structures causes the crash.This fix modifies
Name()method instead, which is safer and works correctly.Fixes #1412
🤖 Generated with Claude Code