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

reflect: add Type.CanSeq/CanSeq2 and Value.Seq/Seq2 methods #66056

Closed
earthboundkid opened this issue Mar 1, 2024 · 29 comments
Closed

reflect: add Type.CanSeq/CanSeq2 and Value.Seq/Seq2 methods #66056

earthboundkid opened this issue Mar 1, 2024 · 29 comments

Comments

@earthboundkid
Copy link
Contributor

Proposal Details

As mentioned in #61897 (comment), it can be tricky to determine if a type is rangeable via reflection. There should be an efficient way to do the check and an efficient way to range over a reflect.Value. The immediate need for this is because of rangefuncs, but this would also be useful for slices, maps, etc.

@gopherbot gopherbot added this to the Proposal milestone Mar 1, 2024
@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Mar 1, 2024
@rsc rsc changed the title proposal: reflect: Add Type.IsRangeable() bool and Value.Range() iter.Seq[[]Value] proposal: reflect: add Type.CanRange and Value.Range methods Mar 8, 2024
@rsc
Copy link
Contributor

rsc commented Mar 8, 2024

Retitled to use the more usual CanVerb form than having to invent a new word "rangeable".

@rsc
Copy link
Contributor

rsc commented Mar 8, 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 rsc moved this from Incoming to Active in Proposals Mar 8, 2024
@rsc
Copy link
Contributor

rsc commented Mar 13, 2024

On the Type side, the new interface method is CanRange() bool.
On the Value, the new method is unclear.
It's unclear if it should support just funcs or all rangeable things.
Let's assume the former, since "all things" is too hard.
At that point what's the signature of Value.FuncRange?
I assume it would have to take a yield function of the right signature,
but at that point you can use Value.Call instead.
Similarly, range over int can be handled by seeing that it is an int and doing the loop separately.
I'm not seeing any good APIs to add. Value.Call seems about as good.

Did you have a specific API in mind?

@earthboundkid
Copy link
Contributor Author

My idea is that it would work like this:

v := reflect.Value(x)
if !v.Type().CanRange() { panic("bad type") }
for ranged := range v.Range() {
   if len(rangedVals) != 2 { /* e.g. it's an iter.Seq0 or iter.Seq */ panic("bad type") }
   key, val := ranged[0], ranged[1] // ranged is []reflect.Value of len between 0 and 2
   // etc
}

I think ideally it could work with all iterable types and just vary the length of the yielded slice to be as long as needed for slices, maps, iter.Seq[0], etc. This is semi-redundant with the existing map iterator, but I think the new API would be more convenient at the cost of being less performant for set operations.

If you wanted to know if it was specifically a range func you could do Kind == Func and CanRange.

@jimmyfrasche
Copy link
Member

why not CanRange{1,2} and Range{1,2}. If you only need to 1-range there's no point in allocating the second value and adding a wrapper slice and if you need to be able 2-range it's not useful for a 1-rangeable to reply true when you query its rangeability

@ianlancetaylor
Copy link
Contributor

A different approach would be

// Seq returns an iter.Seq[reflect.Value] that loops over the elements of v.
// If v's kind is Func, it must be a function that has no results and
// that takes a single argument of type func(T) bool for some type T.
// If v's kind is Pointer, the pointer element type must have kind Array.
// Otherwise v's kind must be Int, Int8, Int16, Int32, Int64, Uint, Uint8, Uint16, Uint32, Uint64, Uintptr,
// Array, Chan, Map, Slice, or String.
func (v Value) Seq() iter.Seq[Value]

// Seq2 is like Seq but for two values.
func (v Value) Seq2() iter.Seq2[Value, Value]

To support either version in the reflect package, when given a function, the reflect package would have to use MakeFunc to build a yield function of the appropriate type. That generated yield function would turn around and call the yield function passed in.

@DeedleFake
Copy link

why not CanRange{1,2} and Range{1,2}. If you only need to 1-range there's no point in allocating the second value and adding a wrapper slice and if you need to be able 2-range it's not useful for a 1-rangeable to reply true when you query its rangeability

Or Range() and Range2(). They fit with things like Slice() and Slice3() and Seq and Seq2 that way.

@Merovius
Copy link
Contributor

I would have proposed @ianlancetaylor's signatures and IMO they are also what makes this proposal worth considering over just Value.Call - it handles the construction of the appropriate yield function for you. It could also, from an API perspective, easily work for slices, maps and channels.

