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
More partial application warnings #9560
Conversation
234bdf4
to
734ccf5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_expect
fails with aExpr_type_clash
on an application which is partial, warning 5 gets reported in addition to the error.