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: spec: range over nil function values should not panic #65629

Closed
tamayika opened this issue Feb 9, 2024 · 9 comments
Closed

proposal: spec: range over nil function values should not panic #65629

tamayika opened this issue Feb 9, 2024 · 9 comments
Labels
Milestone

Comments

@tamayika
Copy link

tamayika commented Feb 9, 2024

Proposal Details

Environment: Go 1.22 with GOEXPERIMENT=rangefunc

Currently, we can iterate nil slice and nil map with for range and nil channel blocks forever.
But, nil iter.Seq[T] panics.
I think it's better if we can iterate nil sequence like nil slice.
Also, I can't find this is desinged or not at https://go.dev/wiki/RangefuncExperiment.

func main() {
	var (
		slice []int
		m     map[string]int
		ch    chan int
		iter  iter.Seq[int]
	)
	// ok
	for range slice {

	}
	// ok
	for range m {

	}
	// block forever
	for range ch {

	}
	// panic
	for _ = range iter {

	}
}
@gopherbot gopherbot added this to the Proposal milestone Feb 9, 2024
@earthboundkid
Copy link
Contributor

Seems more logically consistent and it makes it easier to return a blank iterator in different spots. You might do var seq iter.Seq[int]; if cond { seq = something() }; for n := range seq { /**/ } like you do with channels. This saves you from needing to write a dummy iterator or put one in the iter package.

@timothy-king timothy-king changed the title proposal: range over nil iter.Seq[T] should not panic proposal: range over nil function values should not panic Feb 9, 2024
@timothy-king
Copy link
Contributor

Personally I am somewhat torn on this one. nil maps and slices have consistent meanings in different operations, an empty map and an empty slice. Other operations make sense on them, len(s) == 0, or m["key"]==false. A nil function value f panics when called. The other operation is comparison to nil. This proposal is effectively saying 'nil function values [with such and such a signature] are empty sequences'. That is definitely more forgiving and has its uses. It is also relatively easy to adjust the implementation.

Ranging over a nil pointer to array value does panic if two iteration variables are present (and len(array)>=1). So there is some precedent for panicking. It would arguably be more confusing for it to be considered an empty sequence. I think the mental model being put forward is for for range f { ...} to be f(func(int)bool{ ... }). That would panic when f is nil.

I think both views are somewhat reasonable.

@AndrewHarrisSPU
Copy link

AndrewHarrisSPU commented Feb 10, 2024

ISTM there would be expected behaviors for the most referentially transparent iter.Seq generated from a nil slice/map/channel. The behavior would be different between finite slices and maps, and arbitrarily long / infinite channels. With finite sequences, terminating the loop seems obvious, and blocking would be perplexing. I wonder if terminating when one really expects an infinite sequence is equally perplexing - the root cause of resulting problems might not be as immediate as the consequences at run time.

If iter.Seq generalizes finite and infinite sequences and complications in-between, a nil panic isn't pleasant but isn't subtle either.

@Jorropo
Copy link
Member

Jorropo commented Feb 11, 2024

As a side effect if we change this that means any manual handling of the iterator will need to manually handle both.
Currently you can write:

func Concat[V any](iterators ...iter.Seq[V]) iter.Seq[V] {
 return func(yield func(V) bool) bool {
  for _, i := range iterators {
   if !i(yield) {
    return false
   }
  }
  return true
 }
}

vs:

 func Concat[V any](iterators ...iter.Seq[V]) iter.Seq[V] {
  return func(yield func(V) bool) bool {
   for _, i := range iterators {
+   if i == nil {
+    continue
+   }
+
    if !i(yield) {
     return false
    }
   }
   return true
  }
 }

@rsc rsc changed the title proposal: range over nil function values should not panic proposal: spec: range over nil function values should not panic Mar 1, 2024
@rsc
Copy link
Contributor

rsc commented Mar 1, 2024

Iterators are functions; calling nil functions panics. If a direct call and a use in range behaved differently, then that would be a gotcha when thinking about having to convert code from one form to the other.

Nil iterators are a mistake and should not be used, same as nil pointers.

@rsc
Copy link
Contributor

rsc commented Mar 1, 2024

Some ranges do panic by the way: https://go.dev/play/p/B_tw9VJ11ZU

@rsc
Copy link
Contributor

rsc commented Mar 1, 2024

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

@rsc
Copy link
Contributor

rsc commented Mar 8, 2024

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

@rsc
Copy link
Contributor

rsc commented Mar 15, 2024

No change in consensus, so declined.
— rsc for the proposal review group

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Declined
Development

No branches or pull requests

7 participants