Repository navigation
fix(merge): keep a blessed hash whole in perl, and close cycles - #171
Merged
Merged
Conversation
Since 7e54712 perl's merge copies a plain node that wins, but it told kinds apart by list or map alone and recursed with no record of the nodes it was already copying. - A blessed object over a plain map was descended into rather than kept, and a plain map over one was merged into it, changing the caller's object. typescript keeps an instance whole and copies a plain map over one. - clone turns a blessed object into a plain map that keeps any reference back to itself, and transform merges that clone, so validate and select over such an object copied each cycle out to the depth limit. A generated perl SDK's entity tests ran out of memory selecting over their own list results. _merge_pair now treats a blessed node and a plain one as different kinds, and a node already being merged further up the path returns its copy, so the copy closes the cycle. Acyclic input, shared nodes included, is copied exactly as before. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01BoEed9CaF6sTNMcXxQaH6w
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc11e77875
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review of dc11e77 found two gaps. - The cycle check ran only when the override was copied. Where the destination held a node of the same kind, as when both operands refer back to themselves, the merge descended again: two maps whose x and y each point back to their own root still unrolled to MAXDEPTH, two ways at every level. A pair already being merged further up now returns at once. - A blessed array counted as an instance, so merge([[1], $b2, $b3]) returned $b2 and then wrote $b3 into it. An Array subclass is a list in typescript, and a blessed array is one here. The copy record now holds only copies, so an override that closes under a destination with keys of its own closes on a fresh copy and takes none of those keys, as typescript's unrolled copy does not. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01BoEed9CaF6sTNMcXxQaH6w
From the second review of d7ca6e0. A closed cycle is one node for every level, so a later element that writes into one level writes into all of them; typescript's unrolled copy takes the write at one level only. A winning blessed array is copied as a plain array, as typescript copies an Array subclass, and one instance over another merges into it. The select test now asserts its one match rather than only a list. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01BoEed9CaF6sTNMcXxQaH6w
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.
The cause
Since 7e54712 (#158), perl's
mergecopies a plain node that wins rather than sharing it._merge_pairtold kinds apart by list or map alone, though, and it recursed with no record of the nodes it was already merging. Two failures followed:typifycalls a blessed hash a map, somerge([{x => 10}, $obj])descended into the object and returned a plain copy.merge([{a => $obj}, {a => {x => 11}}])merged the map into the object, setting$obj->{x}to 11. typescript keeps a class instance as it is and copies a plain map over one, leaving the instance alone (merge-specialinStructUtility.test.ts).cloneturns a blessed object into a plain map that keeps any reference back to itself, andtransformmergesclone($data). Sovalidateandselectover such an object copied every cycle toMAXDEPTH, and with more than one way round that is exponential. Found while revendoring sdkgen: a generated perl SDK's entity tests ran out of memory (5 GB and climbing) inselectover their own list results, whose entities refer back to themselves through their context. The same tests finish on the previous checkpoint's struct, whose merge shared rather than copied.Change
perl/lib/Voxgig/Struct.pm,_merge_pair:Kinds. A blessed hash is a class instance, so it and a plain map are different kinds, as a list and a map are; a blessed array is still a list, as an Array subclass is in typescript (
_isinstance). An instance that wins is kept as it is, one over another merges into it, and a plain node that wins is copied, as before.Cycles. Two records, both scoped to the path:
Acyclic input, including a node shared by two keys, is copied exactly as before: once per place it appears, as typescript does.
typify,validateandclonestill read a blessed hash as a map.perl/DOCS.mdsays so beside the new behaviour: a closed cycle is one node for every level, and a winning blessed array is copied as a plain array. #172 tracks bringing them in line with typescript'sT_instance. That changes what generated perl SDKs do with blessed options (sdkgen's templates document and work aroundcloneunblessing them), so it is not attempted here.Tests
perl/t/struct.t:merge-instance: canonical'smerge-specialassertions with blessed objects.isidentity) over a map, under a key and under an absent key.merge-cycle:xandyeach point back to their own root merge into the first, which keeps its cycle.selectover a blessed object that refers back to itself through a map and a list returns its one match.Controls
Struct.pmwith these tests: sixmerge-instanceassertions fail, then the run stops atmerge-cyclewithOut of memory!under a 1.5 GB limit.Struct.pm: the two blessed-array assertions and the self-loop's "takes no key" assertion fail. The run then times out at the cycle over a cycle.mergealready unblessed byclone. With the cycle records they finish in a second each.Verification
prove -Ilib t/passes 107 of 107, the shared corpus included.perlcritic --gentle lib t,make commentsandmake scan-prose(vale andcheck_prose) are clean, as are cspell and markdownlint onperl/DOCS.md.moon_entity.tandplanet_entity.tpass once sdkgen's perl runner takes the data hop, which sdkgen's revendor fixes. The threesecrets.tfailures left are on main too: lua and perl secrets tests drive an entity op that needs a parent id, and fail on a nested first entity sdkgen#393.Other ports
lua and php compare the full
typify, which separates an instance from a map, like typescript. swift's value type has no instances. zig copies the destination at every level and is unchanged. Only perl merges withoutwalkand also treats an object as a map.Since review
Codex reviewed dc11e77 and found two gaps, both confirmed and fixed in d7ca6e0, with tests that fail on dc11e77:
A second Claude reviewer found d7ca6e0 sound. It drove 216 merge shapes through typescript, main, dc11e77 and d7ca6e0, and 11 select, validate and transform probes:
Its notes:
40a36c6 says the first two in
perl/DOCS.mdand makes the select test assert its one match.🤖 Generated with Claude Code
https://claude.ai/code/session_01BoEed9CaF6sTNMcXxQaH6w