Disallow basic types as keys in context.WithValue() #245
Conversation
| @@ -1436,6 +1437,47 @@ func (f *file) lintTimeNames() { | |||
| }) | |||
| } | |||
|
|
|||
| // lintContextKeyTypes checks for call expressions to context.WithValue with | |||
| // basic types used for the key parameter. | |||
| }) | ||
| } | ||
|
|
||
| // checkContextKeyType checks a single call expression to determine if it |
There was a problem hiding this comment.
"checkContextKeyType reports an error if the call expression calls context.WithValue with a key argument of basic type."
| key := f.pkg.typesInfo.Types[x.Args[1]] | ||
|
|
||
| if _, ok := key.Type.(*types.Basic); ok { | ||
| f.errorf(x, 1.0, category("context"), fmt.Sprintf("should not use basic type %s as key in context.WithValue()", key.Type)) |
| fmt.Println() // not in package context | ||
| context.Background() // wrong function | ||
| context.TODO() // wrong function | ||
| context.WithValue(context.Background(), "foo", "bar") // MATCH /should not use basic type( untyped|)? string as key in context.WithValue()/ |
There was a problem hiding this comment.
Use a variable c instead of context.Backround() and reduce this wall of text.
|
LGTM |
|
Merged as c6242af |
|
There has to be a way to turn off this check... where is it? Code fixes for this linting error are causing various subtle issues in web implementations that are from trivial to fix, with apparently little benefit (I personally don't see anything wrong with using strings as key type, which is perfectly supported by go). |
You really don't want to use strings as the keys. That means there's no isolated namespaces between packages, so different packages can collide and use the same keys, since everybody has access to the |
|
Problem is that something inserted into a context in one package with one key, can only fetched from that context with the exact same key instance in another package... only exact same one, and not a new one. This works fine with primitives, and not really with structs. |
|
Any package using context.WithValue and defining key types should either:
Two packages should not both be using context.WithValue using the same type, unless it's exported by another package. |
|
This doesn't work too well if the package where key is defined (and calls context withValue) instantiates child package where context keys are used to fetch values from context using .Value(), leading to cyclical dep issues/compilation issues and ultimately need to define a third package where such exported context keys are really defined as constants. Anyway, my question is where is the flag to turn off this lint check? Maybe this check is useful to some, but not to all. |
|
We don't plan to provide a flag to disable this check. We strongly believe you should not misuse context in such a way. |
|
Misuse? Huh. Oki, thanks. |
|
That we consider it misuse shouldn't come as a surprise. We wouldn't have added a lint rule otherwise. |
|
Just to reiterate - enforcing this rule leads to bugs in code that doesn't use keys defined as const instances of stucts, across packages. I understand the issue this check is trying to solve regarding name collision on primitive keys, however it's nothing new to most developers since it's present in most other context/session management frameworks in other languages. However the subtlety of struct keys - and their equivalence for purposes of key in context is difficult to get right, and is complex to implement. Thus this rule should be configurable and individual teams should be able to decide on whether to use it. |
|
I am a newly minted Go programmer. I've started using contexts and I think they are great. I noticed something interesting. The linter warns for use of primitive types as keys for WithValue, however it does not warn when using ctx.Value(key) when key is string Strings are not identified there. I like the idea of having a warning there as well. HFC |
Fixes golang/go#17293.
I was asked to move this check into
golintinstead ofgo vet.Here's the original CL: https://go-review.googlesource.com/c/30084.
After addressing @adonovan's comments, I performed some minor tweaks to make it work with
golintinstead.However I am not certain about, and would appreciate suggestions on:
EDIT: CI was broken, but with minor modifications, this change now works with Go 1.5, 1.6, and 1.7.1.
CC @bradfitz @robpike @adonovan @dsymonds