fix(encoding): render <nil> for typed-nil Stringer pointers instead of panicking#27
Open
SAY-5 wants to merge 1 commit intophsym:mainfrom
Open
fix(encoding): render <nil> for typed-nil Stringer pointers instead of panicking#27SAY-5 wants to merge 1 commit intophsym:mainfrom
SAY-5 wants to merge 1 commit intophsym:mainfrom
Conversation
…f panicking When an attribute value is a typed-nil pointer whose element type has a String method (the canonical case being (*time.Time)(nil), because time.Time.String auto-generates a pointer receiver wrapper), the interface assertion 'case fmt.Stringer' still matched, and calling v.String() dispatched the method on a nil receiver. The runtime then panicked with 'value method time.Time.String called using nil *Time pointer' and took down the handler - surfacing in real apps as a crash whenever an optional *time.Time field was unset (phsym#22). Guard the Stringer branch with reflect: if the interface value holds a nil pointer, write the literal <nil> and return, matching the default Go format for a nil Stringer. For any non-nil Stringer, or a Stringer that does not resolve to a pointer (struct-by-value implementations, custom types), behaviour is unchanged. Add a regression test that passes a nil *time.Time as an attribute to the handler and asserts the output is 'INF foobar expiration=<nil>' rather than a panic. The test fails on master with the exact panic signature from the report. Closes phsym#22 Signed-off-by: SAY-5 <SAY-5@users.noreply.github.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
What
Fixes #22.
When an attribute value is a typed-nil pointer whose element type has a
Stringmethod - the canonical case being(*time.Time)(nil), becausetime.Time.Stringauto-generates a pointer-receiver wrapper - the interface assertioncase fmt.Stringerstill matched. Callingv.String()then dispatched the method on a nil receiver and the runtime panicked with:which took down the handler. In real apps this surfaced as a crash every time an optional
*time.Timefield was logged without being populated.Fix
Guard the Stringer branch with reflect: if the interface value holds a nil pointer, write the literal
<nil>and return - matching the default Go format for a nil Stringer. For any non-nil Stringer, or a Stringer that does not resolve to a pointer (struct-by-value implementations, custom types), behaviour is unchanged.Tests
Added
TestHandler_NilStringerPointerthat passes a nil*time.Timeas an attribute to the handler and asserts the output is exactlyINF foobar expiration=<nil>\nrather than a panic. The test fails on master with the exact panic signature from the report; with the fix it passes.Verification
Locally on macOS, go 1.26.2:
gofmt -s -l: cleango vet ./...: cleango test -race -count=1 ./...: full suite passCloses #22