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: expression m[k] on a nil map m should generate a run-time panic instead of returning zero value #39572

Closed
ahrtr opened this issue Jun 13, 2020 · 9 comments
Labels
FrozenDueToAge LanguageChange Proposal v2 A language change or incompatible library change WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone

Comments

@ahrtr
Copy link

ahrtr commented Jun 13, 2020

What version of Go are you using (go version)?

go version go1.14.2 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"

What did you do?

I wrote a very simple program as below,

package main

import "fmt"

func main() {
	var m map[int]int = nil
	v, ok := m[3]
	fmt.Println(v, ok, m == nil)
}

The above program printed:

0 false true

What did you expect to see?

The above result is correct based on the current Golang spec. But I think it makes more sense to generate a run-time panic for expression m[k] on a nil map. It's confusing for m[k] on a nil map m to return the same values as an empty map.

@gopherbot gopherbot added this to the Proposal milestone Jun 13, 2020
@as
Copy link
Contributor

as commented Jun 13, 2020

The above result is correct based on the golang spec.

What part of the spec are you referring to?

https://golang.org/ref/spec#Map_types

The initial capacity does not bound its size: maps grow to accommodate the number of items stored in them, with the exception of nil maps. A nil map is equivalent to an empty map except that no elements may be added.

@davecheney
Copy link
Contributor

My understanding is that nil in golang is similar to null in java or c++, so it would be better to have similar behavior.

A nil map is specifically not like the behaviour you would find in C++ or java and this is for a good reason -- it eliminates the class of defensive programming that library users must employ.

@martisch martisch added LanguageChange v2 A language change or incompatible library change labels Jun 13, 2020
@martisch
Copy link
Contributor

martisch commented Jun 13, 2020

This will be a backwards incompatible change as it will cause existing programs that run fine (according to the spec) to panic at runtime.

nil values in go are useable e.g. a nil slice is as useable as a slice with capacity of zero. nil maps and slices are useful for e.g. performance as they allow to have values of the type that avoid allocations. the current behaviour for nil values in go seems consistent (e.g. nil slices can be copied and appended to without panics too) for go to me. Both nil maps and slices can be ranged over without panics. making nil maps special will cause churn for existing code bases, loss of performance (causing unneeded allocations to maps with no elements) and introduce an inconsistency in the usability of nil values of different types.

@mvdan
Copy link
Member

mvdan commented Jun 13, 2020

Please note that you should fill https://github.com/golang/proposal/blob/master/go2-language-changes.md when proposing a language change.

@mvdan mvdan added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Jun 13, 2020
@ahrtr
Copy link
Author

ahrtr commented Jun 14, 2020

Thanks for all the feedbacks.

What part of the spec are you referring to?

@as Note that I was saying the code is correct based on the current Golang spec. I am proposing to change both the implementation and Golang spec.

it eliminates the class of defensive programming that library users must employ.

@davecheney what you said is partially correct. Indeed, developers do not need to check nil map or slice in a range clause, or in build-in function len. But they need to check nil or something similar in many other cases. For example, a runtime panic will occur if the parameter s is a nil slice,

func do(s []int) {
	fmt.Println(s[len(s)])
}

So developers need to add protection as below,

	if nil == s {
		fmt.Println("nil slice")
	}

or

	if len(s) == 0 {
		fmt.Println("The length is 0")
	}

This will be a backwards incompatible change as it will cause existing programs that run fine (according to the spec) to panic at runtime.

@martisch Agreed. This is a valid comment. So this proposal is for Golang 2.x instead of 1.x.

the current behaviour for nil values in go seems consistent (e.g. nil slices can be copied and appended to without panics too) for go to me

@martisch What you said is partially correct. Some current behaviors for slice for nil values are consistent with map, but not all. For example,
1. As you mentioned, developers can append values into a nil slice, but can't append entries(key/value pair) to a nil map.
2. m[k] is valid for a nil map, but s[index] isn't valid for a nil slice.

Please note that you should fill https://github.com/golang/proposal/blob/master/go2-language-changes.md when proposing a language change.

@mvdan Thanks for the info. This is my first time to raise proposal for Golang, and I'd like to get some feedbacks here before filling the template. Is that OK?

@davecheney
Copy link
Contributor

davecheney commented Jun 14, 2020

As you mentioned, developers can append values into a nil slice, but can't append entries(key/value pair) to a nil map.

This is because a map is a reference value, a slice is not. A nil slice is actually the pointer in the slice value pointing to nil. Assigning nil to a slice variable is assigning nil to the pointer to the slices backing array.

  1. m[k] is valid for a nil map, but s[index] isn't valid for a nil slice.

This is because a nil slice has a length of zero, and value of index is therefore out of bounds.

@ahrtr
Copy link
Author

ahrtr commented Jun 14, 2020

@davecheney From Golang's internal implementation, I agree with what you said. But I am proposing a Golang spec/API change, so it would be better to think about it from the users' perspective. It's confusing to me for m[k] on a nil map m to return the same values as an empty map.

@ahrtr ahrtr changed the title proposal: read on a nil map should generate a run-time panic instead of returning zero value proposal: expression m[k] on a nil map m should generate a run-time panic instead of returning zero value Jun 14, 2020
@martisch
Copy link
Contributor

martisch commented Jun 14, 2020

@martisch What you said is partially correct. Some current behaviors for slice for nil values are consistent with map, but not all.

Yes there is a difference in direct element access as you point out but I think that is inherent in what a map is in Go instead of an oversight. The map has a comma ok form to check if an element is really there while slices have not.

I think making m[k] panic will remove more consistency than it adds. Now a nil map and an empty map behave differently when reading elements from them which means one has to check if the map is initialized more often than before. Usually coders should not care about that similar to how differentiating between nil slices and 0 len slices is discouraged (at least in code bases I often cotribute to).

@ahrtr
Copy link
Author

ahrtr commented Jun 14, 2020

@martisch Thanks for the feedback, which makes sense, although I still have a minor comment. Developers have to differentiate a nil map and an empty map, because adding entries into a nil map will cause a panic, while it's ok for an empty map. So it should be OK to differentiate a nil map and an empty map when reading elements as well, but it isn't a big deal.

Actually just as @davecheney pointed out, failure of reading from a nil slice is due to out of range instead of nil slice. So it's consistent to support reading a nil map.

So I will close this ticket.

@ahrtr ahrtr closed this as completed Jun 14, 2020
@golang golang locked and limited conversation to collaborators Jun 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge LanguageChange Proposal v2 A language change or incompatible library change WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

6 participants