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: Go 2: language: add variadic maps #26459

Closed
metakeule opened this issue Jul 19, 2018 · 31 comments
Closed

proposal: Go 2: language: add variadic maps #26459

metakeule opened this issue Jul 19, 2018 · 31 comments
Labels
FrozenDueToAge LanguageChange Proposal v2 A language change or incompatible library change
Milestone

Comments

@metakeule
Copy link

Proposal: Add variadic maps to Go2

Problem

To get a nice API, it is common practice to misuse variadic arguments like this:

// keyvals is a variadic sequence of alternating keys and values
func Broken(keyvals ...interface{}) error { 
    if len(keyvals) % 2 != nil {
       // return some error
    }
    m := make(map[string]interface{}, len(keyvals) / 2)
    for i := 0; i < len(keyvals); i += 2 {
        k, v := keyvals[i], keyvals[i+1]
        m[fmt.Sprint(k)] = v
    }
    
    // act upon m
}

when the proper argument would have been:

func Proper(m map[string]interface{}) error { 
    // act upon m
}

The reason for this common hack is the comfort when using

Broken("age", 12, "location", "NYC")

compared to

Proper(map[string]interface{}{"age": 12, "location": "NYC"})

For the discussion the types of keyvals and the keys and values of m don't really matter - they
just reflect typical examples.

What matters however is that the type signature of Broken does not reflect the real type that is needed and that results in code needed to manually check (or panic) what the compiler does ensure naturally in Proper.

Solution

Since there seems to be a real need to have a more pleasant way to pass map keys and values to a function, it is proposed to introduce variadic maps to Go2.

This is what the example would look like, when rewritten with a variadic map:

func Fixed(m :::map[string]interface{}) error { 
    // act upon m
}

Fixed("age": 12, "location": "NYC")

Benefits:

  • type signature shows the resulting type like Proper
  • type safety like Proper
  • invocation is nice and clean and easily associated with a map

variadic slice vs variadic map

For the sake of clarity, the variadic argument of Go1 is called
variadic slice here, since they result in a slice while the variadic map
results in a map.

Similarities:

  • A variadic can only be last argument of a function (this excludes the usage of a variadic slice
    and a variadic map within the same function)
  • A function may have non-variadic arguments before the variadic.

Differences:

variadic slice variadic map
collection values end up in a slice keys and values end up in a map
type in function signature type of the value prefixed by ... type of the resulting map prefixed by :::
function call arguments that will be collected by a variadic slice are seperated with a , arguments that will be collected by a variadic map are key-value pairs, seperated with a ,, where each pair is the key, followed by a : and followed by the value.
expansion a slice may be passed to a function as a variadic slice by postfixing it with ... a map may be passed to a function as a variadic map by postfixing it with :::

complete example:

package main

import "fmt"

func Log(prefix string, vals :::map[string]interface{}) {
	fmt.Print(prefix+" ")
	for k,v := range vals {
	  fmt.Printf("%v: %v, ", k,v)
	}
}

func main(){
	Log("DEBUG", "age": 12, "location": "NYC")
	m := map[string]interface{}{
	  "age": 12, 
	  "location": "NYC",
	}

	Log("DEBUG", m:::)
}
@gopherbot gopherbot added this to the Proposal milestone Jul 19, 2018
@Azareal
Copy link

Azareal commented Jul 19, 2018

Proper(map[string]interface{}{"age": 12, "location": "NYC"})

I'm fairly sure you can do:

type blah map[string]interface{}
Proper(blah{"age": 12, "location": "NYC"})

But maybe I'm mistaken?

@mvdan
Copy link
Member

mvdan commented Jul 19, 2018

Note that you can always implement this at run-time with variadic arguments, assigning the key-value pairs in the map. I'm personally not convinced that the added complexity to the language is worth avoiding a tiny amount of code in rare occasions.

@Merovius
Copy link
Contributor

To get a nice API, it is common practice to misuse variadic arguments like this:

How common is that practice? I can't remember to ever have seen this.

Also, how many of those usages are to emulate keyword-arguments and/or use strings as keys? I assume that this would likely (mostly) be abused for that purpose. Leading to a proposal to get real keyword-arguments, because the compiler can't check that keyword args you pass are defined.

@metakeule
Copy link
Author

@Merovius

IMHO real keyword arguments are not needed, since we have structs. This is just for maps with any key types allowed that a regular map allows.

One example for this practice is in here: https://docs.google.com/document/d/1shW9DZJXOeGbG9Mr9Us9MiaPqmlcVatD_D8lrOXRNMU/edit

But I saw it lots of places. Maybe we could write a parser to find these places.

@jservice-rvbd
Copy link

jservice-rvbd commented Jul 19, 2018

@Merovius

It is used in the standard library for strings.NewReplacer.

@metakeule
Copy link
Author

metakeule commented Jul 19, 2018

@mvdan

As I have written, the runtime implementation is not type safe and the call is prone to errors.

If you check for these errors, the caller also has the burden of dealing with them (i.e. functions that otherwise would not need to return and error will need to return one).

Also the proposal makes it easy for the caller and reader to see, where the variadic mapped arguments starts and to see which are keys and which values, compared to the common solution.

@mvdan
Copy link
Member

mvdan commented Jul 19, 2018

the runtime implementation is not type safe

In simple cases, like strings.NewReplacer, it can still be typesafe. And if type safety is more important than brevity, you can always do something like what @Azareal suggested.

functions that otherwise would not need to return and error will need to return one

I don't think this applies. Panicking when a function receives a wrong list of arguments is perfectly normal, as you can see from strings.NewReplacer.

Also the proposal makes it easy for the caller and reader so see, when the variadic mapped arguments starts and to see which are keys and which values, compared to the common solution.

This again reminds me of @Azareal's suggestion above. I think this issue boils down to you wanting to use maps, but not wanting to have to write the map type each time you use it in a call.

I think #12854 is precisely what you want. Then, you could write either of:

ExplicitType(arg1, arg2, map[string]interface{}{"age": 12, "location": "NYC"})
ImplicitType(arg1, arg2, {"age": 12, "location": "NYC"})

@metakeule
Copy link
Author

@mvdan

Panicking when a function receives a wrong list of arguments is perfectly normal, as you can see
from strings.NewReplacer.

Are you kidding?

I think #12854 is precisely what you want.

No, definitely not. #12854 blurs the line between structs and maps from the caller and readers perspective. Also it is more verbose for the caller.

@mvdan
Copy link
Member

mvdan commented Jul 19, 2018

Are you kidding?

I am not, and I'd recommend replying with arguments if you want your proposal to have a chance at being accepted.

#12854 blurs the line between structs and maps from the caller and readers perspective. Also it is more verbose for the caller.

I personally don't think there's any blurring, as all the expressions would still have a specific type. If anything, your proposal has exactly the same issue, as the caller doesn't explicitly state the map type.

I also don't agree with the verbosity point. That proposal would simply add opening and closing braces to your calls, and I doubt that two characters are worth this much effort.

@metakeule
Copy link
Author

@mvdan

Panicking when a function receives a wrong list of arguments is perfectly normal, as you can see
from strings.NewReplacer.

Are you kidding?

I am not, and I'd recommend replying with arguments if you want your proposal to have a chance at being accepted.

Ok, you asked for it. First: refering to the current implementation of strings.NewReplacer to say it is "perfectly normal" to panic "when a function receives a wrong list of arguments" is no argument either.

So do you want to say, that it is better to risk a runtime panic instead of preventing it via the compiler?

A common case for such maps is logging or replacing, so places where a missing key or value would not be critical, at least not so much, that you want to bring the system into an undefined state. Because, if you needed to make sure a key is there, you would have used a struct to begin with.

So maybe you don't want the hospital app to crash when printing or logging, for example.
Also: to keep track of every function in the standard lib whether it might panic or not is not feasible.

Panics should just be done, if you are in an undefined state anyway. Other places in the standard lib are signs that the type system currently lacks and must be handled with care.

