fix: avoid uncompilable != zero check for non-comparable structs (#227) - #228
fix: avoid uncompilable != zero check for non-comparable structs (#227)#228chiliec wants to merge 1 commit into
Conversation
update:ignoreZeroValueField generated `source != ZeroValue` to decide whether to skip a field. When the source is a struct containing a slice, map or function, that struct is not comparable and Go rejects `!=`, producing generated code that does not compile (jmattheis#227). Add xtype.NotZeroValueCheck: it keeps `!= <zero>` for comparable types and for types whose zero value is nil (pointers, slices, maps, channels, funcs, interfaces), and falls back to !reflect.ValueOf(x).IsZero() only for non-comparable structs and arrays. Both zero-check emission sites in builder/struct.go now use it. Closes jmattheis#227.
| return sourceCode.Clone().Op("!=").Add(ZeroValue(t)) | ||
| } | ||
| return jen.Op("!").Add( | ||
| jen.Qual("reflect", "ValueOf").Call(sourceCode.Clone()).Dot("IsZero").Call(), |
There was a problem hiding this comment.
I'm not sure I like the reflection usage here. In my #227 (comment), I said I'd prefer that "goverter gen" errors. In our documentation it's currently listed that the generated code won't include any reflection, adding this would void this.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #228 +/- ##
==========================================
+ Coverage 95.96% 95.98% +0.01%
==========================================
Files 50 50
Lines 2754 2765 +11
==========================================
+ Hits 2643 2654 +11
Misses 80 80
Partials 31 31 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thanks for the review @jmattheis, and fair point — a runtime reflection check contradicts the "generated code contains no reflection" guarantee, so this approach is wrong for goverter. I'm happy to redo it along the lines you described in #227: make |
|
I'd rather do this myself, tho it will take some time as I'm currently busy. |
What
Closes #227.
update:ignoreZeroValueFielddecides whether to skip a field by comparing the source against its zero value with!=:When the source field is a struct that contains a slice, map or function, that struct is not comparable, and Go rejects
!=on it — so the generated code does not compile:(The maintainer confirmed in the issue that goverter should never emit non-compiling code.)
Fix
Added
xtype.NotZeroValueCheck(sourceCode, type), used at both zero-check emission sites inbuilder/struct.go. It:source != <zero>for comparable types, and for types whose zero value isnil(pointers, slices, maps, channels, funcs, interfaces) — so!= nilbehaviour is unchanged;!reflect.ValueOf(source).IsZero()only for non-comparable structs/arrays, which is valid for any type.Generated output for the issue's case becomes:
The
reflectimport is added automatically by the code generator only when the fallback is used.Tests
Added
scenario/update_ignore_zero_value_noncomparable_struct.ymlcovering a non-comparable struct field in update mode.Verified RED→GREEN: with the old
!=codegen restored, the scenario generatessource.Inner != (execution.Inner{})and fails; I also confirmed that exact line does not compile (struct containing []string cannot be compared). With the fix it generates thereflect.ValueOf(...).IsZero()form, which compiles.Validation (real results, Go 1.25)
go test ./...: all pass — the full scenario suite is green, and no existing golden files changed (maps/slices still use!= nil; only genuinely non-comparable structs switch to the reflect form).go build ./...,gofmt -l,go vet ./builder/ ./xtype/: clean.