Go: context: decide whether to keep Context.String format change

[dmitshur]

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 (e40dffe5):

$ 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 https://github.com/golang/go/issues/21485#issuecomment-323153829, 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]

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

[dmitshur]

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]

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.

[bradfitz]

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]

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.

[neild]

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