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

proposal: maps: add a SafeInsert function for the common case of checking a map for existence before inserting #70707

Closed
cjwagner opened this issue Dec 6, 2024 · 6 comments
Labels
Milestone

Comments

@cjwagner
Copy link

cjwagner commented Dec 6, 2024

Proposal Details

When inserting into a map m that may be nil the following kind of snippet is needed:

if m == nil {
    m := make(map[string]string)
}
m[key] = value

It would be nice if the maps package included a generic function to simplify this to one line.

I'd imagine there is a better name for this sort of function, but here is what the implementation could be:

// SafeInsert first creates the map if it is nil, then inserts 'v' into the map at key 'k'.
func SafeInsert[Map ~map[K]V, K comparable, V any](m *Map, k K, v V) {
    if *m == nil {
        *m = Map{}
    }
    (*m)[k] = v
}

This could then be used like:

maps.SafeInsert(&m, key, value)

https://go.dev/play/p/sMRAojtakEu

@gopherbot gopherbot added this to the Proposal milestone Dec 6, 2024
@gabyhelp
Copy link

gabyhelp commented Dec 6, 2024

Related Issues

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Dec 6, 2024
@ianlancetaylor ianlancetaylor changed the title proposal: src/maps: Add a SafeInsert function for the common case of checking a map for existence before inserting. proposal: maps: add a SafeInsert function for the common case of checking a map for existence before inserting Dec 6, 2024
@Jorropo
Copy link
Member

Jorropo commented Dec 6, 2024

Not even discussing if this is a problem that needs a fix.
I don't even think this signature allows to do what you say you want it to do. It "solves" a nil deref panic by adding an other one.
If anything copying append's signature makes more sense to me:

// SafeInsert first creates the map if it is nil, then inserts 'v' into the map at key 'k'.
func SafeInsert[Map ~map[K]V, K comparable, V any](m Map, k K, v V) Map

@DeedleFake
Copy link

No, the original suggestion would work.

var m map[string]int // &m will be a non-nil pointer to a nil map.
maps.SafeInsert(&m, "example", 3)

I do think that the append()-style one is better, though.

@jimmyfrasche
Copy link
Member

jimmyfrasche commented Dec 6, 2024

I get why this would be bundled with insert but I think that would look odd when there are multiple inserts as only the first would be the Safe variety. Either version could just ensure a non-nil map:

func X[Map ~map[K]V, K comparable, V any](m *Map) {
    if *m == nil {
        *m = Map{}
    }
}
// or
func X[Map ~map[K]V, K comparable, V any](m Map) Map {
    if m == nil {
        return Map{}
    }
    return m
}

though with #12854 that line of code is just

if m == nil {
	m = {}
}

and if we also get universal zero-compariability and cmp.Or is extended to any type it could be m = cmp.Or(m, {}) (edit: probably don't want to allocate a new map eagerly like that!)

@Jorropo
Copy link
Member

Jorropo commented Dec 6, 2024

No, the original suggestion would work.

I agree but then you have to make sure you give it a non-nil *Map pointer otherwise it panics which seems weird to me because the goal is to solve this bug.

@cjwagner
Copy link
Author

cjwagner commented Dec 6, 2024

No, the original suggestion would work.

I agree but then you have to make sure you give it a non-nil *Map pointer otherwise it panics which seems weird to me because the goal is to solve this bug.

I figured that map pointer variables are pretty rare and unusual so callers are unlikely to make the mistake of passing in a nil pointer and will instead use '&myMap' which will never be nil. "rare and unusual" is not "always" though 😄

The append style does seem better to me since it returns a valid value rather than requiring a non-nil pointer. I'll close this. Thanks for the discussion.

@cjwagner cjwagner closed this as completed Dec 6, 2024
@cjwagner cjwagner closed this as not planned Won't fix, can't repro, duplicate, stale Dec 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants