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.

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

Hover 'pub const MAX: Self = -1' for i128::MAX #18816

Closed
UseK opened this issue Jan 2, 2025 · 3 comments · Fixed by #18819
Closed

Hover 'pub const MAX: Self = -1' for i128::MAX #18816

UseK opened this issue Jan 2, 2025 · 3 comments · Fixed by #18819
Assignees
Labels
C-bug Category: bug

Comments

@UseK
Copy link

UseK commented Jan 2, 2025

rust-analyzer version: rust-analyzer version: 0.3.2237-standalone (59bc7b4 2024-12-29)
rustc version: rustc 1.83.0 (90b35a623 2024-11-26)
editor or extension: VSCode 1.96.2, Zed 0.167.1
relevant settings: Nothing in particular
repository link (if public, optional): (eg. rust-analyzer)
code snippet to reproduce:

let x = i128::MAX;

I use hover function to 'MAX' part and expect:

pub const MAX: Self = 170141183460469231731687303715884105727

But it shows:

pub const MAX: Self = -1
Image

related issues: #18116

@UseK UseK added the C-bug Category: bug label Jan 2, 2025
@Veykril
Copy link
Member

Veykril commented Jan 2, 2025

We don't seem to handle the bitshift here?

@ChayimFriedman2 ChayimFriedman2 self-assigned this Jan 2, 2025
@Veykril
Copy link
Member

Veykril commented Jan 2, 2025

Ah, the code here does all the integer math on i128 which is obviously wrong for unsigned ones

} 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:?}")));
}
}
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 shift_amount as usize >= lc.len() * 8 {
return Err(MirEvalError::Panic(format!(
"Overflow in {op:?}"
)));
}
if let Some(r) = r {
break 'b r;
}
};
return Err(MirEvalError::Panic(format!("Overflow in {op:?}")));
};
Owned(r.to_le_bytes()[..lc.len()].to_vec())
}
BinOp::Offset => not_supported!("offset binop"),

(which causes the shift on the u128 that is used by the definition to carry the sign)

@UseK
Copy link
Author

UseK commented Jan 5, 2025

rust-analyzer version: rust-analyzer version: 0.4.2243-standalone (6725e04 2025-01-04)

I found this issue is fixed in the Pre-Release Version of rust-analyzer extension in VSCode.
Image

Thanks! I love rust-analyzer!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants