Skip to content

fix: Fix some mir related bugs #14705

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

Merged
merged 3 commits into from
May 2, 2023
Merged

fix: Fix some mir related bugs #14705

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3a3c363
fix break-outside-of-loop false positive in try block
HKalbasi May 1, 2023
266ceb7
Fix floating point binop evaluation
HKalbasi May 2, 2023
38544f5
Catch overflow in shift binop evaluation
HKalbasi May 2, 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
3 changes: 2 additions & 1 deletion crates/hir-def/src/body/lower.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -505,6 +505,7 @@ impl ExprCollector<'_> {
.map(|it| Interned::new(TypeRef::from_ast(&this.ctx(), it)));

let prev_is_lowering_generator = mem::take(&mut this.is_lowering_generator);
let prev_try_block_label = this.current_try_block_label.take();

let body = this.collect_expr_opt(e.body());

Expand All @@ -520,11 +521,11 @@ impl ExprCollector<'_> {
} else {
ClosureKind::Closure
};
this.is_lowering_generator = prev_is_lowering_generator;
let capture_by =
if e.move_token().is_some() { CaptureBy::Value } else { CaptureBy::Ref };
this.is_lowering_generator = prev_is_lowering_generator;
this.current_binding_owner = prev_binding_owner;
this.current_try_block_label = prev_try_block_label;
this.body.exprs[result_expr_id] = Expr::Closure {
args: args.into(),
arg_types: arg_types.into(),
Expand Down
18 changes: 16 additions & 2 deletions crates/hir-ty/src/consteval/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,22 @@ fn bit_op() {
check_number(r#"const GOAL: u8 = !0 & !(!0 >> 1)"#, 128);
check_number(r#"const GOAL: i8 = !0 & !(!0 >> 1)"#, 0);
check_number(r#"const GOAL: i8 = 1 << 7"#, (1i8 << 7) as i128);
// FIXME: report panic here
check_number(r#"const GOAL: i8 = 1 << 8"#, 0);
check_number(r#"const GOAL: i8 = -1 << 2"#, (-1i8 << 2) as i128);
check_fail(r#"const GOAL: i8 = 1 << 8"#, |e| {
e == ConstEvalError::MirEvalError(MirEvalError::Panic("Overflow in Shl".to_string()))
});
}

#[test]
fn floating_point() {
check_number(
r#"const GOAL: f64 = 2.0 + 3.0 * 5.5 - 8.;"#,
i128::from_le_bytes(pad16(&f64::to_le_bytes(10.5), true)),
);
check_number(
r#"const GOAL: f32 = 2.0 + 3.0 * 5.5 - 8.;"#,
i128::from_le_bytes(pad16(&f32::to_le_bytes(10.5), true)),
);
}

#[test]
Expand Down
14 changes: 14 additions & 0 deletions crates/hir-ty/src/mir.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -649,6 +649,20 @@ pub enum BinOp {
Offset,
}

impl BinOp {
fn run_compare<T: PartialEq + PartialOrd>(&self, l: T, r: T) -> bool {
match self {
BinOp::Ge => l >= r,
BinOp::Gt => l > r,
BinOp::Le => l <= r,
BinOp::Lt => l < r,
BinOp::Eq => l == r,
BinOp::Ne => l != r,
x => panic!("`run_compare` called on operator {x:?}"),
}
}
}

impl Display for BinOp {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.write_str(match self {
Expand Down
171 changes: 114 additions & 57 deletions crates/hir-ty/src/mir/eval.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ macro_rules! from_bytes {
($ty:tt, $value:expr) => {
($ty::from_le_bytes(match ($value).try_into() {
Ok(x) => x,
Err(_) => return Err(MirEvalError::TypeError("mismatched size")),
Err(_) => return Err(MirEvalError::TypeError(stringify!(mismatched size in constructing $ty))),
}))
};
}
Expand Down Expand Up @@ -797,70 +797,127 @@ impl Evaluator<'_> {
lc = self.read_memory(Address::from_bytes(lc)?, size)?;
rc = self.read_memory(Address::from_bytes(rc)?, size)?;
}
let is_signed = matches!(ty.as_builtin(), Some(BuiltinType::Int(_)));
let l128 = i128::from_le_bytes(pad16(lc, is_signed));
let r128 = i128::from_le_bytes(pad16(rc, is_signed));
match op {
BinOp::Ge | BinOp::Gt | BinOp::Le | BinOp::Lt | BinOp::Eq | BinOp::Ne => {
let r = match op {
BinOp::Ge => l128 >= r128,
BinOp::Gt => l128 > r128,
BinOp::Le => l128 <= r128,
BinOp::Lt => l128 < r128,
BinOp::Eq => l128 == r128,
BinOp::Ne => l128 != r128,
_ => unreachable!(),
};
let r = r as u8;
Owned(vec![r])
if let TyKind::Scalar(chalk_ir::Scalar::Float(f)) = ty.kind(Interner) {
match f {
chalk_ir::FloatTy::F32 => {
let l = from_bytes!(f32, lc);
let r = from_bytes!(f32, rc);
match op {
BinOp::Ge
| BinOp::Gt
| BinOp::Le
| BinOp::Lt
| BinOp::Eq
| BinOp::Ne => {
let r = op.run_compare(l, r) as u8;
Owned(vec![r])
}
BinOp::Add | BinOp::Sub | BinOp::Mul | BinOp::Div => {
let r = match op {
BinOp::Add => l + r,
BinOp::Sub => l - r,
BinOp::Mul => l * r,
BinOp::Div => l / r,
_ => unreachable!(),
};
Owned(r.to_le_bytes().into())
}
x => not_supported!(
"invalid binop {x:?} on floating point operators"
),
}
}
chalk_ir::FloatTy::F64 => {
let l = from_bytes!(f64, lc);
let r = from_bytes!(f64, rc);
match op {
BinOp::Ge
| BinOp::Gt
| BinOp::Le
| BinOp::Lt
| BinOp::Eq
| BinOp::Ne => {
let r = op.run_compare(l, r) as u8;
Owned(vec![r])
}
BinOp::Add | BinOp::Sub | BinOp::Mul | BinOp::Div => {
let r = match op {
BinOp::Add => l + r,
BinOp::Sub => l - r,
BinOp::Mul => l * r,
BinOp::Div => l / r,
_ => unreachable!(),
};
Owned(r.to_le_bytes().into())
}
x => not_supported!(
"invalid binop {x:?} on floating point operators"
),
}
}
}
BinOp::BitAnd
| BinOp::BitOr
| BinOp::BitXor
| BinOp::Add
| BinOp::Mul
| BinOp::Div
| BinOp::Rem
| BinOp::Sub => {
let r = match op {
BinOp::Add => l128.overflowing_add(r128).0,
BinOp::Mul => l128.overflowing_mul(r128).0,
BinOp::Div => l128.checked_div(r128).ok_or_else(|| {
MirEvalError::Panic(format!("Overflow in {op:?}"))
})?,
BinOp::Rem => l128.checked_rem(r128).ok_or_else(|| {
MirEvalError::Panic(format!("Overflow in {op:?}"))
})?,
BinOp::Sub => l128.overflowing_sub(r128).0,
BinOp::BitAnd => l128 & r128,
BinOp::BitOr => l128 | r128,
BinOp::BitXor => l128 ^ r128,
_ => unreachable!(),
};
} else {
let is_signed = matches!(ty.as_builtin(), Some(BuiltinType::Int(_)));
let l128 = i128::from_le_bytes(pad16(lc, is_signed));
let r128 = i128::from_le_bytes(pad16(rc, is_signed));
let check_overflow = |r: i128| {
// FIXME: this is not very correct, and only catches the basic cases.
let r = r.to_le_bytes();
for &k in &r[lc.len()..] {
if k != 0 && (k != 255 || !is_signed) {
return Err(MirEvalError::Panic(format!("Overflow in {op:?}")));
}
}
Owned(r[0..lc.len()].into())
}
BinOp::Shl | BinOp::Shr => {
let shift_amount = if r128 < 0 {
return Err(MirEvalError::Panic(format!("Overflow in {op:?}")));
} else if r128 > 128 {
return Err(MirEvalError::Panic(format!("Overflow in {op:?}")));
} else {
r128 as u8
};
let r = match op {
BinOp::Shl => l128 << shift_amount,
BinOp::Shr => l128 >> shift_amount,
_ => unreachable!(),
};
Owned(r.to_le_bytes()[0..lc.len()].into())
Ok(Owned(r[0..lc.len()].into()))
};
match op {
BinOp::Ge | BinOp::Gt | BinOp::Le | BinOp::Lt | BinOp::Eq | BinOp::Ne => {
let r = op.run_compare(l128, r128) as u8;
Owned(vec![r])
}
BinOp::BitAnd
| BinOp::BitOr
| BinOp::BitXor
| BinOp::Add
| BinOp::Mul
| BinOp::Div
| BinOp::Rem
| BinOp::Sub => {
let r = match op {
BinOp::Add => l128.overflowing_add(r128).0,
BinOp::Mul => l128.overflowing_mul(r128).0,
BinOp::Div => l128.checked_div(r128).ok_or_else(|| {
MirEvalError::Panic(format!("Overflow in {op:?}"))
})?,
BinOp::Rem => l128.checked_rem(r128).ok_or_else(|| {
MirEvalError::Panic(format!("Overflow in {op:?}"))
})?,
BinOp::Sub => l128.overflowing_sub(r128).0,
BinOp::BitAnd => l128 & r128,
BinOp::BitOr => l128 | r128,
BinOp::BitXor => l128 ^ r128,
_ => unreachable!(),
};
check_overflow(r)?
}
BinOp::Shl | BinOp::Shr => {
let r = 'b: {
if let Ok(shift_amount) = u32::try_from(r128) {
let r = match op {
BinOp::Shl => l128.checked_shl(shift_amount),
BinOp::Shr => l128.checked_shr(shift_amount),
_ => unreachable!(),
};
if let Some(r) = r {
break 'b r;
}
};
return Err(MirEvalError::Panic(format!("Overflow in {op:?}")));
};
check_overflow(r)?
}
BinOp::Offset => not_supported!("offset binop"),
}
BinOp::Offset => not_supported!("offset binop"),
}
}
Rvalue::Discriminant(p) => {
Expand Down
18 changes: 18 additions & 0 deletions crates/ide-diagnostics/src/handlers/break_outside_of_loop.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,24 @@ fn test() {
// ^^^^^^^ error: can't break with a value in this position
}
}
"#,
);
}

#[test]
fn try_block_desugaring_inside_closure() {
// regression test for #14701
check_diagnostics(
r#"
//- minicore: option, try
fn test() {
try {
|| {
let x = Some(2);
Some(x?)
};
};
}
"#,
);
}
Expand Down