From 3a3c3630a25bdd08db0869022e31f9f380f895f6 Mon Sep 17 00:00:00 2001 From: hkalbasi Date: Tue, 2 May 2023 02:28:14 +0330 Subject: [PATCH 1/3] fix break-outside-of-loop false positive in try block --- crates/hir-def/src/body/lower.rs | 3 ++- .../src/handlers/break_outside_of_loop.rs | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/crates/hir-def/src/body/lower.rs b/crates/hir-def/src/body/lower.rs index d31340fe8f38..611031eb8c41 100644 --- a/crates/hir-def/src/body/lower.rs +++ b/crates/hir-def/src/body/lower.rs @@ -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()); @@ -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(), diff --git a/crates/ide-diagnostics/src/handlers/break_outside_of_loop.rs b/crates/ide-diagnostics/src/handlers/break_outside_of_loop.rs index 89aa437d75d9..7baa7b642682 100644 --- a/crates/ide-diagnostics/src/handlers/break_outside_of_loop.rs +++ b/crates/ide-diagnostics/src/handlers/break_outside_of_loop.rs @@ -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?) + }; + }; +} "#, ); } From 266ceb7b4de6864480877e2dbcb8463d622ac257 Mon Sep 17 00:00:00 2001 From: hkalbasi Date: Tue, 2 May 2023 11:48:04 +0330 Subject: [PATCH 2/3] Fix floating point binop evaluation --- crates/hir-ty/src/consteval/tests.rs | 12 ++ crates/hir-ty/src/mir.rs | 14 +++ crates/hir-ty/src/mir/eval.rs | 174 +++++++++++++++++---------- 3 files changed, 139 insertions(+), 61 deletions(-) diff --git a/crates/hir-ty/src/consteval/tests.rs b/crates/hir-ty/src/consteval/tests.rs index e95e17553631..b700864f7dd9 100644 --- a/crates/hir-ty/src/consteval/tests.rs +++ b/crates/hir-ty/src/consteval/tests.rs @@ -101,6 +101,18 @@ fn bit_op() { check_number(r#"const GOAL: i8 = 1 << 8"#, 0); } +#[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] fn casts() { check_number(r#"const GOAL: usize = 12 as *const i32 as usize"#, 12); diff --git a/crates/hir-ty/src/mir.rs b/crates/hir-ty/src/mir.rs index 4846bbfe5fd7..3ac208666a72 100644 --- a/crates/hir-ty/src/mir.rs +++ b/crates/hir-ty/src/mir.rs @@ -649,6 +649,20 @@ pub enum BinOp { Offset, } +impl BinOp { + fn run_compare(&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 { diff --git a/crates/hir-ty/src/mir/eval.rs b/crates/hir-ty/src/mir/eval.rs index 01d27f2672c2..9811cd9192bf 100644 --- a/crates/hir-ty/src/mir/eval.rs +++ b/crates/hir-ty/src/mir/eval.rs @@ -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))), })) }; } @@ -797,70 +797,122 @@ 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]) - } - 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!(), - }; - 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:?}"))); + 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" + ), } } - 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()) + } 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)); + 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!(), + }; + 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()) + } + BinOp::Offset => not_supported!("offset binop"), } - BinOp::Offset => not_supported!("offset binop"), } } Rvalue::Discriminant(p) => { From 38544f56ab284152cc84363b81b0653d39db2990 Mon Sep 17 00:00:00 2001 From: hkalbasi Date: Tue, 2 May 2023 12:57:34 +0330 Subject: [PATCH 3/3] Catch overflow in shift binop evaluation --- crates/hir-ty/src/consteval/tests.rs | 6 ++-- crates/hir-ty/src/mir/eval.rs | 41 ++++++++++++++++------------ 2 files changed, 27 insertions(+), 20 deletions(-) diff --git a/crates/hir-ty/src/consteval/tests.rs b/crates/hir-ty/src/consteval/tests.rs index b700864f7dd9..5a850f6d5705 100644 --- a/crates/hir-ty/src/consteval/tests.rs +++ b/crates/hir-ty/src/consteval/tests.rs @@ -97,8 +97,10 @@ 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] diff --git a/crates/hir-ty/src/mir/eval.rs b/crates/hir-ty/src/mir/eval.rs index 9811cd9192bf..7ff68774bc98 100644 --- a/crates/hir-ty/src/mir/eval.rs +++ b/crates/hir-ty/src/mir/eval.rs @@ -860,6 +860,16 @@ impl Evaluator<'_> { 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:?}"))); + } + } + 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; @@ -888,28 +898,23 @@ impl Evaluator<'_> { BinOp::BitXor => l128 ^ r128, _ => unreachable!(), }; - 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()) + check_overflow(r)? } BinOp::Shl | BinOp::Shr => { - let shift_amount = if r128 < 0 { + 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:?}"))); - } 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()) + check_overflow(r)? } BinOp::Offset => not_supported!("offset binop"), }