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

flag: UnquoteUsage checks for IsBoolFlag() method presence but doesn't actually call it #53473

Closed
ldemailly opened this issue Jun 21, 2022 · 6 comments
Closed

flag: UnquoteUsage checks for IsBoolFlag() method presence but doesn't actually call it #53473

ldemailly opened this issue Jun 21, 2022 · 6 comments
Assignees
@robpike
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Backlog

Comments

@ldemailly
Copy link

What version of Go are you using (go version)?

$ go version
go version go1.18.3 darwin/arm64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

not relevant (but apple silicon M1 macos)

What did you do?

Implemented a generic set of flag extension and part of the generic includes:

func (d *DynValue[T]) IsBoolFlag() bool {
	var v T
	switch any(v).(type) {
	case bool:
		return true
	default:
		return false
	}
}

Yet despite returning false for the non bool, flag.PrintDefaults through UnquoteUsage thinks all my types are booleans (don't require a value) and thus my usage output regressed from for instance

-  -loglevel value
+  -loglevel
         loglevel, one of [Debug Verbose Info Warning Error Critical Fatal]
 (default Info)

What did you expect to see?

value still output

What did you see instead?

nothing

ps: I can workaround by making the bool type special but it's unfortunate
ldemailly added a commit to fortio/fortio that referenced this issue Jun 21, 2022
@ldemailly
had to remove IsBoolFlag from generic because of golang/go#53473 - ad…
aee1fd5 
…ded tests for PrintDefaults output checking for 3 cases
@robpike
Copy link
Contributor

robpike commented Jun 21, 2022

Fix is easy, I think. Can you please provide a complete runnable example that shows the problem?

ldemailly added a commit to fortio/fortio that referenced this issue Jun 21, 2022
@ldemailly
dflags generics - added WithInputMutator and WithValueMutator too (#593)
6626cd6 
* exit with status 1 if grpcping -health isn't SERVING. fixes #588

* add a _down grpc server that will be always NOT_SERVING

* add a test for _down

* add also status e2e test and preop for 1.33.0

* fix the status test

* Use generics to tremendously shrink/improve the dflag/ code (left the tests unchanged or added to them to test indeed the implementation is compatible; they could be cleaned as well

* Add test for Parse error case. Added test in logger to cover the setting normalization

* update the sample and move the common stuff into the new generic file, add one more test

* use type aliases for backward compatibility

* added slice and set to generics version

* linter

* last one: dynjson; also improve testify compatibility by showing actual line numbers

* had to remove IsBoolFlag from generic because of golang/go#53473 - added tests for PrintDefaults output checking for 3 cases

* linter
ldemailly added a commit to ldemailly/go-flag-bug that referenced this issue Jun 21, 2022
@ldemailly
show the golang/go#53473 issue
0983f11
@ldemailly
Copy link
Author

ldemailly commented Jun 21, 2022 •
edited

Thanks @robpike, here you go (simplified without all the generics etc):

package main

import "flag"

type NotBool struct{}

func (f *NotBool) IsBoolFlag() bool {
	return false
}

func (f *NotBool) Set(v string) error {
	return nil
}
func (f *NotBool) String() string {
	return ""
}

func main() {
	value := NotBool{}
	flag.Var(&value, "test", "test usage, line above should be -test value")
	flag.PrintDefaults()
}

output

% go run .
  -test
    	test usage, line above should be -test value

expected:

% go run .
  -test value
    	test usage, line above should be -test value

also on https://go.dev/play/p/URV2A7fhI4- and https://github.com/ldemailly/go-flag-bug/blob/main/bool_flag_issue.go

@robpike robpike self-assigned this Jun 21, 2022
@ianlancetaylor ianlancetaylor added the NeedsFix The path to resolution is known, but the work has not been done. label Jun 21, 2022
@ianlancetaylor ianlancetaylor added this to the Backlog milestone Jun 21, 2022
@gopherbot
Copy link

Change https://go.dev/cl/431102 mentions this issue: flag: test IsBoolFlag when creating the usage message

ldemailly added a commit to fortio/log that referenced this issue Feb 11, 2023
@ldemailly
dflags generics - added WithInputMutator and WithValueMutator too (#593)
5f6876a 
* exit with status 1 if grpcping -health isn't SERVING. fixes #588

* add a _down grpc server that will be always NOT_SERVING

* add a test for _down

* add also status e2e test and preop for 1.33.0

* fix the status test

* Use generics to tremendously shrink/improve the dflag/ code (left the tests unchanged or added to them to test indeed the implementation is compatible; they could be cleaned as well

* Add test for Parse error case. Added test in logger to cover the setting normalization

* update the sample and move the common stuff into the new generic file, add one more test

* use type aliases for backward compatibility

* added slice and set to generics version

* linter

* last one: dynjson; also improve testify compatibility by showing actual line numbers

* had to remove IsBoolFlag from generic because of golang/go#53473 - added tests for PrintDefaults output checking for 3 cases

* linter
ldemailly added a commit to fortio/dflag that referenced this issue Feb 11, 2023
@ldemailly
dflags generics - added WithInputMutator and WithValueMutator too (#593)
3544f6c 
* exit with status 1 if grpcping -health isn't SERVING. fixes #588

* add a _down grpc server that will be always NOT_SERVING

* add a test for _down

* add also status e2e test and preop for 1.33.0

* fix the status test

* Use generics to tremendously shrink/improve the dflag/ code (left the tests unchanged or added to them to test indeed the implementation is compatible; they could be cleaned as well

* Add test for Parse error case. Added test in logger to cover the setting normalization

* update the sample and move the common stuff into the new generic file, add one more test

* use type aliases for backward compatibility

* added slice and set to generics version

* linter

* last one: dynjson; also improve testify compatibility by showing actual line numbers

* had to remove IsBoolFlag from generic because of golang/go#53473 - added tests for PrintDefaults output checking for 3 cases

* linter
@ldemailly
Copy link
Author

any chance to backport to 1.19?

@ianlancetaylor
Copy link
Contributor

This doesn't seem to me to fit the backport requirements (https://github.com/golang/go/wiki/MinorReleases).

@ldemailly
Copy link
Author

yes I guess it's not a big bug (even though the fix is really small and safe) - oh well... will wait then, thanks

@golang golang locked and limited conversation to collaborators Feb 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@robpike robpike

Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
4 participants
@ianlancetaylor @ldemailly @robpike @gopherbot