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

Uplift clippy::precedence lint #117161

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions compiler/rustc_lint/messages.ftl
Expand Up @@ -455,6 +455,12 @@ lint_path_statement_drop = path statement drops value

lint_path_statement_no_effect = path statement with no effect

lint_precedence_unary = unary operator `{$op}` has lower precedence than method call
.suggestion = consider adding parentheses to clarify your intent

lint_precedence_unwary = operator precedence can trip the unwary
.suggestion = consider adding parentheses to clarify your intent

lint_ptr_null_checks_fn_ptr = function pointers are not nullable, so checking them for null will always return false
.help = wrap the function pointer inside an `Option` and use `Option::is_none` to check for null pointer value
.label = expression has type `{$orig_ty}`
Expand Down
3 changes: 3 additions & 0 deletions compiler/rustc_lint/src/lib.rs
Expand Up @@ -80,6 +80,7 @@ mod noop_method_call;
mod opaque_hidden_inferred_bound;
mod pass_by_value;
mod passes;
mod precedence;
mod ptr_nulls;
mod redundant_semicolon;
mod reference_casting;
Expand Down Expand Up @@ -118,6 +119,7 @@ use nonstandard_style::*;
use noop_method_call::*;
use opaque_hidden_inferred_bound::*;
use pass_by_value::*;
use precedence::*;
use ptr_nulls::*;
use redundant_semicolon::*;
use reference_casting::*;
Expand Down Expand Up @@ -180,6 +182,7 @@ early_lint_methods!(
RedundantSemicolons: RedundantSemicolons,
UnusedDocComment: UnusedDocComment,
UnexpectedCfgs: UnexpectedCfgs,
Precedence: Precedence,
]
]
);
Expand Down
47 changes: 47 additions & 0 deletions compiler/rustc_lint/src/lints.rs
Expand Up @@ -617,6 +617,53 @@ pub struct ExpectationNote {
pub rationale: Symbol,
}

// precedence.rs
#[derive(LintDiagnostic)]
pub enum PrecedenceDiag {
#[diag(lint_precedence_unary)]
Unary {
op: &'static str,
#[subdiagnostic]
suggestion: PrecedenceUnarySuggestion,
},
#[diag(lint_precedence_unwary)]
Unwary {
#[subdiagnostic]
suggestion: PrecedenceUnwarySuggestion,
},
}

#[derive(Subdiagnostic)]
#[multipart_suggestion(lint_suggestion, applicability = "machine-applicable")]
pub struct PrecedenceUnarySuggestion {
#[suggestion_part(code = "(")]
pub start_span: Span,
#[suggestion_part(code = ")")]
pub end_span: Span,
}

#[derive(Subdiagnostic)]
pub enum PrecedenceUnwarySuggestion {
#[multipart_suggestion(lint_suggestion, applicability = "machine-applicable")]
OneExpr {
#[suggestion_part(code = "(")]
start_span: Span,
#[suggestion_part(code = ")")]
end_span: Span,
},
#[multipart_suggestion(lint_suggestion, applicability = "machine-applicable")]
TwoExpr {
#[suggestion_part(code = "(")]
start_span: Span,
#[suggestion_part(code = ")")]
end_span: Span,
#[suggestion_part(code = "(")]
start2_span: Span,
#[suggestion_part(code = ")")]
end2_span: Span,
},
}

// ptr_nulls.rs
#[derive(LintDiagnostic)]
pub enum PtrNullChecksDiag<'a> {
Expand Down
116 changes: 116 additions & 0 deletions compiler/rustc_lint/src/precedence.rs
@@ -0,0 +1,116 @@
use rustc_ast::token::LitKind;
use rustc_ast::{BinOpKind, Expr, ExprKind, MethodCall, UnOp};
use rustc_span::source_map::Spanned;

use crate::lints::{PrecedenceDiag, PrecedenceUnarySuggestion, PrecedenceUnwarySuggestion};
use crate::{EarlyContext, EarlyLintPass, LintContext};

declare_lint! {
/// The `ambiguous_precedence` lint checks for operations where
/// precedence may be unclear and suggests adding parentheses.
///
/// ### Example
///
/// ```rust
/// # #![allow(unused)]
/// 1 << 2 + 3; // equals 32, while `(1 << 2) + 3` equals 7
/// -1i32.abs(); // equals -1, while `(-1i32).abs()` equals 1
/// ```
///
/// {{produces}}
///
/// ### Explanation
///
/// Unary operations take precedence on binary operations and method
/// calls take precedence over unary precedence. Setting the precedence
/// explicitly makes the code clearer and avoid potential bugs.
pub AMBIGUOUS_PRECEDENCE,
Warn,
"operations where precedence may be unclear"
}

declare_lint_pass!(Precedence => [AMBIGUOUS_PRECEDENCE]);

impl EarlyLintPass for Precedence {
fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &Expr) {
if expr.span.from_expansion() {
return;
}

if let ExprKind::Binary(Spanned { node: op, .. }, ref left, ref right) = expr.kind {
if !is_bit_op(op) {
return;
}

let suggestion = match (is_arith_expr(left), is_arith_expr(right)) {
(true, true) => PrecedenceUnwarySuggestion::TwoExpr {
start_span: left.span.shrink_to_lo(),
end_span: left.span.shrink_to_hi(),
start2_span: right.span.shrink_to_lo(),
end2_span: right.span.shrink_to_hi(),
},
(true, false) => PrecedenceUnwarySuggestion::OneExpr {
start_span: left.span.shrink_to_lo(),
end_span: left.span.shrink_to_hi(),
},
(false, true) => PrecedenceUnwarySuggestion::OneExpr {
start_span: right.span.shrink_to_lo(),
end_span: right.span.shrink_to_hi(),
},
(false, false) => return,
};

cx.emit_spanned_lint(
AMBIGUOUS_PRECEDENCE,
expr.span,
PrecedenceDiag::Unwary { suggestion },
);
}

if let ExprKind::Unary(unop, operand) = &expr.kind
&& matches!(unop, UnOp::Neg)
&& let ExprKind::MethodCall(..) = operand.kind
{
let mut arg = operand;
while let ExprKind::MethodCall(box MethodCall { receiver, .. }) = &arg.kind {
arg = receiver;
}

if let ExprKind::Lit(lit) = &arg.kind
&& let LitKind::Integer | LitKind::Float = &lit.kind
&& !arg.span.from_expansion()
{
cx.emit_spanned_lint(
AMBIGUOUS_PRECEDENCE,
expr.span,
PrecedenceDiag::Unary {
op: UnOp::to_string(*unop),
suggestion: PrecedenceUnarySuggestion {
start_span: operand.span.shrink_to_lo(),
end_span: operand.span.shrink_to_hi(),
},
},
);
}
}
}
}

fn is_arith_expr(expr: &Expr) -> bool {
match expr.kind {
ExprKind::Binary(Spanned { node: op, .. }, _, _) => is_arith_op(op),
_ => false,
}
}

#[must_use]
fn is_bit_op(op: BinOpKind) -> bool {
use rustc_ast::ast::BinOpKind::{BitAnd, BitOr, BitXor, Shl, Shr};
matches!(op, BitXor | BitAnd | BitOr | Shl | Shr)
}

#[must_use]
fn is_arith_op(op: BinOpKind) -> bool {
use rustc_ast::ast::BinOpKind::{Add, Div, Mul, Rem, Sub};
matches!(op, Add | Sub | Mul | Div | Rem)
}
2 changes: 1 addition & 1 deletion library/core/tests/num/i32.rs
Expand Up @@ -15,7 +15,7 @@ fn test_arith_operation() {
assert_eq!(i32_a * i32_a * i32_a, 1000);
assert_eq!(i32_a * i32_a * i32_a * i32_a, 10000);
assert_eq!(i32_a * i32_a / i32_a * i32_a, 100);
assert_eq!(i32_a * (i32_a - 1) << (2 + i32_a as usize), 368640);
assert_eq!((i32_a * (i32_a - 1)) << (2 + i32_a as usize), 368640);
let i32_b: isize = 0x10101010;
assert_eq!(i32_b + 1 - 1, i32_b);
assert_eq!(i32_b << 1, i32_b << 1);
Expand Down
1 change: 0 additions & 1 deletion src/tools/clippy/clippy_lints/src/declared_lints.rs
Expand Up @@ -561,7 +561,6 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
crate::pass_by_ref_or_value::TRIVIALLY_COPY_PASS_BY_REF_INFO,
crate::pattern_type_mismatch::PATTERN_TYPE_MISMATCH_INFO,
crate::permissions_set_readonly_false::PERMISSIONS_SET_READONLY_FALSE_INFO,
crate::precedence::PRECEDENCE_INFO,
crate::ptr::CMP_NULL_INFO,
crate::ptr::INVALID_NULL_PTR_USAGE_INFO,
crate::ptr::MUT_FROM_REF_INFO,
Expand Down
2 changes: 0 additions & 2 deletions src/tools/clippy/clippy_lints/src/lib.rs
Expand Up @@ -264,7 +264,6 @@ mod partialeq_to_none;
mod pass_by_ref_or_value;
mod pattern_type_mismatch;
mod permissions_set_readonly_false;
mod precedence;
mod ptr;
mod ptr_offset_with_cast;
mod pub_use;
Expand Down Expand Up @@ -809,7 +808,6 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
store.register_late_pass(|_| Box::new(returns::Return));
store.register_early_pass(|| Box::new(collapsible_if::CollapsibleIf));
store.register_late_pass(|_| Box::new(items_after_statements::ItemsAfterStatements));
store.register_early_pass(|| Box::new(precedence::Precedence));
store.register_late_pass(|_| Box::new(needless_parens_on_range_literals::NeedlessParensOnRangeLiterals));
store.register_early_pass(|| Box::new(needless_continue::NeedlessContinue));
store.register_early_pass(|| Box::new(redundant_else::RedundantElse));
Expand Down
1 change: 1 addition & 0 deletions src/tools/clippy/clippy_lints/src/renamed_lints.rs
Expand Up @@ -52,6 +52,7 @@ pub static RENAMED_LINTS: &[(&str, &str)] = &[
("clippy::invalid_utf8_in_unchecked", "invalid_from_utf8_unchecked"),
("clippy::let_underscore_drop", "let_underscore_drop"),
("clippy::mem_discriminant_non_enum", "enum_intrinsics_non_enums"),
("clippy::precedence", "ambiguous_precedence"),
Urgau marked this conversation as resolved.
Show resolved Hide resolved
("clippy::panic_params", "non_fmt_panics"),
("clippy::positional_named_format_parameters", "named_arguments_used_positionally"),
("clippy::temporary_cstring_as_ptr", "temporary_cstring_as_ptr"),
Expand Down
4 changes: 2 additions & 2 deletions src/tools/clippy/tests/ui/neg_multiply.fixed
@@ -1,6 +1,6 @@
#![warn(clippy::neg_multiply)]
#![allow(clippy::no_effect, clippy::unnecessary_operation, clippy::precedence)]
#![allow(unused)]
#![allow(clippy::no_effect, clippy::unnecessary_operation)]
#![allow(unused, ambiguous_precedence)]

use std::ops::Mul;

Expand Down
4 changes: 2 additions & 2 deletions src/tools/clippy/tests/ui/neg_multiply.rs
@@ -1,6 +1,6 @@
#![warn(clippy::neg_multiply)]
#![allow(clippy::no_effect, clippy::unnecessary_operation, clippy::precedence)]
#![allow(unused)]
#![allow(clippy::no_effect, clippy::unnecessary_operation)]
#![allow(unused, ambiguous_precedence)]

use std::ops::Mul;

Expand Down
60 changes: 0 additions & 60 deletions src/tools/clippy/tests/ui/precedence.fixed

This file was deleted.

60 changes: 0 additions & 60 deletions src/tools/clippy/tests/ui/precedence.rs

This file was deleted.