core: improve binop and cmpop error messages #439

Merged
sb10q merged 1 commits from improve-error-432 into master 2024-08-17 17:37:21 +08:00
5 changed files with 261 additions and 105 deletions

View File

@ -1202,11 +1202,11 @@ pub fn gen_binop_expr_with_values<'ctx, G: CodeGenerator>(
{ {
let llvm_usize = generator.get_size_type(ctx.ctx); let llvm_usize = generator.get_size_type(ctx.ctx);
if is_aug_assign { if op.variant == BinopVariant::AugAssign {
todo!("Augmented assignment operators not implemented for lists") todo!("Augmented assignment operators not implemented for lists")
} }
match op { match op.base {
Operator::Add => { Operator::Add => {
debug_assert_eq!(ty1.obj_id(&ctx.unifier), Some(PrimDef::List.id())); debug_assert_eq!(ty1.obj_id(&ctx.unifier), Some(PrimDef::List.id()));
debug_assert_eq!(ty2.obj_id(&ctx.unifier), Some(PrimDef::List.id())); debug_assert_eq!(ty2.obj_id(&ctx.unifier), Some(PrimDef::List.id()));

View File

@ -486,18 +486,20 @@ pub fn typeof_binop(
lhs: Type, lhs: Type,
rhs: Type, rhs: Type,
) -> Result<Option<Type>, String> { ) -> Result<Option<Type>, String> {
let op = Binop { base: op, variant: BinopVariant::Normal };
let is_left_list = lhs.obj_id(unifier).is_some_and(|id| id == PrimDef::List.id()); let is_left_list = lhs.obj_id(unifier).is_some_and(|id| id == PrimDef::List.id());
let is_right_list = rhs.obj_id(unifier).is_some_and(|id| id == PrimDef::List.id()); let is_right_list = rhs.obj_id(unifier).is_some_and(|id| id == PrimDef::List.id());
let is_left_ndarray = lhs.obj_id(unifier).is_some_and(|id| id == PrimDef::NDArray.id()); let is_left_ndarray = lhs.obj_id(unifier).is_some_and(|id| id == PrimDef::NDArray.id());
let is_right_ndarray = rhs.obj_id(unifier).is_some_and(|id| id == PrimDef::NDArray.id()); let is_right_ndarray = rhs.obj_id(unifier).is_some_and(|id| id == PrimDef::NDArray.id());
Ok(Some(match op { Ok(Some(match op.base {
Operator::Add | Operator::Sub | Operator::Mult | Operator::Mod | Operator::FloorDiv => { Operator::Add | Operator::Sub | Operator::Mult | Operator::Mod | Operator::FloorDiv => {
if is_left_list || is_right_list { if is_left_list || is_right_list {
if ![Operator::Add, Operator::Mult].contains(&op) { if ![Operator::Add, Operator::Mult].contains(&op.base) {
return Err(format!( return Err(format!(
"Binary operator {} not supported for list", "Binary operator {} not supported for list",
binop_name(op) op.op_info().symbol
)); ));
} }

View File

@ -1,11 +1,14 @@
use std::collections::HashMap; use std::collections::HashMap;
use std::fmt::Display; use std::fmt::Display;
use crate::typecheck::typedef::TypeEnum; use crate::typecheck::{magic_methods::HasOpInfo, typedef::TypeEnum};
use super::typedef::{RecordKey, Type, Unifier}; use super::{
magic_methods::Binop,
typedef::{RecordKey, Type, Unifier},
};
use itertools::Itertools; use itertools::Itertools;
use nac3parser::ast::{Location, StrRef}; use nac3parser::ast::{Cmpop, Location, StrRef};
#[derive(Debug, Clone)] #[derive(Debug, Clone)]
pub enum TypeErrorKind { pub enum TypeErrorKind {
@ -26,6 +29,18 @@ pub enum TypeErrorKind {
expected: Type, expected: Type,
got: Type, got: Type,
}, },
UnsupportedBinaryOpTypes {
operator: Binop,
lhs_type: Type,
rhs_type: Type,
expected_rhs_type: Type,
},
UnsupportedComparsionOpTypes {
operator: Cmpop,
lhs_type: Type,
rhs_type: Type,
expected_rhs_type: Type,
},
FieldUnificationError { FieldUnificationError {
field: RecordKey, field: RecordKey,
types: (Type, Type), types: (Type, Type),
@ -101,6 +116,26 @@ impl<'a> Display for DisplayTypeError<'a> {
let args = missing_arg_names.iter().join(", "); let args = missing_arg_names.iter().join(", ");
write!(f, "Missing arguments: {args}") write!(f, "Missing arguments: {args}")
} }
UnsupportedBinaryOpTypes { operator, lhs_type, rhs_type, expected_rhs_type } => {
let op_symbol = operator.op_info().symbol;
let lhs_type_str = self.unifier.stringify_with_notes(*lhs_type, &mut notes);
let rhs_type_str = self.unifier.stringify_with_notes(*rhs_type, &mut notes);
let expected_rhs_type_str =
self.unifier.stringify_with_notes(*expected_rhs_type, &mut notes);
write!(f, "Unsupported operand type(s) for {op_symbol}: '{lhs_type_str}' and '{rhs_type_str}' (right operand should have type {expected_rhs_type_str})")
}
UnsupportedComparsionOpTypes { operator, lhs_type, rhs_type, expected_rhs_type } => {
let op_symbol = operator.op_info().symbol;
let lhs_type_str = self.unifier.stringify_with_notes(*lhs_type, &mut notes);
let rhs_type_str = self.unifier.stringify_with_notes(*rhs_type, &mut notes);
let expected_rhs_type_str =
self.unifier.stringify_with_notes(*expected_rhs_type, &mut notes);
write!(f, "'{op_symbol}' not supported between instances of '{lhs_type_str}' and '{rhs_type_str}' (right operand should have type {expected_rhs_type_str})")
}
UnknownArgName(name) => { UnknownArgName(name) => {
write!(f, "Unknown argument name: {name}") write!(f, "Unknown argument name: {name}")
} }

View File

@ -4,6 +4,7 @@ use std::iter::once;
use std::ops::Not; use std::ops::Not;
use std::{cell::RefCell, sync::Arc}; use std::{cell::RefCell, sync::Arc};
use super::typedef::OperatorInfo;
use super::{ use super::{
magic_methods::*, magic_methods::*,
type_error::TypeError, type_error::TypeError,
@ -641,6 +642,7 @@ impl<'a> Inferencer<'a> {
obj: Type, obj: Type,
params: Vec<Type>, params: Vec<Type>,
ret: Option<Type>, ret: Option<Type>,
operator_info: Option<OperatorInfo>,
) -> InferenceResult { ) -> InferenceResult {
if let TypeEnum::TObj { params: class_params, fields, .. } = &*self.unifier.get_ty(obj) { if let TypeEnum::TObj { params: class_params, fields, .. } = &*self.unifier.get_ty(obj) {
if class_params.is_empty() { if class_params.is_empty() {
@ -654,6 +656,7 @@ impl<'a> Inferencer<'a> {
ret: sign.ret, ret: sign.ret,
fun: RefCell::new(None), fun: RefCell::new(None),
loc: Some(location), loc: Some(location),
operator_info,
}; };
if let Some(ret) = ret { if let Some(ret) = ret {
self.unifier self.unifier
@ -688,6 +691,7 @@ impl<'a> Inferencer<'a> {
ret, ret,
fun: RefCell::new(None), fun: RefCell::new(None),
loc: Some(location), loc: Some(location),
operator_info,
}); });
self.calls.insert(location.into(), call); self.calls.insert(location.into(), call);
let call = self.unifier.add_ty(TypeEnum::TCall(vec![call])); let call = self.unifier.add_ty(TypeEnum::TCall(vec![call]));
@ -1523,6 +1527,7 @@ impl<'a> Inferencer<'a> {
fun: RefCell::new(None), fun: RefCell::new(None),
ret: sign.ret, ret: sign.ret,
loc: Some(location), loc: Some(location),
operator_info: None,
}; };
self.unifier.unify_call(&call, func.custom.unwrap(), sign).map_err(|e| { self.unifier.unify_call(&call, func.custom.unwrap(), sign).map_err(|e| {
HashSet::from([e.at(Some(location)).to_display(self.unifier).to_string()]) HashSet::from([e.at(Some(location)).to_display(self.unifier).to_string()])
@ -1545,6 +1550,7 @@ impl<'a> Inferencer<'a> {
fun: RefCell::new(None), fun: RefCell::new(None),
ret, ret,
loc: Some(location), loc: Some(location),
operator_info: None,
}); });
self.calls.insert(location.into(), call); self.calls.insert(location.into(), call);
let call = self.unifier.add_ty(TypeEnum::TCall(vec![call])); let call = self.unifier.add_ty(TypeEnum::TCall(vec![call]));
@ -1765,7 +1771,14 @@ impl<'a> Inferencer<'a> {
} }
}; };
self.build_method_call(location, method.into(), left_ty, vec![right_ty], ret) self.build_method_call(
location,
method.into(),
left_ty,
vec![right_ty],
ret,
Some(OperatorInfo::IsBinaryOp { self_type: left.custom.unwrap(), operator: op }),
)
} }
fn infer_unary_ops( fn infer_unary_ops(
@ -1779,7 +1792,14 @@ impl<'a> Inferencer<'a> {
let ret = typeof_unaryop(self.unifier, self.primitives, op, operand.custom.unwrap()) let ret = typeof_unaryop(self.unifier, self.primitives, op, operand.custom.unwrap())
.map_err(|e| HashSet::from([format!("{e} (at {location})")]))?; .map_err(|e| HashSet::from([format!("{e} (at {location})")]))?;
self.build_method_call(location, method, operand.custom.unwrap(), vec![], ret) self.build_method_call(
location,
method,
operand.custom.unwrap(),
vec![],
ret,
Some(OperatorInfo::IsUnaryOp { self_type: operand.custom.unwrap(), operator: op }),
)
} }
fn infer_compare( fn infer_compare(
@ -1825,6 +1845,10 @@ impl<'a> Inferencer<'a> {
a.custom.unwrap(), a.custom.unwrap(),
vec![b.custom.unwrap()], vec![b.custom.unwrap()],
ret, ret,
Some(OperatorInfo::IsComparisonOp {
self_type: left.custom.unwrap(),
operator: *c,
}),
)?); )?);
} }

View File

@ -8,12 +8,15 @@ use std::rc::Rc;
use std::sync::{Arc, Mutex}; use std::sync::{Arc, Mutex};
use std::{borrow::Cow, collections::HashSet}; use std::{borrow::Cow, collections::HashSet};
use nac3parser::ast::{Location, StrRef}; use nac3parser::ast::{Cmpop, Location, StrRef, Unaryop};
use super::magic_methods::Binop;
use super::type_error::{TypeError, TypeErrorKind}; use super::type_error::{TypeError, TypeErrorKind};
use super::unification_table::{UnificationKey, UnificationTable}; use super::unification_table::{UnificationKey, UnificationTable};
use crate::symbol_resolver::SymbolValue; use crate::symbol_resolver::SymbolValue;
use crate::toplevel::{helper::PrimDef, DefinitionId, TopLevelContext, TopLevelDef}; use crate::toplevel::helper::PrimDef;
use crate::toplevel::{DefinitionId, TopLevelContext, TopLevelDef};
use crate::typecheck::magic_methods::OpInfo;
use crate::typecheck::type_inferencer::PrimitiveStore; use crate::typecheck::type_inferencer::PrimitiveStore;
#[cfg(test)] #[cfg(test)]
@ -73,6 +76,28 @@ pub fn iter_type_vars(var_map: &VarMap) -> impl Iterator<Item = TypeVar> + '_ {
var_map.iter().map(|(&id, &ty)| TypeVar { id, ty }) var_map.iter().map(|(&id, &ty)| TypeVar { id, ty })
} }
#[derive(Debug, Clone)]
pub enum OperatorInfo {
/// The call was written as an unary operation, e.g., `~a` or `not a`.
IsUnaryOp {
/// The [`Type`] of the `self` object
self_type: Type,
operator: Unaryop,
},
/// The call was written as a binary operation, e.g., `a + b` or `a += b`.
IsBinaryOp {
/// The [`Type`] of the `self` object
self_type: Type,
operator: Binop,
},
/// The call was written as a binary comparison operation, e.g., `a < b`.
IsComparisonOp {
/// The [`Type`] of the `self` object
self_type: Type,
operator: Cmpop,
},
}
#[derive(Clone)] #[derive(Clone)]
pub struct Call { pub struct Call {
pub posargs: Vec<Type>, pub posargs: Vec<Type>,
@ -80,6 +105,9 @@ pub struct Call {
pub ret: Type, pub ret: Type,
pub fun: RefCell<Option<Type>>, pub fun: RefCell<Option<Type>>,
pub loc: Option<Location>, pub loc: Option<Location>,
/// Details about the associated Python user operator expression of this call, if any.
pub operator_info: Option<OperatorInfo>,
} }
#[derive(Debug, Clone)] #[derive(Debug, Clone)]
@ -627,111 +655,178 @@ impl Unifier {
let TypeEnum::TFunc(signature) = &*self.get_ty(b) else { unreachable!() }; let TypeEnum::TFunc(signature) = &*self.get_ty(b) else { unreachable!() };
// Get details about the input arguments // Get details about the input arguments
let Call { posargs, kwargs, ret, fun, loc } = call; let Call { posargs, kwargs, ret, fun, loc, operator_info } = call;
let num_args = posargs.len() + kwargs.len(); let num_args = posargs.len() + kwargs.len();
// Now we check the arguments against the parameters // Now we check the arguments against the parameters,
// and depending on what `call_info` is, we might change how the behavior `unify_call()`
// in hopes to improve user error messages when type checking fails.
match operator_info {
Some(OperatorInfo::IsBinaryOp { self_type, operator }) => {
// The call is written in the form of (say) `a + b`.
// Technically, it is `a.__add__(b)`, and they have the following constraints:
assert_eq!(posargs.len(), 1);
assert_eq!(kwargs.len(), 0);
assert_eq!(num_params, 1);
// Helper lambdas let other_type = posargs[0]; // the second operand
let mut type_check_arg = |param_name, expected_arg_ty, arg_ty| { let expected_other_type = signature.args[0].ty;
let ok = self.unify_impl(expected_arg_ty, arg_ty, false).is_ok();
if ok { let ok = self.unify_impl(expected_other_type, other_type, false).is_ok();
Ok(()) if !ok {
} else { self.restore_snapshot();
// Typecheck failed, throw an error. return Err(TypeError::new(
self.restore_snapshot(); TypeErrorKind::UnsupportedBinaryOpTypes {
Err(TypeError::new( operator: *operator,
TypeErrorKind::IncorrectArgType { lhs_type: *self_type,
name: param_name, rhs_type: other_type,
expected: expected_arg_ty, expected_rhs_type: expected_other_type,
got: arg_ty, },
}, *loc,
*loc, ));
)) }
} }
}; Some(OperatorInfo::IsComparisonOp { self_type, operator })
if OpInfo::supports_cmpop(*operator) // Otherwise that comparison operator is not supported.
=>
{
// The call is written in the form of (say) `a <= b`.
// Technically, it is `a.__le__(b)`, and they have the following constraints:
assert_eq!(posargs.len(), 1);
assert_eq!(kwargs.len(), 0);
assert_eq!(num_params, 1);
// Check for "too many arguments" let other_type = posargs[0]; // the second operand
if num_params < posargs.len() { let expected_other_type = signature.args[0].ty;
let expected_min_count =
signature.args.iter().filter(|param| param.is_required()).count();
let expected_max_count = num_params;
self.restore_snapshot(); let ok = self.unify_impl(expected_other_type, other_type, false).is_ok();
return Err(TypeError::new( if !ok {
TypeErrorKind::TooManyArguments { self.restore_snapshot();
expected_min_count, return Err(TypeError::new(
expected_max_count, TypeErrorKind::UnsupportedComparsionOpTypes {
got_count: num_args, operator: *operator,
}, lhs_type: *self_type,
*loc, rhs_type: other_type,
)); expected_rhs_type: expected_other_type,
} },
*loc,
// NOTE: order of `param_info_by_name` is leveraged, so use an IndexMap ));
let mut param_info_by_name: IndexMap<StrRef, ParamInfo> = signature }
.args
.iter()
.map(|arg| (arg.name, ParamInfo { has_been_supplied: false, param: arg }))
.collect();
// Now consume all positional arguments and typecheck them.
for (&arg_ty, param) in zip(posargs, signature.args.iter()) {
// We will also use this opportunity to mark the corresponding `param_info` as having been supplied.
let param_info = param_info_by_name.get_mut(&param.name).unwrap();
param_info.has_been_supplied = true;
// Typecheck
type_check_arg(param.name, param.ty, arg_ty)?;
}
// Now consume all keyword arguments and typecheck them.
for (&param_name, &arg_ty) in kwargs {
// We will also use this opportunity to check if this keyword argument is "legal".
let Some(param_info) = param_info_by_name.get_mut(&param_name) else {
self.restore_snapshot();
return Err(TypeError::new(TypeErrorKind::UnknownArgName(param_name), *loc));
};
if param_info.has_been_supplied {
// NOTE: Duplicate keyword argument (i.e., `hello(1, 2, 3, arg = 4, arg = 5)`)
// is IMPOSSIBLE as the parser would have already failed.
// We only have to care about "got multiple values for XYZ"
self.restore_snapshot();
return Err(TypeError::new(
TypeErrorKind::GotMultipleValues { name: param_name },
*loc,
));
} }
_ => {
// Handle [`CallInfo::IsNormalFunctionCall`] and other uninteresting variants
// of [`CallInfo`] (e.g, `CallInfo::IsUnaryOp` and unsupported comparison operators)
param_info.has_been_supplied = true; // Helper lambdas
let mut type_check_arg = |param_name, expected_arg_ty, arg_ty| {
let ok = self.unify_impl(expected_arg_ty, arg_ty, false).is_ok();
if ok {
Ok(())
} else {
// Typecheck failed, throw an error.
self.restore_snapshot();
Err(TypeError::new(
TypeErrorKind::IncorrectArgType {
name: param_name,
expected: expected_arg_ty,
got: arg_ty,
},
*loc,
))
}
};
// Typecheck // Check for "too many arguments"
type_check_arg(param_name, param_info.param.ty, arg_ty)?; if num_params < posargs.len() {
} let expected_min_count =
signature.args.iter().filter(|param| param.is_required()).count();
let expected_max_count = num_params;
// After checking posargs and kwargs, check if there are any self.restore_snapshot();
// unsupplied required parameters, and throw an error if they exist. return Err(TypeError::new(
let missing_arg_names = param_info_by_name TypeErrorKind::TooManyArguments {
.values() expected_min_count,
.filter(|param_info| param_info.param.is_required() && !param_info.has_been_supplied) expected_max_count,
.map(|param_info| param_info.param.name) got_count: num_args,
.collect_vec(); },
if !missing_arg_names.is_empty() { *loc,
self.restore_snapshot(); ));
return Err(TypeError::new(TypeErrorKind::MissingArgs { missing_arg_names }, *loc)); }
}
// Finally, check the Call's return type // NOTE: order of `param_info_by_name` is leveraged, so use an IndexMap
self.unify_impl(*ret, signature.ret, false).map_err(|mut err| { let mut param_info_by_name: IndexMap<StrRef, ParamInfo> = signature
self.restore_snapshot(); .args
if err.loc.is_none() { .iter()
err.loc = *loc; .map(|arg| (arg.name, ParamInfo { has_been_supplied: false, param: arg }))
.collect();
// Now consume all positional arguments and typecheck them.
for (&arg_ty, param) in zip(posargs, signature.args.iter()) {
// We will also use this opportunity to mark the corresponding `param_info` as having been supplied.
let param_info = param_info_by_name.get_mut(&param.name).unwrap();
param_info.has_been_supplied = true;
// Typecheck
type_check_arg(param.name, param.ty, arg_ty)?;
}
// Now consume all keyword arguments and typecheck them.
for (&param_name, &arg_ty) in kwargs {
// We will also use this opportunity to check if this keyword argument is "legal".
let Some(param_info) = param_info_by_name.get_mut(&param_name) else {
self.restore_snapshot();
return Err(TypeError::new(
TypeErrorKind::UnknownArgName(param_name),
*loc,
));
};
if param_info.has_been_supplied {
// NOTE: Duplicate keyword argument (i.e., `hello(1, 2, 3, arg = 4, arg = 5)`)
// is IMPOSSIBLE as the parser would have already failed.
// We only have to care about "got multiple values for XYZ"
self.restore_snapshot();
return Err(TypeError::new(
TypeErrorKind::GotMultipleValues { name: param_name },
*loc,
));
}
param_info.has_been_supplied = true;
// Typecheck
type_check_arg(param_name, param_info.param.ty, arg_ty)?;
}
// After checking posargs and kwargs, check if there are any
// unsupplied required parameters, and throw an error if they exist.
let missing_arg_names = param_info_by_name
.values()
.filter(|param_info| {
param_info.param.is_required() && !param_info.has_been_supplied
})
.map(|param_info| param_info.param.name)
.collect_vec();
if !missing_arg_names.is_empty() {
self.restore_snapshot();
return Err(TypeError::new(
TypeErrorKind::MissingArgs { missing_arg_names },
*loc,
));
}
// Finally, check the Call's return type
self.unify_impl(*ret, signature.ret, false).map_err(|mut err| {
self.restore_snapshot();
if err.loc.is_none() {
err.loc = *loc;
}
err
})?;
} }
err }
})?;
*fun.borrow_mut() = Some(b); *fun.borrow_mut() = Some(b);