@metakeule
Copy link
Author

metakeule commented Jul 19, 2018

@mvdan

I personally don't think there's any blurring, as all the expressions would still have a specific type. If anything, your proposal has exactly the same issue, as the caller doesn't explicitly state the map type.

#12854 does blur for the reader, wether it is a struct or a map that is created in the background. My proposal and syntax is very clear that it is creating a map, since it is only about maps anyway. The map type is only needed, when the call is implemented, so there would be no difference to #12854.

But when reading loads of code after years, you might not want to lookup every function signature just to know, if a map or a struct was used (they perform very differently).

@metakeule
Copy link
Author

@mvdan

https://dave.cheney.net/2012/01/18/why-go-gets-exceptions-right

Dave Cheney:

panics are always fatal to your program. In panicing you never assume that your caller can solve
the problem. Hence panic is only used in exceptional circumstances, ones where it is not possible
for your code, or anyone integrating your code to continue.

@Merovius
Copy link
Contributor

So do you want to say, that it is better to risk a runtime panic instead of preventing it via the compiler?

This isn't a black-and-white issue. It is neither always better to have runtime panics (otherwise Go would be Python), nor to never have runtime panics (otherwise Go would be Agda). Instead, Go takes a measured approach of tradeoffs, weighing the costs of more powerful type checking against the cost of bugs prevented. Sometimes, that tradeoff makes panicing a perfectly acceptable solution and it's used all over the stdlib and Go code in general.

This is why it's important to actually talk about how large the risks involved are (i.e. how often is this pattern leading to bugs, crashes or toil?) and how commonly this pattern is used. There has to be quantitative discussion, not just a qualitative one.

Another way to approach this, BTW, if the pattern is sufficiently rarely used (which I suspect) is to have a specific tool to check for this, which can be used by packages who use the pattern. It's not ideal, but it'd get you static checking while also not having to pay the cost of adding this to the language.

A common case for such maps is logging or replacing, so places where a missing key or value would not be critical, at least not so much, that you want to bring the system into an undefined state.

No one is forcing you to panic or return an error, you can simply drop the call, if it's that uncritical.

So maybe you don't want the hospital app to crash when printing or logging, for example.

I wouldn't recommend Go for life-critical applications (and I highly doubt it would be approved for such uses) for exactly this reason.

@mvdan mvdan added the v2 A language change or incompatible library change label Jul 19, 2018
@lthibault
Copy link

As others have noted, I really don't see the advantage over a config pattern. Configs have the (huge, IMHO) benefit of being type-safe.

Example:

type Cfg struct {
    Path string
    Timeout time.Duration
    Weight int
}

func NewThing(cfg Cfg) *Thing {
    // ...
}

t := NewThing(Cfg{ Path: "/foo/bar", Timeout: time.Second * 30, Weight: 10})

As far as I'm concerned, the issue at hand is really: does Go want to support real (i.e.: python-like) keyword arguments or not?

  • Arguments in favor of this boil down to programmer convenience and type-safty
  • Arguments against boil down to simplicity

Certainly, half-baked solutions are significantly worse than the config pattern.

@thejerf
Copy link

thejerf commented Jul 19, 2018

One of the things I've not understood is the logic behind when you are allowed to elide types or not in Go. It seems like there are many places where it is unambiguous what the type is, but you are not allowed to elide it. It seems like one solution to having to type

Proper(map[string]interface{}{"age": 12, "location": "NYC"})

would be to permit

Proper({"age": 12, "location": "NYC"})

Is there some reason this is not permitted, and/or some reason it is somehow completely undesirable? (Above and beyond just "that's not how it's currently done".) I will vouch for the fact that while this is not my largest problem with the language and it certainly doesn't keep me from using it for many things, this does have a real impact on my APIs, and I have also really had the temptation to use interface{} to do a key/value function without having to type out a full imported type name (often on a line that already has an imported function name).

@ianlancetaylor ianlancetaylor changed the title Proposal: Add variadic maps to Go2 proposal: Go 2: language: add variadic maps Jul 19, 2018
@ianlancetaylor
Copy link
Contributor

