-
Notifications
You must be signed in to change notification settings - Fork 18k
proposal: pointer package #38298
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
Comments
I often need these, and also get annoyed by the proliferation of silly helper packages. I think a nicer fix would be a small language change to allow easier construction of pointers from literals:
I thought we had an issue for something like this, but now I'm having trouble finding it. |
Requesting inclusion in #33502. |
In my https://github.com/rocketlaunchr/igo#address-operator package, I solved it doing this: v := "string"
vptr := &[]string{"string"}[0] // inlined instead of using &v |
I am unclear on the perf implications, but it seems like everything escapes to the heap, meaning this is all just sugar: (though technically package main
import "fmt"
func main() {
a := "string"
p0 := &a
p1 := func() *string { b := "string"; return &b }()
p2 := func(c string) *string { return &c }("string")
p3 := &[]string{"string"}[0]
fmt.Println(a, p0, p1, p2, p3)
}
|
@carnott-snap For my https://github.com/rocketlaunchr/igo, in your opinion which technique should I use in terms of performance or memory? (obviously they are all close to negligible but |
Benchmarking seems to show that all of these are operating at 30 ns/op, so that should not matter. var p4 *string
{
a := "string"
p4 = &a
} |
Another alternative is to give new a value parameter.
I don't like this idea. |
@as Even though you thumbed down your suggestion, eg import p "pointerpkg"
b := p.Bool(true) vs b := new(bool, true) |
While I don’t like it, I admit it may be of value for the discussion. It seems more orthogonal than a package for every scalar type. |
Just a random observation for evaluating the proposal: if the goal to be achieved by taking the pointer is signaling whether the value is defined or not (this comes up often e.g. when handling serialization formats like JSON or protobuf to distinguish between a value being unset or having the zero value), then arguably a better solution would be, with generics, some kind of optional container (like |
My proposal does not speak to why pointers of scalars are needed, in part to prevent bikeshedding, but there are a number of other use cases that require a pointer to a value: To your point about optionals, I agree and tend to prefer multi return, |
It is important to discuss why a proposal is needed. That is key to evaluating its benefit. If all a proposal has are costs, we will not adopt it. In this case, I don't actually see why this proposal is needed, except for protocol buffers, which already define these helpers. The comment above this one mentions package flag and encoding/json. But this package wouldn't seem to help either. For package flag, you register a flag with
That doesn't require the new pointer package. There is also a helper flag.IntVar that takes a *int, which can be used like this:
(This is more awkward to write but sometimes you want count to be a plain int instead of a *int.)
because now you have no way to access the flag value. You could write:
but that's equivalent to the first example above (using For package json, the main place that requires a pointer is the call to Unmarshal. Just like with flag.IntVar, if you pass the result of pointer.Foo into Unmarshal, you have no way to retrieve the value. It's true that some people write structs with What other use cases would justify adding this package? |
Apologies, I missed one; will have added it to the list at the beginning. My argument is that this proliferation of these helpers is problematic. It is not just protocol buffers, note that thrift does the same thing. Do we expect every new IDL or interested author to implement them? There is a clear need from the community, and no canonical implementation, so can we publish one somewhere? It need not be stdlib, I am happy with it living in
This sentiment is contrary to the preferences I have seen in the wild. Customers will pull in the whole AWS sdk just to get
I see two primary use cases: mutability and optionality: For mutability, I agree that taking an inline pointer to a stack variable is likely sufficient, and frequently what I do, but some people still prefer to grab a pointer helper, and store For optionality, we can say that you should use meaningful defaults, but people currently do not listen, or need to be able to represent |
I think the point is, |
Honestly, it was probably a mistake in proto to use pointers for optionality. I don't really want to encourage more of that.
If the customer is already using AWS, that seems fine. If not, that seems a bit strange (and maybe a teachable moment about dependencies). I certainly don't think it's the norm to import aws just for a It is worth noting that this whole package reduces to a single line with the contracts proposal that @ianlancetaylor talked about at Gophercon last year:
and you could use it like PointerTo(1), without needing to say (int). Are there uses other than proto-like IDLs? |
A bit more detail on some proto-like use cases other than proto itself. I see this problem in our (@Khan) codebase with GraphQL APIs (for us, via gqlgen and shurcooL/graphql), and with database models (for us, via Google Cloud Datastore). Only shurcooL/graphql includes pointer helpers, although obviously the others could. Perhaps using pointers for optionality was the wrong choice for these packages, too, but it's definitely not just proto that encourages it. Datastore does have an omitempty option, but the way gqlgen works makes it less clear how that could be done. And for both we do sometimes need both zero and null, sometimes for semantic reasons and sometimes for legacy reasons or interop with other languages. One thing to add to that: the most common place where this comes out to be a problem for us is in tests. In non-test code the value you want to take the address of often comes from application logic, and assigning it to a variable may already be necessary or improve readability. But in tests you often just want to use a literal string/int/etc., and it's clearest to put it inline. (Personally, I still end up ambivalent on the proposal: I don't like the cognitive overhead of having a function that you might have to look up what it does. A standard library package is less bad than a third-party package one is less likely to have seen, but it also sanctions the practice. Assigning a variable, or some of the options in #9097, have more obvious effect even if you've never seen them before.) |
In practice and because of my functional background, I agree, but I have yet to find a good solution for optionality. My desired outcome is a solution to pointer helper diaspora, so I am open to options. Is there interest in an E.g. without generics we cannot get the filter/map/reduce benefits: var s string = option.Some("string").Map(strings.ToLower).Or("default") And are just left with the same pointer interface, just cluttered with different boilerplate: var s option.String // var s *string
s = strings.Some("string") // s = func() *string() { s := "string"; return &s }()
if s.Nil() { /* ... */ } // if s == nil { /* ... */ }
return s.Value() // return *s |
Generic options might be interesting but wouldn't retroactively change the existing APIs. Based on the discussion above, this seems like a likely decline. |
I think we are on the same page, but I would also like to change the status quo. Can we codify your claim that "[i]t was probably a mistake in proto to use pointers for optionality. I don't really want to encourage more of that". One option would be to give a clear suggestion for optionality, like:
Also, I am curious if there is nowhere in the listed import paths where this package could be published. I agree that stdlib is not right, but putting a deprecated package in |
Speaking personally, I think the right choice for optionality is going to depend on other details. In something like proto, I think the natural approaches would be to add a presence field for each value field, or to use one or more uint bitmasks recording presence information for each field. The point is that it should not be implemented with pointers, as that puts more load on the garbage collector and destroys memory locality. For data structures other than proto, different guidelines may apply. It depends on how they are used. Anybody can create a go-gettable pointer package, it doesn't have to be in x/exp. |
Do you think there is anywhere that would be reasonable to document the problems and alternatives of using pointers for optionality? This question has come up several times at work, but I do not have anything I can point to, at least in part due to my own naivete. |
Write a blog post? I don't know of a good place for that kind of information on the Go website. Sorry. |
No change in consensus, so declined. |
Description
Due to addressability rules, some literals cannot have pointers made of them inline. This has led to a number of third party implementations of this logic:
I think pointer is a better package than the alternative, ptr, since that could conflict with common variable names. I would like to propose that this logic be merged into one of the following locations:
pointer
golang.org/x/pointer
golang.org/x/tools/pointer
golang.org/x/exp/pointer
Rebuttal
While I am pretty sure this could be solved with the current generics proposal, or reflection, it is my opinion that these 18 functions provide sufficient benefit to justify their existence today.
Exclusions
Many third party implementations include other primitives that help with other pointer issues. Due to a variety of issues I do not think they warrant inclusion today, and are broken out below for completeness.
func Xxx(pkg.Xxx) *pkg.Xxx
Functions for named types like
time.Time
ortime.Duration
are also useful, but even limiting ourselves to the stdlib could be a lot of symbols.func XxxEqual(*xxx, *xxx) bool
While this can be helpful, I think
reflect.DeepEqual
meets the needs, at some runtime cost.func XxxSlice([]xxx) []*xxx
Pointering a slice can be painful, but the lack of support for custom types means this is more about convenience than covering addressability issues. This feels like a problem for generics.
func XxxYyyMap(map[xxx]yyy) map[xxx]*yyy
This just feels really niche, and would benefit more from a generic solution. Even more than slices, because of explosion of possibilities, this suffers from the lack of support for custom types.
func XxxValue(*xxx) xxx
This is really just a convenience function for "give me the default value if nil, instead of
panic
king". It is helpful, but really should apply to all types, so I vote generic solution.func XxxYyyValueMap(map[xxx]*yyy) map[xxx]yyy
/func XxxValueSlice([]xxx) []xxx
All the problems with
Map
andSlice
, plus the complexities ofValue
: use generics.Prior Art
google.golang.org/protobuf/proto
github.com/AlekSi/pointer
go.uber.org/thriftrw/ptr
github.com/turbinelabs/nonstdlib/ptr
github.com/wacul/ptr
github.com/go-courier/ptr
github.com/aws/aws-sdk-go/aws
The text was updated successfully, but these errors were encountered: