Remove useless allocations; use (*Type)(nil) instead #545
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughReplaces compile-time interface assertion patterns from value composite literals (&Type{}) to nil pointer types ((Type)(nil)) across Client and Server* request, notification, and result types in mcp/utils.go. No runtime logic or behavior changes; only the assertion form is updated. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested reviewers
✨ 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
🧹 Nitpick comments (1)
mcp/utils.go (1)
10-58: Optional: Group assertions into var-blocks and add a short comment for future readersGrouping by kind improves readability and makes the intent explicit without changing behavior. Example diff for the ClientRequest block:
-// ClientRequest types -var _ ClientRequest = (*PingRequest)(nil) -var _ ClientRequest = (*InitializeRequest)(nil) -var _ ClientRequest = (*CompleteRequest)(nil) -var _ ClientRequest = (*SetLevelRequest)(nil) -var _ ClientRequest = (*GetPromptRequest)(nil) -var _ ClientRequest = (*ListPromptsRequest)(nil) -var _ ClientRequest = (*ListResourcesRequest)(nil) -var _ ClientRequest = (*ReadResourceRequest)(nil) -var _ ClientRequest = (*SubscribeRequest)(nil) -var _ ClientRequest = (*UnsubscribeRequest)(nil) -var _ ClientRequest = (*CallToolRequest)(nil) -var _ ClientRequest = (*ListToolsRequest)(nil) +// ClientRequest types (compile-time assertions without allocation) +var ( + _ ClientRequest = (*PingRequest)(nil) + _ ClientRequest = (*InitializeRequest)(nil) + _ ClientRequest = (*CompleteRequest)(nil) + _ ClientRequest = (*SetLevelRequest)(nil) + _ ClientRequest = (*GetPromptRequest)(nil) + _ ClientRequest = (*ListPromptsRequest)(nil) + _ ClientRequest = (*ListResourcesRequest)(nil) + _ ClientRequest = (*ReadResourceRequest)(nil) + _ ClientRequest = (*SubscribeRequest)(nil) + _ ClientRequest = (*UnsubscribeRequest)(nil) + _ ClientRequest = (*CallToolRequest)(nil) + _ ClientRequest = (*ListToolsRequest)(nil) +)You can do the same for the other five groups if you like this style.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
mcp/utils.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ezynda3
PR: mark3labs/mcp-go#461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
📚 Learning: 2025-06-30T07:13:17.052Z
Learnt from: ezynda3
PR: mark3labs/mcp-go#461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
Applied to files:
mcp/utils.go
🧬 Code Graph Analysis (1)
mcp/utils.go (3)
mcp/types.go (22)
ClientRequest(1110-1110)PingRequest(507-510)InitializeRequest(413-417)CompleteRequest(1020-1024)SetLevelRequest(748-752)ListResourcesRequest(560-563)ReadResourceRequest(588-592)SubscribeRequest(619-623)UnsubscribeRequest(634-638)ClientNotification(1113-1113)CancelledNotification(392-395)ProgressNotification(516-519)InitializedNotification(447-449)RootsListChangedNotification(1105-1107)ClientResult(1116-1116)EmptyResult(377-377)CreateMessageResult(847-854)ListRootsResult(1082-1085)ServerRequest(1119-1119)CreateMessageRequest(827-830)ListRootsRequest(1074-1077)ServerNotification(1122-1122)mcp/prompts.go (5)
GetPromptRequest(23-27)ListPromptsRequest(9-12)PromptListChangedNotification(100-102)GetPromptResult(38-43)ListPromptsResult(16-19)mcp/tools.go (5)
CallToolRequest(54-58)ListToolsRequest(18-21)ToolListChangedNotification(542-544)CallToolResult(40-51)ListToolsResult(25-28)
🔇 Additional comments (6)
mcp/utils.go (6)
11-22: Good switch to typed nil pointers for ClientRequest assertionsUsing (*T)(nil) removes the init-time allocation that &T{} would trigger while preserving the compile-time check. Semantics remain unchanged and this is idiomatic Go for these assertions.
25-28: ClientNotification assertions migrated correctlySame benefits here: no allocation, same compile-time shape assurance. Looks good.
31-33: ClientResult assertions migrated correctlyNo behavior change; avoids allocation. LGTM.
36-38: ServerRequest assertions migrated correctlyChange is consistent with the rest of the file and avoids unnecessary init-time work.
41-47: ServerNotification assertions migrated correctlyConsistent, allocation-free pattern. Looks good.
50-58: ServerResult assertions migrated correctlyPattern applied consistently; no runtime effect, avoids allocations. LGTM.
Comment on lines
10
to
+58
| // ClientRequest types | ||
| var _ ClientRequest = &PingRequest{} | ||
| var _ ClientRequest = &InitializeRequest{} | ||
| var _ ClientRequest = &CompleteRequest{} | ||
| var _ ClientRequest = &SetLevelRequest{} | ||
| var _ ClientRequest = &GetPromptRequest{} | ||
| var _ ClientRequest = &ListPromptsRequest{} | ||
| var _ ClientRequest = &ListResourcesRequest{} | ||
| var _ ClientRequest = &ReadResourceRequest{} | ||
| var _ ClientRequest = &SubscribeRequest{} | ||
| var _ ClientRequest = &UnsubscribeRequest{} | ||
| var _ ClientRequest = &CallToolRequest{} | ||
| var _ ClientRequest = &ListToolsRequest{} | ||
| var _ ClientRequest = (*PingRequest)(nil) | ||
| var _ ClientRequest = (*InitializeRequest)(nil) | ||
| var _ ClientRequest = (*CompleteRequest)(nil) | ||
| var _ ClientRequest = (*SetLevelRequest)(nil) | ||
| var _ ClientRequest = (*GetPromptRequest)(nil) | ||
| var _ ClientRequest = (*ListPromptsRequest)(nil) | ||
| var _ ClientRequest = (*ListResourcesRequest)(nil) | ||
| var _ ClientRequest = (*ReadResourceRequest)(nil) | ||
| var _ ClientRequest = (*SubscribeRequest)(nil) | ||
| var _ ClientRequest = (*UnsubscribeRequest)(nil) | ||
| var _ ClientRequest = (*CallToolRequest)(nil) | ||
| var _ ClientRequest = (*ListToolsRequest)(nil) | ||
|
|
||
| // ClientNotification types | ||
| var _ ClientNotification = &CancelledNotification{} | ||
| var _ ClientNotification = &ProgressNotification{} | ||
| var _ ClientNotification = &InitializedNotification{} | ||
| var _ ClientNotification = &RootsListChangedNotification{} | ||
| var _ ClientNotification = (*CancelledNotification)(nil) | ||
| var _ ClientNotification = (*ProgressNotification)(nil) | ||
| var _ ClientNotification = (*InitializedNotification)(nil) | ||
| var _ ClientNotification = (*RootsListChangedNotification)(nil) | ||
|
|
||
| // ClientResult types | ||
| var _ ClientResult = &EmptyResult{} | ||
| var _ ClientResult = &CreateMessageResult{} | ||
| var _ ClientResult = &ListRootsResult{} | ||
| var _ ClientResult = (*EmptyResult)(nil) | ||
| var _ ClientResult = (*CreateMessageResult)(nil) | ||
| var _ ClientResult = (*ListRootsResult)(nil) | ||
|
|
||
| // ServerRequest types | ||
| var _ ServerRequest = &PingRequest{} | ||
| var _ ServerRequest = &CreateMessageRequest{} | ||
| var _ ServerRequest = &ListRootsRequest{} | ||
| var _ ServerRequest = (*PingRequest)(nil) | ||
| var _ ServerRequest = (*CreateMessageRequest)(nil) | ||
| var _ ServerRequest = (*ListRootsRequest)(nil) | ||
|
|
||
| // ServerNotification types | ||
| var _ ServerNotification = &CancelledNotification{} | ||
| var _ ServerNotification = &ProgressNotification{} | ||
| var _ ServerNotification = &LoggingMessageNotification{} | ||
| var _ ServerNotification = &ResourceUpdatedNotification{} | ||
| var _ ServerNotification = &ResourceListChangedNotification{} | ||
| var _ ServerNotification = &ToolListChangedNotification{} | ||
| var _ ServerNotification = &PromptListChangedNotification{} | ||
| var _ ServerNotification = (*CancelledNotification)(nil) | ||
| var _ ServerNotification = (*ProgressNotification)(nil) | ||
| var _ ServerNotification = (*LoggingMessageNotification)(nil) | ||
| var _ ServerNotification = (*ResourceUpdatedNotification)(nil) | ||
| var _ ServerNotification = (*ResourceListChangedNotification)(nil) | ||
| var _ ServerNotification = (*ToolListChangedNotification)(nil) | ||
| var _ ServerNotification = (*PromptListChangedNotification)(nil) | ||
|
|
||
| // ServerResult types | ||
| var _ ServerResult = &EmptyResult{} | ||
| var _ ServerResult = &InitializeResult{} | ||
| var _ ServerResult = &CompleteResult{} | ||
| var _ ServerResult = &GetPromptResult{} | ||
| var _ ServerResult = &ListPromptsResult{} | ||
| var _ ServerResult = &ListResourcesResult{} | ||
| var _ ServerResult = &ReadResourceResult{} | ||
| var _ ServerResult = &CallToolResult{} | ||
| var _ ServerResult = &ListToolsResult{} | ||
| var _ ServerResult = (*EmptyResult)(nil) | ||
| var _ ServerResult = (*InitializeResult)(nil) | ||
| var _ ServerResult = (*CompleteResult)(nil) | ||
| var _ ServerResult = (*GetPromptResult)(nil) | ||
| var _ ServerResult = (*ListPromptsResult)(nil) | ||
| var _ ServerResult = (*ListResourcesResult)(nil) | ||
| var _ ServerResult = (*ReadResourceResult)(nil) | ||
| var _ ServerResult = (*CallToolResult)(nil) | ||
| var _ ServerResult = (*ListToolsResult)(nil) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Heads-up: These lines do not enforce interface conformance because Client/Server are aliases to any**
In mcp/types.go, the following are defined as type aliases to any:
- ClientRequest, ClientNotification, ClientResult
- ServerRequest, ServerNotification, ServerResult
Given that, declarations like var _ ClientRequest = (*PingRequest)(nil) always type-check and do not provide compile-time interface enforcement. If these lines are meant purely as documentation/grouping, this refactor is fine. If the intent is to enforce conformance, consider replacing the any aliases with marker interfaces and implementing them on the relevant types.
Example approach (separate follow-up PR):
- Define marker interfaces:
- type ClientRequest interface{ isClientRequest() }
- type ServerRequest interface{ isServerRequest() }
- … similarly for Notification and Result
- Add empty method receivers on each concrete type:
- func (*PingRequest) isClientRequest() {}
- etc.
Alternatively, if the aim is just to avoid allocations while retaining the “catalog,” consider adding a brief comment explaining that these are non-enforcing catalogs of types.
You can verify the current aliases with this script:
🏁 Script executed:
#!/bin/bash
# Verify that Client*/Server* types are aliases to `any`
set -euo pipefail
echo "Searching for any-alias declarations for Client*/Server*…"
rg -n -A 1 -B 1 $'type\\s+(ClientRequest|ClientNotification|ClientResult|ServerRequest|ServerNotification|ServerResult)\\s+any'
echo
echo "If the above matches show aliases to 'any', these assertions do not enforce interface conformance."Length of output: 1464
Enforce compile-time checks for Client/Server types
The aliases in mcp/types.go (lines 1110–1125)
type ClientRequest any
type ClientNotification any
…
type ServerResult anymean that the assertions in mcp/utils.go (lines 10–58)
var _ ClientRequest = (*PingRequest)(nil)
…
var _ ServerResult = (*ListToolsResult)(nil)are purely documentation and do not provide compile-time interface enforcement.
To fix this, either:
• Replace each any alias with a marker interface in mcp/types.go:
- type ClientRequest any
+ type ClientRequest interface { isClientRequest() }(and likewise for ClientNotification, ClientResult, ServerRequest, etc.)
• Implement the empty methods on each concrete type—e.g. in mcp/utils.go:
func (*PingRequest) isClientRequest() {}
func (*InitializeRequest) isClientRequest() {}
// …and so on for every ClientRequest typeOr, if you only intended a non-enforcing catalog of types, please add a clarifying comment above the assertion block in mcp/utils.go.
🤖 Prompt for AI Agents
In mcp/utils.go around lines 10–58 the var _ Type = (*Concrete)(nil) assertions
are no-ops because the corresponding aliases in mcp/types.go (lines ~1110–1125)
use `type ClientRequest any` (and similar), so they do not enforce compile-time
checks; choose one of three fixes: (1) change each alias in mcp/types.go into a
marker interface (e.g. type ClientRequest interface { isClientRequest() }) and
keep the current assertions; or (2) implement the empty marker methods on every
concrete type listed in mcp/utils.go (e.g. add isClientRequest methods on
PingRequest, InitializeRequest, etc., and analogous methods for
ClientNotification/ClientResult/ServerRequest/ServerNotification/ServerResult)
so the assertions become real; or (3) if enforcement is not desired, add a clear
comment above the assertion block stating these are only documentation and will
not be enforced, so reviewers know it is intentional.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The file
utils.gohas around 50 useless allocations for what appears to be a way to use the Go compiler to ensure that all methods are implemented for the named type for the respective interfaces. Here is a section of that code:Go has a better way to do that that do not require useless allocations:
(*Type)(nil). Here is what the above looks like using that technque:Type of Change
Checklist
MCP Spec Compliance
Additional Information
This change requires no testing and avoids creating unnesseary instances in the heap that are immediately thrown away.
Summary by CodeRabbit
Refactor
Chores
No user-facing changes.