As for naming, based on the conventions suggested for iteration APIs, the name should arguably be All. But that reads a bit weird on a generic type that could be anything - and CanAll feels very bad. I dislike Range, as for x := range v.Range() stutters. My favorite so far is Seq, as it would correctly describe what the method (in Ian's suggestion) does and CanSeq isn't any worse than CanInt to me.

Also, there should also be a Seq0/CanSeq0, I believe? At least my Go installation still allows the variant of ranging without values, even though there is no iter.Seq0, I lost track of whether or not that's intended and why.

@earthboundkid
Copy link
Contributor Author

What does the Seq iterator yield for a slice? Just the indices wrapped as Values, so it matches x, _ := range s? That seems logical but unhelpful. Yielding the value is helpful but a little illogical.

@Merovius
Copy link
Contributor

We could always resolve that ambiguity by only allowing Seq2 for slices (and maps) and Seq for channels.

@jimmyfrasche
Copy link
Member

Should there be something like SeqJust2() iter.Seq[Value] that does for _, v := range s while avoiding extraneous allocation of the unneeded key?

@rsc
Copy link
Contributor

rsc commented Mar 27, 2024

It sounds like maybe people are happy with

// Seq returns an iter.Seq[reflect.Value] that loops over the elements of v.
// If v's kind is Func, it must be a function that has no results and
// that takes a single argument of type func(T) bool for some type T.
// If v's kind is Pointer, the pointer element type must have kind Array.
// Otherwise v's kind must be Int, Int8, Int16, Int32, Int64, Uint, Uint8, Uint16, Uint32, Uint64, Uintptr,
// Array, Chan, Map, Slice, or String.
func (v Value) Seq() iter.Seq[Value]

// Seq2 is like Seq but for two values.
func (v Value) Seq2() iter.Seq2[Value, Value]

along with CanSeq and CanSeq2.

@Merovius
Copy link
Contributor

@rsc To clarify: "loops over the elements of v" means that it yields elements, not indices? Also, what does it yield for strings?

@rsc
Copy link
Contributor

rsc commented Mar 27, 2024

@Merovius perhaps the text should be made clearer, but the idea is that Seq and Seq2 would do exactly what range does.

@earthboundkid
Copy link
Contributor Author

Looks good. The final docs should be explicit that Seq just returns an index Value for most Kinds and that Seq2 doesn't work for channels.

@rsc
Copy link
Contributor

rsc commented Apr 4, 2024

Have all remaining concerns about this proposal been addressed?

The proposal is in #66056 (comment).

@earthboundkid
Copy link
Contributor Author

On #66631, the question came up of whether to avoid a dependency on the iter package. Should these maybe just be iterators directly instead of returning a named iter.Seq? Having the dependency in this direction could hurt if iter becomes a utility package of some kind. It could always fallback to reflectlite I guess and it’s speculation anyway. I could be convinced either way.

@earthboundkid earthboundkid changed the title proposal: reflect: add Type.CanRange and Value.Range methods proposal: reflect: add Type.CanSeq/CanSeq2 and Value.Seq/Seq2 methods Apr 5, 2024
@rsc
Copy link
Contributor

rsc commented Apr 10, 2024

Iter is going to be a very low-level package and will not have significant dependencies added. Using it in reflect is fine.

@rsc rsc moved this from Active to Likely Accept in Proposals Apr 10, 2024
@rsc
Copy link
Contributor

rsc commented Apr 10, 2024

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

The proposal is in #66056 (comment).

@earthboundkid
Copy link
Contributor Author

Should I start working on a CL for the naive implementation of this, or does someone else already know how to do a more performant implementation?

qiulaidongfeng added a commit to qiulaidongfeng/go that referenced this issue Apr 14, 2024
WIP

DO NOT SUBMIT

FIxes golang#66056

Change-Id: I23f1ae11fea7b412a294b47775b8756fd961a53b
@qiulaidongfeng
Copy link
Member

Should I start working on a CL for the naive implementation of this, or does someone else already know how to do a more performant implementation?

I have an implementation that I haven't finished yet.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/578815 mentions this issue: reflect: add iterative related methods

@earthboundkid
Copy link
Contributor Author

@qiulaidongfeng, the behavior for Seq() on maps is to return the map values, not keys. I think that’s surprising. I think it should be like range and the Seq is just the keys, and the Seq2 is the keys and values. I’m not sure what it should do about slices.

@fzipp
Copy link
Contributor

fzipp commented Apr 14, 2024

I’m not sure what it should do about slices.

@earthboundkid Russ' comment was clear:

the idea is that Seq and Seq2 would do exactly what range does

Seq returns the index for slices, and Seq2 index and value.

@qiulaidongfeng
Copy link
Member

@qiulaidongfeng, the behavior for Seq() on maps is to return the map values, not keys. I think that’s surprising. I think it should be like range and the Seq is just the keys, and the Seq2 is the keys and values. I’m not sure what it should do about slices.

Thanks for you report. Now CL has solved the unfinished ones. CL can be review but submit need wait https://go-review.googlesource.com/c/go/+/557836.

@rsc
Copy link
Contributor

rsc commented Apr 24, 2024

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

The proposal is in #66056 (comment).

@rsc rsc moved this from Likely Accept to Accepted in Proposals Apr 24, 2024
@rsc rsc changed the title proposal: reflect: add Type.CanSeq/CanSeq2 and Value.Seq/Seq2 methods reflect: add Type.CanSeq/CanSeq2 and Value.Seq/Seq2 methods Apr 24, 2024
@rsc rsc modified the milestones: Proposal, Backlog Apr 24, 2024
qiulaidongfeng added a commit to qiulaidongfeng/go that referenced this issue May 7, 2024
WIP

DO NOT SUBMIT

FIxes golang#66056

Change-Id: I23f1ae11fea7b412a294b47775b8756fd961a53b
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/584515 mentions this issue: reflect: let documentation for Value.Seq and Value.Seq2 clearer

@qiulaidongfeng
Copy link
Member

qiulaidongfeng commented May 9, 2024

I noticed this 41d6687084f1837311021a134968479cb40f9021 after CL was committed.
According to the go specification, the type of the iterated variable should be the same as the type of the range integer.

For example, the iteration variable for range int8 should be int8, see https://go.dev/play/p/HYJzYbV3Suc prove.

But the existing reflect implementation, value.seq, iterates over integers and always returns a Value representing int64 or uint64, even for int8-like types.

Should this be done to make Value.Seq iterated integers conform to the go specification?
Make the implementation more complex, such as abstracting the range integer with a generic function and changing the iterated variable to the specified type with type arguments, and calling the generic function with the correct type arguments in Value.Seq.

qiulaidongfeng added a commit to qiulaidongfeng/go that referenced this issue May 9, 2024
… spec

See CL 557596,According to the go specification, the iterated variable type should be the same as the iterated integer type.

For golang#66056

Change-Id: I6a19cc211d081ebd8142c5a702601c8b49a6d736
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/584516 mentions this issue: reflect: let Value.Seq iterate integer conform to the spec

@dmitshur dmitshur modified the milestones: Backlog, Go1.23 May 9, 2024
gopherbot pushed a commit that referenced this issue May 9, 2024
For #66056

Change-Id: Ib47c07b2527d8213584b72e2575a353f2deaed68
GitHub-Last-Rev: 525a5c3
GitHub-Pull-Request: #67268
Reviewed-on: https://go-review.googlesource.com/c/go/+/584515
Reviewed-by: Cherry Mui <[email protected]>
Commit-Queue: Ian Lance Taylor <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Auto-Submit: Ian Lance Taylor <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
qiulaidongfeng added a commit to qiulaidongfeng/go that referenced this issue May 10, 2024
… spec

See CL 557596,According to the go specification, the iterated variable type should be the same as the iterated integer type.

For golang#66056

Change-Id: I6a19cc211d081ebd8142c5a702601c8b49a6d736
gopherbot pushed a commit that referenced this issue May 10, 2024
See CL 557596, according to the go specification,
the iterated variable type should
be the same as the iterated integer type.

For #66056

Change-Id: I96c87440328c2c50c40d76ecf2f222a331be1ce9
GitHub-Last-Rev: 8f80e40
GitHub-Pull-Request: #67269
Reviewed-on: https://go-review.googlesource.com/c/go/+/584516
Reviewed-by: Ian Lance Taylor <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Auto-Submit: Cherry Mui <[email protected]>
Reviewed-by: Cherry Mui <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Development

Successfully merging a pull request may close this issue.

10 participants