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

fmt: add FormatString(State) string #51668

Closed
kevinburke opened this issue Mar 14, 2022 · 20 comments
Closed

fmt: add FormatString(State) string #51668

kevinburke opened this issue Mar 14, 2022 · 20 comments

Comments

@kevinburke
Copy link
Contributor

@kevinburke kevinburke commented Mar 14, 2022

I'm currently working with a custom fmt.Formatter implementation. Similar to the request in #51195, I would like to implement some methods and then "fall back" to the default fmt implementation for unimplemented verbs.

It's difficult to do this because State does not allow you to reconstruct the original format string, unless you enumerate all of the possible characters in a format string and call Flag(char) on each one. This is lengthy and error prone.

I would like to formally propose what @bcmills suggested in #25150, which is to add a String() api to fmt.State.

// State represents the printer state passed to custom formatters.
// It provides access to the io.Writer interface plus information about
// the flags and options for the operand's format specifier.
type State interface {
	// Write is the function to call to emit formatted output to be printed.
	Write(b []byte) (n int, err error)
	// Width returns the value of the width option and whether it has been set.
	Width() (wid int, ok bool)
	// Precision returns the value of the precision option and whether it has been set.
	Precision() (prec int, ok bool)

	// Flag reports whether the flag c, a character, has been set.
	Flag(c int) bool

         // String returns the original format string that was used to create this State (e.g. "%#v")
         String() string
}

I doubt that there are many implementations of the API, which would limit the amount of breakage from adding a new method.

In the standard library, there is currently only one implementation of fmt.State - in the pp struct.

If someone can give me pointers on how to do a search across all of Github, I would be happy to check whether there are in-the-wild implementations of fmt.State. I'd also appreciate if someone could do the same inside of Google.

Thanks to Bryan Mills, Github user seebs and Eric Lagergren for initial suggestions and discussion.

@bcmills
Copy link
Member

@bcmills bcmills commented Mar 14, 2022

To clarify, I don't think we should add the String method to the fmt.State interface — just to the implementation that the fmt package itself passes to Format methods.

(In retrospect I think it was a mistake to make State an interface in the first place, but given how early the fmt package was designed I can understand how that mistake could have been made at the time. 😅)

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 14, 2022

Technically we can't add a method to the fmt.State interface. But we can add a String method to the underlying type fmt.pp. Retitling.

@ianlancetaylor ianlancetaylor changed the title proposal: fmt: add String() method to fmt.State interface proposal: fmt: add String method to fmt.pp, which implements fmt.State Mar 14, 2022
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 14, 2022

CC @robpike

@kevinburke
Copy link
Contributor Author

@kevinburke kevinburke commented Mar 14, 2022

But we can add a String method to the underlying type fmt.pp. Retitling.

What would that let us do, though? Where I am imagining this would be most useful is in third party code that is inside of a Format() function call:

func (b myType) Format(w fmt.State, verb rune) {
    // here
}

The general shape of that function, I would imagine, is something like the other proposal:

  1. Implement some of the verbs and flags with my custom behavior.
  2. For the rest of them, invoke fmt.Fprintf(w, ???, b) or similar.

where ??? is, in theory, the recovered passed in format string.

If w is a fmt.pp under the hood, I don't think I can cast it to a fmt.Stringer or a fmt.pp there, can I? Sorry for being dense.

edit: ahhh... if we added String() you could do fmt.Sprint(w fmt.State) or whatever and recover the original string. Which is indirect but still gets us what we want.

@wxolp
Copy link

@wxolp wxolp commented Mar 14, 2022

I dont think this suggestion would work the way you are expecting. If I take your example:

func (b myType) Format(w fmt.State, verb rune) {
    // here
}

and then expand upon it:

func (b myType) Format(w fmt.State, verb rune) {
   fmt.Fprintf(w, w.String(), b)
}

you get a stack overflow. Can you post an example of how you would use String(), if it was implemented?

@extemporalgenome
Copy link
Contributor

@extemporalgenome extemporalgenome commented Mar 14, 2022

@ianlancetaylor presumably it'd look like:

func (b myType) Format(w fmt.State, verb rune) {
   type derivedType myType
   fmt.Fprintf(w, w.String(), derivedType(b))
}

(that would mask any myType.String method though, and %T and %#v fallbacks wouldn't display the right type name, but otherwise, it'd be a generally feasible approach)

@extemporalgenome
Copy link
Contributor

@extemporalgenome extemporalgenome commented Mar 14, 2022

That said, I believe #51195 (comment) is cleaner.

Perhaps we could just document that when fmt.State receives no writes, it'll just re-interpret the value as though it were not a fmt.Formatter, and proceed accordingly (thus the "fallback" would merely be returning from the Format method without the fmt.State.Write method having been called).

@wxolp
Copy link

@wxolp wxolp commented Mar 14, 2022

it'd be a generally feasible approach

I disagree with this. If String() was implemented as your example, then yes it does solve the problem of getting the original format string. However as you demonstrated, it creates another problem in that you have to create a new type in order to avoid a stack overflow. In addition, as you mentioned, you wont get the original type in the formatted output.

So really, you're solving one problem, and creating two more.

@rsc
Copy link
Contributor

@rsc rsc commented Mar 16, 2022

I'm not sure we should be adding invisible methods here. The solution proposed in #51195 is completely visible - it doesn't need people to know things that aren't in the type system. A new method you type-assert would be different.

@rsc
Copy link
Contributor

@rsc rsc commented Mar 16, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals Mar 16, 2022
@robpike robpike self-assigned this Mar 22, 2022
@rsc
Copy link
Contributor

@rsc rsc commented Mar 22, 2022

One possibility is to have a top-level function from fmt.State to string, like

package fmt
func FormatString(State) string

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 19, 2022

Change https://go.dev/cl/400875 mentions this issue: fmt: add a function to recover the original format string given a State

@StevenACoffman
Copy link

@StevenACoffman StevenACoffman commented May 2, 2022

@knz implemented https://github.com/knz/go-fmtfwd with some difficulty to work around this very issue.

@rsc
Copy link
Contributor

@rsc rsc commented May 4, 2022

@robpike has posted a CL at https://go-review.googlesource.com/c/go/+/400875. The API is almost what I wrote earlier, adding 'verb rune' to the argument list:

package fmt
func FormatString(state State, verb rune) string

This provides access to what was requested, just without a new State method.

Does anyone object to this API?

@rsc rsc changed the title proposal: fmt: add String method to fmt.pp, which implements fmt.State proposal: fmt: add FormatString(State) string May 4, 2022
@robpike
Copy link
Contributor

@robpike robpike commented May 5, 2022

Actually it's not quite the same. The actual change is

package fmt
func FormatString(state State, verb rune) string

as State does not contain the verb. The verb is always at the end, so the caller could add it, but doing it here can remove an allocation.

@knz
Copy link

@knz knz commented May 5, 2022

@robpike
Copy link
Contributor

@robpike robpike commented May 5, 2022

No, because State is an interface.

@rsc
Copy link
Contributor

@rsc rsc commented May 11, 2022

Updated comment to record 'verb rune' in API.

@rsc
Copy link
Contributor

@rsc rsc commented May 11, 2022

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Accept in Proposals May 11, 2022
@rsc
Copy link
Contributor

@rsc rsc commented May 18, 2022

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: fmt: add FormatString(State) string fmt: add FormatString(State) string May 18, 2022
@rsc rsc modified the milestones: Proposal, Backlog May 18, 2022
@seankhliao seankhliao changed the title fmt: add FormatString(State) string proposal: fmt: add FormatString(State) string May 18, 2022
@dmitshur dmitshur modified the milestones: Backlog, Go1.20 Jul 6, 2022
@rsc rsc changed the title proposal: fmt: add FormatString(State) string fmt: add FormatString(State) string Aug 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Development

No branches or pull requests