-
Notifications
You must be signed in to change notification settings - Fork 17.6k
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
go/types: add iterator methods #66626
Comments
Change https://go.dev/cl/575455 mentions this issue: |
I would have expected an API like this: func (t *Interface) Methods() iter.Seq[*Func]
func (t *Interface) ExplicitMethods() iter.Seq[*Func]
func (t *Interface) EmbeddedTypes() iter.Seq[Type]
func (t *Named) Methods() iter.Seq[*Func]
func (s *Scope) Children() iter.Seq[*Scope]
func (s *Struct) Fields() iter.Seq[*Var]
func (t *Tuple) Variables() iter.Seq[*Var]
func (s *MethodSet) Methods() iter.Seq[*Selection]
func (u *Union) Terms() iter.Seq[*Term]
func (l *TypeParamList) TypeParams() iter.Seq[*TypeParam]
func (l *TypeList) Types() iter.Seq[Type] |
That works, but leads to unnecessary eta abstraction: the implementation of the method has to create a lambda abstraction that the caller (range statement) immediately reduces. It's more syntax for both parties, and less efficient. Recall that in Go, the method value |
@adonovan it seems like the I personally would also prefer the simpler pattern proposed here, avoiding the unnecessary closure. But we should be consistent in the standard library. |
The performance difference is negligible. (Apologies for my short-lived earlier draft containing an obvious major blunder!)
package iter_test
import "testing"
type T struct{}
func (T) A() func(yield func(elem int) bool) {
return func(yield func(elem int) bool) {
_ = yield(1) && yield(2)
}
}
func (T) B(yield func(elem int) bool) {
_ = yield(1) && yield(2)
}
func BenchmarkA(b *testing.B) {
sum := 0
for range b.N {
for i := range new(T).A() {
sum += i
}
}
if sum == -1 {
println(sum)
}
}
func BenchmarkB(b *testing.B) {
sum := 0
for range b.N {
for i := range new(T).B {
sum += i
}
}
if sum == -1 {
println(sum)
}
} |
These should be written to take no arguments and return an iter.Seq[...] but otherwise looks good. |
This change defines two helpers, Elements(seq) and Entries(mapping), that adapt the Iterable and IterableMapping interfaces to one- and two-variable go1.23 range loops. The new syntax is more concise and frees the caller from worrying about Iterator.Done. We do not yet update any calls to use them, so that the project can continue to be build with pre-go1.23 releases of Go. Also, define Elements methods on {*List,Tuple,*Set} and an Entries method on *Dict. These optimized iterators (which can fetch both k and v in one go) will be used by the Elements and Entries standalone functions when the operand supports it. (User-defined types can implement these methods too, although the interface isn't currently documented.) Also, a go1.23-only test of the new iteration.
OK, https://go.dev/cl/575455 has been updated to reflect that.
|
This proposal has been added to the active column of the proposals project |
According to golang/go#66626 (comment), the convention for iterator methods is that they should return an iterator, not be an iterator. The caller thus uses this form: for _ elem := range v.Elements() { ... } not this one: for _ elem := range v.Elements { ... } This is unfortunately a breaking API change, but since go1.23 is not yet released, no-one should be using the new range syntax yet.
According to golang/go#66626 (comment), the convention for iterator methods is that they should return an iterator, not be an iterator. The caller thus uses this form: for _ elem := range v.Elements() { ... } not this one: for _ elem := range v.Elements { ... } This is unfortunately a breaking API change, but since go1.23 is not yet released, no-one should be using the new range syntax yet.
The API tool should record those as returning iter.Seq[foo] not func(func(foo)bool). |
Have all remaining concerns about this proposal been addressed? The proposal is to add the API listed in #66626 (comment), but using iter.Seq instead of explicit func types. |
Ah, I remember now: I used the underlying type of (I don't think there's any problem with the API tool.) |
Based on the discussion above, this proposal seems like a likely accept. The proposal is to add the API listed in #66626 (comment), but using iter.Seq instead of explicit func types. |
No change in consensus, so accepted. 🎉 The proposal is to add the API listed in #66626 (comment), but using iter.Seq instead of explicit func types. |
Proposal Details
There are many data types in the API of go/types that expose a sequence through a pair of methods
NumFoo() int
andFoo(int) Foo
. For each of them, we propose to define new convenience methods that are go1.23 push iterators:Fortunately, the most natural name (usually the plural) was available in all cases.
@findleyr @gri @mdempsky
The text was updated successfully, but these errors were encountered: