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

unused_parens now fires on cast expression #110189

74 changes: 65 additions & 9 deletions compiler/rustc_lint/src/unused.rs
Original file line number Diff line number Diff line change
Expand Up @@ -572,6 +572,7 @@ enum UnusedDelimsCtx {
AnonConst,
MatchArmExpr,
IndexExpr,
CastExpr,
}

impl From<UnusedDelimsCtx> for &'static str {
Expand All @@ -592,10 +593,18 @@ impl From<UnusedDelimsCtx> for &'static str {
UnusedDelimsCtx::ArrayLenExpr | UnusedDelimsCtx::AnonConst => "const expression",
UnusedDelimsCtx::MatchArmExpr => "match arm expression",
UnusedDelimsCtx::IndexExpr => "index expression",
UnusedDelimsCtx::CastExpr => "cast expression",
}
}
}

#[derive(Copy, Clone, Eq, PartialEq)]
enum UnusedDelimCtxFollowedTokenKind {
Block,
Else,
Cast,
}

/// Used by both `UnusedParens` and `UnusedBraces` to prevent code duplication.
trait UnusedDelimLint {
const DELIM_STR: &'static str;
Expand Down Expand Up @@ -629,9 +638,14 @@ trait UnusedDelimLint {

fn is_expr_delims_necessary(
inner: &ast::Expr,
followed_by_block: bool,
followed_by_else: bool,
followed_token: Option<UnusedDelimCtxFollowedTokenKind>,
) -> bool {
let followed_by_block =
matches!(followed_token, Some(UnusedDelimCtxFollowedTokenKind::Block));
let followed_by_else =
matches!(followed_token, Some(UnusedDelimCtxFollowedTokenKind::Else));
let followed_by_cast =
matches!(followed_token, Some(UnusedDelimCtxFollowedTokenKind::Cast));
if followed_by_else {
match inner.kind {
ast::ExprKind::Binary(op, ..) if op.node.lazy() => return true,
Expand All @@ -640,6 +654,25 @@ trait UnusedDelimLint {
}
}

if followed_by_cast {
match inner.kind {
// `as` has higher precedence than any binary operator
ast::ExprKind::Binary(..)
// #88519
| ast::ExprKind::Block(..)
| ast::ExprKind::Match(..)
| ast::ExprKind::If(..)
// #51185
| ast::ExprKind::Closure(..)
// This pattern is technically unnecessary, because unary-minus has higher
// precedence than cast. However, casts followed by it may be confusing
// snippet to code readers. If we want to revisit this, this case shall be
// handled by some external approach.
| ast::ExprKind::Unary(ast::UnOp::Neg, ..) => return true,
_ => {}
}
}

// Check if LHS needs parens to prevent false-positives in cases like `fn x() -> u8 { ({ 0 } + 1) }`.
{
let mut innermost = inner;
Expand Down Expand Up @@ -964,9 +997,18 @@ impl UnusedDelimLint for UnusedParens {
) {
match value.kind {
ast::ExprKind::Paren(ref inner) => {
let followed_by_else = ctx == UnusedDelimsCtx::AssignedValueLetElse;
if !Self::is_expr_delims_necessary(inner, followed_by_block, followed_by_else)
&& value.attrs.is_empty()
if !Self::is_expr_delims_necessary(
inner,
if followed_by_block {
Some(UnusedDelimCtxFollowedTokenKind::Block)
} else if ctx == UnusedDelimsCtx::AssignedValueLetElse {
Some(UnusedDelimCtxFollowedTokenKind::Else)
} else if ctx == UnusedDelimsCtx::CastExpr {
Some(UnusedDelimCtxFollowedTokenKind::Cast)
} else {
None
},
) && value.attrs.is_empty()
&& !value.span.from_expansion()
&& (ctx != UnusedDelimsCtx::LetScrutineeExpr
|| !matches!(inner.kind, ast::ExprKind::Binary(
Expand All @@ -989,6 +1031,18 @@ impl UnusedDelimLint for UnusedParens {
false,
);
}
ast::ExprKind::Cast(ref expr, _) => match expr.kind {
_ => self.check_unused_delims_expr(
cx,
expr,
UnusedDelimsCtx::CastExpr,
followed_by_block,
None,
None,
false,
),
},

_ => {}
}
}
Expand Down Expand Up @@ -1248,10 +1302,12 @@ impl UnusedDelimLint for UnusedBraces {
// FIXME(const_generics): handle paths when #67075 is fixed.
if let [stmt] = inner.stmts.as_slice() {
if let ast::StmtKind::Expr(ref expr) = stmt.kind {
if !Self::is_expr_delims_necessary(expr, followed_by_block, false)
&& (ctx != UnusedDelimsCtx::AnonConst
|| (matches!(expr.kind, ast::ExprKind::Lit(_))
&& !expr.span.from_expansion()))
if !Self::is_expr_delims_necessary(
expr,
followed_by_block.then_some(UnusedDelimCtxFollowedTokenKind::Block),
) && (ctx != UnusedDelimsCtx::AnonConst
|| (matches!(expr.kind, ast::ExprKind::Lit(_))
&& !expr.span.from_expansion()))
&& !cx.sess().source_map().is_multiline(value.span)
&& value.attrs.is_empty()
&& !value.span.from_expansion()
Expand Down
2 changes: 1 addition & 1 deletion library/alloc/src/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ mod tests;
/// This is a global invariant, and also applies when using a compare-exchange loop.
///
/// See comment in `Arc::clone`.
const MAX_REFCOUNT: usize = (isize::MAX) as usize;
const MAX_REFCOUNT: usize = isize::MAX as usize;

/// The error in case either counter reaches above `MAX_REFCOUNT`, and we can `panic` safely.
const INTERNAL_OVERFLOW_ERROR: &str = "Arc counter overflow";
Expand Down
2 changes: 1 addition & 1 deletion library/core/src/num/dec2flt/number.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ impl Number {
// normal fast path
let value = F::from_u64(self.mantissa);
if self.exponent < 0 {
value / F::pow10_fast_path((-self.exponent) as _)
value / F::pow10_fast_path(-self.exponent as _)
} else {
value * F::pow10_fast_path(self.exponent as _)
}
Expand Down
2 changes: 1 addition & 1 deletion library/core/src/num/dec2flt/parse.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ fn parse_8digits(mut v: u64) -> u64 {
v = (v * 10) + (v >> 8); // will not overflow, fits in 63 bits
let v1 = (v & MASK).wrapping_mul(MUL1);
let v2 = ((v >> 16) & MASK).wrapping_mul(MUL2);
((v1.wrapping_add(v2) >> 32) as u32) as u64
(v1.wrapping_add(v2) >> 32) as u32 as u64
}

/// Parse digits until a non-digit character is found.
Expand Down
2 changes: 1 addition & 1 deletion library/core/src/num/dec2flt/slow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ pub(crate) fn parse_long_mantissa<F: RawFloat>(s: &[u8]) -> BiasedFp {
_ => 1,
}
} else {
get_shift((-d.decimal_point) as _)
get_shift(-d.decimal_point as _)
};
d.left_shift(shift);
if d.decimal_point > Decimal::DECIMAL_POINT_RANGE {
Expand Down
4 changes: 2 additions & 2 deletions library/std/src/io/buffered/bufreader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -244,8 +244,8 @@ impl<R: ?Sized + Seek> BufReader<R> {
pub fn seek_relative(&mut self, offset: i64) -> io::Result<()> {
let pos = self.buf.pos() as u64;
if offset < 0 {
if let Some(_) = pos.checked_sub((-offset) as u64) {
self.buf.unconsume((-offset) as usize);
if let Some(_) = pos.checked_sub(-offset as u64) {
self.buf.unconsume(-offset as usize);
return Ok(());
}
} else if let Some(new_pos) = pos.checked_add(offset as u64) {
Expand Down
2 changes: 1 addition & 1 deletion library/std/src/sys/unix/os.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ extern "C" {
/// Returns the platform-specific value of errno
#[cfg(not(any(target_os = "dragonfly", target_os = "vxworks")))]
pub fn errno() -> i32 {
unsafe { (*errno_location()) as i32 }
unsafe { *errno_location() as i32 }
}

/// Sets the platform-specific value of errno
Expand Down
4 changes: 2 additions & 2 deletions library/std/src/thread/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -159,10 +159,10 @@ where
let (tx, rx) = channel();

let x: Box<_> = Box::new(1);
let x_in_parent = (&*x) as *const i32 as usize;
let x_in_parent = &*x as *const i32 as usize;

spawnfn(Box::new(move || {
let x_in_child = (&*x) as *const i32 as usize;
let x_in_child = &*x as *const i32 as usize;
tx.send(x_in_child).unwrap();
}));

Expand Down
4 changes: 2 additions & 2 deletions src/tools/miri/tests/fail/stacked_borrows/illegal_dealloc1.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ use std::alloc::{alloc, dealloc, Layout};
fn main() {
unsafe {
let x = alloc(Layout::from_size_align_unchecked(1, 1));
let ptr1 = (&mut *x) as *mut u8;
let ptr2 = (&mut *ptr1) as *mut u8;
let ptr1 = &mut *x as *mut u8;
let ptr2 = &mut *ptr1 as *mut u8;
// Invalidate ptr2 by writing to ptr1.
ptr1.write(0);
// Deallocate through ptr2.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ fn main() {
name!(n);
let x = &mut *(n as *mut _);
name!(x);
let y = (&mut *n) as *mut _;
let y = &mut *n as *mut _;
name!(y);
write_second(x, y);
unsafe fn write_second(x: &mut u8, y: *mut u8) {
Expand Down
18 changes: 9 additions & 9 deletions src/tools/miri/tests/pass/float.rs
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ fn casts() {
assert_eq::<i32>(f32::INFINITY as i32, i32::MAX);
assert_eq::<i32>(f32::NEG_INFINITY as i32, i32::MIN);
assert_eq::<i32>(f32::NAN as i32, 0);
assert_eq::<i32>((-f32::NAN) as i32, 0);
Copy link
Member

@RalfJung RalfJung Jun 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should lint against (-expr) as i32. Looking at -expr as i32 I have no idea whether the as or the - are executed first, and I might care about that (in particular when casting to an unsigned type). (The formatting indicates that - goes first but I don't know if that actually matches how this is parsed.)

Parentheses to disambiguate parsing precedence are a good thing, we should not steer people away from them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agreed :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! The diff in the Miri test can hopefully be removed then. :)

assert_eq::<i32>(-f32::NAN as i32, 0);

// f32 -> u32
test_both_cast::<f32, u32>(0.0, 0);
Expand All @@ -223,7 +223,7 @@ fn casts() {
assert_eq::<u32>(f32::INFINITY as u32, u32::MAX);
assert_eq::<u32>(f32::NEG_INFINITY as u32, 0);
assert_eq::<u32>(f32::NAN as u32, 0);
assert_eq::<u32>((-f32::NAN) as u32, 0);
assert_eq::<u32>(-f32::NAN as u32, 0);

// f32 -> i64
test_both_cast::<f32, i64>(4294967296.0, 4294967296);
Expand Down Expand Up @@ -281,7 +281,7 @@ fn casts() {
assert_eq::<i64>(f64::INFINITY as i64, i64::MAX);
assert_eq::<i64>(f64::NEG_INFINITY as i64, i64::MIN);
assert_eq::<i64>(f64::NAN as i64, 0);
assert_eq::<i64>((-f64::NAN) as i64, 0);
assert_eq::<i64>(-f64::NAN as i64, 0);

// f64 -> u64
test_both_cast::<f64, u64>(0.0, 0);
Expand All @@ -300,7 +300,7 @@ fn casts() {
assert_eq::<u64>(f64::INFINITY as u64, u64::MAX);
assert_eq::<u64>(f64::NEG_INFINITY as u64, 0);
assert_eq::<u64>(f64::NAN as u64, 0);
assert_eq::<u64>((-f64::NAN) as u64, 0);
assert_eq::<u64>(-f64::NAN as u64, 0);

// f64 -> i128
assert_eq::<i128>(f64::MAX as i128, i128::MAX);
Expand All @@ -313,12 +313,12 @@ fn casts() {
// int -> f32
assert_eq::<f32>(127i8 as f32, 127.0);
assert_eq::<f32>(2147483647i32 as f32, 2147483648.0);
assert_eq::<f32>((-2147483648i32) as f32, -2147483648.0);
assert_eq::<f32>(-2147483648i32 as f32, -2147483648.0);
assert_eq::<f32>(1234567890i32 as f32, /*0x1.26580cp+30*/ f32::from_bits(0x4e932c06));
assert_eq::<f32>(16777217i32 as f32, 16777216.0);
assert_eq::<f32>((-16777217i32) as f32, -16777216.0);
assert_eq::<f32>(-16777217i32 as f32, -16777216.0);
assert_eq::<f32>(16777219i32 as f32, 16777220.0);
assert_eq::<f32>((-16777219i32) as f32, -16777220.0);
assert_eq::<f32>(-16777219i32 as f32, -16777220.0);
assert_eq::<f32>(
0x7fffff4000000001i64 as f32,
/*0x1.fffffep+62*/ f32::from_bits(0x5effffff),
Expand Down Expand Up @@ -370,7 +370,7 @@ fn casts() {
/*0x1.fffffep+127*/ f64::from_bits(0x47efffffe0000000),
);
assert_eq::<f64>(
/*-0x1.fffffep+127*/ (-f32::from_bits(0x7f7fffff)) as f64,
/*-0x1.fffffep+127*/ -f32::from_bits(0x7f7fffff) as f64,
/*-0x1.fffffep+127*/ -f64::from_bits(0x47efffffe0000000),
);
assert_eq::<f64>(
Expand All @@ -389,7 +389,7 @@ fn casts() {
assert_eq::<u32>(((-0.0f64) as f32).to_bits(), (-0.0f32).to_bits());
assert_eq::<f32>(5.0f64 as f32, 5.0f32);
assert_eq::<f32>(/*0x0.0000000000001p-1022*/ f64::from_bits(0x1) as f32, 0.0);
assert_eq::<f32>(/*-0x0.0000000000001p-1022*/ (-f64::from_bits(0x1)) as f32, -0.0);
assert_eq::<f32>(/*-0x0.0000000000001p-1022*/ -f64::from_bits(0x1) as f32, -0.0);
assert_eq::<f32>(
/*0x1.fffffe0000000p-127*/ f64::from_bits(0x380fffffe0000000) as f32,
/*0x1p-149*/ f32::from_bits(0x800000),
Expand Down
6 changes: 3 additions & 3 deletions src/tools/miri/tests/pass/tree-borrows/reserved.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ unsafe fn int_protected_read() {
name!(base);
let x = &mut *(base as *mut u8);
name!(x);
let y = (&mut *base) as *mut u8;
let y = &mut *base as *mut u8;
name!(y);
read_second(x, y); // Foreign Read for callee:x
print_state!(alloc_id);
Expand All @@ -106,7 +106,7 @@ unsafe fn int_unprotected_read() {
let alloc_id = alloc_id!(base);
let x = &mut *(base as *mut u8);
name!(x);
let y = (&mut *base) as *mut u8;
let y = &mut *base as *mut u8;
name!(y);
let _val = *y; // Foreign Read for x
print_state!(alloc_id);
Expand All @@ -120,7 +120,7 @@ unsafe fn int_unprotected_write() {
let alloc_id = alloc_id!(base);
let x = &mut *(base as *mut u8);
name!(x);
let y = (&mut *base) as *mut u8;
let y = &mut *base as *mut u8;
name!(y);
*y = 1; // Foreign Write for x
print_state!(alloc_id);
Expand Down
6 changes: 3 additions & 3 deletions tests/ui/const-ptr/allowed_slices.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,13 @@ pub static S2: &[u32] = unsafe { from_raw_parts(&D0, 1) };
pub static S3: &[MaybeUninit<&u32>] = unsafe { from_raw_parts(&D1, 1) };

// Reinterpreting data is fine, as long as layouts match
pub static S4: &[u8] = unsafe { from_raw_parts((&D0) as *const _ as _, 3) };
pub static S4: &[u8] = unsafe { from_raw_parts(&D0 as *const _ as _, 3) };
// This is only valid because D1 has uninitialized bytes, if it was an initialized pointer,
// that would reinterpret pointers as integers which is UB in CTFE.
pub static S5: &[MaybeUninit<u8>] = unsafe { from_raw_parts((&D1) as *const _ as _, 2) };
pub static S5: &[MaybeUninit<u8>] = unsafe { from_raw_parts(&D1 as *const _ as _, 2) };
// Even though u32 and [bool; 4] have different layouts, D0 has a value that
// is valid as [bool; 4], so this is not UB (it's basically a transmute)
pub static S6: &[bool] = unsafe { from_raw_parts((&D0) as *const _ as _, 4) };
pub static S6: &[bool] = unsafe { from_raw_parts(&D0 as *const _ as _, 4) };

// Structs are considered single allocated objects,
// as long as you don't reinterpret padding as initialized
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/consts/issue-27890.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// run-pass
static PLUS_ONE: &'static (dyn Fn(i32) -> i32 + Sync) = (&|x: i32| { x + 1 })
static PLUS_ONE: &'static (dyn Fn(i32) -> i32 + Sync) = &(|x: i32| { x + 1 })
as &'static (dyn Fn(i32) -> i32 + Sync);

fn main() {
Expand Down
Loading
Loading