-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Refactor to use reflect.TypeFor #6428
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
Conversation
WalkthroughReplaced reflect.TypeOf((*context.Context)(nil)).Elem() with reflect.TypeForcontext.Context for first-parameter type checks in two wrappers, preserving existing behavior for context-aware function wrapping. No exported API changes or control-flow modifications. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/js/gojs/set.go (2)
37-39
: DRY the first-arg type check across wrappers.Both wrappers duplicate the same guard. Consider a small helper for readability and consistency.
Apply within this hunk:
- if fnType.NumIn() == 0 || fnType.In(0) != reflect.TypeFor[context.Context]() { + if !firstArgIsContext(fnType) { return fn // Return original function unchanged if it doesn't have context.Context as first arg }Add once in the package (new file, e.g., pkg/js/gojs/reflectutil.go):
package gojs import ( "context" "reflect" ) var ctxType = reflect.TypeFor[context.Context]() func firstArgIsContext(t reflect.Type) bool { return t.NumIn() > 0 && t.In(0) == ctxType }
89-92
: Wrapper currently not used in RegisterFuncWithSignature.wrapWithContext is commented out; this change won’t affect runtime behavior here. Intentional?
I can wire it up and add a focused test if desired.
pkg/js/gojs/gojs.go (1)
62-64
: Apply shared helper to avoid duplication (optional).Keep the check identical but centralize it.
Apply within this hunk:
- if fnType.NumIn() == 0 || fnType.In(0) != reflect.TypeFor[context.Context]() { + if !firstArgIsContext(fnType) { return fn // Return original function unchanged if it doesn't have context.Context as first arg }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
pkg/js/gojs/gojs.go
(1 hunks)pkg/js/gojs/set.go
(1 hunks)
🔇 Additional comments (3)
pkg/js/gojs/set.go (2)
37-39
: Good switch to reflect.TypeFor: semantics preserved, clearer intent.Using reflect.TypeForcontext.Context removes the dummy value dance and keeps the exact-type check unchanged.
37-39
: Go ≥1.22 Requirement Confirmed in go.mod – Please Verify CI ConfigurationThe project’s
go.mod
specifiesgo 1.24.1
, which exceeds the Go 1.22 minimum needed forreflect.TypeFor
invoke
However, I didn’t locate any GitHub Actions workflows under.github/workflows
; please ensure your CI pipeline is configured to use Go 1.22 or higher (ideally matching 1.24+)—for example, by updatingactions/setup-go
in your workflow files.Key locations using
reflect.TypeFor
:
- pkg/js/gojs/gojs.go:62
- pkg/js/gojs/set.go:37
pkg/js/gojs/gojs.go (1)
62-64
: Consistent migration to reflect.TypeFor.Matches set.go; behavior remains “exactly context.Context as first param or bail.”
Proposed changes
why you might use
reflect.TypeFor
instead ofreflect.TypeOf
in Go.Background
In Go, the
reflect
package provides two similar-looking functions:reflect.TypeOf(x)
Takes a value
x
and returns itsreflect.Type
.Example:
reflect.TypeFor[T]()
(introduced in Go 1.22)A generic function that returns the
reflect.Type
for a type parameterT
without needing a value.Example:
Why use
reflect.TypeFor
instead ofreflect.TypeOf
?No value needed
reflect.TypeOf
requires an actual value at runtime.If you only know the type (not a value), you have to create a dummy value:
reflect.TypeFor
works directly with the type parameter:Compile-time type safety
reflect.TypeOf
, the type is determined from a runtime value, so mistakes may only show up at runtime.reflect.TypeFor
, the type is determined at compile time from the generic type parameter, so it’s safer and clearer.Better for generic code
T
but no value of typeT
.reflect.TypeOf
can’t be used without creating a zero value:reflect.TypeFor
, you can simply do:Avoids unnecessary allocations
reflect.TypeOf
may allocate memory (especially for composite types).reflect.TypeFor
avoids this overhead entirely.Summary Table
reflect.TypeOf
reflect.TypeFor
(Go 1.22+)✅ Recommendation:
Use
reflect.TypeFor
when:Use
reflect.TypeOf
when:Checklist
Summary by CodeRabbit