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

Jump to bottom

doc: flag: document behaviour of redefined flags #31694

Closed
ghost opened this issue Apr 26, 2019 · 4 comments
Closed

doc: flag: document behaviour of redefined flags #31694

ghost opened this issue Apr 26, 2019 · 4 comments
Labels
Documentation FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unplanned

Comments

@ghost
Copy link

ghost commented Apr 26, 2019

Defining a flag with a name that has been previously defined results in the following:

package main

import (
	"flag"
)

func main() {
	fs := flag.NewFlagSet("sample", flag.ExitOnError)

	_ = fs.String("name", "", "name value")
	_ = fs.String("name", "", "name value")

	fs.Parse([]string{`--name`, `test`})
}
sample flag redefined: name
panic: sample flag redefined: name

goroutine 1 [running]:
flag.(*FlagSet).Var(0x430180, 0x147130, 0x40c158, 0x115596, 0x4, 0x11628a, 0xa, 0x7a61)
	/usr/local/go/src/flag/flag.go:850 +0x520
flag.(*FlagSet).StringVar(...)
	/usr/local/go/src/flag/flag.go:753
flag.(*FlagSet).String(0x430180, 0x115596, 0x4, 0x0, 0x0, 0x11628a, 0xa, 0x7a61, 0x40c150, 0x7a61)
	/usr/local/go/src/flag/flag.go:766 +0xc0
main.main()
	/tmp/sandbox904772544/main.go:11 +0xc0

This is OK, but the documentation does not seem to mention that flag names need to be unique for each FlagSet, nor that a panic will occur when trying to create a flag with an already-defined name.
@ghost
Copy link
Author

ghost commented Apr 26, 2019

I also just noticed that f.Output() is written to before the panic call.

fmt.Fprintln(f.Output(), msg)
panic(msg) // Happens only if flags are declared with identical name

This seems like odd be behaviour to me, so perhaps a source comment could be added as to why this is (even better might be to remove the Fprintln, but someone else would have to make that decision (@robpike)).

@bcmills
Copy link
Contributor

bcmills commented May 28, 2019

the documentation does not seem to mention that flag names need to be unique for each FlagSet

That should probably be fixed, but note that the existence of the Lookup and Set methods already imply that names must be unique.

nor that a panic will occur when trying to create a flag with an already-defined name.

I think that would over-specify the behavior: it is a programmer error to attempt to register a duplicate flag, so the flag package should not guarantee any particular failure mode if that should occur. (For example, it could just as reasonably log an error message and invoke os.Exit.)

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 28, 2019
@bcmills bcmills added this to the Unplanned milestone May 28, 2019
@gopherbot
Copy link

Change https://golang.org/cl/206126 mentions this issue: flag: clarify that a flag cannot be re-defined

@gopherbot
Copy link

Change https://golang.org/cl/206127 mentions this issue: flag: remove spurious print statement

@golang golang locked and limited conversation to collaborators Nov 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Documentation FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
2 participants
@bcmills @gopherbot