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

fix: handle non-pointer values in the flag parser #3720

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kolesnikovae
Copy link
Collaborator

Currently, the parser panics in certain cases. For example, here, a struct value is used as a flag implementing the encoding.Text(Un)marshaler interface.

It's recommended to use Value.UnsafePointer() to get uintptr of a value via reflection.

// Pointer returns v's value as a uintptr.
// It panics if v's Kind is not [Chan], [Func], [Map], [Pointer], [Slice], or [UnsafePointer].
//
// If v's Kind is [Func], the returned pointer is an underlying
// code pointer, but not necessarily enough to identify a
// single function uniquely. The only guarantee is that the
// result is zero if and only if v is a nil func Value.
//
// If v's Kind is [Slice], the returned pointer is to the first
// element of the slice. If the slice is nil the returned value
// is 0.  If the slice is empty but non-nil the return value is non-zero.
//
// It's preferred to use uintptr(Value.UnsafePointer()) to get the equivalent result.
func (v Value) Pointer() uintptr

@kolesnikovae kolesnikovae requested a review from a team as a code owner November 26, 2024 12:28
@aleks-p
Copy link
Contributor

aleks-p commented Nov 26, 2024

Based on the context, this seems related to docs. Do we need to re-run doc generation or did the panic have no impact on this?

Copy link
Contributor

@marcsanmi marcsanmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

tools/doc-generator/parse/parser.go Show resolved Hide resolved
@kolesnikovae
Copy link
Collaborator Author

kolesnikovae commented Nov 26, 2024

Based on the context, this seems related to docs. Do we need to re-run doc generation or did the panic have no impact on this?

Yes, the flag parser is used to generate docs. This fix has no impact as long as v2 flag is not set. I discovered the problem when was testing v2 configs (just wanted to make sure that the options have meaningful description). This is (almost) a bug-to-bug compatible change :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants