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: x/exp/slices: add First and Last #53510

Closed
dsnet opened this issue Jun 23, 2022 · 19 comments
Closed

proposal: x/exp/slices: add First and Last #53510

dsnet opened this issue Jun 23, 2022 · 19 comments

Comments

@dsnet
Copy link
Member

dsnet commented Jun 23, 2022

The pattern s[len(s)-1] appears fairly often.
However, s[len(s)-1] is neither readable nor easy to type,
especially if s is a relatively complex expression.

Thus, I propose the addition of slices.Last that
retrieves the last elements, respectively.
I also propose adding slices.First for consistency.

Supporting evidence

Frequency of constant slice indexes relative to start or end:

Index Occurrences Percentage
0 4953854 41.684%
1 2192516 18.449%
2 1135707  9.556%
3 1370480 11.532%
4 521199  4.386%
other 1391696 11.710%
-5 196  0.002%
-4 443  0.004%
-3 1682  0.014%
-2 10336  0.087%
-1 306168  2.576%

Even though obtaining the last element is an order-of-magnitude
less frequent than getting the first element,
there are still ~300k occurences.

Of the number of cases where s[len(s)-1] appears,
it appears alongside s[0] 78% of the time.
For example:

minTime, _ := extractTimeRange(lastShard.spans[0])
_, maxTime := extractTimeRange(lastShard.spans[len(lastShard.spans)-1])

For this reason, I propose the addition of slices.First,
so that these case will read naturally:

minTime, _ := extractTimeRange(slices.First(lastShard.spans))
_, maxTime := extractTimeRange(slices.Last(lastShard.spans))

Considerations

This API does not help the following statement:

s[len(s)-1] = ...

since the returned value of slices.Last cannot be assigned to.
This occurs 32344 times (10.6%).

It also does not help the following expression:

&s[len(s)-1]

since you cannot take the address of the return value of slice.Last.
This occurs 8102 times (2.6%).

To support these 13% of cases, we could have slices.Last
return a pointer to the element, so that you could do:

*slices.Last(s) = ...

However, that is clunky to use.

Alternatives

Last is unhelpful if you want the penultimate element (i.e., s[len(s)-2]),
but there are an order-of-magnitude fewer use-cases for that compared
to just getting the last element.

We could consider a Python-like indexing operation with negative indexing:

// At retrieves the element at i.
// If i is negative, then it is indexed relative to the end.
func At[S ~[]E, E any](s S, i int) E

This provides a general-purpose way to retrieve elements relative to the end.

Negative indexing has been proposed many times in the past for slicing and was rejected
because it is easy for programming bug to go unnoticed.
Using an explicit API like slices.At could be clear enough signal that we truly want
negative indexing. Perhaps that is an appropriate trade-off.

I personally would like to have At as well,
but the evidence does not seem to indicate that it is as prevalent
compared to the need for Last.

\cc @ianlancetaylor @eliben

@dsnet dsnet added the Proposal label Jun 23, 2022
@gopherbot gopherbot added this to the Proposal milestone Jun 23, 2022
@ianlancetaylor
Copy link
Contributor

Should First and Last panic for an empty slice?

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Jun 23, 2022
@dsnet
Copy link
Member Author

dsnet commented Jun 23, 2022

Should First and Last panic for an empty slice?

I would expect so, in the same way that s[0] panics for an empty slice.

@fzipp
Copy link
Contributor

fzipp commented Jun 23, 2022

I also propose adding slices.First for consistency.

I'm not sure if symmetry is enough justification to add a function that basically does nothing of value. I find

minTime, _ := extractTimeRange(lastShard.spans[0])
_, maxTime := extractTimeRange(slices.Last(lastShard.spans))

perfectly fine.

@eliben
Copy link
Member

eliben commented Jun 23, 2022

Wouldn't it be better to just implement #25594, or a variant thereof?

First seems unnecessary and less readable than [0], and for just Last, IMHO [-1] would be better.

@dsnet
Copy link
Member Author

dsnet commented Jun 23, 2022

Thanks for highlighting #25594. I like the solution that it proposes more than this proposal, but it seems that similar languages changes like it have been proposed multiple times and rejected. Barring a language change, perhaps a library change is more palatable. I would argue that the frequency that similar proposals keep getting filed indicates that this is a real pain point for users that is sufficiently common.

@dsnet
Copy link
Member Author

dsnet commented Jun 23, 2022

IMHO [-1] would be better.

Not only is it better for readability, but it also resolves the inability for slices.Last to be addressed or assigned to.

@carlmjohnson
Copy link
Contributor

What about slices.At(s []T, n int) *T? It can accept -1 for last, it returns an address, and it doesn’t have to panic if the n is out of range. If you know your slice is going to be long enough, you can just dereference without testing for nil first.

@fzipp
Copy link
Contributor

fzipp commented Jun 26, 2022

What about slices.At(s []T, n int) *T?

'At' would almost always only be used with negative indices, because s[i] is clearer for the other cases, so why not a 'FromEnd' function:

slices.FromEnd(s, 0) // last element
slices.FromEnd(s, 1) // penultimate element
...

@rsc
Copy link
Contributor

rsc commented Jul 1, 2022

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 (old) Jul 1, 2022
@rsc
Copy link
Contributor

rsc commented Jul 13, 2022

This feels like it starts to fight the language.
Surely we don't want to get into arguments about whether to write s[0] or slices.First(s).
And similarly s[len(s)-1] is a clear, common idiom.
I'm not sure that we need a function for every idiom we have, even common ones.
This doesn't seem like a win, and I worry about the slices package getting bloated.

/cc @robpike

@dsnet
Copy link
Member Author

dsnet commented Jul 13, 2022

This feels like it starts to fight the language.

Is #25594 up for consideration or is a language change off the table? I find #25594 a notable improvement to both writability and readability, while avoiding pitfalls with negative indexing due to integer arithmetic bugs.

@robpike
Copy link
Contributor

robpike commented Jul 13, 2022

The comment that this is turning an idiom into a function is a good one. The addition of slices.First is unnecessary, more verbose than the idiom, except to complement slices.Last. So is Last necessary?

It can help when the slice expression is messy, but many things we do in any programming language get messy when the expression is messy. In other words, it doesn't add functionality, only convenience, and that requires a high bar that Last might not meet.

And once we have First and Last, there will be calls for Index and Nth and Last2 and LastN and so on. It's a can you might not want to open.

The philosophical question is: do we want to turn the language into this? We could have had maps and slices and so on be run with functions or methods, but we didn't. It would be sad to wake up one day and find many uses of slices eschewing the index notation.

@icholy
Copy link

icholy commented Jul 13, 2022

When you're dealing with a short local variable, the s[len(s)-1] idiom is totally fine. But it can get out of hand when accessing nested fields.

some.Nested.Values[len(some.Nested.Values)-1]

vs

slices.At(some.Nested.Values, -1)

@fzipp
Copy link
Contributor

fzipp commented Jul 14, 2022

@icholy You can always assign a long expression to a short local variable.

v := some.Nested.Values
v[len(v)-1]

@dsnet

This comment has been hidden.

@fzipp

This comment has been hidden.

@dsnet

This comment has been hidden.

@rsc rsc moved this from Active to Likely Decline in Proposals (old) Jul 20, 2022
@rsc
Copy link
Contributor

rsc commented Jul 20, 2022

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

@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Jul 27, 2022
@rsc
Copy link
Contributor

rsc commented Jul 27, 2022

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

9 participants