It seems I had misunderstood what the current implementation does when -nolabel is given.
I believed that it was preventing arguments from being reordered during applications, and furthermore requiring optional arguments to be given with a label.

As it turns, this is not the case: optional arguments can be reordered, but only them. That is: you can swap an arbitrary number of adjacent optional parameters, as long as nothing else gets reorder.
For instance, given let f ?x ?y ?z () ?a ?b = x; y; z; a; b; (), then all of the following are fine:

let () = f ~x:() ~y:() ~z:() () ~a:() ~b:();; (* 1 *)

let () = f ~y:() ~x:() ~z:() () ~a:() ~b:();;(* 2 *)

let () = f ~x:() ~z:() ~y:() () ~a:() ~b:();; (* 3 *)

let () = f ~x:() ~y:() ~z:() () ~b:() ~a:();; (* 4 *)

But these, are not:

let () = f ~x:() ~y:() () ~z:() ~a:() ~b:();; (* 5 *)
(*                     ^^
   Error: The function applied to this argument has type
         ?z:'a -> unit -> ?a:'b -> ?b:'c -> unit
   This argument cannot be applied without label
*)

let () = f ~x:() ~y:() ~a:() () ~z:() ~b:();; (* 6 *)
(*                        ^^
   Error: The function applied to this argument has type
         ?z:'a -> unit -> ?a:'b -> ?b:'c -> unit
   This argument cannot be applied with label ~a
*)

let () = f ~x:() ~y:() ~z:() ~a:() () ~b:();; (* 7 *)
(*                              ^^
   Error: The function applied to this argument has type
         unit -> ?a:'a -> ?b:'b -> unit
   This argument cannot be applied with label ~a
*)

N.B. this is with 4.10.

The present PR partially breaks this behavior, it forbids optional arguments followed by a non-optional argument to be reordered.
That is, example 4 above is fine, but the following are not:

let () = f ~y:() ~x:() ~z:() () ~a:() ~b:();;(* 2 *)
let () = f ~x:() ~z:() ~y:() () ~a:() ~b:();;(* 3 *)
(*                           ^^
Error: The function applied to this argument has type
         ?x:'a -> ?y:'b -> ?z:'c -> unit -> ?a:'d -> ?b:'e -> unit
This argument cannot be applied without label)
*)

(furthermore, the error message is worse than it could be, but that should be fixable without too much trouble)

My intuition is that this a bug of the current implementation, and that it should be changed. To either always allow reordering arguments given with a label, or always forbid it.
There is of course no test for this behavior. I'm also going to make another wild guess in saying that: I don't think anyone is relying on it.

@garrigue: what do you think?

Could I suggest a potentially simpler solution -- get rid of -nolabels?

Could I suggest a potentially simpler solution -- get rid of -nolabels?

Isn't it used to compile the labeled modules in the stdlib ?

I think it only saves us from having some files that looks like:

...
let foo ~x ~y z = foo x y z
...

that seems lower maintenance than -nolabels itself

-nolabels is about ignoring labels on non-optional arguments in unification, i.e. it works at the types level.
As such, I think that the current implementation is correct.
The precise semantics is described in my PPL 2001 paper: "Labeled and optional arguments for Objective Caml", as -classic mode.
It was introduced as a compatibility mode, when we attempted to add labels to the standard library, so its goal is not to be stricter than the -labels mode.
The addition of labels was not accepted in the end, and this is rather the -labels mode that was made more lax, by allowing full applications without labels (note that the above paper does not describe this relaxed mode).

I agree that almost nobody uses it nowadays, which actually means that people are comfortable with the current flexible label behaviour. That's good.
It has a role in the standard library. Note that to remove it, it is not enough to manually write eta-expansions in the standard library itself, but one would also have to do the inverse operation in tests/lib-stdlabels, which also relies on -nolabels to check that no function was forgotten.
It's not clear to me what is the cost of maintaining it. It's just about understanding what it means.
Another approach would be to deprecate it, by giving it a more private name, to be sure that people do not use it for normal programming.

Sorry not review your PR today, because I have a headache. But tell me when you want me to look at it.

Yet another option could be to keep the -nolabels mode, but with a different semantics, more oriented towards coercion of compatible types than usability. For instance, juste ignore all labels in types (including optional ones), and disallow them in function abstractions and applications.
That would be enough for what is used in stdlib (and possibly elsewhere), and avoid interactions with type_application.

I was indeed wrong in assuming this was a bug, the implementation does match what is described in the paper, which I believe can be sumed up as follows:

When -nolabel is given:

• named and anonymous arguments must be given in the correct order
• named arguments can be given without a label
• optional arguments must be given with a label
• optional arguments can be commuted

This last point, which surprised me before, is an historical choice: it makes it a lot more convenient to use libraries like lablgtk where functions have a lot of optional arguments.
However, -nolabel doesn't appear to be very popular nowadays¹ , and these packages don't seem to be relying on that last behavior.

@garrigue and I talked offline, and I'd like to propose a change to the behavior of -nolabel: forbid optional arguments from being commuted.
This is less disruptive than simply removing -nolabel, and it brings us the same gains in terms of simplifying the implementation of type_application.
Indeed, -nolabel shares the same code path as full applications where all labels are omitted, so we would still need to retain that specific code path; which this proposal simplifies.

I have now updated this PR to:

• add some more tests illustrating the current behavior
• implement the proposed change
• implement some further "low key cleanups" (where have I heard that before?), which shouldn't change the way things are typed or compiled.

1: grepping the source of all opam packages showed only a handful of packages building with -nolabels, all of which (but one) still compiled with the change proposed here. (the only one with a build failure is gen, and it requires a two lines change, to commute two optional arguments)

I reviewed the code. I believe it is simpler and correct (assuming we want the change in the behavior of optional labels in the -nolabel mode), but there too many subtleties to be 100% sure.

Success of the tests increases my confidence :).

the only one with a build failure is gen, and it requires a two lines change, to commute two optional arguments

Ping @c-cube btw, the diff is the following and can be applied now, regardless of whether this PR gets merged or not:

diff --git a/src/gen.ml b/src/gen.ml
index fbf6b0f..2540fd6 100644
--- a/src/gen.ml
+++ b/src/gen.ml
@@ -1902,7 +1902,7 @@ module Restart = struct
       match !cached with
       | Some mlist -> GenMList.to_gen mlist
       | None ->
-          let mlist = GenMList.of_gen_lazy ?caching ?max_chunk_size g in
+          let mlist = GenMList.of_gen_lazy ?max_chunk_size ?caching g in
           cached := Some mlist;
           GenMList.to_gen mlist
 end
@@ -1936,7 +1936,7 @@ let persistent gen =
 *)
 
 let persistent_lazy ?caching ?max_chunk_size gen =
-  let l = GenMList.of_gen_lazy ?caching ?max_chunk_size gen in
+  let l = GenMList.of_gen_lazy ?max_chunk_size ?caching gen in
   fun () -> GenMList.to_gen l
 
 (*$T

What about @garrigue's proposal of simplifying -nolabel (ignoring labels in types, and forbidding them in expressions)? Would it break a lot of code?

While we are at it, I was always a bit puzzled by the fact that it is allowed to drop all labelled arguments on a full application (in particular, I find it confusing that for a f : x:int -> y:int -> int -> int, both f 1 and f 1 2 3 are well typed, but not f 1 2). Is this feature used in practice? I guess this is intended to simplify the choice of adding labels to an existing API without breaking too much client code, but is that still a valid default behavior today? At least, shouldn't warning 6 be enabled by default?

What about @garrigue's proposal of simplifying -nolabel (ignoring labels in types, and forbidding them in expressions)? Would it break a lot of code?

In practice probably not much more than my proposal (there really aren't that many users of -nolabel on opam), if at all.
I don't think it would make the implementation noticeably easier to read, but it does make the semantics slightly simpler. I'm willing to try it out if people like it better.

While we are at it, I was always a bit puzzled by the fact that it is allowed to drop all labelled arguments on a full application [...]. Is this feature used in practice?

I have no idea. I have definitely seen it used in various random places. We would have to build all of opam to check.

At least, shouldn't warning 6 be enabled by default?

I think it should.

I'm in favor of doing first the minimal simple change that you have already implemented and has been approved. Is there something left to do before merging?

On the implementation side no, I think we're good.

In terms of approval I would like @garrigue to confirm that this is reasonable and matches what we discussed.

One last check: what is the error message in gen ?
I.e., I agree that the breakage is minimal, but it should be self-explaining enough that one doesn't have to look at the Changes log to find an explanation.

Also, the paper cited above is basically the only reference which precisely explains how labels work in ocaml. It was already not 100% accurate because of the full-application rule which was added later, but this change requires another patch. High time to revise it, and put it somewhere visible enough.

All in all, I'm not sure rushing this in 4.11 is a good idea. On the other hand, I completely agree that this is a really minor change, which is not going to break anything serious.

I'll write a review after checking the code side-by-side.

@alainfrisch

While we are at it, I was always a bit puzzled by the fact that it is allowed to drop all labelled arguments on a full application (in particular, I find it confusing that for a f : x:int -> y:int -> int -> int, both f 1 and f 1 2 3 are well typed, but not f 1 2). Is this feature used in practice? I guess this is intended to simplify the choice of adding labels to an existing API without breaking too much client code, but is that still a valid default behavior today? At least, shouldn't warning 6 be enabled by default?

I tend to agree with you, as this gives a much cleaner semantics. However, we're not going to discuss this in this PR. You're free to open a new PR for that, as this is an important question.

One last check: what is the error message in gen ?

# File "src/gen.ml", line 1905, characters 44-51:
# 1905 |           let mlist = GenMList.of_gen_lazy ?caching ?max_chunk_size g in
#                                                    ^^^^^^^
# Error: The function applied to this argument has type
#          ?max_chunk_size:int ->
#          ?caching:bool -> 'a GenMList.gen -> 'a GenMList.t
# This argument cannot be applied with label ?caching

All in all, I'm not sure rushing this in 4.11 is a good idea. On the other hand, I completely agree that this is a really minor change, which is not going to break anything serious.

Given that the only opam package broken by this has already been patched, I see no reason to delay the change.
I also doubt that delaying this by 6 months means that it will receive more testing: I don't know how many people test trunk regularly (rough estimate: not that many), but I'd be surprised if (m)any of them were using -nolabels.

We can merge this as soon as it is ready. It was ready yesterday, but now we have to wait for Jacques' review :-)

OK, just to clarify.
The original behaviour of optional arguments in -nolabels was the same as with -labels, except that as one cannot distinguish between (non-optional) labels and unlabelled arguments, it required optional arguments to come in groups. There was an explicit semantics.
The new behaviour is that an optional argument is discarded as soon as an argument with a different label (or no label) is applied at its position.
While this new behaviour kind of makes sense, and has the advantage of maintaining some amount of compatibility (i.e., the breakage in gen is easy to fix), this is a new semantics.
I am concerned with changing the semantics in 4.11 without further discussion.
If there is a consensus that this is the desired behaviour, I'm fine (and the correction I requested is only changing a variable name), but are we sure that we all agree on what are the semantics of this change (as they are not specified explicitly), and that we can have a consensus of everybody concerned over the week-end.

One last check: what is the error message in gen ?

# File "src/gen.ml", line 1905, characters 44-51:
# 1905 |           let mlist = GenMList.of_gen_lazy ?caching ?max_chunk_size g in
#                                                    ^^^^^^^
# Error: The function applied to this argument has type
#          ?max_chunk_size:int ->
#          ?caching:bool -> 'a GenMList.gen -> 'a GenMList.t
# This argument cannot be applied with label ?caching

Reading the message, and re-reading the code, I realize that the semantics is even more involved.
I.e., it is prohibited to discard a parameter if it appears later in the arguments.
I suppose this is an extra security to avoid changing the behaviour on existing code, but it makes it harder to define a simple semantics.
Also, logically the error message should explain the problem.

My understanding was that you (@garrigue) and @trefis had discussed this offline and that you both agreed that the new semantics is a good idea. Given that no one cares about -nolabels anymore, personally I would have thought that it is good enough. If you do not like the new semantics, or if you want to consult a lot more people than whoever is already in this PR, then of course there is no time to do this before 4.11 and you should not rush things. It's your call.

There may have been a misunderstanding. I have indeed said that I am open to a change in semantics, as -nolabels is very rarely used in a meaningful way. But I was not expecting a short time decision, and I had not looked at the details of the code yet. The code is fine, but it showed me that my understanding of this semantics was only partial.

Or I am just too nervous, and it can be proven easily that the new behaviour is only a restriction of the current one?
In this case, it might be fine to be less careful.

Well, no. It would still require that the -nolabels code check for the presence of an unlabelled argument, which it doesn't. Sorry for thinking aloud.

Let's leave @garrigue, who is the author and caretaker of this code, to make further decisions on this PR. I suspect that my comments have only increased the noise in decision-making, and I'll unsubscribe now.

@gasche: I agree that @garrigue should probably have the last word on this topic, but I think it's good (and helpful) that people (Alain and you) spontaneously voiced an opinion on this topic.
Anyway.

@garrigue and I chatted again, and we reached the following conclusion:

• while the behavior of the compiler with this PR was "reasonable" on the example he gave above, it means that this PR is change of semantics, not simply a restricted version of the previous one: his example is accepted with both versions, at a different type.
  So we agreed to change that, and restrict even further: optional arguments cannot be commuted with -nolabels, and can only be eliminated if some unlabelled arguments remain. That change means that his example is now rejected.
  I added the example in a preliminary commit at the beginning of the patch series, and then implemented the restriction.
• in situations where the program would typecheck in -nolabels if optional arguments were allowed to commute, we should enrich the error message to explain that the compiler is stricter since 4.11.
  The last commit of the series does that.
  I'm not a big fan of the message, and it is sometimes displayed in cases that were already rejected before (i.e. cases were the optional argument would need to commute with a non-optional one). But I would say that's acceptable.