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

proposal: context: implement NewContext and FromContext with generics #57025

Closed
zensh opened this issue Dec 1, 2022 · 12 comments
Closed

proposal: context: implement NewContext and FromContext with generics #57025

zensh opened this issue Dec 1, 2022 · 12 comments
Labels
FrozenDueToAge generics Issue is related to generics Proposal
Milestone

Comments

@zensh
Copy link

zensh commented Dec 1, 2022

We can implements a very concisely NewContext and FromContext functions with generics:

package context

import "internal/reflectlite"

func NewContext[T any](parent Context, v *T) Context {
	return WithValue(parent, reflectlite.TypeOf((*T)(nil)), v)
}

func FromContext[T any](ctx Context) *T {
	v, _ := ctx.Value(reflectlite.TypeOf((*T)(nil))).(*T)
	return v
}

It will be very useful:

// define User
type User struct {...}

// set user into ctx
user := &User{...}
ctx := context.NewContext(context.Background(), user)

// get user from ctx
user := context.FromContext[User](ctx)
@zensh zensh added the Proposal label Dec 1, 2022
@gopherbot gopherbot added this to the Proposal milestone Dec 1, 2022
@ianlancetaylor
Copy link
Contributor

See also #49189.

@ianlancetaylor ianlancetaylor added the generics Issue is related to generics label Dec 1, 2022
@ianlancetaylor
Copy link
Contributor

CC @Sajmani @dsnet

@zensh
Copy link
Author

zensh commented Dec 2, 2022

CC @neild

@zensh
Copy link
Author

zensh commented Dec 2, 2022

Maybe we can add a helper function:

func DoIfValid[T any, V interface {
	*T
	Valid() bool
}](ctx Context, fn func(v *T)) {
	if v := FromContext[T](ctx); V(v).Valid() {
		fn(v)
	}
}

@zensh
Copy link
Author

zensh commented Dec 16, 2022

Better naming:

context.ValueInto[T any](parent Context, v *T) Context
context.ValueFrom[T any](ctx Context) *T

@Sajmani
Copy link
Contributor

Sajmani commented Dec 16, 2022

I don't see the benefit of context.NewContext: it's not safer or more concise over the existing context.WithValue function.

The context.FromContext function moves the type assertion inside the function and returns nil if the assertion fails. This seems less safe to me than what people do today, as it skips the error handling that users should be doing.

@zensh
Copy link
Author

zensh commented Dec 17, 2022

@Sajmani Thanks for your comment.

It can simplify code for so many go apps, even the go source itself.

Here is a example, people must to define a private type, a private variable as context key and two functions to manipulate it:

// type key int

We can now eliminate these duplicate code definitions if golang provides a generic version.

Also, I think this is one of the best examples of using generics.

@zensh
Copy link
Author

zensh commented Dec 17, 2022

it maybe returns a bool to indicate the assertion fails. I just don't think it's necessary. A nil *T can indicate assertion fails.

context.ValueFrom[T any](ctx Context) (*T, bool)

@Sajmani
Copy link
Contributor

Sajmani commented Dec 18, 2022

I see: this function associates a value with a context using the value's reflect.Type as the key. I agree this is convenient.

The problem with this is that two packages that are unaware of each other might try to associate the same type with a context, such as a Logger or database handle. If the programmer is not aware of this possibility, this could lead to very strange bugs.

We could provide guidance that this feature should only be used with a package's own types, but I think many people will be tempted by the convenience to misuse this feature.

In my opinion the risks outweigh the benefits, but I'm willing to hear from others.

@zensh
Copy link
Author

zensh commented Jan 13, 2023

@ianlancetaylor @dsnet @neild
Does anybody like this proposal? If nobody like it, I will close the issue.

I have used it in my projects:

https://github.com/ldclabs/ldvm/blob/main/util/value/context.go#L12

https://github.com/teambition/gear/blob/master/context.go#L41

@ianlancetaylor
Copy link
Contributor

I don't see a lot of support for this proposal, and it can be implemented outside of the standard library. Perhaps it should be an external package for now, and we can consider adopting it into the standard library if it sees wide use.

@zensh zensh closed this as completed Jan 14, 2023
@rsc
Copy link
Contributor

rsc commented Mar 29, 2023

This proposal has been declined as retracted.
— rsc for the proposal review group

@golang golang locked and limited conversation to collaborators Mar 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge generics Issue is related to generics Proposal
Projects
None yet
Development

No branches or pull requests

5 participants