typecore: remove redundant try … with#1881
Conversation
trefis
left a comment
There was a problem hiding this comment.
I just looked at the first one with Leo today and we agree that the try with was never useful.
And as discussed previously the other one has also been unused for a while.
|
Note that I agree with your point that the error message about the scope escape could be better, but that's actually the case of all the scope escape messages, as you well know. So I think it's fine to remove this dead code, leaving a comment explaining that the error message could be improved (which I believe you've already started working on). |
|
Well if it is a bug to have a piece of dead code, one solution is to remove it and the other is to resurrect it. In this situation it's not clear that removing is the right thing to do, if we believe that it would be nicer to resurrect it -- removing feels like treating the symptom rather than the cure. We happen to have @Octachron around as a motivated doctor, so why not go for the right treatment right away? |
|
In this case, I don't think there is any benefit to trying to resurrect it. The first one has never been exercised, and I think it actually prints the wrong error message. The second one has been replaced with a better error -- the message may be more generic, but it is likely to have a much better location. |
|
To clarify (because my previous message might have been unclear on that point): I don't think resurrecting the previous error would be nicer, I'm just saying that we should work towards improving scope escape error messages in general (and it is hopefully obvious to everyone why we don't do that right away). |
|
@gasche: I don't think that the The problem is that currently it is hard to either add such contextual information to the unification trace, or inspect the unification trace to see if the information is pertinent. I am hoping to improve this point first and only then go back to this specific error message. It probably makes sense to remove the dead code meanwhile; I will do so in a separate PR. |
|
ok, ok :-) |
This PR proposes to remove two possibly redundant
try … withclauses intyping/typecore.ml.The first commit removes a
try … withclause in the typechecking of coercion that was added while fixing MPR#7260. However, the example added with this fix does not raise such an exception.The second commit removes the
try … withclause used when typechecking localmodule bindings. This clause wrap around a call to
unify_var env newvarthat is called subsequently to a call to
type_expect. As far as I can tell (and after a discussion with @trefis and @Drup ), this call totype_expectwould have raised an exception earlier ifunify_varwere to raise an exception itself, thus thetry ... withstatement is redundant.This removal makes clear that the error message for locally bound module (
Scoping_let_module) has been unheard of since 4.00, i.e. the prime example of this errorraises a generic scope escape message since 4.00:
I have therefore updated the comment about this error message to reflect that it is currently dead code but it might be nice to restore it.
@lpw25 , @garrigue : would you have the time to comment on those minor issues?