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: spec: allow assignment-compatible values in append and copy #15209

Open
rogpeppe opened this issue Apr 9, 2016 · 23 comments
Open

proposal: spec: allow assignment-compatible values in append and copy #15209

rogpeppe opened this issue Apr 9, 2016 · 23 comments
Labels
LanguageChange NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Proposal v2 A language change or incompatible library change
Milestone

Comments

@rogpeppe
Copy link
Contributor

rogpeppe commented Apr 9, 2016

As discussed here, it might be useful to make the append and copy built-ins somewhat more accepting of different types.

I have made a proposal document here:

https://docs.google.com/document/d/1HilKzERLb521XaG0Lgi6zkkj25VlXqc1oHEUmQ1v3aI

@robpike
Copy link
Contributor

robpike commented Apr 9, 2016

This will require allocating a slice under the covers. []int and []interface{} do not have the same memory and unless something magic happens, the only way to achieve this is to allocate the []interface slice and run the loop. In other words, except for microoptimizations this must do what the current user-written code does, and involves an allocation and an O(len(slice)) copy.

I am not making an argument for or against, just saying something that should appear in the proposal.

@rogpeppe
Copy link
Contributor Author

rogpeppe commented Apr 9, 2016

Perhaps confusingly, I've actually made two proposals here. In the first one, only the copy and append primitives are affected, and no slice will need to be allocated under the covers. In the second ("Alternative proposal") one, yes, a new slice would need to be allocated, which I hope I've made a little clearer now.

@cznic
Copy link
Contributor

cznic commented Apr 9, 2016

The more magic builtins copy/append will learn, the less readable the code using those features gets.

The explicit loop communicates the intent, well, explicitly. And there's room for the compiler to recognize and optimize the hell out of those loops.

@rogpeppe
Copy link
Contributor Author

rogpeppe commented Apr 9, 2016

cznic: At this point I think I'm mostly trending towards the second proposal, because it's actually reasonably intuitive that the "..." modifier could (when necessary) assign each element in turn,
and use of that modifier is a good potential signifier of a "allocation may occur here" place.

As for readability, I don't think it would make for less readable code, but perhaps you're seeing
something I'm not. Can you give an example of where it would make code less readable?

@cznic
Copy link
Contributor

cznic commented Apr 9, 2016

Code requiring awareness of more features packed to an "overloaded" function is less readable in my books. I should have perhaps said comprehensible instead, to be more accurate.

@bits01
Copy link

bits01 commented Apr 9, 2016

I believe the name copy() communicates the intention of what is happening very well, it does not make it look like magic. Plus it already does some magic, at least for string/[]byte copies. But also the type conversions string([]byte]) and []byte(string) do hidden copies.

@bradfitz bradfitz modified the milestone: Unplanned Apr 9, 2016
@champioj
Copy link

I really want to like the alternative proposal, but I think it is important that you must explicitly indicate that your are making an allocation. Like fmt.Println(range xs…) but that syntax feels out of place...any ideas?

Another possibility (is it the right place?) that would fulfill the use case in the document would be to only have make assignment-compatible. That is, allow the seconde parameter (replacing the size) to be a slice* of a type assignable to the slice type of the first parameter.
Keeping the use case in the document, that would give:

func printInts(xs []int) {
    args := make([]interface{}, xs)
    fmt.Println(xs...)
}

func allReader(fs []*os.File) io.Reader {
    rs := make([]io.Reader, fs)
    return io.MultiReader(rs...)
}`

which feels less verbose than the first proposal while still completely explicit.
It is less overloaded that the alternative proposal too.

While it cover the usecase in the proposal, its scope is narrower as you cannot append with it. So I think it's important to define what is really important and what is less.

What I like about using make is that, unlike append, it is quite never used in convoluted code so it doesn't render like the code is less comprehensible.

*and perhaps map/channel, but that's another story.

@earthboundkid
Copy link
Contributor

earthboundkid commented Apr 19, 2016

From the proposal:

With these

Is an incomplete sentence.

And

    args := append([]string, string(ts…))
    strings.Join(args, “ “)

should be

    args := append([]string{}, string(ts...))
    strings.Join(args, " ")

FWIW, I prefer the first proposal to the alternative proposal. This "looks like Go" to me:

    args := make([]interface{}, len(xs))
    copy(args, xs)

In my view, Go generally tries not to allocate a slice without explicit instruction to do so.

@extemporalgenome
Copy link
Contributor

extemporalgenome commented Apr 19, 2016

update: I just saw the part about the alternative proposal -- indeed that would involve a slice allocation in the worst case.

@robpike (not taking a stance on this proposal), but I don't believe that a slice "allocation" would be necessary as you described. There are two cases:

  1. append(dst, src...)
  2. append(dst, elem1, elem2, elem3)

In the first case, the source is already a slice (thus no allocation is needed). append and copy would simply need to be generalized to accept a src whose elements are assignment-compatible to dst, and perform an assignment loop; the current memcpy case would just be an optimization.

In the second case, the compiler can place an array on the stack (in the above example, a [3]T). It could derive a slice header from this stack-resident array, and place that on the stack as well -- in any case, this is a throwaway slice that clearly does not escape, and thus should never end up on the heap. If it's considered an allocation, it's certainly not an expensive one.

@bradfitz
Copy link
Contributor

/cc @griesemer

@adg
Copy link
Contributor

adg commented Sep 26, 2016

ping @griesemer

@griesemer
Copy link
Contributor

Sorry, just haven't had time to look into this yet.

@rsc
Copy link
Contributor

rsc commented Mar 6, 2017

There doesn't seem to be a pressing need for this, so leaving for Go 2.

@earthboundkid
Copy link
Contributor

Normally I interpret "Go 2" to mean language changes that are impossible without breaking backwards compatibility. Has the definition been changed to "non-urgent language changes"? Or did I misunderstand "Go 2"?

@griesemer
Copy link
Contributor

@carlmjohnson What comprises "Go 2" or it's timeframe have not been defined yet. That said, while many agree that this might be a nice expansion on append and copy, there's no urgency. In the interest of keeping unnecessary language churn at a minimum, the decision was to postpone so we can look at this within the larger context of Go 2.

(There has been at least one other proposal in related direction, and we should probably consider all our options before venturing out to a relatively involved change to append.)

@rsc rsc added the v2 A language change or incompatible library change label Mar 7, 2017
@dolmen
Copy link
Contributor

dolmen commented Apr 4, 2017

It seems from the examples given that the need that triggered this proposal is array types conversion. But the syntax proposed is awkward. Go already has a type conversion syntax and it already has magic (that does allocation, copy and conversion) to convert []byte to string and the reverse.
So instead of adding magic to append and copy, a simpler syntax change would just be to just extend the type conversion syntax to have magic for any array type.
Here are the examples of the proposal adapted for this syntax:

func printInts(xs []int) {
    fmt.Println([]interface{}(xs)...)
}

func allReader(fs []*os.File) io.Reader {
    return io.MultiReader([]io.Reader(rs)...)
}

However I'm not endorsing this proposal. Instead, I am against it as it hides a costly conversion (that requires allocation, loop, conversion of each array element) under a very short syntax that people will abuse. Currently code for array conversions is verbose because this is costly at runtime, and I'm fine with that.

@ianlancetaylor ianlancetaylor modified the milestones: Unplanned, Proposal Jan 9, 2018
@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 9, 2018
@akavel
Copy link
Contributor

akavel commented Jan 11, 2018

One thing implicit in the proposal, but not explicitly mentioned in the examples (not sure why), is that with the proposed change to append, the following snippet (copied from the proposal):

    func printInts(xs []int) {
        args := make([]interface{}, len(xs))
        for i, x := range xs {
            args[i] = x
        }
        fmt.Println(args...)
    }

could actually be shortened even more, to an append-based snippet (instead of the copy-based one shown in the proposal):

    func printInts(xs []int) {
        fmt.Println(append([]interface{}{}, xs...))
    }

Also, regarding append doing "extra allocations" and "O(n) complexity", I don't really undrestand the concern: AFAIK, both copy & append are already expected to do an extra allocation (of the new array; possibly amortized in case of append), and have O(n) complexity (both of them are by definition a shorthand for a loop).

Also, for me personally, the above proposed pattern seems short enough, and at the same time implicit enough (the loop and the allocation are all clearly visible by virtue of using append, and the target type is also clearly listed), that I don't see a need for the "alternative proposal".

edit: Also, I see it as further advantage of the proposal that it could become the single recommended solution the 3+ "commonly used" variants of the "covariance conversion code pattern", i.e.:

    // variant a
    args := make([]interface{}, len(xs))
    for i, x := range xs {
        args[i] = x
    }

    // variant b
    args := []interface{}{}
    for _, x := range xs {
        args = append(args, x)
    }

    // variant c
    args := make([]interface{}, 0, len(xs))
    for _, x := range xs {
        args = append(args, x)
    }

removing the need for choice here and attempts at microoptimization by using len(xs) here and there. And as such, could possibly become a recommended response to #7512, maybe even landing in the FAQ.

edit 2: Regarding "implicitness" of the conversion, note that in the above loops (variant b and c), the append already does an implicit conversion of the x from int to interface{}. In my book, keeping the []interface{} phrase closer to the append could make it actually more explicit.

@cznic
Copy link
Contributor

cznic commented Jan 11, 2018

AFAIK, both copy & append are already expected to do an extra allocation ...

copy never allocates.

@akavel
Copy link
Contributor

akavel commented Jan 11, 2018

copy never allocates.

Ouch, right; but the O(n) still stands I believe.

@DeedleFake
Copy link

Is it possible to implement a converting copy via generics, as the draft proposal stands now? I can't think of a way to specify that one generic type is convertible to another:

func Convert[T someConstraint, F otherConstraint](f F) T {
  return T(f) // Is it possible to write a constraint to allow this?
}

If this function was possible to write, a converting copy could be implemented somewhere in the standard library, such as a theoretical slices package, and the existing built-in copy() could just handle the simpler variant where no allocation is necessary. Same thing for append(), minus the allocation concerns.

@go101
Copy link

go101 commented Nov 12, 2020

According to what I know, there is not a simple way to define a constraint to represent "values of F may be converted to T" by the current generic draft.

@ianlancetaylor
Copy link
Contributor

In the current design draft, that is correct. See https://go.googlesource.com/proposal/+/refs/heads/master/design/go2draft-type-parameters.md#no-way-to-express-convertibility .

@bcmills
Copy link
Contributor

bcmills commented Nov 12, 2020

@DeedleFake, I examined append and copy under the generics draft in significant depth a while back. See append*.go2 in https://github.com/bcmills/go2go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LanguageChange NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Proposal v2 A language change or incompatible library change
Projects
None yet
Development

No branches or pull requests