[go] reflect: add iterator equivalents for NumField, NumIn, NumOut and NumMethod
Gerrit Bot (Gerrit)
Gerrit Bot has uploaded the change for review![]( "Open in Gerrit")
Commit message
reflect: add iterator equivalents for NumField, NumIn, NumOut and NumMethod
The new methods are Type.Fields, Type.Methods, Type.Ins, Type.Outs, Value.Fields and Value.Methods.
Fixes #66631
Change diff
diff --git a/src/reflect/type.go b/src/reflect/type.go
index fc6edb1..ccc1f3d 100644
--- a/src/reflect/type.go
+++ b/src/reflect/type.go
@@ -18,6 +18,7 @@
import (
"internal/abi"
"internal/goarch"
+ "iter"
"runtime"
"strconv"
"sync"
@@ -255,6 +256,20 @@
// CanSeq2 reports whether a [Value] with this type can be iterated over using [Value.Seq2].
CanSeq2() bool
+ // Fields yields each field of a struct type.
+ // It panics if the type's Kind is not Struct.
+ Fields() iter.Seq[StructField]
+ // Methods yields each method in the type's method set.
+ // See [Type.Method] for information on the yielded methods.
+ Methods() iter.Seq[Method]
+
+ // Ins yields each input parameter of a function type, in order.
+ // It panics if the type's Kind is not Func.
+ Ins() iter.Seq[Type]
+ // Outs yields each output parameter of a function type, in order.
+ // It panics if the type's Kind is not Func.
+ Outs() iter.Seq[Type]
+
common() *abi.Type
uncommon() *uncommonType
}
@@ -934,6 +949,46 @@
return yield.InCount == 2 && yield.OutCount == 1 && yield.Out(0).Kind() == abi.Bool
}
+func (t *rtype) Fields() iter.Seq[StructField] {
+ return func(yield func(StructField) bool) {
+ for i := range t.NumField() {
+ if !yield(t.Field(i)) {
+ return
+ }
+ }
+ }
+}
+
+func (t *rtype) Methods() iter.Seq[Method] {
+ return func(yield func(Method) bool) {
+ for i := range t.NumMethod() {
+ if !yield(t.Method(i)) {
+ return
+ }
+ }
+ }
+}
+
+func (t *rtype) Ins() iter.Seq[Type] {
+ return func(yield func(Type) bool) {
+ for i := range t.NumIn() {
+ if !yield(t.In(i)) {
+ return
+ }
+ }
+ }
+}
+
+func (t *rtype) Outs() iter.Seq[Type] {
+ return func(yield func(Type) bool) {
+ for i := range t.NumOut() {
+ if !yield(t.Out(i)) {
+ return
+ }
+ }
+ }
+}
+
// add returns p+x.
//
// The whySafe string is ignored, so that the function still inlines
diff --git a/src/reflect/value.go b/src/reflect/value.go
index c0ac45de..1012dcc 100644
--- a/src/reflect/value.go
+++ b/src/reflect/value.go
@@ -10,6 +10,7 @@
"internal/goarch"
"internal/itoa"
"internal/unsafeheader"
+ "iter"
"math"
"runtime"
"unsafe"
@@ -2620,6 +2621,47 @@
panic(&ValueError{"reflect.Value.UnsafePointer", v.kind()})
}
+// Fields yields each field of v, along with its description.
+//
+// It panics if v's Kind is not Struct.
+//
+// The i'th field yielded by Fields is the same as [Type.Field(i)] and [Value.Field(i)].
+func (v Value) Fields() iter.Seq2[StructField, Value] {
+ return func(yield func(StructField, Value) bool) {
+ rtype := v.Type()
+ for i := range v.NumField() {
+ if !yield(rtype.Field(i), v.Field(i)) {
+ return
+ }
+ }
+ }
+}
+
+// Methods yields a function value corresponding to each method of v,
+// along with a description of the method.
+//
+// The arguments to a Call on the yielded function value should not include a receiver;
+// the returned function will always use v as the receiver. Note that [Method.Type]
+// in each yielded [Method] does include the receiver, and thus is not the same as
+// [Value.Type()].
+//
+// Methods panics if v is a nil interface value.
+//
+// The i'th method yielded by Methods is the same as [Type.Method(i)] and [Value.Method(i)].
+//
+// Calling this method will force the linker to retain all exported methods in all packages.
+// This may make the executable binary larger but will not affect execution time.
+func (v Value) Methods() iter.Seq2[Method, Value] {
+ return func(yield func(Method, Value) bool) {
+ rtype := v.Type()
+ for i := range v.NumMethod() {
+ if !yield(rtype.Method(i), v.Method(i)) {
+ return
+ }
+ }
+ }
+}
+
// StringHeader is the runtime representation of a string.
// It cannot be used safely or portably and its representation may
// change in a later release.
Change information
- M src/reflect/type.go
- M src/reflect/value.go
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gopher Robot (Gerrit)
Gopher Robot added 1 comment![]( "Open in Gerrit")
I spotted some possible problems with your PR:
1. You have a long 99 character line in the commit message body. Please add line breaks to long lines that should be wrapped. Lines in the commit message body should be wrapped at ~76 characters unless needed for things like URLs or tables. (Note: GitHub might render long lines as soft-wrapped, so double-check in the Gerrit commit message shown above.)
Please address any problems by updating the GitHub PR.
When complete, mark this comment as 'Done' and click the [blue 'Reply' button](https://go.dev/wiki/GerritBot#i-left-a-reply-to-a-comment-in-gerrit-but-no-one-but-me-can-see-it) above. These findings are based on heuristics; if a finding does not apply, briefly reply here saying so.
To update the commit title or commit message body shown here in Gerrit, you must edit the GitHub PR title and PR description (the first comment) in the GitHub web interface using the 'Edit' button or 'Edit' menu entry there. Note: pushing a new commit to the PR will not automatically update the commit message used by Gerrit.
For more details, see:
- [how to update commit messages](https://go.dev/wiki/GerritBot/#how-does-gerritbot-determine-the-final-commit-message) for PRs imported into Gerrit.
- the Go project's [conventions for commit messages](https://go.dev/doc/contribute#commit_messages) that you should follow.
(In general for Gerrit code reviews, the change author is expected to [log in to Gerrit](https://go-review.googlesource.com/login/) with a Gmail or other Google account and then close out each piece of feedback by marking it as 'Done' if implemented as suggested or otherwise reply to each review comment. See the [Review](https://go.dev/doc/contribute#review) section of the Contributing Guide for details.)
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sean Liao (Gerrit)
Sean Liao voted and added 1 comment![]( "Open in Gerrit")
Votes added by Sean Liao
| Hold | +1 |
1 comment
Related details
Code-Review
No-Holds
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sean Liao (Gerrit)
Sean Liao voted and added 2 comments![]( "Open in Gerrit")
Votes added by Sean Liao
| Commit-Queue | +1 |
| Hold | +0 |
2 comments
this needs an accepted https://go.dev/s/proposal
ah I didn't see the proposal...
This will need additions to api/ and doc/.
CI should fail, check the test logs for the expected additions for api/
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jorropo (Gerrit)
Jorropo voted and added 2 comments![]( "Open in Gerrit")
Votes added by Jorropo
| Commit-Queue | +1 |
2 comments
I think this needs tests.
I guess there must be tests for the existing non iterator versions, it would make sense to update them to run on both the iterator and slices backed versions.
There is an accepted proposal with this exact API at https://github.com/golang/go/issues/66631#issuecomment-3335716931
Also it is linked in the `Fixes #66631` in the commit message.
Related details
- Sean Liao
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sean Liao (Gerrit)
Sean Liao added 3 comments![]( "Open in Gerrit")
The new methods are Type.Fields, Type.Methods, Type.Ins, Type.Outs, Value.Fields and Value.Methods.wrap lines
// Methods yields each method in the type's method set.add newlines between the previous method and the next comment
// The i'th field yielded by Fields is the same as [Type.Field(i)] and [Value.Field(i)].I don't think the linking syntax is correct.
check with go doc -htrp
Related details
- Quentin Quaadgras
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Quentin Quaadgras (Gerrit)
Quentin Quaadgras added 1 comment![]( "Open in Gerrit")
// The i'th field yielded by Fields is the same as [Type.Field(i)] and [Value.Field(i)].I don't think the linking syntax is correct.
check with go doc -htrp
There are a few cases of this, including in Type, where [Type.Method] is referenced but does not render as a link in pkg.go.dev (this particularly example also affects the existing Type.CanSeq declaration, which refers to [Value] and [Value.Seq]).
In addition to this, it's not actually possible to render inline links to interface methods or struct fields, which effects "[Type.Method]", as well as "[Method.Type]".
I think @aus...@google.com may have written the proposal documentation?
There are a number of limitations and unexpected quirks like this with Go documentation rendered in pkg.do.dev, I opened a related issue on this (https://golang.org/issue/75484) and my workaround there was to fallback to using footer links when things like this didn't render as expected.
Unfortunately, this workaround doesn't apply to interface method documentation which doesn't render links in the same way as exported methods. Should we be tracking any issues on this?
Related details
- Austin Clements
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI
Related details
- Austin Clements
- Jorropo
- Sean Liao
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Quentin Quaadgras (Gerrit)
Quentin Quaadgras added 1 comment![]( "Open in Gerrit")
add newlines between the previous method and the next comment
Done
Related details
- Austin Clements
- Jorropo
- Sean Liao
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Austin Clements
- Jorropo
- Sean Liao
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Quentin Quaadgras (Gerrit)
Quentin Quaadgras added 1 comment![]( "Open in Gerrit")
The new methods are Type.Fields, Type.Methods, Type.Ins, Type.Outs, Value.Fields and Value.Methods.Quentin Quaadgraswrap lines
Done
Related details
- Austin Clements
- Jorropo
- Sean Liao
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Quentin Quaadgras (Gerrit)
Quentin Quaadgras added 1 comment![]( "Open in Gerrit")
I spotted some possible problems with your PR:
1. You have a long 99 character line in the commit message body. Please add line breaks to long lines that should be wrapped. Lines in the commit message body should be wrapped at ~76 characters unless needed for things like URLs or tables. (Note: GitHub might render long lines as soft-wrapped, so double-check in the Gerrit commit message shown above.)Please address any problems by updating the GitHub PR.
When complete, mark this comment as 'Done' and click the [blue 'Reply' button](https://go.dev/wiki/GerritBot#i-left-a-reply-to-a-comment-in-gerrit-but-no-one-but-me-can-see-it) above. These findings are based on heuristics; if a finding does not apply, briefly reply here saying so.
To update the commit title or commit message body shown here in Gerrit, you must edit the GitHub PR title and PR description (the first comment) in the GitHub web interface using the 'Edit' button or 'Edit' menu entry there. Note: pushing a new commit to the PR will not automatically update the commit message used by Gerrit.
For more details, see:
- [how to update commit messages](https://go.dev/wiki/GerritBot/#how-does-gerritbot-determine-the-final-commit-message) for PRs imported into Gerrit.
- the Go project's [conventions for commit messages](https://go.dev/doc/contribute#commit_messages) that you should follow.
(In general for Gerrit code reviews, the change author is expected to [log in to Gerrit](https://go-review.googlesource.com/login/) with a Gmail or other Google account and then close out each piece of feedback by marking it as 'Done' if implemented as suggested or otherwise reply to each review comment. See the [Review](https://go.dev/doc/contribute#review) section of the Contributing Guide for details.)
Done
Quentin Quaadgras (Gerrit)
Quentin Quaadgras added 2 comments![]( "Open in Gerrit")
I think this needs tests.
I guess there must be tests for the existing non iterator versions, it would make sense to update them to run on both the iterator and slices backed versions.
There doesn't appear to be any existing tests specifically for comprehensively testing NumField, NumMethod, NumIn or NumOut (there aren't any slice backed versions). Although they are used as part of other tests.
You may be thinking of VisibleFields which does have tests and is slice backed but this proposal doesn't provide an equivalent for it. Should this CL introduce some tests similar to the VisibleFields tests?
One question I had on the implementation, Type.Fields, Type.Ins and Type.Outs are documented to panic when used on incompatible types, currently this panic is due to the underlying call to NumField/NumIn/NumOut, which will raise a panic like "reflect: NumIn of non-func type T", should the iterators raise a panic in their own name?
ie.
"reflect: Ins of non-func type T"
Jorropo (Gerrit)
Jorropo added 1 comment![]( "Open in Gerrit")
Quentin QuaadgrasI think this needs tests.
I guess there must be tests for the existing non iterator versions, it would make sense to update them to run on both the iterator and slices backed versions.
There doesn't appear to be any existing tests specifically for comprehensively testing NumField, NumMethod, NumIn or NumOut (there aren't any slice backed versions). Although they are used as part of other tests.
You may be thinking of VisibleFields which does have tests and is slice backed but this proposal doesn't provide an equivalent for it. Should this CL introduce some tests similar to the VisibleFields tests?
MB I see, a fairer thing for me to ask is for the iterators versions to match .NumXXX & .XXX versions in testing quality.
I would replace the existing tests that happen to call the manually iterated forms to use yours.
Yours still happen to call the existing one.
Related details
- Austin Clements
- Sean Liao
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jorropo (Gerrit)
Jorropo added 1 comment![]( "Open in Gerrit")
One question I had on the implementation, Type.Fields, Type.Ins and Type.Outs are documented to panic when used on incompatible types, currently this panic is due to the underlying call to NumField/NumIn/NumOut, which will raise a panic like "reflect: NumIn of non-func type T", should the iterators raise a panic in their own name?
ie."reflect: Ins of non-func type T"
It's not terrible given panic shows the backtrace, but panicking in their own names is better. It is slightly confusing to show `reflect: NumIn of non-func type T` when the use can't find `NumIn` anywhere in their code.
Quentin Quaadgras (Gerrit)
Quentin Quaadgras added 1 comment![]( "Open in Gerrit")
Quentin QuaadgrasI think this needs tests.
I guess there must be tests for the existing non iterator versions, it would make sense to update them to run on both the iterator and slices backed versions.
JorropoThere doesn't appear to be any existing tests specifically for comprehensively testing NumField, NumMethod, NumIn or NumOut (there aren't any slice backed versions). Although they are used as part of other tests.
You may be thinking of VisibleFields which does have tests and is slice backed but this proposal doesn't provide an equivalent for it. Should this CL introduce some tests similar to the VisibleFields tests?
MB I see, a fairer thing for me to ask is for the iterators versions to match .NumXXX & .XXX versions in testing quality.
I would replace the existing tests that happen to call the manually iterated forms to use yours.
Yours still happen to call the existing one.
I believe there other places in the standard library which use manual iteration, would it be appropriate for this CL to find and replace them?
Jorropo (Gerrit)
Jorropo added 1 comment![]( "Open in Gerrit")
Quentin QuaadgrasI think this needs tests.
I guess there must be tests for the existing non iterator versions, it would make sense to update them to run on both the iterator and slices backed versions.
JorropoThere doesn't appear to be any existing tests specifically for comprehensively testing NumField, NumMethod, NumIn or NumOut (there aren't any slice backed versions). Although they are used as part of other tests.
You may be thinking of VisibleFields which does have tests and is slice backed but this proposal doesn't provide an equivalent for it. Should this CL introduce some tests similar to the VisibleFields tests?
Quentin QuaadgrasMB I see, a fairer thing for me to ask is for the iterators versions to match .NumXXX & .XXX versions in testing quality.
I would replace the existing tests that happen to call the manually iterated forms to use yours.
Yours still happen to call the existing one.
I believe there other places in the standard library which use manual iteration, would it be appropriate for this CL to find and replace them?
No, smaller CLs are easier to review and require less coordination.
This would be a welcome change in future or stacked CLs.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- Austin Clements
- Jorropo
- Sean Liao
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan added 2 comments![]( "Open in Gerrit")
// Fields yields each field of a struct type.Fields does not yield, it returns an iterator that yields.
Use the phrasing "Fields returns the sequence of fields of a struct type", etc.
// The i'th field yielded by Fields is the same as [Type.Field(i)] and [Value.Field(i)].Quentin QuaadgrasI don't think the linking syntax is correct.
check with go doc -htrp
There are a few cases of this, including in Type, where [Type.Method] is referenced but does not render as a link in pkg.go.dev (this particularly example also affects the existing Type.CanSeq declaration, which refers to [Value] and [Value.Seq]).
In addition to this, it's not actually possible to render inline links to interface methods or struct fields, which effects "[Type.Method]", as well as "[Method.Type]".
I think @aus...@google.com may have written the proposal documentation?
There are a number of limitations and unexpected quirks like this with Go documentation rendered in pkg.do.dev, I opened a related issue on this (https://golang.org/issue/75484) and my workaround there was to fallback to using footer links when things like this didn't render as expected.
Unfortunately, this workaround doesn't apply to interface method documentation which doesn't render links in the same way as exported methods. Should we be tracking any issues on this?
Don't (ab)use doc links this way. Either write the expression plainly (e.g. `v.Type().Field(i)`) or use a doc link (e.g. `the order of fields corresponds to that of [Type.Field]`).
Related details
- Austin Clements
- Jorropo
- Sean Liao
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Quentin Quaadgras (Gerrit)
Quentin Quaadgras added 1 comment![]( "Open in Gerrit")
// The i'th field yielded by Fields is the same as [Type.Field(i)] and [Value.Field(i)].Quentin QuaadgrasI don't think the linking syntax is correct.
check with go doc -htrp
Alan DonovanThere are a few cases of this, including in Type, where [Type.Method] is referenced but does not render as a link in pkg.go.dev (this particularly example also affects the existing Type.CanSeq declaration, which refers to [Value] and [Value.Seq]).
In addition to this, it's not actually possible to render inline links to interface methods or struct fields, which effects "[Type.Method]", as well as "[Method.Type]".
I think @aus...@google.com may have written the proposal documentation?
There are a number of limitations and unexpected quirks like this with Go documentation rendered in pkg.do.dev, I opened a related issue on this (https://golang.org/issue/75484) and my workaround there was to fallback to using footer links when things like this didn't render as expected.
Unfortunately, this workaround doesn't apply to interface method documentation which doesn't render links in the same way as exported methods. Should we be tracking any issues on this?
Don't (ab)use doc links this way. Either write the expression plainly (e.g. `v.Type().Field(i)`) or use a doc link (e.g. `the order of fields corresponds to that of [Type.Field]`).
Like I mentioned, since Type is an interface, `[Type.Field]` does not render as a doc link on https://pkg.go.dev.
Related details
- Austin Clements
- Jorropo
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan added 1 comment![]( "Open in Gerrit")
// The i'th field yielded by Fields is the same as [Type.Field(i)] and [Value.Field(i)].Quentin QuaadgrasI don't think the linking syntax is correct.
check with go doc -htrp
Alan DonovanThere are a few cases of this, including in Type, where [Type.Method] is referenced but does not render as a link in pkg.go.dev (this particularly example also affects the existing Type.CanSeq declaration, which refers to [Value] and [Value.Seq]).
In addition to this, it's not actually possible to render inline links to interface methods or struct fields, which effects "[Type.Method]", as well as "[Method.Type]".
I think @aus...@google.com may have written the proposal documentation?
There are a number of limitations and unexpected quirks like this with Go documentation rendered in pkg.do.dev, I opened a related issue on this (https://golang.org/issue/75484) and my workaround there was to fallback to using footer links when things like this didn't render as expected.
Unfortunately, this workaround doesn't apply to interface method documentation which doesn't render links in the same way as exported methods. Should we be tracking any issues on this?
Quentin QuaadgrasDon't (ab)use doc links this way. Either write the expression plainly (e.g. `v.Type().Field(i)`) or use a doc link (e.g. `the order of fields corresponds to that of [Type.Field]`).
Like I mentioned, since Type is an interface, `[Type.Field]` does not render as a doc link on https://pkg.go.dev.
Sorry, I misread the remark about comments _on_ fields rather than comments _about_ fields. Yes, you're right that Doc Links don't officially support fields (though as a convenience, gopls's jump-to-definition feature does actually work on fields). So let's omit the link syntax here.
Quentin Quaadgras (Gerrit)
Quentin Quaadgras added 1 comment![]( "Open in Gerrit")
// The i'th field yielded by Fields is the same as [Type.Field(i)] and [Value.Field(i)].Quentin QuaadgrasI don't think the linking syntax is correct.
check with go doc -htrp
Alan DonovanThere are a few cases of this, including in Type, where [Type.Method] is referenced but does not render as a link in pkg.go.dev (this particularly example also affects the existing Type.CanSeq declaration, which refers to [Value] and [Value.Seq]).
In addition to this, it's not actually possible to render inline links to interface methods or struct fields, which effects "[Type.Method]", as well as "[Method.Type]".
I think @aus...@google.com may have written the proposal documentation?
There are a number of limitations and unexpected quirks like this with Go documentation rendered in pkg.do.dev, I opened a related issue on this (https://golang.org/issue/75484) and my workaround there was to fallback to using footer links when things like this didn't render as expected.
Unfortunately, this workaround doesn't apply to interface method documentation which doesn't render links in the same way as exported methods. Should we be tracking any issues on this?
Quentin QuaadgrasDon't (ab)use doc links this way. Either write the expression plainly (e.g. `v.Type().Field(i)`) or use a doc link (e.g. `the order of fields corresponds to that of [Type.Field]`).
Alan DonovanLike I mentioned, since Type is an interface, `[Type.Field]` does not render as a doc link on https://pkg.go.dev.
Sorry, I misread the remark about comments _on_ fields rather than comments _about_ fields. Yes, you're right that Doc Links don't officially support fields (though as a convenience, gopls's jump-to-definition feature does actually work on fields). So let's omit the link syntax here.
The reflect `[Type.Field]` example makes this more confusing, since it's an interface method, not a field (doc links on https://pkg.go.dev don't support linking to interface methods within the same package either).
Strangely enough, https://pkg.go.dev/log/slog renders `[io.Writer.Write]` as a link but it doesn't actually take you to the Writer interface.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- Austin Clements
- Jorropo
- Sean Liao
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Quentin Quaadgras (Gerrit)
Quentin Quaadgras added 4 comments![]( "Open in Gerrit")
This will need additions to api/ and doc/.
CI should fail, check the test logs for the expected additions for api/
I've added from what I can tell are the expected entries into api/ and doc/
I've made adjustments to the method documentation from what was originally specified in the proposal so that the terminology and language is more precise and to avoid incorrect/ambiguous use of doc links.
I've also replaced plain three-clause for loops with the iterator equivalent within the reflect package (including tests).
I've added what I understand are the expected entries for API changes and release notes.
I've also relocated the `reflect.Type` methods declarations to sit next to their existing non-iterator equivalents - both to make these functions more discoverable and because `reflect.Type` is split into two sections, methods that work with any kind of type and kind-specific methods `Methods` has been moved into the former.
Fields does not yield, it returns an iterator that yields.
Use the phrasing "Fields returns the sequence of fields of a struct type", etc.
Marked as resolved.
// The i'th field yielded by Fields is the same as [Type.Field(i)] and [Value.Field(i)].Quentin QuaadgrasI don't think the linking syntax is correct.
check with go doc -htrp
Alan DonovanThere are a few cases of this, including in Type, where [Type.Method] is referenced but does not render as a link in pkg.go.dev (this particularly example also affects the existing Type.CanSeq declaration, which refers to [Value] and [Value.Seq]).
In addition to this, it's not actually possible to render inline links to interface methods or struct fields, which effects "[Type.Method]", as well as "[Method.Type]".
I think @aus...@google.com may have written the proposal documentation?
There are a number of limitations and unexpected quirks like this with Go documentation rendered in pkg.do.dev, I opened a related issue on this (https://golang.org/issue/75484) and my workaround there was to fallback to using footer links when things like this didn't render as expected.
Unfortunately, this workaround doesn't apply to interface method documentation which doesn't render links in the same way as exported methods. Should we be tracking any issues on this?
Quentin QuaadgrasDon't (ab)use doc links this way. Either write the expression plainly (e.g. `v.Type().Field(i)`) or use a doc link (e.g. `the order of fields corresponds to that of [Type.Field]`).
Alan DonovanLike I mentioned, since Type is an interface, `[Type.Field]` does not render as a doc link on https://pkg.go.dev.
Quentin QuaadgrasSorry, I misread the remark about comments _on_ fields rather than comments _about_ fields. Yes, you're right that Doc Links don't officially support fields (though as a convenience, gopls's jump-to-definition feature does actually work on fields). So let's omit the link syntax here.
The reflect `[Type.Field]` example makes this more confusing, since it's an interface method, not a field (doc links on https://pkg.go.dev don't support linking to interface methods within the same package either).
Strangely enough, https://pkg.go.dev/log/slog renders `[io.Writer.Write]` as a link but it doesn't actually take you to the Writer interface.
- Alan Donovan
- Austin Clements
- Jorropo
- Sean Liao
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alan Donovan
| Code-Review | +2 |
1 comment
// The arguments to a Call on the yielded function value should not include a receiver;This "yielded" terminology is slightly awkward: the yield function is a rather low-level detail of the process, whereas the abstraction is a sequence of elements. Let's frame it in those terms, something like:
```
// Methods returns a sequence of elements, one per method of v, each
// pairing the the method's description (as a [Method]) with its value
// (as a func [Value]).
// ...
// In a [Call] to the method value, the arguments should not include the
// receiver as the method value implicitly binds v as the receiver.
// (See https://go.dev/ref/spec#Method_values.)
// By contrast the [Method.Type] of each [Method] element does include
// the receiver, and thus differs from [Value.Type].
```
Related details
- Austin Clements
- Jorropo
- Sean Liao
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- Code-Review: +2 by Alan Donovan
Related details
- Alan Donovan
- Austin Clements
- Jorropo
- Sean Liao
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Quentin Quaadgras (Gerrit)
Quentin Quaadgras added 2 comments![]( "Open in Gerrit")
Quentin QuaadgrasI think this needs tests.
I guess there must be tests for the existing non iterator versions, it would make sense to update them to run on both the iterator and slices backed versions.
JorropoThere doesn't appear to be any existing tests specifically for comprehensively testing NumField, NumMethod, NumIn or NumOut (there aren't any slice backed versions). Although they are used as part of other tests.
You may be thinking of VisibleFields which does have tests and is slice backed but this proposal doesn't provide an equivalent for it. Should this CL introduce some tests similar to the VisibleFields tests?
Quentin QuaadgrasMB I see, a fairer thing for me to ask is for the iterators versions to match .NumXXX & .XXX versions in testing quality.
I would replace the existing tests that happen to call the manually iterated forms to use yours.
Yours still happen to call the existing one.
JorropoI believe there other places in the standard library which use manual iteration, would it be appropriate for this CL to find and replace them?
No, smaller CLs are easier to review and require less coordination.
This would be a welcome change in future or stacked CLs.
Done
// The arguments to a Call on the yielded function value should not include a receiver;This "yielded" terminology is slightly awkward: the yield function is a rather low-level detail of the process, whereas the abstraction is a sequence of elements. Let's frame it in those terms, something like:
```
// Methods returns a sequence of elements, one per method of v, each
// pairing the the method's description (as a [Method]) with its value
// (as a func [Value]).
// ...
// In a [Call] to the method value, the arguments should not include the
// receiver as the method value implicitly binds v as the receiver.
// (See https://go.dev/ref/spec#Method_values.)
// By contrast the [Method.Type] of each [Method] element does include
// the receiver, and thus differs from [Value.Type].
```
Agree that it's a bit wordy, I've removed the term 'yielded' from the documentation, note that this word has been used in the standard library for a couple of functions that return an `iter.Seq`, I didn't find any public API function using the "returns a sequence of", so I've opted to keep "returns an iterator over" which is consistent with functions in the slices, iter and maps packages.
Related details
- Alan Donovan
- Austin Clements
- Jorropo
- Sean Liao
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alan Donovan
| Code-Review | +2 |
1 comment
// method [Value], this is a function with v bound as the receiver. As such, therun-on sentence
Use a semicolon, or the appositive (strike "this is ").
Related details
- Austin Clements
- Jorropo
- Sean Liao
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan added 1 comment![]( "Open in Gerrit")
// The arguments to a Call on the yielded function value should not include a receiver;Quentin QuaadgrasThis "yielded" terminology is slightly awkward: the yield function is a rather low-level detail of the process, whereas the abstraction is a sequence of elements. Let's frame it in those terms, something like:
```
// Methods returns a sequence of elements, one per method of v, each
// pairing the the method's description (as a [Method]) with its value
// (as a func [Value]).
// ...
// In a [Call] to the method value, the arguments should not include the
// receiver as the method value implicitly binds v as the receiver.
// (See https://go.dev/ref/spec#Method_values.)
// By contrast the [Method.Type] of each [Method] element does include
// the receiver, and thus differs from [Value.Type].
```
Agree that it's a bit wordy, I've removed the term 'yielded' from the documentation, note that this word has been used in the standard library for a couple of functions that return an `iter.Seq`, I didn't find any public API function using the "returns a sequence of", so I've opted to keep "returns an iterator over" which is consistent with functions in the slices, iter and maps packages.
- Austin Clements
- Jorropo
- Quentin Quaadgras
- Sean Liao
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- Code-Review: +2 by Alan Donovan
Related details
- Alan Donovan
- Austin Clements
- Jorropo
- Quentin Quaadgras
- Sean Liao
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |