Mantis Bug Tracker

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0006352OCamlOCaml typingpublic2014-03-24 10:482014-04-03 09:35
Assigned Togarrigue 
PrioritynormalSeverityminorReproducibilityhave not tried
PlatformOSOS Version
Product Version 
Target VersionFixed in Version4.02.0+dev 
Summary0006352: Automatic removal of optional arguments and sequencing
DescriptionOne expects ((); e) to be equivalent to e for type-checking purposes. The following illustrates that this is not the case:

# let foo (f : unit -> unit) = ();;
val foo : (unit -> unit) -> unit = <fun>
# let g ?x () = ();;
val g : ?x:'a -> unit -> unit = <fun>
# foo g;;
- : unit = ()
# foo ((); g);;
Error: This expression has type ?x:'a -> unit -> unit
       but an expression was expected of type unit -> unit

This form (with something more interesting than () on the l.h.s of the sequence) appeared while instrumenting the code with a locally modified version of Bisect (I don't know if this would also be the case with the official version).

A possible fix is to traverse Pexp_sequence in the definition of is_inferred in Typecore.type_argument:

  let rec is_inferred sexp =
    match sexp.pexp_desc with
      Pexp_ident _ | Pexp_apply _ | Pexp_send _ | Pexp_field _ -> true
    | Pexp_open (_, _, e) -> is_inferred e
    | Pexp_sequence (_, e) -> is_inferred e
    | _ -> false

Is is safe to do so?

(I'm not particularly a fan of the automatic removal of optional arguments, but as long as we have it, we should try to make it robust.)
TagsNo tags attached.
Attached Files

- Relationships
related to 0005748assignedgarrigue anonymous functions with optional arguments 

-  Notes
garrigue (manager)
2014-03-26 01:35

This problem is a variant of 0005748: due to a tension between inference and propagation, discarding only works for some syntactic forms.
Modifying is_inferred in the way you suggest is safe, but adding cases one by one is just making the criterion less clear.
(And having a sequence inside a function argument does seem wrong for anything other than automatically generated code.)
At the cost of some cluttering (passing extra arguments to type_expect), we could be much more agressive.
Another solution would be, as suggested in 0005748, to support just "let in", as most cases should be convertible to that form, and it disappears during the translation to lambda.
frisch (developer)
2014-03-26 11:31

> At the cost of some cluttering (passing extra arguments to type_expect), we could be much more agressive.

I don't think that this feature justifies adding complexity to the type-checker.
Personally, I'd be fine with something which fixes the sequence case (and also, even if it makes the criterion slightly more complex (but is this documented anyway?).

In the longer term, what about deprecating the feature? At least, we could introduce a warning. (In addition to being fragile, the feature also creates allocations where user might not expect.)
frisch (developer)
2014-03-26 19:12

Commit 14498 adds a new warning (48) to report uses of this feature (implicit removal of optional arguments). I've eliminated all uses in the compiler code base itself and enabled this warning.

*If* we decide to deprecate the feature in the future, this will be a big help for user to prepare in advance. Otherwise, people can decide not to use the feature on their own code base.
garrigue (manager)
2014-03-27 02:51

This feature is essential for lablgtk.
So I don't see how we could remove it.
The only goal of this warning should be debugging purposes, and I don't much like the idea of even having it as it will be triggered by any lablgtk code.
frisch (developer)
2014-03-27 22:15

I interpreted one of your comment on 0005748 -- "I was not aware that this feature was widely used" -- as the recognition that the feature was not essential.

> The only goal of this warning should be debugging purposes

Well, since the feature has a "somewhat adhoc behavior" and can lead to surprising result (such as a simple reference to an identifier forcing some allocation -- plus the behavior reported in this ticket and 0005748), it doesn't sound crazy to decide to reject it in some code bases.

There are already warnings which can turn out to be incompatible with the style imposed by a library on the client code (e.g. warning 42). Warnings are "opt-in", and any project enabling all warnings by default should be prepared to disable some of them at some point.

But if you feel strongly about it, I'll revert the addition of the warning. (Considering how non-intrusive it is, I'll consider keeping it for LexiFi's version.)
garrigue (manager)
2014-03-27 23:50

Sorry, the meaning was "I didn't know it was widely used outside of lablgtk".
Debugging warnings are not a bad idea in itself.
But it would be better if they were not included in A, so that people using A do not have to disable them one by one.
This is not a new problem.
frisch (developer)
2014-03-28 09:20

What do you call debugging warnings? For me it's rather a matter of choosing whether to accept the feature or not in a given code base, rather than punctual debugging.

Out of curiosity, can you elaborate on the typical use of the feature in lablgtk client-code?
garrigue (manager)
2014-03-31 10:33

OK, feature warning may be a better naming.
I.e., a warning about some perfectly legitimate feature, and in this case a feature that is already in relatively wide use.

In the examples directory of lablgtk, I get 178 occurrences of the warning.
Most are passing a partially applied version of "parent#pack" to the ?packing parameter of the child.
Various other methods can also be used for this parameter.
There are also some uses with the ?callback parameter.

This is a rather general programming pattern, which is in agreement with the formal dynamic semantics of optional arguments. The restrictions needed to make it work with typing are a bit unfortunate, but this doesn't negate its usefulness.
It would be interesting to know where it is used in the wild.
Outside of Martin Jambon's bug report, I remember Jun Furuse using it with datatypes rather than functions, but I didn't go and check around.

By the way, you say that you removed uses inside the distribution. Where was that?
frisch (developer)
2014-04-02 22:44

> By the way, you say that you removed uses inside the distribution. Where was that?

Inside the compiler part, at least. This were commits 14495, 14496, 14497. A few isolated cases, and a repeated on in ctype, because of the optional arguments on the copy function (used as " copy ...").
frisch (developer)
2014-04-02 22:44

Jacques: if you prefer me to remove the new warning, let me know!
garrigue (manager)
2014-04-03 06:06

Fix in trunk at revision 14524.

After some hesitation, I have adopted the following definition for is_inferred,
which covers all cases where no inward propagation occurs anyway.
Note that let and match cannot be included, because of GADTs.

 let rec is_inferred sexp =
    match sexp.pexp_desc with
      Pexp_ident _ | Pexp_apply _ | Pexp_field _ | Pexp_constraint _
    | Pexp_coerce _ | Pexp_send _ | Pexp_new _ -> true
    | Pexp_sequence (_, e) | Pexp_open (_, _, e) -> is_inferred e
    | Pexp_ifthenelse (_, e1, Some e2) -> is_inferred e1 && is_inferred e2
    | _ -> false

Concerning the new warning, I'm ambivalent.
Clearly these coercions do something (even if they keep the semantics),
and as shown by their use in the compiler they allow leaving some use places
unchanged when the definition changes. But this is the goal.
I would say that to really introduce a new warning, there should be a case
where these coercions caused a bug to go unnoticed, or at least to generate
an error message very hard to understand.
garrigue (manager)
2014-04-03 06:11

Forgot to change status...
frisch (developer)
2014-04-03 09:35

> I would say that to really introduce a new warning...

I discovered the problem while instrumenting the code with Bisect. This is fixed now (Pexp_sequence), but I can imagine other local rewritings that could break it (such as turning "e1; e2" into "let () = e1 in e2", for whatever reason). I don't have any big problem with the feature, but since I don't find it particularly useful, and a little fragile, I'd prefer to avoid depending on it for our code base, hence the warning. As said, I'm perfectly fine with maintaining it locally, if needed.

- Issue History
Date Modified Username Field Change
2014-03-24 10:48 frisch New Issue
2014-03-26 01:25 garrigue Relationship added related to 0005748
2014-03-26 01:35 garrigue Note Added: 0011100
2014-03-26 01:35 garrigue Assigned To => garrigue
2014-03-26 01:35 garrigue Status new => confirmed
2014-03-26 11:09 frisch Description Updated View Revisions
2014-03-26 11:31 frisch Note Added: 0011102
2014-03-26 19:12 frisch Note Added: 0011108
2014-03-27 02:51 garrigue Note Added: 0011110
2014-03-27 22:15 frisch Note Added: 0011112
2014-03-27 23:50 garrigue Note Added: 0011114
2014-03-28 09:20 frisch Note Added: 0011120
2014-03-31 10:33 garrigue Note Added: 0011137
2014-04-02 22:44 frisch Note Added: 0011181
2014-04-02 22:44 frisch Note Added: 0011182
2014-04-03 06:06 garrigue Note Added: 0011195
2014-04-03 06:11 garrigue Note Added: 0011196
2014-04-03 06:11 garrigue Status confirmed => resolved
2014-04-03 06:11 garrigue Fixed in Version => 4.02.0+dev
2014-04-03 06:11 garrigue Resolution open => fixed
2014-04-03 09:35 frisch Note Added: 0011197

Copyright © 2000 - 2011 MantisBT Group
Powered by Mantis Bugtracker