chore: improve unit test coverage for decoder logic area

[NawafSwe]

PR Description:

Adding unit tests for the decoder implementation

Summary

• Add comprehensive unit tests for the decoder logic
• Tests cover primitives, pointers, slices, maps, structs, interfaces, and Pkl-specific types)
• Tests include error paths for unsupported types, invalid codes, and unknown object codes

Coverage improvement

Function	main	This PR
Decode	80.0%	100.0%
decodePklObject	82.6%	95.7%
decodeInt64	0.0%	80.0%
decodeUint64	0.0%	80.0%
decodeFloat64	50.0%	78.6%
decodeMap	70.0%	80.0%
decodeSlice	70.0%	80.0%
decodeInterface	68.4%	73.7%
parseStructOpts	83.3%	100.0%

Overall package coverage: 63.9% -> 65.8%

Observation: potential panic in decodeInterface

While writing tests, I discovered that decodeInterface (decoder.go:181-182) passes the raw interface{} type to decodeMapImpl when it encounters a msgpack map. decodeMapImpl calls reflect.MakeMapWithSize(inType, ...) which panics because interface{} is not a map type.

• decodePklObject already handles this correctly at line 221 by hardcoding map[any]any{}
• Question: Is a raw msgpack map ever expected at the decodeInterface level, or does it always come wrapped in a Pkl preamble array? If it can occur, the fix would be to default to map[any]any{} before calling decodeMapImpl.

Test plan

• All new tests pass (go test ./pkl/ -run TestDecoder_Decode)
• All existing tests still pass (go test ./pkl/...)
• No production code changes

Note: I will continue improving the test coverage and report any issues along the way. I opened this PR early to avoid big-scoped PRs.

[NawafSwe]

Hi @HT154 ,

While working on the decoder tests, I noticed that Decode() in decoder.go uses a large switch statement over reflect.Kind (currently ~50 lines, 16 cases). I'd like to propose refactoring it to a map-based pattern in a follow-up PR.

Current approach:

func (d *decoder) Decode(typ reflect.Type) (*reflect.Value, error) {
    // ...
    switch typ.Kind() {
    case reflect.Ptr:
        return d.decodePointer(typ)
    case reflect.Struct:
        return d.decodePklObject(typ, true)
    case reflect.Bool:
        return d.decodeBool()
    // ... 13 more cases
    default:
        return nil, &InternalError{...}
    }
}

Proposed approach:

type decoderFunc func(typ reflect.Type) (*reflect.Value, error)

// initialized once per decoder instance
kindsDecoders map[reflect.Kind]decoderFunc

func (d *decoder) Decode(typ reflect.Type) (*reflect.Value, error) {
    // ...
    decoderFunc, found := d.kindsDecoders[typ.Kind()]
    if !found {
        return nil, &InternalError{...}
    }
    return decoderFunc(typ)
}

Why I think this could be beneficial:

1. Readability - The dispatch logic is separated from the function routing, making Decode() a clear 10-line function instead of a 50-line switch
2. Extensibility - Adding support for a new reflect.Kind means adding one map entry rather than inserting into a switch
3. Testability - Individual decoder functions are already well-isolated; the map makes the routing itself easier to verify
4. No critical path changes - The actual decode functions (decodeBool, decodeString, etc.) remain completely untouched. Only the dispatch mechanism changes

What I'd keep in mind:

• The reflect.Int64 case has special durationType handling, which could be wrapped in a closure in the map entry, keeping the same behavior
• I understand this is a style preference and the existing switch is idiomatic Go. I'm happy to not proceed if the team prefers the current approach
• I would keep this as a separate PR from the test coverage work to avoid mixing concerns

Would love to hear your thoughts on whether this direction is welcome before I invest time in it. Thanks!

[NawafSwe]

Hello @HT154, could you please enable the CI 🙌

[NawafSwe]

@HT154, PR now ready for review 👌

[NawafSwe]

Hello @HT154, really appreciate if you have a chance to review the PR🙏

[HT154]

Mostly clarifying questions for you, plus a bit of housekeeping

[NawafSwe]

@HT154, really helpful comments, I have left my justifications, and also addressed all of the comments.
Let me know if you have any concerns 🙏🏻

[HT154]

Looks good! Thanks

[NawafSwe]

@HT154 Thanks A lot!
Can we discuss this as well, as I am moving forward with the other PRs 🙌

[HT154]

Can we discuss this as well, as I am moving forward with the other PRs 🙌

I don't think we'd be opposed to changing this, but my primary concern with the map approach is performance. If we do take that change, I would want to see benchmarking to show that switching does not cause a performance regression.

[NawafSwe]

@HT154
I strongly agree. I will make it the last thing to focus on. I will continue with enhancing the test coverage first.