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.

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

runtime: panic should print named string types as is #37531

Closed
dsnet opened this issue Feb 28, 2020 · 5 comments
Closed

runtime: panic should print named string types as is #37531

dsnet opened this issue Feb 28, 2020 · 5 comments
Assignees
Labels
Milestone

Comments

@dsnet
Copy link
Member

@dsnet dsnet commented Feb 28, 2020

Consider the following:

type MyString string
panic(MyString("hello"))

This currently prints:

panic: (main.MyString) (0x48aa00,0x4c0840)

I expect it to print the actual string contents:

panic: main.MyString("hello")

Keeping the name of the type is debatable, but printing the address is unhelpful.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 28, 2020

The relevant code is printany in runtime/error.go. I think the suggestion amounts to: in the default case of the type switch, do another switch on the kind field of the type, and print the value with the type.

@ianlancetaylor ianlancetaylor added this to the Backlog milestone Feb 28, 2020
@odeke-em odeke-em self-assigned this Mar 1, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 2, 2020

Change https://golang.org/cl/221779 mentions this issue: runtime: during panic, print value instead of address, if kind is printable

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 2, 2020

Change https://golang.org/cl/221687 mentions this issue: runtime: panic should print value of self-defined types

@gopherbot gopherbot closed this in 972df38 Mar 3, 2020
@dmitshur
Copy link
Member

@dmitshur dmitshur commented Mar 4, 2020

@dsnet In the original use case you shared, have you considered using the pattern of panicking with a value of type error, and using existing high level mechanisms for constructing the error value? For example:

type MyString string
panic(fmt.Errorf("%v", MyString("hello")))

// Output:
// panic: hello

That pattern enables one to make use of all high-level printing capabilities of fmt.Errorf.

You can customize it with "%T" verb if you want to see the type name, add a description in front of the value, etc.

It can makes the diff smaller if you decide to change the panic into a return statement (since error is likely the type you'd want to return).

It scales better. Consider that CL 221779 adds some complexity to the runtime package in order to improve its support for custom types, but it still doesn't handle some cases that high-level printing functions like fmt.Errorf do. For example, compare:

type MyStruct struct {
	Text string
}
panic(MyStruct{"hello"})

// Output:
// panic: (main.MyStruct) (0x9ba80,0x40c018)
type MyStruct struct {
	Text string
}
panic(fmt.Errorf("problem with foo: %#v", MyStruct{"hello"}))

// Output:
// panic: problem with foo: main.MyStruct{Text:"hello"}

I don't think we should try to add all of complexity of fmt to runtime. If users can use panic(fmt.Errorf(...)) which handles this case and more, perhaps we should not make a change to panic at all? See also #37642 (comment).

What do you think about this?

@dsnet
Copy link
Member Author

@dsnet dsnet commented Mar 4, 2020

Generally, I use fmt.Sprintf when panicking in a form similar to what you're promoting.

This specific situation occurred because of something like the following:

panic("invalid name: " + name)

where name is a named string type (i.e. protoreflect.FullName). Since I had a string type already on hand, I decided to forgo the usual fmt.Sprintf since it seemed simpler to just concatenate them.

However, I missed the fact that concatenating an untyped string constant to a typed string variable results in a string of the named string type. This results in an unhelpful panic message containing only pointers which made debugging the issue from a server log more difficult.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.