Skip to content

Commit 4b23295

Browse files
committed
fix(map): ensure string-to-string flags maintain stable pointers
This revision tweaks the internals of the StringToString flag type to ensure that pointers are consistent across invocations of FlagSet.Parse(), Flag.Set(), and FlagSet.GetStringToString(). Prior to this change, each call to GetStringToString would allocate and return a new map. Although this provides nice encapsulation, it means that users cannot manipulate the map (in tests, for instance). To address this, GetStringToString now returns the map directly from Flag.Value. Set() now avoids new allocations as well, updating the existing map over allocating new ones. Documentation for all StringToString flags have been reworked for improved clarity. The tests for this flag type have been completely reworked. A small example demonstrating this flag type was also added.
1 parent b85eb9e commit 4b23295

6 files changed

Lines changed: 369 additions & 69 deletions

File tree

example_test.go

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,3 +34,26 @@ func ExampleFlagSet_ShorthandLookup() {
3434

3535
fmt.Println(flag.Name)
3636
}
37+
38+
func ExampleFlagSet_StringToString() {
39+
args := []string{
40+
"--arg", "a=1,b=2",
41+
"--arg", "a=2",
42+
"--arg=d=4",
43+
}
44+
45+
fs := pflag.NewFlagSet("Example", pflag.ContinueOnError)
46+
fs.StringToString("arg", make(map[string]string), "string-to-string arg accepting key=value pairs")
47+
48+
if err := fs.Parse(args); err != nil {
49+
panic(err)
50+
}
51+
52+
value, err := fs.GetStringToString("arg")
53+
if err != nil {
54+
panic(err)
55+
}
56+
57+
fmt.Println(value)
58+
// Output: map[a:2 b:2 d:4]
59+
}

flag.go

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -400,7 +400,12 @@ func (f *FlagSet) lookup(name NormalizedName) *Flag {
400400
return f.formal[name]
401401
}
402402

403-
// func to return a given type for a given flag name
403+
// getFlagType performs a lookup of a flag with the given name and ftype. The flag is stringified and passed through
404+
// convFunc before being returned to enforce flag immutablility.
405+
//
406+
// convFunc may be nil, in which case the raw flag value is returned directly and no immutability is enforced. This is
407+
// particularly useful when users need to access the pointer of the underlying flag value for manipulation (e.g.
408+
// resetting flag values in tests).
404409
func (f *FlagSet) getFlagType(name string, ftype string, convFunc func(sval string) (interface{}, error)) (interface{}, error) {
405410
flag := f.Lookup(name)
406411
if flag == nil {
@@ -413,6 +418,10 @@ func (f *FlagSet) getFlagType(name string, ftype string, convFunc func(sval stri
413418
return nil, err
414419
}
415420

421+
if convFunc == nil {
422+
return flag.Value, nil
423+
}
424+
416425
sval := flag.Value.String()
417426
result, err := convFunc(sval)
418427
if err != nil {

golangflag.go

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -158,4 +158,3 @@ func ParseSkippedFlags(osArgs []string, goFlagSet *goflag.FlagSet) error {
158158
}
159159
return goFlagSet.Parse(skippedFlags)
160160
}
161-

string_slice.go

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -98,9 +98,12 @@ func (f *FlagSet) GetStringSlice(name string) ([]string, error) {
9898
// The argument p points to a []string variable in which to store the value of the flag.
9999
// Compared to StringArray flags, StringSlice flags take comma-separated value as arguments and split them accordingly.
100100
// For example:
101-
// --ss="v1,v2" --ss="v3"
101+
//
102+
// --ss="v1,v2" --ss="v3"
103+
//
102104
// will result in
103-
// []string{"v1", "v2", "v3"}
105+
//
106+
// []string{"v1", "v2", "v3"}
104107
func (f *FlagSet) StringSliceVar(p *[]string, name string, value []string, usage string) {
105108
f.VarP(newStringSliceValue(value, p), name, "", usage)
106109
}
@@ -114,9 +117,12 @@ func (f *FlagSet) StringSliceVarP(p *[]string, name, shorthand string, value []s
114117
// The argument p points to a []string variable in which to store the value of the flag.
115118
// Compared to StringArray flags, StringSlice flags take comma-separated value as arguments and split them accordingly.
116119
// For example:
117-
// --ss="v1,v2" --ss="v3"
120+
//
121+
// --ss="v1,v2" --ss="v3"
122+
//
118123
// will result in
119-
// []string{"v1", "v2", "v3"}
124+
//
125+
// []string{"v1", "v2", "v3"}
120126
func StringSliceVar(p *[]string, name string, value []string, usage string) {
121127
CommandLine.VarP(newStringSliceValue(value, p), name, "", usage)
122128
}
@@ -130,9 +136,12 @@ func StringSliceVarP(p *[]string, name, shorthand string, value []string, usage
130136
// The return value is the address of a []string variable that stores the value of the flag.
131137
// Compared to StringArray flags, StringSlice flags take comma-separated value as arguments and split them accordingly.
132138
// For example:
133-
// --ss="v1,v2" --ss="v3"
139+
//
140+
// --ss="v1,v2" --ss="v3"
141+
//
134142
// will result in
135-
// []string{"v1", "v2", "v3"}
143+
//
144+
// []string{"v1", "v2", "v3"}
136145
func (f *FlagSet) StringSlice(name string, value []string, usage string) *[]string {
137146
p := []string{}
138147
f.StringSliceVarP(&p, name, "", value, usage)
@@ -150,9 +159,12 @@ func (f *FlagSet) StringSliceP(name, shorthand string, value []string, usage str
150159
// The return value is the address of a []string variable that stores the value of the flag.
151160
// Compared to StringArray flags, StringSlice flags take comma-separated value as arguments and split them accordingly.
152161
// For example:
153-
// --ss="v1,v2" --ss="v3"
162+
//
163+
// --ss="v1,v2" --ss="v3"
164+
//
154165
// will result in
155-
// []string{"v1", "v2", "v3"}
166+
//
167+
// []string{"v1", "v2", "v3"}
156168
func StringSlice(name string, value []string, usage string) *[]string {
157169
return CommandLine.StringSliceP(name, "", value, usage)
158170
}

string_to_string.go

Lines changed: 78 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ func newStringToStringValue(val map[string]string, p *map[string]string) *string
2121
return ssv
2222
}
2323

24-
// Format: a=1,b=2
24+
// Set updates the flag value from the given string, adding additional mappings or updating existing ones.
2525
func (s *stringToStringValue) Set(val string) error {
2626
var ss []string
2727
n := strings.Count(val, "=")
@@ -47,13 +47,17 @@ func (s *stringToStringValue) Set(val string) error {
4747
}
4848
out[kv[0]] = kv[1]
4949
}
50+
51+
// clear out any default flag values
5052
if !s.changed {
51-
*s.value = out
52-
} else {
53-
for k, v := range out {
54-
(*s.value)[k] = v
53+
for k := range *s.value {
54+
delete(*s.value, k)
5555
}
5656
}
57+
58+
for k, v := range out {
59+
(*s.value)[k] = v
60+
}
5761
s.changed = true
5862
return nil
5963
}
@@ -84,85 +88,100 @@ func (s *stringToStringValue) String() string {
8488
return "[" + strings.TrimSpace(buf.String()) + "]"
8589
}
8690

87-
func stringToStringConv(val string) (interface{}, error) {
88-
val = strings.Trim(val, "[]")
89-
// An empty string would cause an empty map
90-
if len(val) == 0 {
91-
return map[string]string{}, nil
92-
}
93-
r := csv.NewReader(strings.NewReader(val))
94-
ss, err := r.Read()
95-
if err != nil {
96-
return nil, err
97-
}
98-
out := make(map[string]string, len(ss))
99-
for _, pair := range ss {
100-
kv := strings.SplitN(pair, "=", 2)
101-
if len(kv) != 2 {
102-
return nil, fmt.Errorf("%s must be formatted as key=value", pair)
103-
}
104-
out[kv[0]] = kv[1]
105-
}
106-
return out, nil
107-
}
108-
109-
// GetStringToString return the map[string]string value of a flag with the given name
91+
// GetStringToString return the map value of a flag with the given name from f. The returned map shares memory with the
92+
// internal flag value [Flag.Value].
11093
func (f *FlagSet) GetStringToString(name string) (map[string]string, error) {
111-
val, err := f.getFlagType(name, "stringToString", stringToStringConv)
94+
val, err := f.getFlagType(name, "stringToString", nil)
11295
if err != nil {
11396
return map[string]string{}, err
11497
}
115-
return val.(map[string]string), nil
116-
}
117-
118-
// StringToStringVar defines a string flag with specified name, default value, and usage string.
119-
// The argument p points to a map[string]string variable in which to store the values of the multiple flags.
120-
// The value of each argument will not try to be separated by comma
121-
func (f *FlagSet) StringToStringVar(p *map[string]string, name string, value map[string]string, usage string) {
122-
f.VarP(newStringToStringValue(value, p), name, "", usage)
123-
}
124-
125-
// StringToStringVarP is like StringToStringVar, but accepts a shorthand letter that can be used after a single dash.
126-
func (f *FlagSet) StringToStringVarP(p *map[string]string, name, shorthand string, value map[string]string, usage string) {
127-
f.VarP(newStringToStringValue(value, p), name, shorthand, usage)
128-
}
12998

130-
// StringToStringVar defines a string flag with specified name, default value, and usage string.
131-
// The argument p points to a map[string]string variable in which to store the value of the flag.
132-
// The value of each argument will not try to be separated by comma
133-
func StringToStringVar(p *map[string]string, name string, value map[string]string, usage string) {
134-
CommandLine.VarP(newStringToStringValue(value, p), name, "", usage)
135-
}
99+
fv, ok := val.(*stringToStringValue)
100+
if !ok {
101+
panic(fmt.Errorf("illegal state: unspected internal type for stringToString flag '%s'", name))
102+
}
103+
if fv.value == nil {
104+
return nil, nil
105+
}
136106

137-
// StringToStringVarP is like StringToStringVar, but accepts a shorthand letter that can be used after a single dash.
138-
func StringToStringVarP(p *map[string]string, name, shorthand string, value map[string]string, usage string) {
139-
CommandLine.VarP(newStringToStringValue(value, p), name, shorthand, usage)
107+
return *fv.value, nil
140108
}
141109

142-
// StringToString defines a string flag with specified name, default value, and usage string.
143-
// The return value is the address of a map[string]string variable that stores the value of the flag.
144-
// The value of each argument will not try to be separated by comma
110+
// StringToString defines a map flag with specified name, default value, and usage string.
111+
//
112+
// StringToString flags are used to pass key=value pairs to applications. The same flag can be provided more than once
113+
// with all key=value pairs being merged into a final map. Multiple key=value pairs may be provided in a single arg,
114+
// separated by commas. A few simple examples include:
115+
//
116+
// --arg a=1 -> map[string]string{ "a": "1" }
117+
// --arg a=1 --arg b=2 -> map[string]string{ "a": "1", "b": "2" }
118+
// --arg a=1,b=2 -> map[string]string{ "a": "1", "b": "2" }
119+
// --arg=a=1 -> map[string]string{ "a": "1" }
120+
//
121+
// As a special case, a single key=value pair whose value contains a comma will be interpreted as shown below:
122+
//
123+
// --arg a=1,2 -> map[string]string{ "a": "1,2" }
124+
//
125+
// Returns a pointer to the map which will be updated upon invocation of [FlagSet.Parse], [Flag.Value.Set], and others.
145126
func (f *FlagSet) StringToString(name string, value map[string]string, usage string) *map[string]string {
146127
p := map[string]string{}
147128
f.StringToStringVarP(&p, name, "", value, usage)
148129
return &p
149130
}
150131

151-
// StringToStringP is like StringToString, but accepts a shorthand letter that can be used after a single dash.
132+
// StringToStringP is like [FlagSet.StringToString], but also accepts a shorthand letter that can be used after a single
133+
// dash.
134+
//
135+
// See [FlagSet.StringToString].
152136
func (f *FlagSet) StringToStringP(name, shorthand string, value map[string]string, usage string) *map[string]string {
153137
p := map[string]string{}
154138
f.StringToStringVarP(&p, name, shorthand, value, usage)
155139
return &p
156140
}
157141

142+
// StringToStringVar is like [FlagSet.StringToString], but also accepts a map pointer argument p which is updated with
143+
// the parsed key-value pairs.
144+
//
145+
// See [FlagSet.StringToString].
146+
func (f *FlagSet) StringToStringVar(p *map[string]string, name string, value map[string]string, usage string) {
147+
f.VarP(newStringToStringValue(value, p), name, "", usage)
148+
}
149+
150+
// StringToStringVarP is like [FlagSet.StringToString], but also accepts a map pointer argument p which is updated with
151+
// the parsed key-value pairs, and a shorthand letter that can be used after a single dash.
152+
//
153+
// See [FlagSet.StringToString].
154+
func (f *FlagSet) StringToStringVarP(p *map[string]string, name, shorthand string, value map[string]string, usage string) {
155+
f.VarP(newStringToStringValue(value, p), name, shorthand, usage)
156+
}
157+
158158
// StringToString defines a string flag with specified name, default value, and usage string.
159-
// The return value is the address of a map[string]string variable that stores the value of the flag.
160-
// The value of each argument will not try to be separated by comma
159+
//
160+
// See [FlagSet.StringToString].
161161
func StringToString(name string, value map[string]string, usage string) *map[string]string {
162162
return CommandLine.StringToStringP(name, "", value, usage)
163163
}
164164

165-
// StringToStringP is like StringToString, but accepts a shorthand letter that can be used after a single dash.
165+
// StringToStringP is like [FlagSet.StringToString], but also accepts a shorthand letter that can be used after a single
166+
// dash.
167+
//
168+
// See [FlagSet.StringToString].
166169
func StringToStringP(name, shorthand string, value map[string]string, usage string) *map[string]string {
167170
return CommandLine.StringToStringP(name, shorthand, value, usage)
168171
}
172+
173+
// StringToStringVar is like [FlagSet.StringToString], but also accepts a map pointer argument p which is updated with
174+
// the parsed key-value pairs.
175+
//
176+
// See [FlagSet.StringToString].
177+
func StringToStringVar(p *map[string]string, name string, value map[string]string, usage string) {
178+
CommandLine.VarP(newStringToStringValue(value, p), name, "", usage)
179+
}
180+
181+
// StringToStringVarP is like [FlagSet.StringToString], but also accepts a map pointer argument p which is updated with
182+
// the parsed key-value pairs, and a shorthand letter that can be used after a single dash.
183+
//
184+
// See [FlagSet.StringToString].
185+
func StringToStringVarP(p *map[string]string, name, shorthand string, value map[string]string, usage string) {
186+
CommandLine.VarP(newStringToStringValue(value, p), name, shorthand, usage)
187+
}

0 commit comments

Comments
 (0)