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

go/doc: treat function returning array as constructor #22856

Closed
willfaught opened this issue Nov 23, 2017 · 13 comments
Closed

go/doc: treat function returning array as constructor #22856

willfaught opened this issue Nov 23, 2017 · 13 comments

Comments

@willfaught
Copy link
Contributor

We should consistently implement #18063 with all types parameterized by one type that make sense. The missing ones are array and receiving channel. This would give us:

type T t
    func NewT1() T
    func NewT2() []T
    func NewT3() [N]T
    func NewT4() chan T
    func NewT5() <-chan T

Plus all the *T equivalents, obviously.

@gopherbot gopherbot added this to the Proposal milestone Nov 23, 2017
@mvdan
Copy link
Member

mvdan commented Nov 23, 2017

Do you have any examples? We did not do arrays in the previous issue because noone could find a decent, real use case.

@willfaught
Copy link
Contributor Author

willfaught commented Nov 23, 2017 via email

@mvdan
Copy link
Member

mvdan commented Nov 23, 2017

Those still seem like examples you came up with, not real examples from the standard library or well known packages.

I think it's important that we base this on common practices. Otherwise, it would be hard to tell when to stop. For example, what about funcs that return a func returning T? And what if we combine some of these rules, and end up with a func like:

func Foo() chan func() [2]T

The reason that slices were added was because there were a few occurences in the standard library, so it was a clear win. I can't think of any such real cases involving arrays and channels off the top of my head, hence why I'm asking if you have any.

@willfaught
Copy link
Contributor Author

Those still seem like examples you came up with, not real examples from the standard library or well known packages.

As far as I know, there aren't any in the standard library. I don't know about golang.org/x or other well-known packages.

I think it's important that we base this on common practices. Otherwise, it would be hard to tell when to stop. For example, what about funcs that return a func returning T? And what if we combine some of these rules, and end up with a func like:

func Foo() chan func() [2]T

The reason that slices were added was because there were a few occurences in the standard library, so it was a clear win. I can't think of any such real cases involving arrays and channels off the top of my head, hence why I'm asking if you have any.

I hear what you're saying about pragmatism, and I agree that it can steer you right in a lot of cases. My concern is that consistency is important too. Things that intuitively seem alike should be alike.

The pattern of T, *T, []T, and []*T is something like "T or *T or types parameterized only by T or *T that you can get out". func() T, chan func() T, and [][]T don't match it. That scope seems manageable to me.

Here are perhaps some better array examples:

func NewIP(s string) [4]Octet
// ...
// ...
// ...
// ...
type Octet byte

versus

type Octet byte
    func NewIP(s string) [4]Octet

And perhaps some better channel examples:

func NewEventStream() chan Event
// ...
// ...
// ...
// ...
type Event int

versus

type Event int
    func NewEventStream() chan Event

@rsc rsc changed the title proposal: cmd/godoc: treat function returning array or receiving channel as constructor proposal: go/doc: treat function returning array or receiving channel as constructor Nov 27, 2017
@rsc
Copy link
Contributor

rsc commented Nov 27, 2017

Arrays probably should have gone in with slices. I'm skeptical about functions returning channels (or maps) always being constructors. time.Tick returns a chan time.Time but it's not a time.Time constructor.

proposal-review agrees with arrays but let's stop there.

@rsc rsc changed the title proposal: go/doc: treat function returning array or receiving channel as constructor go/doc: treat function returning array as constructor Nov 27, 2017
@rsc rsc modified the milestones: Proposal, Go1.11 Nov 27, 2017
@rsc
Copy link
Contributor

rsc commented Nov 27, 2017

Accepted; CLs welcome for Go 1.11.

@willfaught
Copy link
Contributor Author

I'm skeptical about functions returning channels (or maps) always being constructors. time.Tick returns a chan time.Time but it's not a time.Time constructor.

Then please allow me one last shot at convincing you. :)

Regarding maps, T or *T or types parameterized only by T or *T that you can get out doesn't match maps, because there are two type parameters, so it's unclear which was constructed, and where the other came from. The same is true of functions.

Regarding channels, it depends on how you define "constructor". Values are constructed in many ways from many sources, and not all of those sources are "constructors" as we mean here. IMO, it's simple and intuitive to define a constructor for a named type as above. You can even omit the "that you can get out" part, since it's common sense, leaving just T or *T or types parameterized only by one T or *T. (Note that I added "one" to clarify.)

So, the way I see it, Tick is a Time constructor. It actually does construct Times, which are given by the channel. Like a Time channel constructor, a Time slice constructor also constructs Times, which are given by the slice. I can understand if others don't see it that way, though.

@DeedleFake
Copy link

DeedleFake commented Nov 28, 2017

So, the way I see it, Tick is a Time constructor. It actually does construct Times, which are given by the channel. Like a Time channel constructor, a Time slice constructor also constructs Times, which are given by the slice. I can understand if others don't see it that way, though.

I think it's because the entire purpose of classifying constructors is to make it easier to figure out what functions to call in order to get new values of that type. The primary purpose of time.Tick() is not to get new time.Time values; it's to have values sent to a channel at regular intervals. The fact that they're time.Times is kind of ancillary.

The case that I sometimes have problems with is that of a function that constructs multiple types at the same time. For example, SDL has a function, SDL_CreateWindowAndRenderer, that creates an SDL_Window and an SDL_Renderer at the same time. In C, this is done by passing two double pointers, so the function that creates them takes a **SDL_Window and an **SDL_Renderer as two of its arguments. Most Go SDL bindings handle this by having the function return three values, *Window, *Renderer and, error. This causes the function to get classified as a constructor for Window, but this means that if I type go doc sdl.renderer, it doesn't show up, even though it's a standard function for creating a new Renderer.

