Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reflect: add Value.Clear #55002

Closed
Merovius opened this issue Sep 11, 2022 · 45 comments
Closed

reflect: add Value.Clear #55002

Merovius opened this issue Sep 11, 2022 · 45 comments
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Proposal Proposal-Accepted
Milestone

Comments

@Merovius
Copy link
Contributor

Merovius commented Sep 11, 2022

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.

@gopherbot gopherbot added this to the Proposal milestone Sep 11, 2022
@mvdan
Copy link
Member

mvdan commented Sep 12, 2022

cc @dsnet

@dsnet
Copy link
Member

dsnet commented Sep 12, 2022

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
Copy link
Contributor Author

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

@ianlancetaylor
Copy link
Contributor

Suddenly I like #20660.

@rsc
Copy link
Contributor

rsc commented Sep 28, 2022

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
Copy link
Contributor

rsc commented Sep 28, 2022

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

@go101

This comment was marked as off-topic.

@ianlancetaylor

This comment was marked as off-topic.

@go101

This comment was marked as off-topic.

@Merovius

This comment was marked as off-topic.

@go101

This comment was marked as off-topic.

@Merovius
Copy link
Contributor Author

@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
Copy link
Contributor

rsc commented Oct 5, 2022

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 dsnet closed this as completed Oct 5, 2022
@dsnet dsnet reopened this Oct 5, 2022
@dsnet
Copy link
Member

dsnet commented Oct 5, 2022

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
Copy link
Member

bcmills commented Oct 5, 2022

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
Copy link
Contributor

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
Copy link
Member

bcmills commented Oct 6, 2022

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
Copy link

atdiar commented Oct 6, 2022

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
Copy link
Contributor Author

Merovius commented Oct 6, 2022

@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
Copy link
Member

mvdan commented Oct 6, 2022

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
Copy link

atdiar commented Oct 6, 2022

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
Copy link
Member

bcmills commented Oct 6, 2022

@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
Copy link
Contributor Author

Merovius commented Oct 6, 2022

@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

@Merovius
Copy link
Contributor Author

Merovius commented Oct 6, 2022

@atdiar

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?

No, it can still be accessed using range.

@bcmills
Copy link
Member

bcmills commented Oct 6, 2022

@Merovius

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

It is always easy: if k != k.

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.

That may be true, but if a caller puts an irreflexive key in a map they're almost certainly going to have trouble with way more than just your library. (How many other Go libraries assume that they can range over a map and look up / delete its values by key? I would bet it's a significant fraction of the ecosystem.)

So I don't think users of your library would lose much, if at all, by having the additional “keys must be reflexive” restriction.

@atdiar
Copy link

atdiar commented Oct 9, 2022

@atdiar

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?

No, it can still be accessed using range.

Maybe "dangling" was a poor choice of a word.
What I meant is the below
https://go.dev/play/p/rJ3UucF1Zlx

@seankhliao
Copy link
Member

(3) would essentially be #45328 which was declined last year

@Merovius
Copy link
Contributor Author

Not just essentially, exactly that mechanism with exactly that syntax was suggested. Interesting.

Obviously, delete(m) would fit my use-case and I'd be fine with it, personally.

@mvdan
Copy link
Member

mvdan commented Oct 12, 2022

I think it's fine given that the NaN keys and wanting a reflect API are both factors which weren't properly considered in the previous proposal.

@rsc in your option 3, would we end up with one or two new reflect APIs? My intuition is that "clear the map except NaN keys" is not worthwhile.

@atdiar
Copy link

atdiar commented Oct 12, 2022

If NaN containing keys are always a mistake, then perhaps that (2) is sufficient and the actual issue is to prevent these NaN containing keys in the first place. Option 3 could actually mask the mistake.

If there are legitimate cases for these NaN containing keys, then (3) should be fine.

I am more inclined toward option 2.

@bcmills
Copy link
Member

bcmills commented Oct 12, 2022

If NaN containing keys are always a mistake, then perhaps that (2) is sufficient and the actual issue is to prevent these NaN containing keys in the first place.

That is pretty much exactly #20660, and I think it's also a good option. (But I may be biased because I filed it. 😅)

@bcmills
Copy link
Member

bcmills commented Oct 12, 2022

  1. Make maps.Clear and reflect.Value.MapClear preserve NaN-containing keys to match the language.

    Bryan and Keith dislike (1) but would be OK with (2)

Just to be clear, I have a strong preference against that particular variation on option (2).

Instead, I would prefer:
2a. Make maps.Clear and reflect.Value.MapClear panic if they are unable to clear the map.

But I think option (3) is probably fine.

@rsc
Copy link
Contributor

rsc commented Oct 20, 2022

#45328 was declined in part because we said the loop was just as good. But now we've realized that the loop does not handle everything, which is why we're back to a language change.

@rsc
Copy link
Contributor

rsc commented Oct 20, 2022

Created #56351 for the language change. If that goes through, we'd add reflect.(*Value).MapClear to do the same thing (clear the map entirely, even NaNs). We wouldn't need maps.Clear(m) because delete(m) already does it.

@rsc
Copy link
Contributor

rsc commented Nov 30, 2022

#56351 is accepted (soon), so we should revive this. Because clear(x) applies to slices and maps, we should call this Value.Clear and then it applies to slices or maps and otherwise panics.

@rsc rsc changed the title proposal: reflect: add (*Value).MapClear proposal: reflect: add Value.Clear Nov 30, 2022
@rsc
Copy link
Contributor

rsc commented Dec 7, 2022

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Dec 14, 2022

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: reflect: add Value.Clear reflect: add Value.Clear Dec 14, 2022
@rsc rsc modified the milestones: Proposal, Backlog Dec 14, 2022
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Dec 14, 2022
@prattmic prattmic modified the milestones: Backlog, Go1.21 Dec 14, 2022
@gopherbot
Copy link

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

@cuonglm cuonglm self-assigned this Jan 18, 2023
@cuonglm
Copy link
Member

cuonglm commented Jan 30, 2023

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

@cuonglm cuonglm reopened this Jan 30, 2023
@cuonglm
Copy link
Member

cuonglm commented Feb 1, 2023

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

@cuonglm cuonglm closed this as completed Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Proposal Proposal-Accepted
Projects
Status: Done
Status: Accepted
Development

No branches or pull requests