Skip to content

Commit 583419e

Browse files
author
Bryan C. Mills
committed
cmd/go/internal/{test,vet}: use a standard flag.FlagSet to parse flags
This removes much of the complexity of the implementation and use of the cmd/go/internal/cmdflag package, and makes the behavior of GOFLAGS in 'go test' and 'go vet' more consistent with other subcommands. Some of the complexity reduction has been offset by code comments and bug fixes, particularly for the handling of GOPATH arguments and flag terminators ('--'). Fixes #32471 Fixes #18682 Change-Id: I1f6e46a7c679062e1e409e44a2b9f03b9172883b Reviewed-on: https://go-review.googlesource.com/c/go/+/211358 Reviewed-by: Jay Conrod <[email protected]>
1 parent 8e2dad5 commit 583419e

File tree

14 files changed

+881
-454
lines changed

14 files changed

+881
-454
lines changed

doc/go1.15.html

+11
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,17 @@ <h3 id="go-command">Go command</h3>
4747
TODO
4848
</p>
4949

50+
<h4 id="go-flag-parsing">Flag parsing</h4>
51+
52+
<p><!-- https://golang.org/cl/211358 -->
53+
Various flag parsing issues in <code>go</code> <code>test</code> and
54+
<code>go</code> <code>vet</code> have been fixed. Notably, flags specified
55+
in <code>GOFLAGS</code> are handled more consistently, and
56+
the <code>-outputdir</code> flag now interprets relative paths relative to the
57+
working directory of the <code>go</code> command (rather than the working
58+
directory of each individual test).
59+
</p>
60+
5061
<h2 id="runtime">Runtime</h2>
5162

5263
<p>

src/cmd/go/internal/base/goflags.go

+8-4
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,7 @@ type boolFlag interface {
102102
}
103103

104104
// SetFromGOFLAGS sets the flags in the given flag set using settings in $GOFLAGS.
105-
func SetFromGOFLAGS(flags flag.FlagSet) {
105+
func SetFromGOFLAGS(flags *flag.FlagSet) {
106106
InitGOFLAGS()
107107

108108
// This loop is similar to flag.Parse except that it ignores
@@ -125,14 +125,18 @@ func SetFromGOFLAGS(flags flag.FlagSet) {
125125
if f == nil {
126126
continue
127127
}
128+
129+
// Use flags.Set consistently (instead of f.Value.Set) so that a subsequent
130+
// call to flags.Visit will correctly visit the flags that have been set.
131+
128132
if fb, ok := f.Value.(boolFlag); ok && fb.IsBoolFlag() {
129133
if hasValue {
130-
if err := fb.Set(value); err != nil {
134+
if err := flags.Set(f.Name, value); err != nil {
131135
fmt.Fprintf(flags.Output(), "go: invalid boolean value %q for flag %s (from %s): %v\n", value, name, where, err)
132136
flags.Usage()
133137
}
134138
} else {
135-
if err := fb.Set("true"); err != nil {
139+
if err := flags.Set(f.Name, "true"); err != nil {
136140
fmt.Fprintf(flags.Output(), "go: invalid boolean flag %s (from %s): %v\n", name, where, err)
137141
flags.Usage()
138142
}
@@ -142,7 +146,7 @@ func SetFromGOFLAGS(flags flag.FlagSet) {
142146
fmt.Fprintf(flags.Output(), "go: flag needs an argument: %s (from %s)\n", name, where)
143147
flags.Usage()
144148
}
145-
if err := f.Value.Set(value); err != nil {
149+
if err := flags.Set(f.Name, value); err != nil {
146150
fmt.Fprintf(flags.Output(), "go: invalid value %q for flag %s (from %s): %v\n", value, name, where, err)
147151
flags.Usage()
148152
}

src/cmd/go/internal/cmdflag/flag.go

+86-117
Original file line numberDiff line numberDiff line change
@@ -6,155 +6,124 @@
66
package cmdflag
77

88
import (
9+
"errors"
910
"flag"
1011
"fmt"
11-
"os"
12-
"strconv"
1312
"strings"
14-
15-
"cmd/go/internal/base"
1613
)
1714

1815
// The flag handling part of go commands such as test is large and distracting.
1916
// We can't use the standard flag package because some of the flags from
2017
// our command line are for us, and some are for the binary we're running,
2118
// and some are for both.
2219

23-
// Defn defines a flag we know about.
24-
type Defn struct {
25-
Name string // Name on command line.
26-
BoolVar *bool // If it's a boolean flag, this points to it.
27-
Value flag.Value // The flag.Value represented.
28-
PassToTest bool // Pass to the test binary? Used only by go test.
29-
Present bool // Flag has been seen.
30-
}
20+
// ErrFlagTerminator indicates the distinguished token "--", which causes the
21+
// flag package to treat all subsequent arguments as non-flags.
22+
var ErrFlagTerminator = errors.New("flag terminator")
3123

32-
// IsBool reports whether v is a bool flag.
33-
func IsBool(v flag.Value) bool {
34-
vv, ok := v.(interface {
35-
IsBoolFlag() bool
36-
})
37-
if ok {
38-
return vv.IsBoolFlag()
39-
}
40-
return false
24+
// A FlagNotDefinedError indicates a flag-like argument that does not correspond
25+
// to any registered flag in a FlagSet.
26+
type FlagNotDefinedError struct {
27+
RawArg string // the original argument, like --foo or -foo=value
28+
Name string
29+
HasValue bool // is this the -foo=value or --foo=value form?
30+
Value string // only provided if HasValue is true
4131
}
4232

43-
// SetBool sets the addressed boolean to the value.
44-
func SetBool(cmd string, flag *bool, value string) {
45-
x, err := strconv.ParseBool(value)
46-
if err != nil {
47-
SyntaxError(cmd, "illegal bool flag value "+value)
48-
}
49-
*flag = x
33+
func (e FlagNotDefinedError) Error() string {
34+
return fmt.Sprintf("flag provided but not defined: -%s", e.Name)
5035
}
5136

52-
// SetInt sets the addressed integer to the value.
53-
func SetInt(cmd string, flag *int, value string) {
54-
x, err := strconv.Atoi(value)
55-
if err != nil {
56-
SyntaxError(cmd, "illegal int flag value "+value)
57-
}
58-
*flag = x
37+
// A NonFlagError indicates an argument that is not a syntactically-valid flag.
38+
type NonFlagError struct {
39+
RawArg string
5940
}
6041

61-
// SyntaxError reports an argument syntax error and exits the program.
62-
func SyntaxError(cmd, msg string) {
63-
fmt.Fprintf(os.Stderr, "go %s: %s\n", cmd, msg)
64-
if cmd == "test" {
65-
fmt.Fprintf(os.Stderr, `run "go help %s" or "go help testflag" for more information`+"\n", cmd)
66-
} else {
67-
fmt.Fprintf(os.Stderr, `run "go help %s" for more information`+"\n", cmd)
68-
}
69-
base.SetExitStatus(2)
70-
base.Exit()
42+
func (e NonFlagError) Error() string {
43+
return fmt.Sprintf("not a flag: %q", e.RawArg)
7144
}
7245

73-
// AddKnownFlags registers the flags in defns with base.AddKnownFlag.
74-
func AddKnownFlags(cmd string, defns []*Defn) {
75-
for _, f := range defns {
76-
base.AddKnownFlag(f.Name)
77-
base.AddKnownFlag(cmd + "." + f.Name)
78-
}
79-
}
46+
// ParseOne sees if args[0] is present in the given flag set and if so,
47+
// sets its value and returns the flag along with the remaining (unused) arguments.
48+
//
49+
// ParseOne always returns either a non-nil Flag or a non-nil error,
50+
// and always consumes at least one argument (even on error).
51+
//
52+
// Unlike (*flag.FlagSet).Parse, ParseOne does not log its own errors.
53+
func ParseOne(fs *flag.FlagSet, args []string) (f *flag.Flag, remainingArgs []string, err error) {
54+
// This function is loosely derived from (*flag.FlagSet).parseOne.
8055

81-
// Parse sees if argument i is present in the definitions and if so,
82-
// returns its definition, value, and whether it consumed an extra word.
83-
// If the flag begins (cmd.Name()+".") it is ignored for the purpose of this function.
84-
func Parse(cmd string, usage func(), defns []*Defn, args []string, i int) (f *Defn, value string, extra bool) {
85-
arg := args[i]
86-
if strings.HasPrefix(arg, "--") { // reduce two minuses to one
87-
arg = arg[1:]
56+
raw, args := args[0], args[1:]
57+
arg := raw
58+
if strings.HasPrefix(arg, "--") {
59+
if arg == "--" {
60+
return nil, args, ErrFlagTerminator
61+
}
62+
arg = arg[1:] // reduce two minuses to one
8863
}
64+
8965
switch arg {
9066
case "-?", "-h", "-help":
91-
usage()
67+
return nil, args, flag.ErrHelp
9268
}
93-
if arg == "" || arg[0] != '-' {
94-
return
69+
if len(arg) < 2 || arg[0] != '-' || arg[1] == '-' || arg[1] == '=' {
70+
return nil, args, NonFlagError{RawArg: raw}
9571
}
72+
9673
name := arg[1:]
97-
// If there's already a prefix such as "test.", drop it for now.
98-
name = strings.TrimPrefix(name, cmd+".")
99-
equals := strings.Index(name, "=")
100-
if equals >= 0 {
101-
value = name[equals+1:]
102-
name = name[:equals]
74+
hasValue := false
75+
value := ""
76+
if i := strings.Index(name, "="); i >= 0 {
77+
value = name[i+1:]
78+
hasValue = true
79+
name = name[0:i]
10380
}
104-
for _, f = range defns {
105-
if name == f.Name {
106-
// Booleans are special because they have modes -x, -x=true, -x=false.
107-
if f.BoolVar != nil || IsBool(f.Value) {
108-
if equals < 0 { // Otherwise, it's been set and will be verified in SetBool.
109-
value = "true"
110-
} else {
111-
// verify it parses
112-
SetBool(cmd, new(bool), value)
113-
}
114-
} else { // Non-booleans must have a value.
115-
extra = equals < 0
116-
if extra {
117-
if i+1 >= len(args) {
118-
SyntaxError(cmd, "missing argument for flag "+f.Name)
119-
}
120-
value = args[i+1]
121-
}
122-
}
123-
if f.Present {
124-
SyntaxError(cmd, f.Name+" flag may be set only once")
125-
}
126-
f.Present = true
127-
return
81+
82+
f = fs.Lookup(name)
83+
if f == nil {
84+
return nil, args, FlagNotDefinedError{
85+
RawArg: raw,
86+
Name: name,
87+
HasValue: hasValue,
88+
Value: value,
12889
}
12990
}
130-
f = nil
131-
return
132-
}
13391

134-
// FindGOFLAGS extracts and returns the flags matching defns from GOFLAGS.
135-
// Ideally the caller would mention that the flags were from GOFLAGS
136-
// when reporting errors, but that's too hard for now.
137-
func FindGOFLAGS(defns []*Defn) []string {
138-
var flags []string
139-
for _, flag := range base.GOFLAGS() {
140-
// Flags returned by base.GOFLAGS are well-formed, one of:
141-
// -x
142-
// --x
143-
// -x=value
144-
// --x=value
145-
if strings.HasPrefix(flag, "--") {
146-
flag = flag[1:]
92+
// Use fs.Set instead of f.Value.Set below so that any subsequent call to
93+
// fs.Visit will correctly visit the flags that have been set.
94+
95+
failf := func(format string, a ...interface{}) (*flag.Flag, []string, error) {
96+
return f, args, fmt.Errorf(format, a...)
97+
}
98+
99+
if fv, ok := f.Value.(boolFlag); ok && fv.IsBoolFlag() { // special case: doesn't need an arg
100+
if hasValue {
101+
if err := fs.Set(name, value); err != nil {
102+
return failf("invalid boolean value %q for -%s: %v", value, name, err)
103+
}
104+
} else {
105+
if err := fs.Set(name, "true"); err != nil {
106+
return failf("invalid boolean flag %s: %v", name, err)
107+
}
147108
}
148-
name := flag[1:]
149-
if i := strings.Index(name, "="); i >= 0 {
150-
name = name[:i]
109+
} else {
110+
// It must have a value, which might be the next argument.
111+
if !hasValue && len(args) > 0 {
112+
// value is the next arg
113+
hasValue = true
114+
value, args = args[0], args[1:]
151115
}
152-
for _, f := range defns {
153-
if name == f.Name {
154-
flags = append(flags, flag)
155-
break
156-
}
116+
if !hasValue {
117+
return failf("flag needs an argument: -%s", name)
118+
}
119+
if err := fs.Set(name, value); err != nil {
120+
return failf("invalid value %q for flag -%s: %v", value, name, err)
157121
}
158122
}
159-
return flags
123+
124+
return f, args, nil
125+
}
126+
127+
type boolFlag interface {
128+
IsBoolFlag() bool
160129
}

src/cmd/go/internal/test/flagdefs.go

+34
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
+34
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
// Copyright 2019 The Go Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style
3+
// license that can be found in the LICENSE file.
4+
5+
package test
6+
7+
import (
8+
"flag"
9+
"strings"
10+
"testing"
11+
)
12+
13+
func TestPassFlagToTestIncludesAllTestFlags(t *testing.T) {
14+
flag.VisitAll(func(f *flag.Flag) {
15+
if !strings.HasPrefix(f.Name, "test.") {
16+
return
17+
}
18+
name := strings.TrimPrefix(f.Name, "test.")
19+
if name != "testlogfile" && !passFlagToTest[name] {
20+
t.Errorf("passFlagToTest missing entry for %q (flag test.%s)", name, name)
21+
t.Logf("(Run 'go generate cmd/go/internal/test' if it should be added.)")
22+
}
23+
})
24+
25+
for name := range passFlagToTest {
26+
if flag.Lookup("test."+name) == nil {
27+
t.Errorf("passFlagToTest contains %q, but flag -test.%s does not exist in test binary", name, name)
28+
}
29+
30+
if CmdTest.Flag.Lookup(name) == nil {
31+
t.Errorf("passFlagToTest contains %q, but flag -%s does not exist in 'go test' subcommand", name, name)
32+
}
33+
}
34+
}

0 commit comments

Comments
 (0)