@willfaught
Copy link
Contributor Author

willfaught commented Nov 28, 2017

The primary purpose of time.Tick() is not to get new time.Time values; it's to have values sent to a channel at regular intervals. The fact that they're time.Times is kind of ancillary.

This assumes one only cares about the side effect of channel receiving, which isn’t always the case, and I would argue most of the time isn’t; most of the time, we use the received value. I don’t have the reference in front of me, but aren’t the Tick elements the current times? If so, those can be useful sometimes, and they’re newly constructed.


I suspect there’s a hang-up about long-existing functions now being considered constructors because there’s a bias: for so long they weren’t, so we don’t think of them that way, and any change that changes that is therefore wrong. I suspect if they had been designated as constructors from the start, we would have been fine with that. So perhaps it’s more productive to discuss a hypothetical example, like my NewEventStream constructor above. Does anyone have a counterargument for why it isn’t an Event a constructor?

@DeedleFake
Copy link

Because it isn't for constructing Events. It's for constructing a stream of Events. It's a finicky difference that's pretty opinion-based, but it is a difference.

I could also play devil's advocate and argue that it's again not really for 'constructing' Events. The Events are being constructed elsewhere, and then sent to you later. That's different from returning a slice of values, since those values are created right away and then returned.

@willfaught
Copy link
Contributor Author

willfaught commented Nov 29, 2017

(Edited)

I forgot to respond to a part of @DeedleFake's second-to-last post:

The case that I sometimes have problems with is that of a function that constructs multiple types at the same time. For example, SDL has a function, SDL_CreateWindowAndRenderer, that creates an SDL_Window and an SDL_Renderer at the same time. In C, this is done by passing two double pointers, so the function that creates them takes a **SDL_Window and an **SDL_Renderer as two of its arguments. Most Go SDL bindings handle this by having the function return three values, *Window, *Renderer and, error. This causes the function to get classified as a constructor for Window, but this means that if I type go doc sdl.renderer, it doesn't show up, even though it's a standard function for creating a new Renderer.

One way to simplify the definition of "constructor" is that it only handles one named type (plus an optional error, or whatever). I wrote above, in part:

Regarding maps, T or *T or types parameterized only by T or *T that you can get out doesn't match maps, because there are two type parameters, so it's unclear which was constructed, and where the other came from. The same is true of functions.

So it could be that SDL_CreateWindowAndRenderer isn't a constructor at all.


Because it isn't for constructing Events. It's for constructing a stream of Events. [...] The Events are being constructed elsewhere, and then sent to you later. That's different from returning a slice of values, since those values are created right away and then returned.

Some counterexamples:

// The traditional slice constructor, for comparison.
func Registrations_slice_traditional() []Event {
    var es = make([]Event, 10)

    for i := 0; i < 10; i++ {
        es[i] = getEvent()
    }

    return es
}

// Disproves: "[Slice] values are created right away and then returned."
func Registrations_slice_array() []Event {
    var es [10]Event

    go func() {
        for i := 0; i < 10; i++ {
            es[i] = getEvent() // Constructs events in itself after (probably) the return.
        }
    }()

    return es[:]
}

// Disproves: "Because it isn't for constructing Events. [...] The Events are being constructed elsewhere, and then sent to you later."
func Registrations_chan_buffered() chan Event {
    var es = make(chan Event, 10)

    for i := 0; i < 10; i++ {
        es <- getEvent() // Constructs events in itself before the return, not elsewhere.
    }

    return es
}

// Disproves: "Because it isn't for constructing Events. [...] The Events are being constructed elsewhere, and then sent to you later."
func Registrations_chan_unbuffered() chan Event {
    var es = make(chan Event)

    go func() {
        for i := 0; i < 10; i++ {
            es <- getEvent() // Constructs events in itself after the return, not elsewhere.
        }
    }()

    return es
}

The source of the Events is up to the constructor. It's an implementation detail that doesn't affect the meaning of the function type, which is the only thing we're looking at to determine what is a constructor. As seen in the Registrations_slice_array example above, even slice constructor results can come from elsewhere (as can pointer constructor results, for that matter), so by definition that property isn't necessary for constructors.


Perhaps the functions listed for a type should just simply be any functions that handle only that type, not just functions we think of as "constructors". NewEventStream is definitely only about Event, and, as my Event go doc example above shows, it's simply more useful to list NewEventStream under/near Event, because it deals solely with that named type, and when learning about Event, you want to know about NewEventStream. In this sense, chan<- functions would be included.

@gopherbot
Copy link

Change https://golang.org/cl/85355 mentions this issue: go/doc: classify function returning slice or array of T as constructor

@tomwans
Copy link
Contributor

tomwans commented Dec 22, 2017

Hey all, I was hunting around for little contributions I could make and came across this ticket. Funny enough, I wrote the original slices-of-T change (https://go-review.googlesource.com/c/go/+/54971/6). Since @rsc has accepted to add arrays of type T as constructors, I was able to quickly create the CL for it: https://go-review.googlesource.com/c/go/+/85355

I wasn't sure what to do with the test - since the old test was invalid, I renamed the test after this issue and removed the old test.

@golang golang locked and limited conversation to collaborators Jun 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants