Fix copy/clone of empty value #2122
Conversation
|
PTAL @mtrmac |
mtrmac
left a comment
mtrmac
left a comment
There was a problem hiding this comment.
Thanks, I didn’t realize this is turning nils into empty slice allocations. Definitely worth fixing.
(I don’t think we really want to commit to a difference between nil and []…{} as a matter of API — to the extent the c/common tests essentially test for .Names == nil, the semantics of Names == nil and Names == []string{} are exactly the same and the caller should not care — but as an implementation concern, we should not be making those extra allocations all the time.)
store.go
Outdated
| func copyStringInt64Map(m map[string]int64) map[string]int64 { | ||
| if m == nil { | ||
| return make(map[string]int64, len(m)) | ||
| } | ||
| return maps.Clone(m) | ||
| } | ||
|
|
||
| func copyStringDigestMap(m map[string]digest.Digest) map[string]digest.Digest { | ||
| if m == nil { | ||
| return make(map[string]digest.Digest, len(m)) | ||
| } | ||
| return maps.Clone(m) | ||
| } |
There was a problem hiding this comment.
If these functions need to be added at all:
- Please use a generic function instead of duplicating it
- Name and document the
nilbehavior clearly, so that callers see whymaps.Cloneis not being used.createNonNilMapCloningis a horrible name, there must be a better option – but something expressing that kind of thing at the call site.
Honny1
force-pushed
the
fix-clone-copy
branch
2 times, most recently
from
ded8ccb to
36e0efb
Compare
|
@mtrmac |
mtrmac
left a comment
mtrmac
left a comment
There was a problem hiding this comment.
Thanks!
When creating separate commits for better understanding, that only works when the commits truly contain separate ideas, and precisely those.
Right now:
- The first commit says “fix” without details, but it combines an actual bug fix fix (for slice copies) with a no-change refactor to create
copyMap; and a behavior change where thecopyStringInterfaceMapis newly called, it now allocatesFlagswhere that was not allocated before. That’s three different things, and it’s not immediately obvious. (It might make sense to have a single commit like that, saying “move to new nil-ness rules”, and documenting the 2/3 different rules. But users should not need to decipher that. I think splitting commits differently is easier for everyone.)- (BTW the documentation for
copyMap, at that point, is not correct for non-nilempty maps.)
- (BTW the documentation for
- The second commit to remove
copyStringInterfaceMaplooks like a refactor on a cursory glance, but it actually changes behavior onnil. (That can be fine, but it would be better to document — except …) - … The third commit then changes the behavior of
copyMapagain on non-nilempty maps.
For the Flags field when cloning an object, that’s a third behavior change in a row, partially undoing the change from the second commit. That’s confusing to follow.
So I think some more restructuring (e.g. using git rebase -i / git add --patch) would be useful.
If we name the behaviors copyMapPreferringNil and copyMapNeverNil[1] (and maps.Clone in the middle), some of the create functions want …NeverNil; the object clone functions don’t want …NeverNil, and I don’t feel strongly between …PreferringNil and maps.Clone there (making fewer allocations vs. making a more faithful clone).
([1] Those are not really good names, but I need names of the behaviors just for this discussion.)
What do we actually want to achieve here? I think that’s:
- Make c/common tests pass — could be also done in c/common
- Avoid the unwanted memory allocations
- We have learned something (about bugs I write — perhaps I’m unique, perhaps not). How do we avoid that in the future, by making bugs either impossible, or more visible?
To get only the first two, we can:
- One commit: Revert the
slices.Clonepart, no other code changes - Optionally, one commit: Introduce
copyMap[PreferringNil], use it where appropriate, change nothing else - (Not touch the
copyStringInterfaceMapfunction or its remaining calls.)
To get all three, I think it would make sense to fix the codebase so that we don’t have copy* functions with different nil preferences and nothing obvious in the name.
- Introduce
copySlicePreferringNil(separate commit, or integrated into one of the following) - One or more commits: refactor users of
copyStringSlice/ … several others to callcopySlicePreferringNilinstead, no change to behavior - One commit: Use
copySlicePreferringNilin the object copy functions — behavior change - Introduce
copyMapNeverNil/newMapFrom, refactor various users ofcopy*Mapto call it instead, no change in behavior. - One commit: Introduce
copyMapPreferringNil, use it where appropriate — behavior change - Optionally, a commit that adds comments to
maps.Clone/slices.Cloneabout theirnilbehavior — probably overkill
I think it’s perfectly fine to have a smaller PR that does only the first two; in that case I’ll probably want to work on the other renames / refactors just to protect myself from doing the same mistake, but that’s my personal preference, not something that should block fixing the c/common tests.
store.go
Outdated
| } | ||
|
|
||
| // copyMap returns a shallow clone of map m. | ||
| // If m is empty, an nil is returned. |
There was a problem hiding this comment.
Something like
| // If m is empty, an nil is returned. | |
| // If m is empty, a nil is returned. | |
| // | |
| // (As of, e.g., Go 1.23, maps.Clone preserves nil, but that’s not a documented promise; | |
| // and this function turns even non-nil empty maps into nil.) |
?
I’d ideally prefer a more explicit function name for the behavior, but I can’t think of a good one.
containers.go
Outdated
| BigDataDigests: make(map[string]digest.Digest), | ||
| Created: time.Now().UTC(), | ||
| Flags: copyStringInterfaceMap(options.Flags), | ||
| Flags: copyMap(options.Flags), |
There was a problem hiding this comment.
The code below requires Flags to not be nil.
because it does not return nil when the slice length is 0. This behavior caused the slices.Clone function to allocate a unnecessary amount of memory when the slice length is 0, and the c/common tests failed. Signed-off-by: Jan Rodák <[email protected]>
|
@mtrmac I did revert the |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Honny1, rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
This PR fixes the changed behavior in #2087. The
slices.Clonefunction does not clone an empty slice as nil, breaking the tests in containers/common#2182. Also themaps.Clonefunction clones a nil map as nil. Refactoring of map copying and functions that copy maps will be done by another PR.