Skip to content

Return an error instead of panicking on a map with an unmappable key/value type - #627

Open
chuenchen309 wants to merge 1 commit into
alecthomas:masterfrom
chuenchen309:fix/map-decoder-nil-mapper
Open

Return an error instead of panicking on a map with an unmappable key/value type#627
chuenchen309 wants to merge 1 commit into
alecthomas:masterfrom
chuenchen309:fix/map-decoder-nil-mapper

Conversation

@chuenchen309

Copy link
Copy Markdown
Contributor

A map flag whose key or value type has no registered mapper panics with a nil-pointer dereference at decode time, instead of returning an error:

var cli struct {
    Set map[string]any   // also map[complex128]string, etc.
}
kong.New(&cli).Parse([]string{"--set", "a=b"})
// panic: runtime error: invalid memory address or nil pointer dereference
//   ... kong.(*Registry).RegisterDefaults.mapDecoder.func15 (mapper.go:526)

map[string]any / map[string]interface{} is a common idiom for "arbitrary key/value config", so this is easy to hit by accident.

Root cause (mapper.go, mapDecoder): the key and value mappers are looked up with Registry.ForNamedType, whose doc says "Will return nil if a mapper can not be determined", but the results were used without a nil check (keyDecoder.Decode / valueDecoder.Decode).

The sibling already handles this. sliceDecoder guards the identical case and returns a clean error — []complex128 gives --f: no mapper for element type of []complex128, while map[string]complex128 panics. This just mirrors that guard for maps.

Fix: nil-check both decoders and return no mapper for key type of ... / no mapper for value type of ..., matching the slice path.

Verification (re-runnable from the diff):

  • Before: map[string]any and map[complex128]string panic (nil deref at mapper.go:526); []complex128 already returns a clean error.
  • After: both maps return --set: no mapper for value type of map[string]interface {} / --set: no mapper for key type of map[complex128]string; the slice path is unchanged.
  • Two regression tests added next to TestMapFlagWithSliceValue; they panic on the unfixed tree and pass with the fix. go test ./... is green.

Disclosure: This PR was authored by an AI coding agent (Claude Code) running on this account: the AI found the bug, ran the repro, wrote the tests, and wrote this description. The human account holder reviews every change and is accountable for it. The verification above is real and re-runnable from the diff. If this isn't the kind of contribution you want, say so and I'll close it.

…value type

`mapDecoder` looked up the key and value mappers with `Registry.ForNamedType`,
which is documented to return nil when no mapper can be determined, but used
the results without a nil check. A map flag whose key or value type has no
registered mapper (e.g. `map[string]any`, a very common idiom for arbitrary
config, or `map[complex128]string`) therefore nil-panicked at decode time.

The sibling `sliceDecoder` already guards this exact case and returns
"no mapper for element type of ...". Mirror that for maps so both paths fail
the same way with a descriptive error.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant