Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates map encoding/size handling to avoid work (and avoid emitting output) for nil/empty maps by adding explicit nil + length checks, and bumps the module’s Go/dependency versions.
Changes:
- Add nil/empty-map short-circuit logic in map size/encode paths.
- Expose a runtime-linked
reflect.maplenwrapper ininternal/runtime_reflectto get map length without iterating. - Update
go.mod/go.sum(Go version + dependency upgrades) and a small doc-comment tweak in the generator main.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| proto/map.go | Adds early returns for nil/empty maps during sizing/encoding. |
| internal/runtime_reflect/map.go | Adds a reflect.maplen bridge (GetSize) to check map length. |
| go.mod | Raises Go language version and upgrades key dependencies. |
| go.sum | Updates checksums/transitive deps to match go.mod upgrades. |
| cmd/protoc-gen-golite/main.go | Minor comment formatting tweak. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // ??? map len | ||
|
|
There was a problem hiding this comment.
The comment // ??? map len is unclear and doesn’t document any behavior. Please replace it with a meaningful comment (or remove it) so future readers understand why map length handling is special here.
| // ??? map len | |
| // Nil maps and maps with zero entries contribute no encoded size, so | |
| // size/encode paths return early before creating an iterator. |
| if p = *(*unsafe.Pointer)(p); p == nil { | ||
| return 0 | ||
| } // king add 20260331 // *map => map // map == nil | ||
| if GetSize(p) == 0 { | ||
| return 0 | ||
| } // king add 20260331 // map.len == 0, |
There was a problem hiding this comment.
The new GetSize/early-return behavior changes map encoding semantics for nil/empty maps. There are existing map round-trip tests, but none cover nil or empty maps; please add a test that (1) marshaling a nil/empty map emits no bytes for that field, and (2) unmarshaling does not create a default key/value entry.
| // 传入map的地址 不能为*map | ||
| func GetSize(m unsafe.Pointer) int { return reflect_maplen(m) } | ||
|
|
||
| //go:noescape | ||
| //go:linkname reflect_maplen reflect.maplen | ||
| func reflect_maplen(m unsafe.Pointer) int |
There was a problem hiding this comment.
GetSize is a misleading name (it returns the map length, not a size), and the comment says “pass the map address” which doesn’t match how it’s used (callers pass the map value pointer, not a *map). Please rename to something like MapLen and clarify the parameter expectations in the comment.
| go 1.26 | ||
|
|
There was a problem hiding this comment.
Bumping the module’s go version from 1.18 to 1.26 is a breaking change for consumers and CI/tooling. Please confirm this is intentional (and supported by your build pipeline), or consider keeping the minimum version lower / adding a toolchain directive if you only want to pin the toolchain used by this repo.
| go 1.26 | |
| go 1.18 | |
| toolchain go1.26.0 |
| origLen, m := len(b), MapIter{} | ||
| defer m.Done() | ||
|
|
||
| for m.Init(pointer(t), p); m.HasNext(); m.Next() { |
There was a problem hiding this comment.
Inside the iteration below, valSize is computed using keyCodec.size(...) instead of valCodec.size(...) (see the map-entry sizing inside the loop). This will produce an incorrect length prefix when key/value codecs differ, corrupting the encoded output and potentially breaking decoding.
| if GetSize(p) == 0 { | ||
| return 0 | ||
| } // king add 20260331 // map.len == 0, |
There was a problem hiding this comment.
With the new early return for GetSize(p) == 0, the later if n == 0 { ... } fallback (below this block) should be unreachable. Consider removing that fallback to avoid confusing control-flow and to prevent accidentally reintroducing the old “empty map encodes as one empty entry” behavior.
| if GetSize(p) == 0 { | ||
| return b | ||
| } // king add 20260331 // map.len == 0 不写入buf |
There was a problem hiding this comment.
With the new early return for GetSize(p) == 0, the later if len(b) == origLen { ... } fallback (later in this function) should be unreachable. Consider removing it so empty maps are handled only by the explicit early return and to reduce dead code paths.
No description provided.