Skip to content

Compare native text flag type identities in GetText - #529

Open
kimjooyoon wants to merge 1 commit into
spf13:masterfrom
kimjooyoon:fix-text-type-identity
Open

kimjooyoon wants to merge 1 commit into
spf13:masterfrom
kimjooyoon:fix-text-type-identity

Conversation

@kimjooyoon

@kimjooyoon kimjooyoon commented Oct 21, 2026 •

Copy link
Copy Markdown

Follow-up to #526: GetText can still accept a different native text type when two distinct Go types have identical display names. The regression uses two separately declared stamp types embedding time.Time. Their reflect.Type values differ, but their String() results agree. A cross-type GetText currently returns nil and overwrites the output timestamp instead of rejecting it.

Run the four native type combinations in Go Playground without installing anything. It pins the original development commit and reproduces both cross-type overwrites on hosted Go 1.27.3; it does not run the proposed patch. Pinned source, original remote response and separate local patch observations retain source/runtime identities and an integrity checker.

Compare the actual reflect.Type values for pflag's native textValue. Keep Type() and help labels unchanged, and preserve the existing type-label contract for custom/wrapped Value implementations. The Go reflect documentation explicitly distinguishes display strings from type identity.

Both cross-type combinations fail on main d26a4dcd and pass after the fix, with the rejected output preserved. Both same-type combinations and a wrapped custom-value round trip continue to pass. No helper packages or dependencies are added.

Validation on Go 1.27.3, macOS arm64:

  • New focused regression before/after
  • go test -count=1 ./...
  • go test -race -count=1 ./...
  • go build ./...
  • go vet ./...
  • golangci-lint v2.15.0 run --timeout 2m — 0 issues
  • gofmt and git diff --check

The upstream Go-version CI matrix remains separate from these local checks.

AI disclosure: a Codex agent prepared the patch, tests and this description, and ran the listed validation. No human-only review or upstream acceptance is implied.

Use reflect.Type equality for TextVar values instead of treating their display names as unique identities. Preserve the existing type-label behavior for custom Value implementations and keep help labels unchanged.

Add identical-name mismatch and matching/wrapped-value regression coverage. Prepared and tested with Codex assistance.
@CLAassistant

CLAassistant commented Oct 21, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@tomasaschan

Copy link
Copy Markdown
Collaborator

I'm curious: did you actually run into this bug, or did you send your agent to find weird edge cases in open-source libraries and patch them?

@kimjooyoon

Copy link
Copy Markdown
Author

It was the latter: the Codex agent found this edge case while investigating type checks in GetText. I did not encounter it in a production application. I'm exploring how to make useful contributions to other open-source projects with an agent, and I appreciate the question about whether the problem is worth a maintainer's time.

In plain terms, the old check compares the printed names of two types. Two separately declared Go types can have the same printed name while still being different types. In the reproducer, both are called stamp and embed time.Time. GetText accepts the wrong one, returns no error, and overwrites the destination timestamp.

The proposed change compares actual type identities for pflag's built-in text value. Custom/wrapped values keep the existing label-based behaviour. The agent prepared the patch and tests; the PR records its before/after checks. The Playground example shows the original behaviour without a local setup.

I don't have evidence that this occurs frequently in real applications. My reason for proposing it is that the type check silently accepts a different type, and the fix is small. This reply was also drafted with Codex assistance.

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