Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

attributes: print typed nil values instead of panic #6574

Merged
merged 8 commits into from
Sep 22, 2023
Merged

attributes: print typed nil values instead of panic #6574

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1114f39
attributes: catch panics during typed nil formatting by fmt.Stringer
searKing Aug 23, 2023
dbe2270
attributes: format invalid argument for '%p' to string in style `<typ…
searKing Aug 30, 2023
5936668
resolver: Add TestResolverAddressesWithTypedNilAttribute for ensures …
searKing Aug 30, 2023
e8fe6ad
attributes: use fmt.Sprint(v) instead of v.String() and recover, let …
searKing Sep 16, 2023
55f2b24
resolver: use t.Name() instead of a const scheme
searKing Sep 19, 2023
32abddd
attributes: let badVerb handle `nil`
searKing Sep 19, 2023
2e3e6ee
attributes: get rid of the special handling in the badVerb function a…
searKing Sep 19, 2023
9116500
resolver: Update doc of TestResolverAddressesWithTypedNilAttribute
searKing Sep 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 21 additions & 2 deletions attributes/attributes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ package attributes

import (
"fmt"
"reflect"
"strings"
)

Expand Down Expand Up @@ -121,8 +122,26 @@ func (a *Attributes) String() string {
return sb.String()
}

func str(x any) string {
if v, ok := x.(fmt.Stringer); ok {
func str(x any) (s string) {
defer func() {
if r := recover(); r != nil {
// If it panics with a nil pointer, just say "<nil>". The likeliest causes are a
// [fmt.Stringer] that fails to guard against nil or a nil pointer for a
// value receiver, and in either case, "<nil>" is a nice result.
//
// Adapted from the code in fmt/print.go.
if v := reflect.ValueOf(x); v.Kind() == reflect.Pointer && v.IsNil() {
s = "<nil>"
return
}

// The panic was likely not caused by fmt.Stringer.
panic(r)
}
}()
if x == nil { // NOTE: typed nils will not be caught by this check
return "<nil>"
} else if v, ok := x.(fmt.Stringer); ok {
return v.String()
searKing marked this conversation as resolved.
Show resolved Hide resolved
} else if v, ok := x.(string); ok {
return v
Expand Down
35 changes: 35 additions & 0 deletions attributes/attributes_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,14 @@ func (s stringVal) Equal(o any) bool {
return ok && s.s == os.s
}

type stringerVal struct {
s string
}

func (s stringerVal) String() string {
return s.s
}

func ExampleAttributes() {
type keyOne struct{}
type keyTwo struct{}
Expand All @@ -57,6 +65,33 @@ func ExampleAttributes_WithValue() {
// Key two: {two}
}

func ExampleAttributes_String() {
type key struct{}
var typedNil *stringerVal
a1 := attributes.New(key{}, typedNil) // typed nil implements [fmt.Stringer]
a2 := attributes.New(key{}, (*stringerVal)(nil)) // typed nil implements [fmt.Stringer]
a3 := attributes.New(key{}, (*stringVal)(nil)) // typed nil not implements [fmt.Stringer]
a4 := attributes.New(key{}, nil) // untyped nil
a5 := attributes.New(key{}, 1)
a6 := attributes.New(key{}, stringerVal{s: "two"})
a7 := attributes.New(key{}, stringVal{s: "two"})
fmt.Println("a1:", a1.String())
fmt.Println("a2:", a2.String())
fmt.Println("a3:", a3.String())
fmt.Println("a4:", a4.String())
fmt.Println("a5:", a5.String())
fmt.Println("a6:", a6.String())
fmt.Println("a7:", a7.String())
// Output:
// a1: {"<%!p(attributes_test.key={})>": "<nil>" }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have been noticing this %!p while debugging some tests and its not very pleasing to the eye (and I understand this is not introduced by your changes in this PR). Is there a way to make this better? Like is it possible to print "<attributes_test.key={}>" instead of "<%!p(attributes_test.key={})>"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

made it by change the style from %!verb(type=value) to <type=value>, for invalid argument for '%p'.
Adapted from the code in fmt/print.go.

func ExampleAttributes_String() {
	type key struct{}
	var typedNil *stringerVal
	a1 := attributes.New(key{}, typedNil)            // typed nil implements [fmt.Stringer]
	a2 := attributes.New(key{}, (*stringerVal)(nil)) // typed nil implements [fmt.Stringer]
	a3 := attributes.New(key{}, (*stringVal)(nil))   // typed nil not implements [fmt.Stringer]
	a4 := attributes.New(key{}, nil)                 // untyped nil
	a5 := attributes.New(key{}, 1)
	a6 := attributes.New(key{}, stringerVal{s: "two"})
	a7 := attributes.New(key{}, stringVal{s: "two"})
	a8 := attributes.New(1, true)
	fmt.Println("a1:", a1.String())
	fmt.Println("a2:", a2.String())
	fmt.Println("a3:", a3.String())
	fmt.Println("a4:", a4.String())
	fmt.Println("a5:", a5.String())
	fmt.Println("a6:", a6.String())
	fmt.Println("a7:", a7.String())
	fmt.Println("a8:", a8.String())
	// Output:
	// a1: {"<attributes_test.key={}>": "<nil>" }
	// a2: {"<attributes_test.key={}>": "<nil>" }
	// a3: {"<attributes_test.key={}>": "<0x0>" }
	// a4: {"<attributes_test.key={}>": "<nil>" }
	// a5: {"<attributes_test.key={}>": "<int=1>" }
	// a6: {"<attributes_test.key={}>": "two" }
	// a7: {"<attributes_test.key={}>": "<attributes_test.stringVal={two}>" }
	// a8: {"<int=1>": "<bool=true>" }
}

// a2: {"<%!p(attributes_test.key={})>": "<nil>" }
// a3: {"<%!p(attributes_test.key={})>": "<0x0>" }
// a4: {"<%!p(attributes_test.key={})>": "<nil>" }
// a5: {"<%!p(attributes_test.key={})>": "<%!p(int=1)>" }
// a6: {"<%!p(attributes_test.key={})>": "two" }
// a7: {"<%!p(attributes_test.key={})>": "<%!p(attributes_test.stringVal={two})>" }
}

// Test that two attributes with the same content are Equal.
func TestEqual(t *testing.T) {
type keyOne struct{}
Expand Down