-
Notifications
You must be signed in to change notification settings - Fork 90
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
Improve type variables name allocation when reporting type errors #1512
Conversation
The implementation of the conversion from unification type to human-readable types was a bit confused, using two name tables, not updating correctly the set of already used variables, etc. This commit simplifies the implementation, move free-standing functions into `NameReg`, with the effect of correctly propagating position information for user-written type variables and thus improving the error reporting for the variable level mismath error.
core/src/typecheck/error.rs
Outdated
pub fn into_typecheck_err(self, state: &mut State, pos_opt: TermPos) -> TypecheckError { | ||
let mut names = reporting::NameReg::new(std::mem::take(state.names)); | ||
self.into_typecheck_err_(state, &mut names, pos_opt) |
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.
This function now replaces the NameReg
in state
by an empty one and calls into_typecheck_err_
with the old, filled one. This might end up being very surprising. I wouldn't expect that into_typecheck_err
removes name allocations from the state. Maybe it would make more sense to take ownership of state
?
In any case, the doc-comment needs to be updated.
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.
Good point, but doing that at the caller site might be very annoying, because State
is taken by reference everywhere, so we would end up with a lot of std::mem::take
(EDIT: because into_typecheck_err
is called a lot)
In practice, when you call to into_typecheck_err
, you go the error path, so you don't need the state anymore. But you're right that the behavior isn't necessarily natural. Maybe cloning is the path of least resistance. Another solution is to separate the name tables between the one coming from state, that would be immutably borrowed, and the one corresponding to generated names, that is owned, but that also sounds like more boilerplate (lifetimes, wrapper around get
, etc.) just to avoid one clone.
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'll just clone, it's the simplest.
The implementation of the conversion from unification type to human-readable types was a bit confused, using two name tables without really making it clear which table corresponds to which. The implementation didn't correctly update correctly the set of variables already in use, and failed to propagate position information for user-written type variables.
This PR simplifies the implementation, moving free-standing functions into
NameReg
, diminishing the number of arguments and gets rid of the additional table, with the effect of correctly propagating position information for user-written type variables.This improves the error reporting of the variable level mismatch error. Previously, the offending type variable wasn't pointed at, but after this PR, it is: