Skip to content

fix: handle payload getters in GenericFlag - #2460

Merged
coilysiren merged 1 commit into
urfave:mainfrom
MohammediYunus:fix/generic-typed-callbacks
Oct 9, 2026
Merged

fix: handle payload getters in GenericFlag#2460
coilysiren merged 1 commit into
urfave:mainfrom
MohammediYunus:fix/generic-typed-callbacks

Conversation

@MohammediYunus

@MohammediYunus MohammediYunus commented Oct 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • bug

What this PR does / why we need it:

A GenericFlag can hold a valid cli.Value whose Get method returns its underlying payload, such as a string. Default validation, validation after Set, and flag actions currently assert that payload back to cli.Value and panic before the callback runs. Command.Generic also returns nil for that value after parsing.

  • flag_impl.go adds a private typed callback helper. It preserves a Getter result that already satisfies the callback type, then falls back to the stored value for the internal GenericFlag adapter. The three callback paths use this helper.
  • flag_generic.go applies the same fallback to Command.Generic, with one flag lookup and one Getter call.
  • flag_generic_test.go covers custom and standard-library payload Getters, self-returning and delegated Values, nil values, callback errors and order, aliases, parent lookup, local shadowing, and Getter/invalid-access call counts.

Which issue(s) this PR fixes:

Fixes #2459

Special notes for your reviewer:

This proposes the validator-side fix discussed in the issue. The public interface, FlagBase.Get, and Command.Value are unchanged: callers still receive the Getter's payload. If Get already returns a cli.Value, callbacks and Command.Generic keep using that result, including when it is a different value object. Plain nil GenericFlag values are passed to typed callbacks as nil.

The original reproductions and checks below were run locally.

Testing

On Go 1.26.5, macOS arm64:

  • The new regressions fail against unchanged main at 26bd9a2 and pass with the patch.
  • All 12 original custom/standard-library/self-returning reproduction cases pass, as does the smaller custom example posted in the issue. Payload retrieval remains unchanged.
  • make all passes, including generation, vet, race tests, binary-size checks and the README example check.
  • make lint v3diff passes; generated public API documentation is unchanged.
  • make test with GFLAGS='--tags urfave_cli_no_template' passes.

The pinned gfmrun v1.3.0 macOS arm64 release asset returned 404, so that same version was built locally through the Go-install route documented in CONTRIBUTING before rerunning make all.

Release Notes

Fix GenericFlag validation and actions panicking when a Getter returns an underlying payload. Command.Generic now returns the stored value in that case, while Command.Value continues to return the payload.

@MohammediYunus
MohammediYunus requested a review from a team as a code owner October 7, 2026 20:39
@MohammediYunus MohammediYunus mentioned this pull request Oct 7, 2026
Closed
3 tasks done
@greptile-apps

greptile-apps Bot commented Oct 7, 2026 •
edited
Loading

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[Medium risk] Fixes flag value retrieval for generic types with payload getters.

The PR appears safe to merge; no actionable regression was identified.

Summary

Fixes GenericFlag callbacks and Command.Generic when a Getter returns a payload rather than a cli.Value.

  • Preserves Getter results that already satisfy cli.Value.
  • Adds regression coverage for callbacks, lookup, nil values, and Getter call counts.
Diagram 
%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Read Getter result] --> B{Result satisfies callback type or Value?}
  B -->|Yes| C[Use Getter result]
  B -->|No, GenericFlag| D[Use stored Value]
  B -->|No, other flag| E[Retain existing assertion behavior]
Loading

Reviews (1) · Last reviewed commit: "fix: handle payload getters in GenericFl..." · Reviewed by Greptile

coilysiren
coilysiren approved these changes Oct 7, 2026
View reviewed changes

@coilysiren coilysiren left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm going to hold for @dearchap to confirm, but this LGTM

Juneezee
Juneezee approved these changes Oct 8, 2026
View reviewed changes

@Juneezee Juneezee left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I ran both repros from #2459 (the stdlib flag value and the custom textValue) as an external program, one callback at a time. On 26bd9a2, ValidateDefaults, the Validator and the flag Action each panic. On this branch all three run, Command.Generic returns the stored value (also through a persistent parent flag and an env source), and Command.Value still returns the payload. The new payload tests fail on 26bd9a2 and pass here; the self-returning and delegated controls pass on both.

On the question in #2459: cli.Value embeds flag.Getter, which the standard library describes as "an interface that allows the contents of a Value to be retrieved" (https://pkg.go.dev/flag#Getter). So a Get that returns a payload is a valid cli.Value. Fixing the callbacks instead of changing GenericFlag.Get leaves Command.Value as it is, so this approach looks right to me.

Optional nit: the nil check isn't needed. any(generic.val) is a nil interface when val is nil, and a comma-ok assertion on it returns the zero T:

cli/flag_impl.go

Lines 200 to 206 in 341fa22

if generic, ok := f.value.(*genericValue); ok {
var typed T
if generic.val != nil {
typed = any(generic.val).(T)
}
return typed
} 

	if generic, ok := f.value.(*genericValue); ok {
		typed, _ := any(generic.val).(T)
		return typed
	}

The full test suite passes with this change.

@Juneezee Juneezee mentioned this pull request Oct 9, 2026
Closed
@coilysiren
coilysiren merged commit 31499aa into urfave:main Oct 9, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coilysiren coilysiren coilysiren approved these changes
@Juneezee Juneezee Juneezee approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

GenericFlag validators panic with standard-library flag.Getter values

3 participants

@MohammediYunus @coilysiren @Juneezee