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
Comments
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. |
Comment author: @alainfrisch
I don't think that this feature justifies adding complexity to the type-checker. 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.) |
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. |
Comment author: @garrigue This feature is essential for lablgtk. |
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.
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.) |
Comment author: @garrigue Sorry, the meaning was "I didn't know it was widely used outside of lablgtk". |
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? |
Comment author: @garrigue OK, feature warning may be a better naming. In the examples directory of lablgtk, I get 178 occurrences of the warning. 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. By the way, you say that you removed uses inside the distribution. Where was that? |
Comment author: @alainfrisch
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 ..."). |
Comment author: @alainfrisch Jacques: if you prefer me to remove the new warning, let me know! |
Comment author: @garrigue Fix in trunk at revision 14524. After some hesitation, I have adopted the following definition for is_inferred, let rec is_inferred sexp = Concerning the new warning, I'm ambivalent. |
Comment author: @garrigue Forgot to change status... |
Comment author: @alainfrisch
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. |
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:
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:
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.)
The text was updated successfully, but these errors were encountered: