Skip to content

context: decide whether to keep Context.String format change #31620

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
Closed
dmitshur opened this issue Apr 23, 2019 · 6 comments
Closed

context: decide whether to keep Context.String format change #31620

dmitshur opened this issue Apr 23, 2019 · 6 comments
Labels
FrozenDueToAge release-blocker
Milestone
Go1.13

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Apr 23, 2019 •
edited
Loading

CL 169080 was made with the goal of making context no longer import fmt, which was one of the steps towards resolving #30440. The current implementation resulted in a context.Context.(fmt.Stringer) formatting change.

With Go 1.12.4:

$ goexec -quiet 'type key int; var userKey key;
    fmt.Println(context.WithValue(context.Background(), userKey, "some user"))'
context.Background.WithValue(0, "some user")

With Go tip (e40dffe):

$ PATH="$(gotip env GOROOT)/bin:$PATH" \
goexec -quiet 'type key int; var userKey key;
    fmt.Println(context.WithValue(context.Background(), userKey, "some user"))'
context.Background.WithValue(type main.key, val some user)

This was brought up in a code review comment and Brad said "let's revisit this".

This issue is about making that decision for 1.13. /cc @bradfitz

If I remember correctly, fmt.Stringer formatting is meant for human consumption only and is therefore not guaranteed to be stable across Go releases. But I'm not 100% sure what our policy is. The closest thing I could find now was #21485 (comment), but it was pretty specific to time.Time.String method. Is stability of fmt.Stringer output across Go 1.x releases documented anywhere, or should it be? /cc @dsnet @ianlancetaylor

That means test should not depend on it, but if they do, they need to be ready to be updated. Once a decision here is made, it'll be possible to know how those tests should be updated for 1.13.
@dmitshur dmitshur added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. release-blocker labels Apr 23, 2019
@dmitshur dmitshur added this to the Go1.13 milestone Apr 23, 2019
@dsnet
Copy link
Member

dsnet commented Apr 23, 2019

Historically, the String methods have generally not been considered stable. How many tests break as a result of this change?

@dmitshur
Copy link
Contributor Author

Answered with more detail in a direct message, but to summarize: my findings show very few such tests.

It's also worth pointing out that there is no String method in the public API of the context package, or any mention of fmt.Stringer being implemented by any of its concrete types (i.e., see https://godoc.org/context).

@bradfitz
Copy link
Contributor

I'm also curious the extent of the test failures.

Few options:

  1. we can modify the new String just enough to match the previous output for enough cases, if there's a common enough pattern of failures.

  2. we could make the fmt package blank import some internal package where it registers a formatting hook for contexts. Then the context package could conditionally use that same internal package & hook if non-nil and get the same formatting behavior, but only if fmt is already registered. That's a little surprising behavior, but OTOH it's a little hard to observe the change without fmt anyway, so fmt will typically be loaded in any reasonably-sized program.

I'd like to do (1) if possible, but (2) is an option if we need it.

I suppose option (3) is just fix all the tests, but that's likely more work for us & users if the breakage is widespread.

@dsnet dsnet changed the title context: made a decision whether to keep Context.(fmt.Stringer) formatting change context: make a decision whether to keep Context.(fmt.Stringer) formatting change Apr 26, 2019
@bradfitz
Copy link
Contributor

So far I've only seen one failure and it was obviously brittle. I'm inclined to do nothing so far, but we'll see if more pop up.

@rsc rsc changed the title context: make a decision whether to keep Context.(fmt.Stringer) formatting change context: decide whether to keep Context.String format change Apr 30, 2019
@rsc
Copy link
Contributor

rsc commented Apr 30, 2019

Let's leave this open through at least the beta, but I'm completely OK with the format change. If there are no serious complaints, keep it.

@bradfitz bradfitz assigned neild and unassigned bradfitz May 29, 2019
@neild
Copy link
Contributor

neild commented May 29, 2019

Very few tests affected, and any affected tests are clearly brittle. I think we should keep the change.

@neild neild closed this as completed May 29, 2019
@dmitshur dmitshur removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label May 29, 2019
@golang golang locked and limited conversation to collaborators May 28, 2020
@rsc rsc unassigned neild Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge release-blocker
Projects
None yet
Milestone
Go1.13
Development

No branches or pull requests
6 participants
@bradfitz @neild @rsc @dmitshur @dsnet @gopherbot