Make List.{take,drop} total

[dbuenzli]

See discussion.

Raising is highly non-standard across languages and libraries and the functions have only been there in 5.3 so far.

I think this should go in 5.4 too.

[nojb]

LGTM

[nojb]

I think this should go in 5.4 too.

@Octachron: do you validate?

[Octachron]

If we agree that this PR specification is the right one, then this is a specification bugfix and should be cherry-picked to 5.4.

[nojb]

If we agree that this PR specification is the right one, then this is a specification bugfix and should be cherry-picked to 5.4.

OK, thanks. I am of the opinion that this is a specification bugfix, as is @gasche.

[gasche]

I agree, and I believe this is worth having in 5.4 to minimize the non-totality version interval.

[nojb]

CI failure is unrelated. Merging.

[nojb]

Cherry-pick to 5.4: da2b221

[dbuenzli]

Thanks all!

[giltho]

(Late comment, I was looking at the changelog for 5.4 beta)

This specific exception caught a bug in my code yesterday. Specifically, I had an overflow turn my count into a negative value, and then it was passed to List.take. I'm not sure I'd have specifically guarded about negative values by default here, so it was helpful to have it be the default in stdlib.

Raising is highly non-standard across languages

Note also that other languages do tend to prevent negative counts there, e.g. Java raises an exception, Rust prevents the problem by receiving an unsigned integer, Python's itertool raises an exception etc..

I don't necessarily disagree with this PR, I don't think there's necessarily a "best" answer here, but I thought I'd flag it for future similar discussions.

[dbuenzli]

Personally I also don't have a strong opinion here, I think both view have their own merits.

I do however find totality easier for lazy (in the human sense) edge case reasoning – i.e. rather than proving that n will not be negative, I check that if n is negative it's doesn't have adverse effects on the result of the computation. That kind of reasoning is not available with the raising variant as it kills your computation.

As mentioned here the reason why I wanted to do this change is that I'm contemplating adding corresponding functionality to the String module. Now we could have the raising behaviour there too but most libs that extend the string module tend to have the view of this PR. So if we add these function to String with the raising variants then migrating to the Stdlib.String functionality becomes a painful migration rather than an obvious search and replace.

[giltho]

Agreed. An option would be to add some alternative function that is already defensive (e.g. List.take_exn), but then naming is a nightmare because if I have List.take_exn I'd want List.take to return an option for consistency with existing conventions 🫠
Another option would be to return an option in the first place, and then one can do |> Option.value ~default:[] or |> Option.get. It's safer, but it's also unnecessary allocation/computation when you know there's no risk of the integer to be negative.
So yeah, still no strong opinion on my side, it was really about flagging this.