reflect: add Value.Clear

[Merovius]

I propose adding a new method MapClear to reflect.Value, which efficiently deletes all elements from a map.

I am working on a decoder for a serialization format. If I want decoding into a non-nil map to leave it with only the serialized elements, I currently have two choices: 1. Allocate a new map, or 2. Clear all keys.

Clearing the keys has the advantage that it re-uses existing storage, if any, so it is lighter on allocations. But it takes more time to iterate over the map and clear keys. It also isn't correct, strictly speaking, as it doesn't behave right with NaN-keys.

It would be preferable to have a way to efficiently clear the map, preserving the already allocated buckets. This isn't an operation available in the Go language, so arguably it does not belong in reflect either. But we have #20138 to at least make the iteration-and-deletion idiom transparently efficient in plain Go.

[mvdan]

cc @dsnet

[dsnet]

Interesting observation about NaN. It does mean that the documentation on maps.Clear is somewhat lying:

// Clear removes all entries from m, leaving it empty.
func Clear[M ~map[K]V, K comparable, V any](m M)

since it doesn't always leave it empty.

\cc @ianlancetaylor

[Merovius]

FWIW that observation was made by @randall77 here, I'm not taking credit.

[ianlancetaylor]

Suddenly I like #20660.

[rsc]

It's definitely a little odd to add something to reflect that you can't do in the language proper. Seems like we should fix that somehow first.

[rsc]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

[Merovius]

@rsc For my purposes, I'd also be okay with having MapClear() not remove NaN keys. That is, for my purposes, I'd be fine having to do

v.MapClear()
if v.Len() > 0 { // there where some NaNs in that map
    v.Set(reflect.New(v.Type()).Elem())
}

That way, MapClear wouldn't allow anything the language doesn't allow. But I agree that's kind of a strange API. I'd prefer to find a way to address this NaN-key thing in the language proper as well.

[rsc]

There are never any good answers involving NaNs.
Perhaps we should make maps.Clear do an actual clear, including NaNs (with runtime help as needed)
and then reflect.MapClear can also do an actual clear, including NaNs.
Any objections to changing both?

[dsnet]

Personally, I would like something like that. However, this does open the door to other functionality in maps that might hook into the runtime. Where do we draw the line?

For example, #54454 proposes maps.Shrink which would need runtime support to work.

[bcmills]

Where do we draw the line?

An arbitrary suggestion: maps shouldn't do anything that user code couldn't already do using reflect, but it can maybe use runtime hooks to cheat at optimization.

Then adding reflect.MapClear would pave the way for maps.Clear, because it would provide a stable, supported way for user code to provide the same semantics (if a little slower).

[randall77]

An arbitrary suggestion: maps shouldn't do anything that user code couldn't already do using reflect,...

That's kind of just kicking the can down the road one step. I would argue that reflect shouldn't be able to do anything that can't be done in the language proper, which argues against reflect.MapClear being able to clear NaN keys.

I'm ok with maps.Clear and reflect.MapClear existing but not clearing NaN keys. We just need to document that and explain why (basically, because delete(m, k) doesn't do anything for NaN keys).

[bcmills]

One possible halfway point would be to have maps.Clear panic if it is unable to actually clear the map. That wouldn't go quite as far as preventing them from being put in the map in the first place, but would at least fail in a way that is easy to fuzz and diagnose (as opposed to silently leaving entries in an allegedly-cleared map).

[atdiar]

A bit semantically strange, the existence of map.Clear that wouldn't actually clear the map.
For now that operation doesn't really exist (in the std lib) so everything is still semantically consistent if perhaps confusing.

If there was such an operation, I would expect it to work on clearable maps, i.e. where map keys are proven to have reflexive comparison.

Otheriwse that could easily leak into Set types for example, especially when computing cardinality. (sets of floats or floats composites).

But then if it requires modifying map.Clear from an x package, how many programs it would break is another question too perhaps.

[edit] to be fair, I'm not sure that the true semantics of map clearing have to rely on reflexive comparability. In that case, assistance from the runtime would be relevant. But then there is still a similar issue for user-defined datastructures.

Leaving the NaNs and related in could also leak memory no?

[Merovius]

@bcmills to me, that feels like the kind of perfect compromise in that it leaves everyone unhappy. Especially as it doesn't seem possible to check if maps.Clear would panic, without iterating over it and finding a NaN key (and avoiding the O(N) iteration cost is why I want to use it).

[mvdan]

I find the nuance in this discussion quite interesting, but I thought I'd point out that I have never in eight years of writing Go used map[float]T nor NaN. I still think we want the language, reflect, and maps to be consistent, but we should probably bear in mind that NaN in map keys probably affects a tiny minority of Go users.

[atdiar]

Me neither, but if the issue appears, it might appear for any type which is a composite of floats values. Not just directly float32 or float64.

What happens if a pointer value is stored for a struct key where one field is a NaN float64 for instance? Isn't it now dangling?

[bcmills]

@Merovius, while it's true that one couldn't always check for the panic locally immediately before making the call to Clear, it is still straightforward to avoid adding irreflexive keys to the map in the first place by checking k == k at the insertion site.

[Merovius]

@bcmills I don't like that. As a library author using maps.Clear/reflect.(*Value).MapClear that would require me to document that maps passed to me can't contain NaN keys. But that limitation is not inherent to my use-case - it would be less efficient, but semantically completely fine to just allocate a new map. I guess I can use recover - but I don't think "you have to think about recover and fall back, if clearing panics" makes for safer code than "you have to check if len > 0 after clearing and fall back, if there are NaNs".

Not to mention that checking if an inserted value has this problem isn't always easy.

@mvdan I tend to agree. I feel that right now, this issue - at least as far as NaN is concerned - is simply blocked on #20660 - i.e. "somehow decide what to do about NaN in map keys":

• If we could convince ourselves to disallow inserting them, it would no longer be in doubt how maps.Clear or reflect.(*Value).MapClear would handle the situation - it can't happen.
• If we decided that NaN-keys are allowed but (counter to the spec) compare as equal in that context, both maps.Clear and reflect.(*Value).Clear would have an obvious behavior.
• If we decide that the current behavior of NaN-keys is what we want, well, then maps.Clear and reflect.(*Value).Clear might just not have a good definition in the presence of NaN-keys

[gopherbot]

Change https://go.dev/cl/457895 mentions this issue: reflect,runtime: add Value.Clear

[cuonglm]

Ops, the issue is not done yet, need to wait for compiler implementation.

[cuonglm]

Ah, this proposal is for reflect package, so should be closed after https://go.dev/cl/457895 merged.

[gopherbot]

Change https://go.dev/cl/481935 mentions this issue: runtime: mark map bucket slots as empty during map clear

[gopherbot]

Change https://go.dev/cl/498757 mentions this issue: doc/go1.21: mention changes to the reflect package