Support 1.23 iterator for set

[amikai]

Hi @deckarep, I want to contribute the PR for supporting 1.23 iterator. I don't know if you are interested.

Background

The Iterator implemented in this package returns a channel to support range syntax. Therefore, it requires calling Stop to close the underlying channel. With the 1.23 iterator, we can use the for-range syntax without needing to call Stop. See the prototype implementation in issue #140.

My original thought was to use for-range on the Each method as follows. However, the logic is reversed. In the Each(func(t) bool) method of the set interface, if the passed function returns true, the iteration stops. According to the 1.23 SPEC, if the loop body for the range function terminates, the yield function will return false.

// not work as expect
mySet := mapset.NewSet[int]()
for v := range mySet.Each {
    ...
}

Proposal

Let's allow users to use the for-range loop to iterate through the set without additional operation (such as Stop) after version 1.23.

mySet := mapset.NewSet[int]()
for v := range Values(mySet) {
  ...
}

#140 is one of implementation to achieve that.
The pros of this PR: use func(func(element T) bool as iterator instead of 1.23 iter.Seq. So the go.mod no need to upgrade to 1.23. The user use 1.23 compiler can user for-loop. The user before 1.23 can use old function way.
The cons of using func(func(element T) bool) instead of iter.Seq is reduced readability.

The naming of Values follows the conventions used in the slices package slices.Values and the maps package maps.Values.

[deckarep]

Hello @amikai - this looks great.

• Since Go did not have language supported, built-in iterators all of my solutions were less than ideal.
• I agree it would be nice to use the Each function already provided but I understand that the logic is reversed.
• The naming convention of Values looks good to be in line with the stdlib.

I see no reason to not implement this and it looks like great support. I'd like to leave this PR open a little longer in case others watching the project have input.

[adambaratz]

My only note would be that the method should be called All because sets only contain values: https://groups.google.com/g/golang-nuts/c/le8CBGTK9-E/m/ieAF0llbAAAJ

Otherwise, sounds great! I just came here to see if there was support yet, so glad to see others are thinking about this.

[adambaratz]

Actually, sorry, misread the original note here. I'm wondering whether it would be better to add an All method instead of having this as a package function. for v := range mySet.All() looks a little nicer, to my eyes anyway, than for v:= range mapset.All(mySet). I think the functions from the slices and maps packages make more sense because those types don't have methods.

[deckarep]

Even though Sets only contain Values I think it’s fine keeping the function name as Values vs All.

I like the idea of this falling in line with other collection iterators that may exist.

[adambaratz]

I was a little confused by that, too. Hence my starting that thread. But both slices and maps have keys and values, so they're different from sets (which only have values). Ian Lance Taylor is on the Go team at Google. If he says that a set should have an All method instead of a Values method, I'd take his word on it.

[deckarep]

Thanks I’m very familiar with Ian Lance Taylor, definitely a legend in the industry.

All still feels overly generic to me, I feel more inclined to use Values or even Elements. In the truest, mathematical sense a Set has elements.

This is one reason I like to leave PR’s open is getting feedback like this. Does anyone else have opinions on this? As they say naming things is hard but I prefer to be pragmatic and get it done.

[amikai]

HI @deckarep, I think Elements might be a good choice because this is a set package, and as long as people see elements, they definitely won't misunderstand.

[mitar]

I do not care about the name (it will be confusing anyway because there are already methods with "iterator" name in them), but I would love support for iterators. :-)

[amikai]

Hi @deckarep, thank you for merging PR #140. Could you please let me know if you have any plans to release a tag?

[deckarep]

@amikai - ah yes I can do a release. But, I do have a question: I can’t remember if this becomes a breaking change since it’s using new API’s for users. I suppose I could make a major release or is there a way to require a minimum version of Go on releases?

[mitar]

I think Elements function should be moved to a separate file and with build tags (similar to how they are done in test files) this could be made so that it is available only for Go 1.23 users, in backwards compatible manner. Then there is no need to release a new major version.

[amikai]

@amikai - ah yes I can do a release. But, I do have a question: I can’t remember if this becomes a breaking change since it’s using new API’s for users. I suppose I could make a major release or is there a way to require a minimum version of Go on releases?

Hi @deckarep, there is no breaking change, so I think we don’t need a major release. For users on 1.23, they can use for..range to iterate over Elements(). Before 1.23, they call the function directly in old way, see #140 test case. This can works higher than 1.18, same as now.

I think Elements function should be moved to a separate file and with build tags (similar to how they are done in test files) this could be made so that it is available only for Go 1.23 users, in backwards compatible manner. Then there is no need to release a new major version.

Hi @mitar , Elements don't need to be moved to a separate file. That's why I use func Elements[T comparable](s Set[T]) func(func(element T) bool) instead of func Elements[T comparable](s Set[T]) iter.Seq[T]. This is ensure this function works in the compiler version lower than 1.23. Note that the CI pass the test with golang compiler version from 1.20 to 1.24.

I separate the test for 1.23 (go.mod is 1.18) because I need to use 1.23 for..range iterate the Element() to make sure it works.

[deckarep]

Ok, thanks for clarifying and helping me reason through it. This release should qualify as a Minor release then and I’ll go ahead and get it cut today!

[mitar]

Interesting. Always learning something new. Thanks!

[deckarep]

@amikai - ok, new release is cut and we should see this updated shortly: https://pkg.go.dev/github.com/deckarep/golang-set/[email protected]

Closing this issue now.

Big thanks to everyone on this thread!

[mitar]

I played now a bit with iterators and this package and I noticed that one important functionality is missing: initializing mapset from an iterator. NewSet does not take an iterator as parameter, for example.