Skip to content
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

Automatic removal of optional arguments and sequencing #6352

Closed
vicuna opened this Issue Mar 24, 2014 · 13 comments

Comments

Projects
None yet
2 participants
@vicuna
Copy link
Collaborator

vicuna commented Mar 24, 2014

Original bug ID: 6352
Reporter: @alainfrisch
Assigned to: @garrigue
Status: closed (set by @xavierleroy on 2015-12-11T18:26:30Z)
Resolution: fixed
Priority: normal
Severity: minor
Fixed in version: 4.02.0+dev
Category: typing
Related to: #5748
Monitored by: jmeber @hcarty

Bug description

One 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 = 
# let g ?x () = ();;
val g : ?x:'a -> unit -> unit = 
# 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
  in

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.)

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Mar 26, 2014

Comment author: @garrigue

This problem is a variant of #5748: 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 #5748, to support just "let in", as most cases should be convertible to that form, and it disappears during the translation to lambda.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Mar 26, 2014

Comment author: @alainfrisch

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 let...in), 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.)

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Mar 26, 2014

Comment author: @alainfrisch

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.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Mar 27, 2014

Comment author: @garrigue

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.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Mar 27, 2014

Comment author: @alainfrisch

I interpreted one of your comment on #5748 -- "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 #5748), 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.)

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Mar 27, 2014

Comment author: @garrigue

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.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Mar 28, 2014

Comment author: @alainfrisch

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?

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Mar 31, 2014

Comment author: @garrigue

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?

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Apr 2, 2014

Comment author: @alainfrisch

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 "List.map copy ...").

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Apr 2, 2014

Comment author: @alainfrisch

Jacques: if you prefer me to remove the new warning, let me know!

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Apr 3, 2014

Comment author: @garrigue

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.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Apr 3, 2014

Comment author: @garrigue

Forgot to change status...

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Apr 3, 2014

Comment author: @alainfrisch

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.