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

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

Open
dmitshur opened this issue Apr 23, 2019 · 5 comments

Comments

Projects
None yet
4 participants
@dmitshur
Copy link
Member

commented Apr 23, 2019

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.

@dsnet

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2019

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

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

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

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

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

This comment has been minimized.

Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.