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 x, y..., z in list for variadic function argument #18605

Open
blixt opened this issue Jan 11, 2017 · 17 comments

Comments

@blixt
Copy link
Contributor

commented Jan 11, 2017

(See the bottom of this post for a full example.) This is something I've run into a few times and I've been unable to find a discussion fully explaining why it can't/shouldn't be supported.

I've tried searching for prior discussion about this and have mostly come up with Stack Overflow questions where people have been confused about the current restriction. The best reasoning I've found is in #2873:

To avoid confusion about what slice is being passed, the ... syntax only applies when it is providing the entire ... argument; you can't splice in additional parameters with it.

I disagree with this. It implies the developer is already expected to understand that 1) ...string denotes a slice and 2) there would be two slices (i.e., your variadic arguments were turned into a slice implicitly).

They could also be expected to understand that the second slice (paths in my example below) is appended to the first slice if you were to mix the two approaches. I would even argue that is the expected behavior.

Ultimately, if Go was to support this proposal there are (AFAICT) still only two possible outcomes:

  1. The slice followed by ... is passed on as-is (0 positional arguments)
  2. A new slice is implicitly created (>0 positional arguments)

This proposal extends the second case to also implicitly append the slice followed by ... to the implicit slice (approximately what I've done as a workaround in the example below).

One new behavior resulting from this is that modifying some values in the resulting slice will have no outside effect, while modifying the indexes at the end that correspond to the passed-in slice may modify it. I don't think this is very different from today however, as there is currently no way to know if modifying any index in the argument slice will have any outside effect or not, so touching it should be strongly discouraged regardless. For such cases, an explicit slice should always be passed instead.

Full example

package main

import (
    "bytes"
    "fmt"
    "os/exec"
)

func main() {
    list("/usr/share", "/var/tmp")
}

func list(paths ...string) {
    // Desired:
    //cmd := exec.Command("ls", "-la", paths...)
    // Current workaround:
    cmd := exec.Command("ls", append([]string{"-la"}, paths...)...)
    var out bytes.Buffer
    cmd.Stdout = &out
    err := cmd.Run()
    if err != nil {
        fmt.Printf("error: %v\n", err)
    }
    fmt.Println(out.String())
}

@dsnet dsnet added this to the Proposal milestone Jan 11, 2017

@dsnet dsnet added the Proposal label Jan 11, 2017

@rsc rsc changed the title proposal: allow variadic functions to implicitly append a slice to the argument slice proposal: spec: allow mixing args and ...args in list for variadic function argument Jan 23, 2017

@rsc rsc changed the title proposal: spec: allow mixing args and ...args in list for variadic function argument proposal: spec: allow x, y..., z in list for variadic function argument Jan 23, 2017

@rsc rsc added the LanguageChange label Jan 23, 2017

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2017

Would probably also want to handle args to append:

x = append(x, "x", foo..., "z")

And so needs to be considered together with #15209.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2017

Postponing to Go 2.

@dpinela

This comment has been minimized.

Copy link
Contributor

commented Jan 24, 2018

I've published an experience report about the issues with Go's current behaviour regarding this: https://gist.github.com/dpinela/f7fdeb0ce3e1f0b4f495917ad9210a85

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented May 29, 2018

As mentioned above, given func F(a ...int), right now F(v...) passes v directly, such that if F changes elements of its parameter a those changes will be seen through v. If we accept this proposal, then in calls like F(1, v...) F would get a copy of the slice allocated at the call site, and changes would not be visible to the caller. To avoid subtle problems, we would want to change F(v...) to also copy the slice v. It seems that any other choice would be too confusing. So an important point about this proposal is that cases that are currently efficient would become less efficient (in some cases the compiler might be able to optimize this). And, in some very unusual cases, code that works with Go 1 would fail after this proposal were adopted.

@bcmills

This comment has been minimized.

Copy link
Member

commented May 29, 2018

To avoid subtle problems, we would want to change F(v...) to also copy the slice v.
[…]
[I]n some very unusual cases, code that works with Go 1 would fail after this proposal were adopted.

I suspect that the change in semantics would fix more call sites than it breaks: functions involving variadic arguments for anything other than the append pattern are already prone to subtle bugs.

For example, I've seen a fair amount of code using variadic arguments to add a set of default options to a function that accepts variadic options:

package wrapper

func Dial(target string, opts ...grpc.DialOption) (*grpc.ClientConn, error) {
	return grpc.Dial(target, append(opts, grpc.WithFoo(foo))...)
}

Those wrapper functions are subtly wrong: a caller may reasonably expect to be able to pass a subslice of options, but that pattern introduces a surprising mutation (a form of aliasing bug):

package caller

[…]
	opts := []grpc.DialOption{ …, grpc.WithBar(bar) }

	// Dial c1 with all options except for baz.
	c1, err1 := wrapper.Dial(wrapperTarget, opts[:len(opts-1)]...)

	// Dial c2 with all options‽
	c2, err2 := wrapper.Dial(wrapperTarget, opts...)
[…]

In this snippet, the second Dial unintentionally replaced the WithBar option from package caller with the WithFoo option from the first wrapper.Dial. The bug is hard to catch: the conflicting write happens in a different package (so it isn't visible locally), the unintended substitution may not change the behavior enough to cause tests to fail, and because the two calls occur sequentially they won't trip the race detector.

@blixt

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2018

I think @bcmills put it eloquently, the current optimization of reusing the slice is ripe for subtle bugs, and optimizations should only be done in cases where the compiler can prove that the programmer isn't shooting themselves in the foot.

Also, to quote the original post,

…there is currently no way to know if modifying any index in the argument slice will have any outside effect or not, so touching it should be strongly discouraged regardless. For such cases, an explicit slice should always be passed instead.

Because the implementer of the function (e.g., Dial in @bcmills' example above) can't know whether the call site used arguments or a slice, the responsibility is on them to know about these dangers and write defensive code (always assume it's a slice that must not be touched), or not use variadic arguments.

The only alternative would be to support read only slices, which I doubt is going to be a thing any time soon(?).

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2018

It's worth considering that if we change variadic arguments to always copy the slice, then a printf wrapper, like, say, log.Printf, will introduce extra copying. The definition of log.Printf is

func Printf(format string, v ...interface{}) {
	std.Output(2, fmt.Sprintf(format, v...))
}

so a call to this function passing (msg, s...) will make a new copy of the slice s, and then the call to fmt.Sprintf will make a second copy of the slice. While the compiler may be able to use some form of escape analysis to eliminate the copies, clearly this approach is less efficient.

@champioj

This comment has been minimized.

Copy link

commented Jun 19, 2018

Another point, if we change variadic arguments to always copy the slice, I think it could be possible to make it covariant as the linear cost would be the same.
It's not intuitive that passing an array to Printf dosen't work:

 var sl = []int{1, 2, 3}
 fmt.Println(sl[0], sl[1], sl[2])
 fmt.Println(sl...) // error, but could behave the same

For that simple case, it's a win. Not sure about advanced use case though.
If variadic get changed, I think it's worth considering.

@FlorianUekermann

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2018

@ianlancetaylor said:

printf wrapper, like, say, log.Printf, will introduce extra copying.

As @blixt suggested, the compiler should probably optimize cases where the slice doesn't escape and is not modified, which fits this example.

But, I want to float a very different idea: Deprecate variadic functions, make slice construction easier.

Given that ... in function arguments are mainly syntactic sugar for constructing slices, maybe more powerful type inference would make it unnecessary. I'm thinking:

func F(a []int) {}
F({1, 2, 3})

On the other hand we probably want to allow construction of slices using ... then, to get something resembling the more flexible calling convention discussed in this issue:

a := []int{2, 3}
b := []int{1, a...}
F({b..., 4}) // Equivalent to F({1, 2, 3, 4})

This approach would solve a number of issues around "variadic" arguments:

  • Can be used anywhere in the argument list
  • Decision to use variadic notation vs slice is made by caller
  • More obvious copy VS reference semantics
  • No backwards compatibility issues

I think this would actually simplify the language, while making it more expressive and flexible in many cases.

Edit:
I forgot to give an example how this would actually solve the problem discussed in this particular issue in a backwards compatible way:

a := []interface{}{2, 3}
fmt.Println({1, a...}...)

(I guess that means deprecation of variadic functions is a related but separate discussion).

@bcmills

This comment has been minimized.

Copy link
Member

commented Aug 2, 2018

make slice construction easier

The syntax you describe here seems like a nice generalization of #12854. (It's #12854 plus the ability to use ... to expand out slices within a slice literal.)

@tdewolff

This comment has been minimized.

Copy link

commented Aug 16, 2018

@ianlancetaylor wrote:

It's worth considering that if we change variadic arguments to always copy the slice, then a printf wrapper, like, say, log.Printf, will introduce extra copying.

What about v having set its capacity to its length, cap(v) == len(v)? No copy is needed, unless appended to as per @bcmills example.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2019

Above in #18605 (comment) I mentioned that adopting this change would strongly suggest that we should change F(v...) to create a new slice rather than simply passing the existing slice. I think that not making that change would make this semantics of this proposal rather complex. But if we change how F(v...) works, then the exact same code would compile without errors in two different versions of the language, but would behave differently. With my current understanding of how language transitions should work (https://github.com/golang/proposal/blob/master/design/28221-go2-transitions.md) I think that would be a bad idea. And that makes this proposal difficult to adopt.

@blixt

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2019

Isn't the current passing of the slice at best an optimization, and as shown above, a potential source of bugs? And due to the unknowable condition of the incoming slice, isn't this a safe change to make? Any code that expected only a copy, or only an existing slice, was already buggy.

And every version of Go with bug fixes does allow the exact same code to compile without errors, but behaves differently.

The spec says:

If the final argument is assignable to a slice type []T, it may be passed unchanged as the value for a ...T parameter if the argument is followed by .... In this case no new slice is created.

It generally implies that when possible the slice "may be passed unchanged" but it seems like there is no strong assertion that the passed on slice must not or cannot be a copy.

By the way, this issue wasn't originally tagged Go2. In its original form it is possible to implement for Go 1.x.

@josharian

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

@ianlancetaylor could we do this only for append? Then at least the construction of the slice to pass to the variadic function would be simpler. And append is already special in all sorts of ways.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

@blixt The language in the spec is a bit confusing. The word "may" there means "you may pass the slice unchanged by following it with ...". The behavior if the final argument is followed by ... is specified: the argument must be a slice type that is assignable to []T, and that slice is passed unchanged to the function. So any code that expected a copy was buggy. And code that expected only an existing slice was correct.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

By the way, this issue wasn't originally tagged Go2. In its original form it is possible to implement for Go 1.x.

All language changes are now considered to be Go 2 issues, whether they are Go 1 compatible or not.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

@josharian Doing this just for append is #4096.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.