How to get a value from a for loop in OCaml

Question

I written the following code to randomly select items n items from a list and place the selections in a new list The code is below

let random_extract list n =
    let l =  ((List.length list)- 1) 
    and acc = []  in
    for i = 1 to n do 
        (List.nth list (Random.int l) ) :: acc  (* Line 447 *)
    done;
    acc
;;

When I load the file that contains this code I get the following error

File "all_code.ml", line 447, characters 2-40:
Warning 10: this expression should have type unit.
val random_extract : 'a list -> int -> 'b list = <fun>

Two questions , Question 1: what does this warning mean.

Secondly when I run this function I dont get the list expected but an empty List.

Question 2: how do I get the value of acc from inside the for loop

Answer 1

What Jeffrey Scofield said, and also:

let random_extract list n =
let l =  ((List.length list)- 1) 
and acc = ref []  in
for i = 1 to n do 
    acc := (List.nth list (Random.int l) ) :: !acc  (* Line 447 *)
done;
!acc

If you are going to use a for loop for something like that, then acc must be a reference, i.e., a mutable value. In Ocaml and ML in general let x = ... defines an immutable value. Next, when you wrote (List.nth list (Random.int l)) :: acc, that just meant "look, I can make a list". Nowhere did you say that the newly constructed list has to be assigned to anything, in particular it was not assigned to acc (which it couldn't, because acc is immutable). Ocaml issued a warning because it saw that the body of your for loop produces a list, but Ocaml knows that the body of a for loop ought to produce a unit ("void" in C/C++/Java mis-terminology).

Using a for loop like that is ugly. The Right Way is to do it like this:

let random_extract lst =
  let l = List.length lst - 1 in
  let rec extract = function
   | 0 -> []
   | n -> List.nth lst (Random.int l) :: extract (n - 1)
  in
    extract

If you are new to Ocaml you really should invest time to understand the above code completely. After that, you should study the following code, and make sure you understand why it is more efficient that the first version (look up "tail recursion"):

let random_extract lst =
  let l = List.length lst - 1 in
  let rec extract acc = function
    | 0 -> acc
    | n -> extract (List.nth lst (Random.int l) :: acc) (n - 1)
  in
    extract []

Answer 2

It looks to me like you're starting to work with OCaml and have an imperative programming background. Probably the best advice for thinking about OCaml (and functional programming in general) is to start to think of all your program constructs as expressions, i.e., things with values. In imperative programming, many things (like a for loop, a while loop, an if statement) don't have values, they just "do" things. In functional programming, everything has a value.

Since OCaml isn't a pure functional language, one value is reserved to represent an expression that just "does" something. The value is written like this: (). The type of this value is known as unit (it is the only value of the type unit).

Since a for loop is intended to just "do" things, even in OCaml, the expression inside the loop should have type unit. But you've written something that has a list type. Anything that looks like x :: y must be a list, because the :: constructor creates a list. That's what the compiler is warning you about. The inside of your loop should have type unit, but it has a list type.

Your second problem is that the expression x :: y does not change the value of y. This is the essence of functional programming. All the expression does is compute a new value (a list). It doesn't change any of the values that appear in the expression. Because of this, acc ends up with the same value it started with.

I would really suggest that you solve this problem using recursion (as suggested by Basile Starynkevitch), rather than trying to use imperative constructs like the for loop. It will result in much more idiomatic OCaml code. If you really need to use a for loop for some reason, you need to make your acc variable be a mutable value, and you need to mutate (change) it in the loop.