Ambiguity in the context styleguide.

Hey guys,

We're having some discussions regarding whether or not Gin Context violates the Go context style guide. I'd like to hear your thoughts on this.

According to the Go style guide (reference: Go Style Guide on Custom Contexts), it clearly states:

Do not create custom context types or use interfaces other than context.Context in function signatures. There are no exceptions to this rule.

However, in the case of Gin's HandlerFunc, it passes a custom *Context type.

Even though the Gin Context implements all the methods of the context.Context interface,

Deadline() (deadline time.Time, ok bool)
Done() <-chan struct{}
Err() error
Value(key any) any

as shown here

We still need to interpret the Go style guide. There seem to be two possible views on this:

Gin's Context is acceptable: Since it implements the context.Context interface, it can be considered valid, and thus doesn't violate the style guide. Similarly, we could create other types like this:
```
type interface CustomContext {
    context.Context
    // ...additional APIs that we need
}
```
Strict compliance: To adhere to the Go style guide strictly, we should always pass context.Context directly in function signatures. In this case, the design would need to change to separate the Context from custom APIs. For example, the function signature could be:
```
type HandlerFunc func(ctx context.Context, api CustomAPIs)
```

What do you guys think?

This style guidance was written for Google internally for itself and its projects. Whether you find its guidance useful to you and your team is another matter and similarly what parts you apply and what you don't. That's really a question of business value and requirements, IMO. The material is written to help provide clarity about how Go is used internally and assist open source projects that want to seek good fitness with the internal codebase. I personally think think there is a lot of value in disseminating the style guidance norms somewhat widely since they are instructional in several ways: what lessons have been learned from using Go at scale?

To your question: I had thought that package context's documentation provided some guidance of its own on this question in the past, but doesn't appear to. The reasoning for this part of the guidance is implied by the mention of "static analysis" tools in this fragment:

// Programs that use Contexts should follow these rules to keep interfaces
// consistent across packages and enable static analysis tools to check context
// propagation:

There exist tools that perform context migration and analysis tasks. Custom context types are opaque to them, and nobody wants to maintain the conversion and matching behavior for all permutations.

Then see it through this lens, which occurs below the text you cite from the style guide:

Imagine if every team had a custom context. Every function call from package p to package q would have to determine how to convert a p.Context to a q.Context, for all pairs of packages p and q. This is impractical and error prone for humans, and it makes automated refactorings that add context parameters nearly impossible.

It's a real problem for both static tools and humans (particularly API maintainers). The number of fundamental context types needed for the gamut of problems to solve is quite limited. At the end of the day, the place where a majority of additional behavioral specification that would be likely to be made is in context value management. Skimming over what the Gin context is and does, to me, I'd probably have implemented it in terms of an accessor API:

package ginrequest

type key struct{}
var contextKey key

type Data struct { /* metadata */ }

// TODO: Add additional Gin-specific behaviors to type Data.

func NewContext(ctx context.Context) (context.Context, *Data) {
  // Maybe set default values of md, or alternatively maybe Gin creates md and passes it to
  // NewContext as an arugment.
  var md Data  
  return context.WithValue(ctx, contextKey, &md), &md
}

func FromContext(ctx context.Context) (md *Data, ok bool) {
  md, ok = ctx.Value(contextKey).(*Data)
  return md, ok
}

That leans more in direction of no. 2. The nice part about no. 2 is that the API you need to maintain is Data and its struct fields and methods. By keeping the struct Data separate from the implementation of the context.Context, you don't interleave two disconnected APIs together, which makes a prevents growing the API surface you to have to maintain (think single responsibility principle). Managing Hyrum's Law comes to mind with APIs, so smaller is generally better. Imagine the extra complexity of maintaining a hypothetical ginrequest.Data if it added additionally four methods to implement context.Context. That wouldn't be fun for anyone.

Thanks a lot @matttproud! appreciated the background and insights here!

Hi @matttproud , regardless of the SRP for mixing APIs under the context, still one confusion to me is:

type interface CustomContext {
    context.Context
    // ...additional APIs that we need
}

isn’t a CustomContext implementation for the above interface also considered to be a context.Context type if it also implements all the context.Context APIs due to duck typing? meaning, the Gin Context actually is not a “custom context types” per se, but rather a valid context.Context type as well?

appreciate any insight you may have!

It would be both a custom context type and a valid context type (the latter due to satisfying the semantics of interface context.Context), but that doesn't necessarily change several core considerations:

Internally, package context can utilize special performance-enhancing and resource cost-saving shortcuts for the core context types implemented in the package. Consider that context.WithCancel is implemented internally as context.cancelCtx, and context.WithDeadline and context.WithTimeout as context.timerCtx, and context.WithValue as context.valueCtx. If you look inside of the source for package context, you'll see it employ type switches for getting attached side-channel values and child context cancellation management (deriving from parent cancellation). In certain systems, these types of benefits can add up significantly — enough so that the code was even written as it was here instead of the simplest solution (just using the native API of context alone).
Static tooling to analyze and rewrite programs is a bit tricky. To reliably analyze custom context types, one would need to verify interface satisfaction. This can be done by-hand in AST, but it's rather tedious and error prone; but to do this well, the entirety of the packages need to be loaded for good type checking (think: typed AST versus untyped), and there are special packages that are used for this (e.g., deprecated package loader and current package packages — more Go tools should be implemented in terms of these). A challenge is that the these resolver packages have some vagaries about what kinds of workloads they can be run on (e.g., only on a developer workstation manually versus say a programmatic map reduce with a custom GOPACKAGESDRIVER). So this is to say that for the purpose/fitness reasons I mentioned in my original reply, it becomes rather onerous to build tooling to work with the custom types confidently. You might as well keep the requirements for a source rewriting/analysis tool as naive as possible. Custom context types make that less tenable.
These reasons still ignore the original problem that the style documentation called out: n packages that implement m custom context types. How are they to be converted from one to the other (see the links above)? Could you imagine the packages that implement the custom context types doing these conversions, too? It'd produce really tight coupling.

And then there's this unergonomic mess:

 func f(ctx gin.CustomContext) {
   ctx, cancel := context.WithCancel(ctx, time.Second)  // We need a shorter deadline for ${REASONS}.
   defer cancel()
   g(ctx.(gin.CustomContext))
 }

 func g(ctx gin.CustomContext) { ... }

Seeing Hyrum's Law in action, I would not exclude seeing what I am going to show below in leaf code (what func g is above):

 func h(ctx context.Context) {
   ginCtx, ok := ctx.(gin.CustomContext)
   if !ok {
     panic("Time to die, maintainer!")
   }
   // use ginCtx
 }

Talk about error prone and surprising.

google / styleguide

Ambiguity in the context styleguide. #850