fix: distinguish collection and element matches when walking input - #1082
Merged
Conversation
Resolvers and defaults are found by walking the input type and recording a path to each match. A path to a match on a collection's own type, e.g. a resolver on `type Coords [2]float64`, was indistinguishable from a path to a match on its element type, so `everyPB` guessed by kind: collections were always treated as containers to walk through. Now that fixed arrays are walked too, that guess turned a resolver on a named array type into a panic, and it was already wrong when a collection and its element both had resolvers, running the element's twice and the collection's never. Record a marker step in the path when descending into a collection's elements so both cases are exact, and drop the kind-based guess. Values reached through a map were also visited as copies, which panicked with `reflect: reflect.Value.Set using unaddressable value` when a default was applied and silently discarded resolver mutations. Copy the value out, walk it, then write it back with `SetMapIndex`.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1082 +/- ##
==========================================
- Coverage 93.22% 93.20% -0.03%
==========================================
Files 23 23
Lines 4976 4988 +12
==========================================
+ Hits 4639 4649 +10
- Misses 271 272 +1
- Partials 66 67 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #1076, which surfaced two silent issues in the input walker.
Resolver on a named collection type returned a 500.
type Coords [2]float64with aResolvemethod:everyPBtreated every collection as a container to walk through, so it stepped past the array and tried to run a resolver on eachfloat64. The cause is that_findInTyperecorded the same path whether the match was on the collection's own type or on its element type. It now records a marker step when descending into elements, so both are exact and the kind-based guess is gone. This also fixes the case where a collection and its element both have resolvers, which previously ran the element's twice and the collection's never.Defaults and resolvers reached through a map panicked with
reflect: reflect.Value.Set using unaddressable value, because map values are copies. The value is now copied out, walked, then written back withSetMapIndex, so defaults and resolver mutations inside maps stick.map[string]Structwith adefault:tag panics on main today, so that is fixed too.Tests cover resolvers on named array, slice and nested collection types, collections whose elements also resolve, and defaults plus resolver mutations through maps. Each fails without these changes. Benchmarks are flat except map bodies, about 4% for the write-back.
Known limitation, not addressed here: an omitted optional
[N]Tfield runs resolvers and defaults on N zero elements, since a Go array is always N values. That matches existing behaviour for non-pointer struct fields;[]Tand*[N]Twork as expected.