struct Arg {
    key string
    val interface{}
}

func F(args Arg...) {
    m := make(map[string]interface{})
    for _, arg := range args {
        m[arg.key] = arg.val
    }
}

var V = F(Arg{"Age", 12}, Arg{"Location", "NYC"})

Might be interesting especially in conjunction with #12854.

@DaKine23
Copy link

Your example itself looks like a design issue. map[string]interface{} tells me there is to much JS thinking in your go.

@XANi
Copy link

XANi commented Jul 20, 2018

Just having ability to have named function arguments would be far superior than "map[string]interface{} but prettier".

There are many cases where having optional would make sense but very little where you want to have them and do not want to know type of them

Like

    func f(Name string, ?Location int, ?Age int,) {}
    ....
    f("foo",Age: 12,Location: "NYC")
    f(Name: "bar", Age: 34)
    f("foobar',"NYC",22)

of course, then the code of function would have to check if value of variable is not the default one, but that is far less annoying than having to match types of every possible argument. Basically a config pattern, but with few annoying steps in-between steps removed

@FMNSSun
Copy link

FMNSSun commented Jul 20, 2018

What matters however is that the type signature of Broken does not reflect the real type that is needed and that results in code needed to manually check (or panic) what the compiler does ensure naturally in Proper.
So maybe you don't want the hospital app to crash when printing or logging, for example.
Also: to keep track of every function in the standard lib whether it might panic or not is not feasible.

but when you're dealing with interface{} you already loose a lot of guarantees. You'd essentially have to do this:

func f(args :::map[string]interface{}) {
  // age is int
  argAge, ok := args["age"]

  if !ok {
    // skip. optional
  }

  age, ok := argAge.(int)

  if !ok {
    // panic?
  }
}

Except that then the type assertion will panic because it's nil so you'd probably have to do a whole cascade of control flow structures just to ensure you have all the right arguments with the right types.
If you don't want to return errors you'd have to panic if age isn't of type int. map[string]interface{} doesn't reflect the real types that are needed either. map [string]int would, but map[T]interface{} wouldn't tell you what types to use and you'd have to check them at runtime anyway and either panic or return an error. Don't get me wrong, I'd say having :::map[T]K would be cool in some cases but I don't see too much of a benefit in terms of safety when using map[string]interface{} except that it at least tells you what type your key has to be, but nothing beyond that. Also, depending on the use case you'd have to document what options are available and what types they have:

// Options:
//  age - int (optional)
//  name - string
//  bar - foo
func f(args :::map[string]interface{}) ...

The only scenario where I can see this being useful is if in fact you don't actually have to deal with the types of arguments as you're just passing them on to some other library or when you're working with arbitrary JSON... things like that. Maybe one case is whether you want to check whether an option was set or not which you can't do with

type Options struct {
  Foo int
}

because then you can't distinguish between a set Foo and a zero Foo. The usual solution to this is to have a GetDefaultOptions() *Options function that returns a struct populated with default values you can then change.

The only way I see this making a lot of sense is if you're dealing with lots of dynamic stuff where you have a dynamic amount of arguments you don't even know the names of at compile time but then you get pretty much no compile time guarantees anyway? In every other case I personally think the only way to do it properly is to use a struct that has well defined types because interface{} requires you to do runtime type checking manually sooner or later anyway.

As others have noted, I really don't see the advantage over a config pattern. Configs have the (huge, IMHO) benefit of being type-safe.

I agree with that. If there are a static number of arguments with static types then I'm strongly in favor of using structs rather than map[string]interface{}.

Personally, I'd even prefer the caller to pass in a map and use an utility function to create that map such as:

type NamedArgs map[string]interface{}

func Args(args... interface{}) (NamedArgs, error) {
	n := len(args)

	if n % 2 != 0 {
		return nil, fmt.Errorf("Length of args must be multiple of two. Length of args was %d", n)
	}
	
	m := make(map[string]interface{})
	
	
	argNo := 1
	
	for i := 0; i < n; i += 2 {
		k := args[i]
		val := args[i+1]
		
		if k == nil {
			return nil, fmt.Errorf("Unexpected nil for argument %d", argNo)
		}
		
		key, ok := k.(string)
		
		if !ok {
			return nil, fmt.Errorf("Name for argument %d is not string", argNo)
		}
		
		m[key] = val
		
		argNo++
	}
	
	return m, nil
}

func f(args NamedArgs) {
	fmt.Println(args["age"])
}

func main() {
	args, err := Args("age", 9, "name", "Hans")
	
	if err != nil {
		// handle it
	}
	
	f(args)
}

because then there's less code duplication and the callee doesn't need to deal with all of the errors. FWIW you could also write a very generic Args function that takes in a spec of what options are available and what types they have and whether they are mandatory or not. I think one could easily stitch together an API for APIs that want to use this pattern in a way that it is reusable and generic enough to avoid everybody doing it from scratch.

@thejerf
Copy link

thejerf commented Jul 20, 2018

Let's not get too hung up on the use of interface{} in the example. The general points apply just fine to

X(map[mypackage.ValidatedKey]mypackage.ValidatedValue{"X": "Y"})

Current Go lets us elide the types in the map itself; the question is, can we do anything about the fully-known before that? And/or do we want to?

@metakeule
Copy link
Author

metakeule commented Jul 20, 2018

Ok, to repeat the obvious: This is not about structs nor named parameters. The use case is a map.

There is a reason for Go to have maps. You make it look like there is no need for maps and we could use structs everywhere instead. Also there is a reason for maps to be typed and of no fix length. That fits perfectly with this proposal. You are reading something into this proposal that is not there.

Why is it a better fit to make map out of a variadic slice instead of having a variadic map?

The reason, I am against type elision is that the reader can't see, if Blah in the following example is receiving a struct or a map without looking at a) the signatur of Blah or b) if hi is a variable or constant defined somewhere.

Blah({hi: "ho"})

The reason, I am against

type config map[string]int
Blubb(config{"one": 1})

Is the need to introduce (and often need to export) types for each of such cases as well as comfort.
The same argument could be made against the existing variadic slices:

type o []string
Blubb(o{"a","b"})

So no need for variadics at all - so why does Go have them? (If we'd argue that way.)

@ianlancetaylor
Copy link
Contributor

Sorry, I didn't realize you were that focused on maps. Honestly to me that map case doesn't seem all that interesting.

    Proper(map[string]interface{}{"age": 12, "location": "nyc"})

You are adding a bit of syntactic sugar to something that I can already write, and it's even easier to write if I name the type map[string]interface{}. The syntactic sugar isn't useless, but to me it seems like a relatively minor improvement to a rare use case. Sorry to be so negative.

@metakeule
Copy link
Author

@ianlancetaylor
Sorry, I updated my comment while you were answering. See the update please.

@ianlancetaylor
Copy link
Contributor

Variadic slices seem to me to be a much more common case in practice than variadic maps. The standard library has many examples of variadic slices, starting with fmt.Printf through completely different cases like os/exec.Command. Are there examples of variadic maps in the standard library other than strings.NewReplacer (which is itself a rarely called function)?

@metakeule
Copy link
Author

metakeule commented Jul 21, 2018

@ianlancetaylor
Dunno about the standard lib, but cmd/go/internal/get/vcs.go has one in line 360 and here is some
other places outside the standard lib:

https://godoc.org/github.com/gorilla/mux#Route.Headers
https://godoc.org/github.com/gorilla/mux#Route.HeadersRegexp
https://godoc.org/github.com/gorilla/mux#Route.Queries
https://godoc.org/github.com/gorilla/mux#Route.URL
https://godoc.org/github.com/gorilla/mux#Route.URLHost
https://godoc.org/github.com/gorilla/mux#Route.URLPath

https://godoc.org/github.com/hashicorp/go-hclog#Logger
https://godoc.org/github.com/go-kit/kit/log#Logger

https://godoc.org/github.com/theplant/appkit/kerrs#Wrapv

https://godoc.org/github.com/inconshreveable/log15#Logger

https://godoc.org/gopkg.in/rana/ora.v3#Ses.Ins
https://godoc.org/gopkg.in/rana/ora.v3#Ses.Sel
https://godoc.org/gopkg.in/rana/ora.v3#Ses.Upd

https://godoc.org/github.com/jjeffery/errors#With

https://godoc.org/google.golang.org/grpc/metadata#Pairs

But it is an unfair comparison, since variadic slices already exist and vadiadic maps not. The hack shows only, where they were needed so badly, that the developer chose to use the hack. (The introduction of an avoidable panic should not be taken lightely).

However, if they would exist in the language, they would for sure be used more often than that.

@ChrisHines
Copy link
Contributor

I claim responsibility for the use of the "broken" keyvals ...interface{} style in github.com/go-kit/kit/log. I first encountered the style as a user and contributor to github.com/inconshrevable/log15 and found it quite nice to use. We have found the same to be true in Go kit. The design discussion that led to the use of the style in Go kit took place here: go-kit/kit#21 (comment).

The m map[string]interface{} signature is not in fact "proper" for the Go kit Log method because a map does not preserve the order of the fields. For the logging use case people usually want the order of fields to be consistent from record to record and often want them to match the order defined in the code. It is true that not all implementations of the interface will honor the order, but some do, and collecting the data in a Go map would not allow those implementations.

@langgo
Copy link

langgo commented Jul 21, 2018

It's just a grammar sugar that doesn't solve the underlying problem

@golang golang deleted a comment from langgo Jul 21, 2018
@golang golang deleted a comment from langgo Jul 21, 2018
@lthibault
Copy link

lthibault commented Jul 25, 2018

@metakeule

Ok, to repeat the obvious: This is not about structs nor named parameters. The use case is a map.

With respect, I beg to differ.

Your use-case is a solution to a problem. It's this problem we should be discussing. I don't understand what there is to be gained by restricting the conversation to variadic maps.

I agree with the pain-point that your proposal attempts to address, but for reasons I explained above, I don't think maps are the best solution.

@deanveloper
Copy link

I don't think this is a good solution. The syntax is also unintuitive. (Sure, it comes from the ... operator, but it doesn't look like a ... operator, so it ends up just looking::: bad)

Maps I believe are fine on their own and don't really need an additional syntax just for being passed into an argument. Instead, I'd rather see functions like strings.NewReplacer just take a map rather than use variadic arguments.

The reason variadic slices exist in the first place is because they are used extremely often, so it's good to have a syntax which doesn't require us to type []interface{} { ... } every time we want to print something.

This isn't used often, and as stated earlier, the syntax is confusing. Especially with IDE helpers, people will wonder what the heck a :::map[string]interface{} type is, or see someone pass a map into a function like myMap:::.

I think #12854 is a good way for this to be done though. If it were accepted, we'd just accept a map[string]interface{} (or whatever) on the other side, then the syntax would just be { "key": "val", "key2": 63 }. This also allows us to pass in a map to the function as we normally would, of course.

@ianlancetaylor
Copy link
Contributor

This proposal is syntactic sugar for something we can already do. It's not a sufficiently common case to merit additional syntax.

@david-l-riley
Copy link

I'm late to the party here, but I actually think the "functional options" pattern popular in several Go packages (https://commandcenter.blogspot.com/2014/01/self-referential-functions-and-design.html) solves an awful lot of the issues for this. It's extremely type safe (essentially to the same level as an options struct), allows for variadic argument passing, feels less clunky than trying to handle a zero-or-one length array for variadic options, and doesn't require users to generate structs just to pass in a few options.

You can always make a quick function to marshal the options into a map or a custom struct or whatever you want, and it also lets you do things that are somewhat more sophisticated than a simple options struct. Consider that pattern instead; I think we should evangelize it more.

@golang golang locked and limited conversation to collaborators Dec 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge LanguageChange Proposal v2 A language change or incompatible library change
Projects
None yet
Development

No branches or pull requests