go: flag: panic if flag name begins with - or contains =
Proposal
Currently, the flag package allows any string to be used as the name of the flag. It only validates whether the name of the flag is already registered. (code)
But, if the name of the flag is (1) empty, (2) starts with a hyphen, or (3) contains an equal sign, the flag can not be parsed properly. If you try to use the flag as described in the usage message, it will fail. (code)
So, I suggest that the flag package validates the name of the flag.
+ https://github.com/golang/go/issues/41792#issuecomment-704401130
And these are invalid flag names:
Empty String:
- If the flag name is an empty string and is used in the
-flag xform, the flag starting with hyphen(‘-’‘’) can be the non-flag argument and the flag starting with double hyphen(‘–’‘’) can be the terminator.Flag parsing stops just before the first non-flag argument(“-” is a non-flag argument) or after the terminator “–”.
Starting with hyphen:
- According to the document the flag should start with ‘-’ or ‘–’. But, if the flag name starts with hyphen, this package can’t distinguish between ‘-’‘-flag’ and ‘–’‘flag’.
Containing equal sign:
- This package uses equal sign to split the flag into a name-value pair. So, if the flag name contains equal sign, this package can’t split it properly.
- e.g. If the flag name is ‘foo=bar’ and the flag is ‘-foo=bar=value’, this package can’t find out the actual value.
Example
package main
import (
"flag"
"fmt"
)
func main() {
fs := flag.NewFlagSet("", flag.ExitOnError)
emptyString := fs.String("", "", "empty string")
hyphen := fs.String("-hyphen", "", "hyphen")
equalSign := fs.String("=equalsign", "", "equal sign")
fs.PrintDefaults()
// Output:
// - string
// empty string
// --hyphen string
// hyphen
// -=equalsign string
// equal sign
_ = fs.Parse([]string{"-=foobar"}) // bad flag syntax: -=foobar
// _ = fs.Parse([]string{"--hyphen=foobar"}) // flag provided but not defined: -hyphen
// _ = fs.Parse([]string{"-=equalsign=foobar"}) // bad flag syntax: -=equalsign=foobar
fmt.Println(*emptyString, *hyphen, *equalSign)
}
About this issue
- Original URL
- State: closed
- Created 4 years ago
- Reactions: 5
- Comments: 27 (21 by maintainers)
Commits related to this issue
- checkers: extend flagName diagnostics From the https://github.com/golang/go/issues/41792 And these are invalid flag names: * Empty String: * If the flag name is an empty stri... — committed to go-critic/go-critic by quasilyte 4 years ago
- checkers: extend flagName diagnostics (#1005) From the https://github.com/golang/go/issues/41792 And these are invalid flag names: * Empty String: * If the flag name is... — committed to go-critic/go-critic by quasilyte 4 years ago
- Keep up (#3) * cmd/compile: graceful handle error in noder LoadPackage When syntax.Parse returns error, noder.file will be nil. Currently, we continue accessing it regardlessly and depend on gc.h... — committed to gopher-lang/gopher by auxten 3 years ago
- Remove leading hyphen from "help" flag This is an invalid name for a flag, and Go 1.17 rejects it (see https://github.com/golang/go/issues/41792). — committed to reillywatson/i18n4go by reillywatson 3 years ago
- Remove leading hyphen from "help" flag (#3) This is an invalid name for a flag, and Go 1.17 rejects it (see https://github.com/golang/go/issues/41792). — committed to EverlongProject/i18n4go by reillywatson 3 years ago
OK, retitled to be about panicking for names beginning with - or containing = signs. Does anyone object to that?
@ianthehat, I am still not convinced about this empty string case, but maybe we should set that aside. I don’t really want to change behavior based on which set it is. What about letting “” go through as a way to “comment out” a flag or bypass (sketchily) safehtml’s API, but panic if the flag begins with
-or contains=? Those are definitely wrong and would be confusing. The thing I’m most worried about here is a new Go programmer debugging why flag.Bool(“-b”, …) doesn’t seem to be registering -b. I think we have a clear opportunity to be helpful in that case.FWIW I grepped my Go corpus for
flag.(Bool|Int|String...)\(""and turned up exactly one instance of this mistake, in my own code:Apparently I never used that flag!
Since this problem is caused by ‘defining’ a flag with an invalid flag name, panicking at the time of flag ‘definition’ is more appropriate than panicking at the time of flag parsing. And defining a flag with an invalid flag name is entirely the fault of the author of the program, not the user of the program. But, the error that occur during flag parsing seems like the user’s fault.
@ianthehat I understand your concerns. But, IMHO, solving the problem in a proper way is more important than keeping compatibility for the programs that use the flag package in a strange way.
Nearly all flag names are constants, and most are even literals. Would a
vetcheck make sense?No change in consensus, so accepted (for Go 1.17).
flag.FlagSet.Var, which ends up being what registers any flag no matter what you use to create the flag, already panics if the same flag name is registered multiple times. The name passed to the second call is invalid because it is already registered, so the call panics with a “flag redefined” message.
I doubt it would come up much, but it doesn’t seem like a problem to me for the call to also panic if the name is (1) empty, (2) starts with a hyphen, or (3) contains an equal sign. Those are all clear mistakes.
People also use the flag package in interesting ways that never call
Parse(like using vars as an easy way to go from string to any value, I have seen this multiple times in configuration systems). In these cases the name is essentially irrelevant, often either the empty string (used when the flagset is irrelevant) or some other arbitrary string based on the property name (when multiple vars are added to a single flagset), either of which can break the naming rules suggested at both compile and run time without being an error. For this reason checking atVartime or with avetcheck both seem like a bad breaking change to me.Name rules only apply to
Parse, so that should be the earliest the check can happen. You would still need a more complete description of exactly what the rules are though, which presumably would be based on the actual behavior ofParseitself, any var it can never assign could probably produce an error. Even this is still a dangerous breaking change, if there is some library out there registering a global flag with a bad name it could break everybody that depends (even indirectly) on that library with no easy workaround. Which means you probably have to add an option to enable/disable the check at the least for those cases, and now we are getting into the weeds of increasing API surface and complexity for questionable gains. The only things it would catch are flags that nobody has ever even tried to use, which seem like a fairly low priority problem to fix.My conclusion is if we were starting from nothing I would agree with the checks being added to Parse, but from where we are right now it does not seem like an overall beneficial change to do anything at all.
The empty string seems unlikely, but I could see someone unfamiliar with how the flag package works trying to use flag.Bool(“-b”) to add a -b flag. A panic with a clear message would be better than silently installing an inaccessible flag.
Does anyone object to adding this panic (like duplicate registrations currently panic)?
First we’d need to know what a valid flag name is or is not.
This seems like a solution looking for a problem.