using reflect.TypeFor #25257
using reflect.TypeFor #25257
Conversation
Signed-off-by: pxwanglu <[email protected]>
📝 WalkthroughWalkthroughReplaced several reflect.TypeOf initializations with reflect.TypeFor generics across multiple packages. In protocompat, both gogo and proto v2 type references now point to the same proto.Message type, impacting the isProtov2 decision path. Tests and internal registries updated accordingly. No public APIs changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Protocompat
participant Reflect
Caller->>Protocompat: Handle message (detect proto impl)
Protocompat->>Reflect: Get type for proto.Message (TypeFor)
Note over Protocompat,Reflect: Both gogoType and protov2Type now resolve to the same proto.Message type
alt Detected as proto v2 (isProtov2)
Protocompat-->>Caller: Use proto v2 path
else Detected as gogo (legacy)
Protocompat-->>Caller: Use gogo path
end
Note over Protocompat: Branching may shift since both checks test the same interface
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (4)
baseapp/internal/protocompat/protocompat.go(1 hunks)codec/proto_codec_test.go(1 hunks)codec/types/interface_registry.go(1 hunks)contrib/x/group/internal/orm/iterator.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (3)
codec/types/interface_registry.go (1)
40-40: Good modernization usingreflect.TypeFor.The change from
reflect.TypeOf((*proto.Message)(nil)).Elem()toreflect.TypeFor[proto.Message]()is a good use of Go 1.18+ generics, providing better type safety and cleaner code.contrib/x/group/internal/orm/iterator.go (1)
299-299: Consistent use ofreflect.TypeForfor proto.Message type.Good change that aligns with the overall PR modernization pattern.
codec/proto_codec_test.go (1)
95-95: Good adoption ofreflect.TypeForin tests.The change to use
reflect.TypeFor[testdata.Animal]()in the test is consistent with the broader PR goal and improves code clarity.
| gogoType = reflect.TypeFor[proto.Message]() | ||
| protov2Type = reflect.TypeFor[proto.Message]() |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Potential logic issue: both gogoType and protov2Type now reference the same type.
Both variables are now initialized to reflect.TypeFor[proto.Message](), which means they reference the same interface type. This appears to be incorrect since:
- The variables have distinct names suggesting they should represent different types
- The
isProtov2function at lines 199-204 uses these to distinguish between protov2 and gogo message types - The imports show
gogoproto "github.com/cosmos/gogoproto/proto"(line 8) andproto2 "google.golang.org/protobuf/proto"(line 11), suggesting these should map to different proto interfaces
This change will likely break the type discrimination logic in isProtov2.
Apply this diff to correctly use the appropriate types:
- gogoType = reflect.TypeFor[proto.Message]()
- protov2Type = reflect.TypeFor[proto.Message]()
+ gogoType = reflect.TypeFor[gogoproto.Message]()
+ protov2Type = reflect.TypeFor[proto2.Message]()Let me verify the type usage across the codebase to confirm this issue:
🏁 Script executed:
#!/bin/bash
# Check how gogoType and protov2Type are used in isProtov2 function
rg -A5 -B5 "case typ\.Implements\((gogoType|protov2Type)\)" baseapp/internal/protocompat/
# Check the imports to understand proto vs proto2 usage
rg "^import \(" -A20 baseapp/internal/protocompat/protocompat.go | head -30
# Search for other usages of these variables
rg "gogoType|protov2Type" baseapp/internal/protocompat/Length of output: 2346
Fix incorrect gogoType and protov2Type initializations
The two variables at lines 20–21 currently both capture the same github.com/golang/protobuf/proto.Message interface, so your isProtov2 discriminator will never distinguish between gogo and protov2 messages. They need to reference their respective interfaces:
• File: baseapp/internal/protocompat/protocompat.go
• Lines: 20–21
Apply this diff:
- gogoType = reflect.TypeFor[proto.Message]()
- protov2Type = reflect.TypeFor[proto.Message]()
+ gogoType = reflect.TypeFor[gogoproto.Message]()
+ protov2Type = reflect.TypeFor[proto2.Message]()This change ensures that:
typ.Implements(protov2Type)correctly identifiesgoogle.golang.org/protobuf/proto.Messageimplementationstyp.Implements(gogoType)correctly identifiesgithub.com/cosmos/gogoproto/proto.Messageimplementations
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| gogoType = reflect.TypeFor[proto.Message]() | |
| protov2Type = reflect.TypeFor[proto.Message]() | |
| ++ b/baseapp/internal/protocompat/protocompat.go | |
| @@ Lines 20-21 | |
| - gogoType = reflect.TypeFor[proto.Message]() | |
| gogoType = reflect.TypeFor[gogoproto.Message]() | |
| protov2Type = reflect.TypeFor[proto2.Message]() |
🤖 Prompt for AI Agents
In baseapp/internal/protocompat/protocompat.go around lines 20 to 21, the
gogoType and protov2Type variables are both initialized to the same
proto.Message interface; update them so gogoType references
github.com/cosmos/gogoproto/proto.Message and protov2Type references
google.golang.org/protobuf/proto.Message, and add or adjust imports accordingly
so typ.Implements(protov2Type) and typ.Implements(gogoType) correctly
discriminate between protov2 and gogo messages.
|
@pxwanglu why is this better? what is the benefit of this? |
Thanks for your review. @aljo242
More info golang/go#60088 |
Description
try to use better api reflect.TypeFor instead of reflect.TypeOf when we have known the type.
Closes: #XXXX
Summary by CodeRabbit