Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve type variables name allocation when reporting type errors #1512

Merged
merged 3 commits into from
Aug 9, 2023
Merged

Improve type variables name allocation when reporting type errors #1512

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4f9aabf
Improve type error reporting
yannham Aug 8, 2023
26c79ee
Do not alter state in into_typecheck_err
yannham Aug 8, 2023
42d1d31
Fix doc linking to private item
yannham Aug 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions core/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2128,7 +2128,7 @@ impl IntoDiagnostics<FileId> for TypecheckError {
let mut labels = mk_expr_label(&pos);

if let Some(span) = constant.pos.into_opt() {
labels.push(secondary(&span).with_message("this polymorphic type"));
labels.push(secondary(&span).with_message("this polymorphic type variable"));
}

vec![Diagnostic::error()
Expand All @@ -2142,7 +2142,7 @@ impl IntoDiagnostics<FileId> for TypecheckError {
format!(
"The type of this expression escapes the scope of the \
corresponding `forall` and can't be generalized to the \
polymorphic type `{constant}`"
polymorphic type variable `{constant}`"
),
])]
}
Expand Down
74 changes: 30 additions & 44 deletions core/src/typecheck/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,9 @@ impl UnifError {
///
/// Wrapper that calls [`Self::into_typecheck_err_`] with an empty [name
/// registry][reporting::NameReg].
pub fn into_typecheck_err(self, state: &State, pos_opt: TermPos) -> TypecheckError {
self.into_typecheck_err_(state, &mut reporting::NameReg::new(), pos_opt)
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)
Copy link
Contributor

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.

Copy link
Member Author

@yannham yannham Aug 8, 2023 •
edited
Loading

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.

Copy link
Member Author

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.

}

/// Convert a unification error to a typechecking error.
Expand All @@ -136,105 +137,90 @@ impl UnifError {
pub fn into_typecheck_err_(
self,
state: &State,
names: &mut reporting::NameReg,
names_reg: &mut reporting::NameReg,
pos_opt: TermPos,
) -> TypecheckError {
match self {
UnifError::TypeMismatch(ty1, ty2) => TypecheckError::TypeMismatch(
reporting::to_type(state.table, state.names, names, ty1),
reporting::to_type(state.table, state.names, names, ty2),
names_reg.to_type(state.table, ty1),
names_reg.to_type(state.table, ty2),
pos_opt,
),
UnifError::RowMismatch(ident, uty1, uty2, err) => TypecheckError::RowMismatch(
ident,
reporting::to_type(state.table, state.names, names, uty1),
reporting::to_type(state.table, state.names, names, uty2),
Box::new((*err).into_typecheck_err_(state, names, TermPos::None)),
names_reg.to_type(state.table, uty1),
names_reg.to_type(state.table, uty2),
Box::new((*err).into_typecheck_err_(state, names_reg, TermPos::None)),
pos_opt,
),
// TODO: for now, failure to unify with a type constant causes the same error as a
// usual type mismatch. It could be nice to have a specific error message in the
// future.
UnifError::ConstMismatch(k, c1, c2) => TypecheckError::TypeMismatch(
reporting::to_type(
state.table,
state.names,
names,
UnifType::from_constant_of_kind(c1, k),
),
reporting::to_type(
state.table,
state.names,
names,
UnifType::from_constant_of_kind(c2, k),
),
names_reg.to_type(state.table, UnifType::from_constant_of_kind(c1, k)),
names_reg.to_type(state.table, UnifType::from_constant_of_kind(c2, k)),
pos_opt,
),
UnifError::WithConst(VarKindDiscriminant::Type, c, ty) => TypecheckError::TypeMismatch(
reporting::to_type(state.table, state.names, names, UnifType::Constant(c)),
reporting::to_type(state.table, state.names, names, ty),
names_reg.to_type(state.table, UnifType::Constant(c)),
names_reg.to_type(state.table, ty),
pos_opt,
),
UnifError::WithConst(kind, c, ty) => TypecheckError::ForallParametricityViolation {
kind,
tail: reporting::to_type(
state.table,
state.names,
names,
UnifType::from_constant_of_kind(c, kind),
),
violating_type: reporting::to_type(state.table, state.names, names, ty),
tail: names_reg.to_type(state.table, UnifType::from_constant_of_kind(c, kind)),
violating_type: names_reg.to_type(state.table, ty),
pos: pos_opt,
},
UnifError::IncomparableFlatTypes(rt1, rt2) => {
TypecheckError::IncomparableFlatTypes(rt1, rt2, pos_opt)
}
UnifError::MissingRow(id, uty1, uty2) => TypecheckError::MissingRow(
id,
reporting::to_type(state.table, state.names, names, uty1),
reporting::to_type(state.table, state.names, names, uty2),
names_reg.to_type(state.table, uty1),
names_reg.to_type(state.table, uty2),
pos_opt,
),
UnifError::MissingDynTail(uty1, uty2) => TypecheckError::MissingDynTail(
reporting::to_type(state.table, state.names, names, uty1),
reporting::to_type(state.table, state.names, names, uty2),
names_reg.to_type(state.table, uty1),
names_reg.to_type(state.table, uty2),
pos_opt,
),
UnifError::ExtraRow(id, uty1, uty2) => TypecheckError::ExtraRow(
id,
reporting::to_type(state.table, state.names, names, uty1),
reporting::to_type(state.table, state.names, names, uty2),
names_reg.to_type(state.table, uty1),
names_reg.to_type(state.table, uty2),
pos_opt,
),
UnifError::ExtraDynTail(uty1, uty2) => TypecheckError::ExtraDynTail(
reporting::to_type(state.table, state.names, names, uty1),
reporting::to_type(state.table, state.names, names, uty2),
names_reg.to_type(state.table, uty1),
names_reg.to_type(state.table, uty2),
pos_opt,
),
UnifError::RowConflict(id, uty, left, right) => TypecheckError::RowConflict(
id,
reporting::to_type(state.table, state.names, names, uty),
reporting::to_type(state.table, state.names, names, left),
reporting::to_type(state.table, state.names, names, right),
names_reg.to_type(state.table, uty),
names_reg.to_type(state.table, left),
names_reg.to_type(state.table, right),
pos_opt,
),
UnifError::UnboundTypeVariable(ident) => TypecheckError::UnboundTypeVariable(ident),
err @ UnifError::CodomainMismatch(_, _, _)
| err @ UnifError::DomainMismatch(_, _, _) => {
let (expd, actual, path, err_final) = err.into_type_path().unwrap();
TypecheckError::ArrowTypeMismatch(
reporting::to_type(state.table, state.names, names, expd),
reporting::to_type(state.table, state.names, names, actual),
names_reg.to_type(state.table, expd),
names_reg.to_type(state.table, actual),
path,
Box::new(err_final.into_typecheck_err_(state, names, TermPos::None)),
Box::new(err_final.into_typecheck_err_(state, names_reg, TermPos::None)),
pos_opt,
)
}
UnifError::VarLevelMismatch {
constant_id,
var_kind,
} => TypecheckError::VarLevelMismatch {
type_var: reporting::cst_name(state.names, names, constant_id, var_kind),
type_var: names_reg.gen_cst_name(constant_id, var_kind),
pos: pos_opt,
},
}
Expand Down
Loading
Loading