Conversation
234bdf4 to
734ccf5
Compare
gasche
left a comment
There was a problem hiding this comment.
I reviewed the code and believe it is correct (but see comment below). I am inclined to say that the feature is a good idea, and I trust @stedolan and @whitequark's judgment, but I also know that this is a general area @trefis has given more though about, so he might want to give his opinion as well (if he has time).
typing/typecore.ml
Outdated
| try rue exp | ||
| with Error (_, _, Expr_type_clash _) as err -> | ||
| check_partial_application false exp; | ||
| raise err |
There was a problem hiding this comment.
I agree that the patch seems correct, but I worry (slightly) about trashing of backtraces that may occur during check_partial_application (it uses Format, etc.). We could use the get_raw_backtrace / raise_with_backtrace dance to avoid this, but it makes the code much less pleasant. We could almost use Misc.try_finally, but unfortunately its exceptionally clause does not take the exception as an input, so this is not quite possible. My perfectionist self would recommend tweaking try_finally in this way and using it here, or defining a new Misc combinator for this.
There was a problem hiding this comment.
There's now a shiny new Misc.reraise_preserving_backtrace
(I consider the behaviour of overwriting the global backtrace to be a bug, hopefully one day we can delete reraise_preserving_backtrace as it will just be how all reraises act)
|
I'm not sure I have really thought more than anyone else about this. But anyway, I like this proposal :) |
gasche
left a comment
There was a problem hiding this comment.
Approved, but see my non-trivial code comment.
|
Thanks! Merging. |
As #7221 points out, warning 5 (on maybe-unintended partial applications) would be useful in more situtations, e.g:
This patch makes it report this instead:
Concretely, whenever a
type_expectfails with aExpr_type_clashon an application which is partial, warning 5 gets reported in addition to the error.