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

Implemented `List.init` #1034

Closed
wants to merge 16 commits into
base: trunk
from

Conversation

@Richard-Degenne
Contributor

Richard-Degenne commented Feb 8, 2017

Summary

I was amazed to see that List.init didn't exist in the standard library, and thought that it should be fixed.

This PR implements List.init.

Description

List.init works exactly like Array.init does. It allows the user to create a list given its size and a function which will be mapped to the new list.

The size must be positive. An Invalid_argument exception is raised if the size is negative. Giving 0 as a size will result in the new list being empty.

Signature

val init : int -> (int -> 'a) -> 'a list

Examples

Correct usage

List.init 10 (fun x -> x);;
- : int list = [0; 1; 2; 3; 4; 5; 6; 7; 8; 9]

List.init 5 (fun x -> 2*(x+1));;
- : int list = [2; 4; 6; 8; 10]

List.init 5 (string_of_int);;
- : bytes list = ["0"; "1"; "2"; "3"; "4"]

Incorrect usage

List.init (-1) (fun x -> x);;
Exception: (Invalid_argument List.init).

Design

As you can see here, List.init is tail-recursive; it uses a auxiliary function to apply the given function to every element before appending them to an accumulator, which is then reversed to return the new list in the correct order.

Documentation

As you cas see here, documentation has been added to the function in the .mli file.

Tests

Unit tests have been added to a new folder testsuite/tests/list-functions because I was unable to find any tests for other List functions.

$ make one DIR=tests/list-functions                                                                                                          

Running tests from 'tests/list-functions' ...
 ... testing 'test.ml': ocamlc ocamlopt => passed
@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Feb 8, 2017

Contributor

Thanks for this - especially for including tests! A few things:

  1. Testsuite files do not need copyright headers (some have them).
  2. I'm surprised that we don't have any List tests already (unless I'm being blind) - but could you instead name the directory lib-list for consistency with other stdlib test directories?
  3. I'm personally not convinced by the choice of name. init has a totally different in Haskell and it also, to me, sounds very imperative for a functional structure - especially as it implies mutability. In Standard ML this function is called tabulate. However, it is the name chosen by both Batteries and Core.
Contributor

dra27 commented Feb 8, 2017

Thanks for this - especially for including tests! A few things:

  1. Testsuite files do not need copyright headers (some have them).
  2. I'm surprised that we don't have any List tests already (unless I'm being blind) - but could you instead name the directory lib-list for consistency with other stdlib test directories?
  3. I'm personally not convinced by the choice of name. init has a totally different in Haskell and it also, to me, sounds very imperative for a functional structure - especially as it implies mutability. In Standard ML this function is called tabulate. However, it is the name chosen by both Batteries and Core.
@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Feb 8, 2017

Contributor

You will also need a Changes file entry and there are some testsuite issues to address. Looking only at the list of fails, it looks like you are missing the equivalent change in ListLabels - I expect that the translprim test is a renumbering problem - see what the difference between the result and reference files is.

Contributor

dra27 commented Feb 8, 2017

You will also need a Changes file entry and there are some testsuite issues to address. Looking only at the list of fails, it looks like you are missing the equivalent change in ListLabels - I expect that the translprim test is a renumbering problem - see what the difference between the result and reference files is.

@dra27 dra27 added the stdlib label Feb 8, 2017

@Richard-Degenne

This comment has been minimized.

Show comment
Hide comment
@Richard-Degenne

Richard-Degenne Feb 9, 2017

Contributor

Testsuite files do not need copyright headers (some have them).
I'm surprised that we don't have any List tests already (unless I'm being blind) - but could you instead name the directory lib-list for consistency with other stdlib test directories?

I'll correct that right away, while keeping an eye out for those missing tests.

I'm personally not convinced by the choice of name. init has a totally different in Haskell and it also, to me, sounds very imperative for a functional structure - especially as it implies mutability. In Standard ML this function is called tabulate. However, it is the name chosen by both Batteries and Core.

init is also the name chosen in the Array module. It may be better to stay with that name, even though, as you stated, it seems to imply mutability.

You will also need a Changes file entry.

Should I add the entry in the Next version or in the Next minor version?

And there are some testsuite issues to address.

I'll look into that too.

Anyway, thanks a lot for your feedback. This is my first time contributing and having a bit of help is very helpful.

Contributor

Richard-Degenne commented Feb 9, 2017

Testsuite files do not need copyright headers (some have them).
I'm surprised that we don't have any List tests already (unless I'm being blind) - but could you instead name the directory lib-list for consistency with other stdlib test directories?

I'll correct that right away, while keeping an eye out for those missing tests.

I'm personally not convinced by the choice of name. init has a totally different in Haskell and it also, to me, sounds very imperative for a functional structure - especially as it implies mutability. In Standard ML this function is called tabulate. However, it is the name chosen by both Batteries and Core.

init is also the name chosen in the Array module. It may be better to stay with that name, even though, as you stated, it seems to imply mutability.

You will also need a Changes file entry.

Should I add the entry in the Next version or in the Next minor version?

And there are some testsuite issues to address.

I'll look into that too.

Anyway, thanks a lot for your feedback. This is my first time contributing and having a bit of help is very helpful.

Show outdated Hide outdated stdlib/list.mli
Show outdated Hide outdated stdlib/list.ml
@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Feb 9, 2017

Contributor

I haven't looked at the code, but would like to support the inclusion of this function. The next version of Flambda has a lot of Array.to_list (Array.init ...) at the moment.

Contributor

mshinwell commented Feb 9, 2017

I haven't looked at the code, but would like to support the inclusion of this function. The next version of Flambda has a lot of Array.to_list (Array.init ...) at the moment.

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Feb 9, 2017

Contributor

It would be "Next version" - if this can be polished before Wednesday, it could be ready for 4.05.

@alainfrisch - any objections, once fixes made?

Contributor

dra27 commented Feb 9, 2017

It would be "Next version" - if this can be polished before Wednesday, it could be ready for 4.05.

@alainfrisch - any objections, once fixes made?

Show outdated Hide outdated stdlib/list.mli
Show outdated Hide outdated stdlib/list.ml

Richard-Degenne added a commit to Richard-Degenne/ocaml that referenced this pull request Feb 9, 2017

@dra27

More changes to the comments, please!

Show outdated Hide outdated stdlib/listLabels.mli
Show outdated Hide outdated stdlib/listLabels.mli
Show outdated Hide outdated stdlib/list.mli
@Richard-Degenne

This comment has been minimized.

Show comment
Hide comment
@Richard-Degenne

Richard-Degenne Feb 9, 2017

Contributor

I can't find what's wrong with testsuite/tests/translprim/comparison.ml. The test file itself is pretty confusing and the test output doesn't make much sense to me.

$ diff testsuite/tests/translprim/comparison_table.ml.{reference,result}

246c246
<                 (apply (field 15 (global List!)) (apply uncurry f)
---
>                 (apply (field 16 (global List!)) (apply uncurry f)
294c294
<                     (apply (field 15 (global List!))
---
>                     (apply (field 16 (global List!))

Any idea?

Contributor

Richard-Degenne commented Feb 9, 2017

I can't find what's wrong with testsuite/tests/translprim/comparison.ml. The test file itself is pretty confusing and the test output doesn't make much sense to me.

$ diff testsuite/tests/translprim/comparison_table.ml.{reference,result}

246c246
<                 (apply (field 15 (global List!)) (apply uncurry f)
---
>                 (apply (field 16 (global List!)) (apply uncurry f)
294c294
<                     (apply (field 15 (global List!))
---
>                     (apply (field 16 (global List!))

Any idea?

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Feb 9, 2017

Contributor

It looks fine - just replace the .reference file with the .result file. You've added a member to the List structure which changes the ordinal numbers for the functions when looking further down the compiler pipeline.

Contributor

dra27 commented Feb 9, 2017

It looks fine - just replace the .reference file with the .result file. You've added a member to the List structure which changes the ordinal numbers for the functions when looking further down the compiler pipeline.

@Richard-Degenne

This comment has been minimized.

Show comment
Hide comment
@Richard-Degenne

Richard-Degenne Feb 9, 2017

Contributor

Ok, that's what I thought. Thanks.

Contributor

Richard-Degenne commented Feb 9, 2017

Ok, that's what I thought. Thanks.

Show outdated Hide outdated stdlib/list.mli
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Feb 10, 2017

Member

I believe that calling the function left-to-right, from 0 to n - 1, is an important property of a no-surprise implementation.

Member

gasche commented Feb 10, 2017

I believe that calling the function left-to-right, from 0 to n - 1, is an important property of a no-surprise implementation.

@Richard-Degenne

This comment has been minimized.

Show comment
Hide comment
@Richard-Degenne

Richard-Degenne Feb 10, 2017

Contributor

The answer seems to be up to us, here. As I said yesterday here, Core calls f from n-1 to 0 and the function's documentation doesn't mention it.

Contributor

Richard-Degenne commented Feb 10, 2017

The answer seems to be up to us, here. As I said yesterday here, Core calls f from n-1 to 0 and the function's documentation doesn't mention it.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Feb 10, 2017

Member

Well, my personal opinion is that Core made the wrong design choice here. Users will assume that evaluation is left-to-right, and they will be surprised (or not notice and have bugs in their programs; of course they should not have effects that rely on evaluation order, but in practice they do). Surprise is bad -- in this context.

Member

gasche commented Feb 10, 2017

Well, my personal opinion is that Core made the wrong design choice here. Users will assume that evaluation is left-to-right, and they will be surprised (or not notice and have bugs in their programs; of course they should not have effects that rely on evaluation order, but in practice they do). Surprise is bad -- in this context.

@Richard-Degenne

This comment has been minimized.

Show comment
Hide comment
@Richard-Degenne

Richard-Degenne Feb 10, 2017

Contributor

I think we need more input on this.

Contributor

Richard-Degenne commented Feb 10, 2017

I think we need more input on this.

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Feb 10, 2017

Contributor

If anything, right-to-left order is more consistent with the rest of the compiler, which after some fixes currently in development I think will always use that order.

I don't think we should pay the penalty of twice as much allocation just for those people who don't read the documentation. I think it is reasonable to document the order.

Contributor

mshinwell commented Feb 10, 2017

If anything, right-to-left order is more consistent with the rest of the compiler, which after some fixes currently in development I think will always use that order.

I don't think we should pay the penalty of twice as much allocation just for those people who don't read the documentation. I think it is reasonable to document the order.

@Richard-Degenne

This comment has been minimized.

Show comment
Hide comment
@Richard-Degenne

Richard-Degenne Feb 10, 2017

Contributor

Array.init applies f from 0 to n-1, though. But an Array is an imperative structure, so I'm not sure whether it has to be consistent with List.init.

Contributor

Richard-Degenne commented Feb 10, 2017

Array.init applies f from 0 to n-1, though. But an Array is an imperative structure, so I'm not sure whether it has to be consistent with List.init.

@nojb

This comment has been minimized.

Show comment
Hide comment
@nojb

nojb Feb 10, 2017

Contributor

Is it necessary to make List.init tail-recursive ? Functions which are not naturally tail recursive are typically left like that in the stdlib (e.g. List.map, etc).

Contributor

nojb commented Feb 10, 2017

Is it necessary to make List.init tail-recursive ? Functions which are not naturally tail recursive are typically left like that in the stdlib (e.g. List.map, etc).

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Feb 10, 2017

Contributor

@nojb - having not initially been bothered, it does now seem strange to me for this function not to be tail-recursive (also, of course, there is rev_map provided - indeed, mostly the List module provides the logical alternate tail-recursive function) or at least for one not to provided (and providing two init functions feels like a lot of weight for something so simple).

@gasche - Core does it tail-recursively in reverse, but Batteries only does it left-to-right by cheating with tail-consed lists! Would you be happy if the docs stated that the order of evaluation is unspecified?

@mshinwell - is right-to-left for initialisation of lists specified, or am I correct that it is intentionally not specified? I know it's been the behaviour of [f 0; f 1; f 2] for approximately ever!

Contributor

dra27 commented Feb 10, 2017

@nojb - having not initially been bothered, it does now seem strange to me for this function not to be tail-recursive (also, of course, there is rev_map provided - indeed, mostly the List module provides the logical alternate tail-recursive function) or at least for one not to provided (and providing two init functions feels like a lot of weight for something so simple).

@gasche - Core does it tail-recursively in reverse, but Batteries only does it left-to-right by cheating with tail-consed lists! Would you be happy if the docs stated that the order of evaluation is unspecified?

@mshinwell - is right-to-left for initialisation of lists specified, or am I correct that it is intentionally not specified? I know it's been the behaviour of [f 0; f 1; f 2] for approximately ever!

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Feb 10, 2017

Member

I believe that most users (1) never carefully read the documentation of the function when it has the name that "obviously" gives the meaning they want, (2) will easily make assumptions about evaluation order (usually left-to-right, small to large), and (3) even if they test the behavior and see that their guess was wrong (at this point you made them lose their time and you should start feeling guilty), they will always assume that the behavior they can now observe is not going to change in the future. As a result, I am not very interested in the question of what is (un)specified, but I have a relatively firm opinion on what should be implemented.

(Note that the difficulty (3) could be lifted if all functions with "unspecified" evaluation order actually picked a random evaluation order at runtime, letting user realize the arbitrariness of it by testing.)

I believe that "unspecified" in this context is mostly a cop-out from the designers to be dismissive when the users report bugs caused by bad library design. This is not the case in all circumstances: when you write an advanced API for advanced purposes, you can assume a certain level of discipline among its users. (I was going to write "it is fine if Obj carefully documents what is and isn't specified", oh well...). All your students are going to use List.init.

Member

gasche commented Feb 10, 2017

I believe that most users (1) never carefully read the documentation of the function when it has the name that "obviously" gives the meaning they want, (2) will easily make assumptions about evaluation order (usually left-to-right, small to large), and (3) even if they test the behavior and see that their guess was wrong (at this point you made them lose their time and you should start feeling guilty), they will always assume that the behavior they can now observe is not going to change in the future. As a result, I am not very interested in the question of what is (un)specified, but I have a relatively firm opinion on what should be implemented.

(Note that the difficulty (3) could be lifted if all functions with "unspecified" evaluation order actually picked a random evaluation order at runtime, letting user realize the arbitrariness of it by testing.)

I believe that "unspecified" in this context is mostly a cop-out from the designers to be dismissive when the users report bugs caused by bad library design. This is not the case in all circumstances: when you write an advanced API for advanced purposes, you can assume a certain level of discipline among its users. (I was going to write "it is fine if Obj carefully documents what is and isn't specified", oh well...). All your students are going to use List.init.

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Feb 10, 2017

Contributor

@gasche OK! What's your opinion on the merits of two functions, therefore, proposed earlier? If it does go that way, I might suggest init_left and init_right rather than init and init_rev?

Contributor

dra27 commented Feb 10, 2017

@gasche OK! What's your opinion on the merits of two functions, therefore, proposed earlier? If it does go that way, I might suggest init_left and init_right rather than init and init_rev?

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Feb 10, 2017

Member

We use rev_map, so maybe rev_init would be better than init_rev. Nothing in particular against {left,right}_init, but won't users be looking for a function just named init? (I think someone could make the case that, in the interest of minimality, maybe we only should have one init function. I am not convinced that this is really an argument.)

Member

gasche commented Feb 10, 2017

We use rev_map, so maybe rev_init would be better than init_rev. Nothing in particular against {left,right}_init, but won't users be looking for a function just named init? (I think someone could make the case that, in the interest of minimality, maybe we only should have one init function. I am not convinced that this is really an argument.)

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Feb 10, 2017

Contributor

Except that rev_map actually produces a different kind of list - the output of rev_init and init is always [f 0; f 1; ... ]. (it's also funny that rev_init constructs the list in order and init is the one which has to call rev!!)

Given your (acceptable!) fervour for least-surprise, perhaps not having init is good - someone using Merlin will see that there is init_left and init_right and maybe they'll do the thinking they should be?

Contributor

dra27 commented Feb 10, 2017

Except that rev_map actually produces a different kind of list - the output of rev_init and init is always [f 0; f 1; ... ]. (it's also funny that rev_init constructs the list in order and init is the one which has to call rev!!)

Given your (acceptable!) fervour for least-surprise, perhaps not having init is good - someone using Merlin will see that there is init_left and init_right and maybe they'll do the thinking they should be?

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Feb 10, 2017

Member

Apologies for being unclear. The rev_init I had in mind would return [f (n - 1); ...; f 0], so that rev (rev_foo f n) = foo f n holds for both map and init. That said, I don't have much opinions on functions with a different name (those may be used by more informed users, especially if they only find them by reading the documentation).

Member

gasche commented Feb 10, 2017

Apologies for being unclear. The rev_init I had in mind would return [f (n - 1); ...; f 0], so that rev (rev_foo f n) = foo f n holds for both map and init. That said, I don't have much opinions on functions with a different name (those may be used by more informed users, especially if they only find them by reading the documentation).

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Feb 10, 2017

Contributor

Ah, I see! Any further thoughts @mshinwell and @nojb on this typically long stdlib discussion? 😄

Contributor

dra27 commented Feb 10, 2017

Ah, I see! Any further thoughts @mshinwell and @nojb on this typically long stdlib discussion? 😄

@damiendoligez

This comment has been minimized.

Show comment
Hide comment
@damiendoligez

damiendoligez Feb 15, 2017

Member

@mshinwell Left-to-right is less surprising, so I think it's what we should specify.

Member

damiendoligez commented Feb 15, 2017

@mshinwell Left-to-right is less surprising, so I think it's what we should specify.

@Richard-Degenne

This comment has been minimized.

Show comment
Hide comment
@Richard-Degenne

Richard-Degenne Feb 17, 2017

Contributor

Any input on the current state of the pull request?

Contributor

Richard-Degenne commented Feb 17, 2017

Any input on the current state of the pull request?

@dra27

dra27 approved these changes Feb 17, 2017

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Feb 17, 2017

Contributor

@Richard-Degenne There is still, I'm afraid, another tiny tweak to the .mli comments! I'm not sure if it matters, but in an @raise the exception name should not be wrapped in brackets - so it should be @raise Invalid_argument if [len < 0] - ocamldoc requires the first word after @raise to be the exception name and formats it accordingly.

It's also important to get an answer from the CI - please could you rebase to remove the clashing merge commit?

Contributor

dra27 commented Feb 17, 2017

@Richard-Degenne There is still, I'm afraid, another tiny tweak to the .mli comments! I'm not sure if it matters, but in an @raise the exception name should not be wrapped in brackets - so it should be @raise Invalid_argument if [len < 0] - ocamldoc requires the first word after @raise to be the exception name and formats it accordingly.

It's also important to get an answer from the CI - please could you rebase to remove the clashing merge commit?

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Feb 28, 2017

Contributor

Squashed and merged in trunk. Thanks to @Richard-Degenne and to everyone involved in the discussion!

Contributor

alainfrisch commented Feb 28, 2017

Squashed and merged in trunk. Thanks to @Richard-Degenne and to everyone involved in the discussion!

assert (not (List.exists (fun a -> a > 9) l));
assert (List.exists (fun _ -> true) l);
;;

This comment has been minimized.

@murmour

murmour Nov 3, 2017

Contributor

Um. Shouldn't the following suffice? We aren't testing List.exists here.

assert ((List.init 10 (fun x -> x)) = [ 0; 1; 2; 3; 4; 5; 6; 7; 8; 9 ])
@murmour

murmour Nov 3, 2017

Contributor

Um. Shouldn't the following suffice? We aren't testing List.exists here.

assert ((List.init 10 (fun x -> x)) = [ 0; 1; 2; 3; 4; 5; 6; 7; 8; 9 ])

This comment has been minimized.

@dra27

dra27 Nov 3, 2017

Contributor

FWIW, yes we are!

@dra27

dra27 Nov 3, 2017

Contributor

FWIW, yes we are!

@infinity0

This comment has been minimized.

Show comment
Hide comment
@infinity0

infinity0 Jan 15, 2018

Contributor

Just wanted to document the fact that this PR breaks pre-4.06.0 code like:

let f init x y z =
  let open List in
  (* do something with `init` *)

You'll get an error message like

Error: This expression has type int -> (int -> 'weak7) -> 'weak7 list
       but an expression was expected of type (your actual type) = (your actual type)

Maybe this will help people googling for the error message.

Contributor

infinity0 commented Jan 15, 2018

Just wanted to document the fact that this PR breaks pre-4.06.0 code like:

let f init x y z =
  let open List in
  (* do something with `init` *)

You'll get an error message like

Error: This expression has type int -> (int -> 'weak7) -> 'weak7 list
       but an expression was expected of type (your actual type) = (your actual type)

Maybe this will help people googling for the error message.

@infinity0

This comment has been minimized.

Show comment
Hide comment
@infinity0

infinity0 Jan 15, 2018

Contributor

(Ofc the fix is easy, just rename your init to something else.)

Contributor

infinity0 commented Jan 15, 2018

(Ofc the fix is easy, just rename your init to something else.)

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Jan 15, 2018

Contributor

Sorry you got stung, though this shows the general weakness (in my not remotely humble opinion) of opening stdlib modules! List.( ... ) will probably be safer if you use several symbols from List in a single expression and otherwise for single symbols, List.length is much clearer than length, etc.

Contributor

dra27 commented Jan 15, 2018

Sorry you got stung, though this shows the general weakness (in my not remotely humble opinion) of opening stdlib modules! List.( ... ) will probably be safer if you use several symbols from List in a single expression and otherwise for single symbols, List.length is much clearer than length, etc.

let init len f =
if len < 0 then invalid_arg "List.init" else
if len > 10_000 then rev (init_tailrec_aux [] 0 len f)

This comment has been minimized.

@hhugo

hhugo Jan 15, 2018

Contributor

This will stack-overflow when compiled to JavaScript with js_of_ocaml (because the javascript stack is much smaller). Any chance to lower the limit to ~100 ?

@hhugo

hhugo Jan 15, 2018

Contributor

This will stack-overflow when compiled to JavaScript with js_of_ocaml (because the javascript stack is much smaller). Any chance to lower the limit to ~100 ?

This comment has been minimized.

@Gbury

Gbury Jan 15, 2018

Maybe the constant could depend on Sys.backend_type (cf https://github.com/ocaml/ocaml/blob/trunk/stdlib/sys.mli#L99 ) ?

@Gbury

Gbury Jan 15, 2018

Maybe the constant could depend on Sys.backend_type (cf https://github.com/ocaml/ocaml/blob/trunk/stdlib/sys.mli#L99 ) ?

This comment has been minimized.

@dra27

dra27 Jan 15, 2018

Contributor

It's not wholly clear from its status, but this PR was merged, so please could this go in a new PR or in a Mantis report?

@dra27

dra27 Jan 15, 2018

Contributor

It's not wholly clear from its status, but this PR was merged, so please could this go in a new PR or in a Mantis report?

This comment has been minimized.

@damiendoligez

damiendoligez Apr 5, 2018

Member

Reported at MPR#7764.

@damiendoligez

This comment has been minimized.

@hhugo

hhugo Apr 5, 2018

Contributor
@hhugo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment