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

Jump to bottom

proposal: runtime: make print/println print interfaces #37642

Closed
odeke-em opened this issue Mar 3, 2020 · 8 comments
Closed

proposal: runtime: make print/println print interfaces #37642

odeke-em opened this issue Mar 3, 2020 · 8 comments
Labels
FrozenDueToAge Proposal
Projects
Proposals (old)
Milestone
Proposal

Comments

@odeke-em
Copy link
Member

odeke-em commented Mar 3, 2020

TL;DR: depending on if the suggestion meets the bar by necessity and usage,
given the program https://play.golang.org/p/HyZYzxbzxvf

package main

type MyInt int
type MyString string

func main() {
	println(interface{}(MyInt(10)))
	println(MyInt(10))
	println(interface{}(MyString("hey")))
	println(MyString("hey"))
}

instead of printing

(0x995e0,0xb21fc)
10
(0x99620,0xb22b0)
hey

it should print

10
10
hey
hey

Background

Coming here from a CL that I worked on over the weekend and submitted https://go-review.googlesource.com/c/go/+/221779 to address #37563 in which, given an argument
passed into panic, whereby the argument's type was derived from scalar (not composite) types i.e.

bool, complex64, complex128, float32, float64, int, int8, int16,
int32, int64, string, uint, uint8, uint16, uint32, uint64, uintptr

then we'd reflect and print out the underlying value instead of just the address so

package main

type MyString string
func main() {
    panic(MyString("This one"))
}

and when run go run main.go

Before

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

After

    panic: main.MyString("This one")

In a comment https://go-review.googlesource.com/c/go/+/221779/8#message-96fa520e85ebf3a92c0e97e475edb8b27279ba68, @mvdan asked whether we would should do the same thing for print/println, and I replied with https://go-review.googlesource.com/c/go/+/221779/8#message-96fa520e85ebf3a92c0e97e475edb8b27279ba68
in which I express that it makes sense to implement that change for panic since panic is the only API for its use but print/println are accessible via fmt.Print APIs so not commonly used by Go developers outside of the core team (but this is just my assumption)

Question

Should we perhaps be making a change so that print/println check their interface arguments' type tags and print out the underlying value if we encounter types derived from scalar values?
What is the demand for this feature? Would it perhaps help in better debugging for the runtime, compiler and other package that can't yet access fmt.Print*, maybe because of import cycles or allocation costs? What could be the performance impact for switching on type tags?
@gopherbot gopherbot added this to the Proposal milestone Mar 3, 2020
@cherrymui
Copy link
Member

Personally I really like the address print, which is very useful when debugging low level stuff, e.g. to know where the interface data is allocated, on heap or on stack. Maybe it's only me...

It makes sense to print the value in panic message, but the builtin print function is mostly for debugging, so I think it is okay to be more "primitive".

@dmitshur
Copy link
Contributor

dmitshur commented Mar 4, 2020 •
edited

I understand this proposal was made so that this change can be discussed and considered, given that a similar change to panic behavior was done in CL 221779. Thanks for providing that background and making this proposal.

My opinion is that this is not a change we should make. The rationale follows.

We currently have fmt.Print{,f,ln} functions that offer high-level printing behavior, with many features, customization via verbs, and support for printing structs. We have print{,ln} built-in functions that offer low-level printing behavior, documented as "useful for bootstrapping and debugging".

Making this change will reduce the gap between the two function groups by making print{,ln} higher level. But that doesn't seem to be a net improvement. Users who want higher level behavior should use fmt.Print{,f,ln}, which already offers this feature and much more.

See also a comment I left on the recent panic change.

@dmitshur dmitshur mentioned this issue Mar 4, 2020
Closed
@rsc
Copy link
Contributor

rsc commented Mar 4, 2020

I agree with @cherrymui and @dmitshur - print and println are for when you have nothing else (bootstrapping and debugging) and should not take on additional complexity that might fail. What if you are printing a corrupted interface value? Then print crashes if you start down this path.

If you want a full-featured print, that's what fmt is for.

@bcmills
Copy link
Contributor

bcmills commented Mar 4, 2020 •
edited

FWIW, I tend to use println as a signal for “this is a temporary print that should be flagged if it ever gets into code review”.

For that use-case, I do end up writing println(fmt.Sprint(⋯)) pretty frequently. On the other hand, the nested fmt call isn't much trouble to add.

@rsc rsc added this to Active in Proposals (old) Mar 4, 2020
@rsc rsc changed the title proposal: builtin, cmd/compile: make print/println check interface tags and printout values derived from builtin scalar types, instead of addresses proposal: runtime: make print/println check interface tags and printout values derived from builtin scalar types, instead of addresses Mar 4, 2020
@rsc rsc changed the title proposal: runtime: make print/println check interface tags and printout values derived from builtin scalar types, instead of addresses proposal: runtime: make print/println print interfaces Mar 4, 2020
@rsc rsc mentioned this issue Mar 4, 2020
Open
@odeke-em
Copy link
Member Author

odeke-em commented Mar 6, 2020

Awesome, thank you all for the feedback, it matches my thoughts too.

@mvdan, any comments or ideas to express here, as the proposal seems to be fizzling out because of the same reasons?

@rsc
Copy link
Contributor

rsc commented Mar 6, 2020

Another difference that I just realized is that panic is a function that takes an interface value. You can't help but create an interface when you call panic, so it makes sense to take it apart for printing.

In contrast, print is magical and actually gets the values you pass; it only sees an interface when you pass an interface.

@mvdan
Copy link
Member

mvdan commented Mar 7, 2020

I tend to agree with the above reasoning that print doesn't need fancy pretty printing, because fmt exists. But then, shouldn't the same apply to panic? One could use panic(fmt.Sprintf(...)). I guess the only reason not to do it there is if the package is too low-level to import fmt. Though that seems like a very rare edge case that could be handled manually.

This is roughly what was briefly discussed in #37531. I don't feel strongly about it; I was just surprised by the need to add the code there.

All in all, I'm OK with closing this proposal. @rsc raises a good point in the last comment as well.

@odeke-em
Copy link
Member Author

odeke-em commented Mar 8, 2020

Thank you everyone for the discussion, I shall now close this issue.

@odeke-em odeke-em closed this as completed Mar 8, 2020
@rsc rsc moved this from Active to Declined in Proposals (old) Mar 11, 2020
@golang golang locked and limited conversation to collaborators Mar 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Proposal
Projects
No open projects
Proposals (old)
Declined
Milestone
Proposal
Development

No branches or pull requests
7 participants
@rsc @dmitshur @mvdan @odeke-em @bcmills @gopherbot @cherrymui