Skip to content

Convert to time.Time before asserting in isNeField - #1619

Open
youdie006 wants to merge 1 commit into
go-playground:masterfrom
youdie006:nefield-convert-timetype
Open

youdie006 wants to merge 1 commit into
go-playground:masterfrom
youdie006:nefield-convert-timetype

Conversation

@youdie006

Copy link
Copy Markdown

nefield panics on a named type whose underlying type is time.Time.

type MyTime time.Time

type Test struct {
	Start MyTime
	End   MyTime `validate:"nefield=Start"`
}
validate.Struct(Test{Start: now, End: now})
// panic: interface conversion: interface {} is validator.MyTime, not time.Time

isNeField's struct branch (baked_in.go:1072) guards on ConvertibleTo(timeType), which admits
any named type with a time.Time underlying — then asserts without converting:

if fieldType.ConvertibleTo(timeType) && currentField.Type().ConvertibleTo(timeType) {
	t := getValue(currentField).(time.Time)
	fieldTime := getValue(field).(time.Time)

getValue returns val.Interface() (validator.go:516), so the dynamic type is still MyTime
and the assertion fails.

It is the only one

Of the 28 getValue(...).(time.Time) sites in baked_in.go, 26 convert first. The two that do
not are this pair. isEqField:1402, isGteField:2326, isGtField:2370, isLteField:2554,
isLtField:2598 and isNeCrossStructField:1308 — the same not-equal semantics in the sibling
family — all do:

t := getValue(currentField.Convert(timeType)).(time.Time)

Same input through four of them, before the change:

eqfield   -> ok
ltfield   -> ok
necsfield -> ok
nefield   -> panic: interface conversion: interface {} is validator.MyTime, not time.Time

Change

Two lines, adding the Convert(timeType) the siblings already have. Convert is a no-op for a
plain time.Time, so nothing on the currently covered paths changes, and the v10 API is untouched.

Test added next to the other nefield tests. Verified red/green by reverting only baked_in.go
and keeping the test — it panics with interface conversion: interface {} is validator.namedTime.
It also pins the semantics rather than just the absence of a panic: equal named-time values must
fail the tag, different ones must pass.

go test -cover -race ./... passes across every package. gofmt -l reports doc.go and
translations/ko/ko_test.go both before and after, so this branch does not change it.

validator_test.go:5317-5414 covers nefield against time.Time, *time.Time and
time.Duration, which is why this shape was never exercised.


Disclosure: prepared with AI assistance; I verified the reproduction, the red/green runs and the
race suite myself.

@youdie006
youdie006 requested a review from a team as a code owner August 30, 2026 23:43
@youdie006

Copy link
Copy Markdown
Author

The lint job is red on master too — 51 exhaustruct_v5 findings in benchmarks_test.go and
validator_test.go, none of them on the lines this PR adds (my test is validator_test.go
1943-1962; the flagged lines are 356…903 and 2207…10033). All six test jobs pass on Go 1.25 and
1.26 across Linux, macOS and Windows.

@nodivbyzero

Copy link
Copy Markdown
Contributor

The lint job is red on master too — 51 exhaustruct_v5 findings in benchmarks_test.go and validator_test.go, none of them on the lines this PR adds (my test is validator_test.go 1943-1962; the flagged lines are 356…903 and 2207…10033). All six test jobs pass on Go 1.25 and 1.26 across Linux, macOS and Windows.

I opened a PR to fix the linter issues: #1618

@nodivbyzero

Copy link
Copy Markdown
Contributor

Thank you for your contribution!
Please rebase your PR and I'll merge your changes once that's done.

The struct branch admits any type convertible to time.Time, but isNeField
asserted getValue(...) directly instead of converting first, so a named
type with a time.Time underlying panicked. Every other comparison
validator converts.
@youdie006
youdie006 force-pushed the nefield-convert-timetype branch from 6ba6239 to 3bde176 Compare September 11, 2026 02:31
@youdie006

Copy link
Copy Markdown
Author

Rebased onto master (now on top of dfe35cf). The diff is byte-identical to before the rebase — same two files, baked_in.go +2/-2 and the new test.

dfe35cf ("ci: Fix the linter version") also resolves the red lint job I mentioned above: it pins golangci-lint to v2.13.1 and disables exhaustruct_v5. I ran that exact version locally against this branch — docker run golangci/golangci-lint:v2.13.1 golangci-lint run ./... — and it reports 0 issues.

Re-verified after the rebase:

  • go test ./... green across all packages.
  • Reverting baked_in.go alone reproduces the original panic: interface conversion: interface {} is validator.namedTime, not time.Time.
  • Converting only one of the two operands still panics, so both halves of the change are load-bearing.

For the record on why this shape: isNeField was the only one of the six field-comparison helpers that asserted without converting first. isEqField, isGtField, isGteField, isLtField and isLteField all already call .Convert(timeType) before the assertion — this just brings isNeField in line with its five siblings.

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.

2 participants