Fix dead map identity comparison in dyn.Value.eq#5525
Open
simonfaltum wants to merge 2 commits into
Open
Conversation
The KindMap branch compared addresses of the receiver's local copies, which is never true, so visit rebuilt ancestor maps on every visited path even when nothing changed. Compare the mapping's backing pairs slice instead, mirroring the KindSequence branch. Co-authored-by: Isaac
Contributor
Waiting for approvalBased on git history, these people are best suited to review:
Eligible reviewers: Suggestions based on git history. See OWNERS for ownership rules. |
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Collaborator
|
Commit: c372ab7
22 interesting tests: 15 SKIP, 7 RECOVERED
Top 30 slowest tests (at least 2 minutes):
|
Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
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
2 participants
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.
Why
Found during a full-repo review of the CLI.
dyn.Value.eqis the no-change short-circuit used bylibs/dyn/visit.go: when a transform returns a value unchanged, the visit returns the original parent instead of rebuilding it. The map branch compared&v.v == &w.von value receivers, which takes the addresses of two local copies and is never true. The short-circuit was dead for maps, so every visited path cloned its ancestor maps even when nothing changed. This is wasted allocation only; correctness was unaffected.Changes
Before,
eqalways reported two map values as different; now it reports them as equal when they share the same underlying mapping. TheKindMapbranch compares the identity of the mapping's backing pairs slice (first-element pointer, with the same empty and length checks), mirroring the existingKindSequencebranch.Added unit tests for the map cases of
eqand a visit-level test asserting that an identity transform does not rebuild ancestor maps. Both fail against the previous implementation.Test plan
go test ./libs/dyn/...passes./task fmt-q, golangci-lint onlibs/dyn(0 issues), and./task checkspassThis pull request and its description were written by Isaac.