Rollup merge of #121364 - Urgau:unary_precedence, r=compiler-errors

Implement lint against ambiguous negative literals

This PR implements a lint against ambiguous negative literals with a literal and method calls right after it.

## `ambiguous_negative_literals`

(deny-by-default)

The `ambiguous_negative_literals` lint checks for cases that are confusing between a negative literal and a negation that's not part of the literal.

### Example

```rust,compile_fail
-1i32.abs(); // equals -1, while `(-1i32).abs()` equals 1
```

### Explanation

Method calls take precedence over unary precedence. Setting the precedence explicitly makes the code clearer and avoid potential bugs.

<details>
<summary>Old proposed lint</summary>

## `ambiguous_unary_precedence`

(deny-by-default)

The `ambiguous_unary_precedence` lint checks for use the negative unary operator with a literal and method calls.

### Example

```rust
-1i32.abs(); // equals -1, while `(-1i32).abs()` equals 1
```

### 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.

</details>

-----

Note: This is a strip down version of https://github.com/rust-lang/rust/pull/117161, without the binary op precedence.

Fixes https://github.com/rust-lang/rust/issues/117155
`@rustbot` labels +I-lang-nominated
cc `@scottmcm`
r? compiler
This commit is contained in:
Matthias Krüger 2024-07-25 16:48:17 +02:00 committed by GitHub
commit ae71900ef6
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
12 changed files with 325 additions and 156 deletions

View File

@ -5,6 +5,11 @@ lint_ambiguous_glob_reexport = ambiguous glob re-exports
.label_first_reexport = the name `{$name}` in the {$namespace} namespace is first re-exported here
.label_duplicate_reexport = but the name `{$name}` in the {$namespace} namespace is also re-exported here
lint_ambiguous_negative_literals = `-` has lower precedence than method calls, which might be unexpected
.example = e.g. `-4.abs()` equals `-4`; while `(-4).abs()` equals `4`
.negative_literal = add parentheses around the `-` and the literal to call the method on a negative literal
.current_behavior = add parentheses around the literal and the method call to keep the current behavior
lint_ambiguous_wide_pointer_comparisons = ambiguous wide pointer comparison, the comparison includes metadata which may not be expected
.addr_metadata_suggestion = use explicit `std::ptr::eq` method to compare metadata and addresses
.addr_suggestion = use `std::ptr::addr_eq` or untyped pointers to only compare their addresses

View File

@ -73,6 +73,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;
@ -111,6 +112,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::*;
@ -174,6 +176,7 @@ early_lint_methods!(
RedundantSemicolons: RedundantSemicolons,
UnusedDocComment: UnusedDocComment,
Expr2024: Expr2024,
Precedence: Precedence,
]
]
);

View File

@ -1499,6 +1499,35 @@ pub struct NonLocalDefinitionsCargoUpdateNote {
pub crate_name: Symbol,
}
// precedence.rs
#[derive(LintDiagnostic)]
#[diag(lint_ambiguous_negative_literals)]
#[note(lint_example)]
pub struct AmbiguousNegativeLiteralsDiag {
#[subdiagnostic]
pub negative_literal: AmbiguousNegativeLiteralsNegativeLiteralSuggestion,
#[subdiagnostic]
pub current_behavior: AmbiguousNegativeLiteralsCurrentBehaviorSuggestion,
}
#[derive(Subdiagnostic)]
#[multipart_suggestion(lint_negative_literal, applicability = "maybe-incorrect")]
pub struct AmbiguousNegativeLiteralsNegativeLiteralSuggestion {
#[suggestion_part(code = "(")]
pub start_span: Span,
#[suggestion_part(code = ")")]
pub end_span: Span,
}
#[derive(Subdiagnostic)]
#[multipart_suggestion(lint_current_behavior, applicability = "maybe-incorrect")]
pub struct AmbiguousNegativeLiteralsCurrentBehaviorSuggestion {
#[suggestion_part(code = "(")]
pub start_span: Span,
#[suggestion_part(code = ")")]
pub end_span: Span,
}
// pass_by_value.rs
#[derive(LintDiagnostic)]
#[diag(lint_pass_by_value)]

View File

@ -0,0 +1,70 @@
use rustc_ast::token::LitKind;
use rustc_ast::{Expr, ExprKind, MethodCall, UnOp};
use rustc_session::{declare_lint, declare_lint_pass};
use crate::lints::{
AmbiguousNegativeLiteralsCurrentBehaviorSuggestion, AmbiguousNegativeLiteralsDiag,
AmbiguousNegativeLiteralsNegativeLiteralSuggestion,
};
use crate::{EarlyContext, EarlyLintPass, LintContext};
declare_lint! {
/// The `ambiguous_negative_literals` lint checks for cases that are
/// confusing between a negative literal and a negation that's not part
/// of the literal.
///
/// ### Example
///
/// ```rust,compile_fail
/// # #![allow(unused)]
/// -1i32.abs(); // equals -1, while `(-1i32).abs()` equals 1
/// ```
///
/// {{produces}}
///
/// ### Explanation
///
/// Method calls take precedence over unary precedence. Setting the
/// precedence explicitly makes the code clearer and avoid potential bugs.
pub AMBIGUOUS_NEGATIVE_LITERALS,
Deny,
"ambiguous negative literals operations",
report_in_external_macro
}
declare_lint_pass!(Precedence => [AMBIGUOUS_NEGATIVE_LITERALS]);
impl EarlyLintPass for Precedence {
fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &Expr) {
let ExprKind::Unary(UnOp::Neg, operand) = &expr.kind else {
return;
};
let mut arg = operand;
let mut at_least_one = false;
while let ExprKind::MethodCall(box MethodCall { receiver, .. }) = &arg.kind {
at_least_one = true;
arg = receiver;
}
if at_least_one
&& let ExprKind::Lit(lit) = &arg.kind
&& let LitKind::Integer | LitKind::Float = &lit.kind
{
cx.emit_span_lint(
AMBIGUOUS_NEGATIVE_LITERALS,
expr.span,
AmbiguousNegativeLiteralsDiag {
negative_literal: AmbiguousNegativeLiteralsNegativeLiteralSuggestion {
start_span: expr.span.shrink_to_lo(),
end_span: arg.span.shrink_to_hi(),
},
current_behavior: AmbiguousNegativeLiteralsCurrentBehaviorSuggestion {
start_span: operand.span.shrink_to_lo(),
end_span: operand.span.shrink_to_hi(),
},
},
);
}
}
}

View File

@ -1,38 +1,17 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::source::snippet_with_applicability;
use rustc_ast::ast::{BinOpKind, Expr, ExprKind, MethodCall, UnOp};
use rustc_ast::token;
use rustc_ast::ast::{BinOpKind, Expr, ExprKind};
use rustc_errors::Applicability;
use rustc_lint::{EarlyContext, EarlyLintPass};
use rustc_session::declare_lint_pass;
use rustc_span::source_map::Spanned;
const ALLOWED_ODD_FUNCTIONS: [&str; 14] = [
"asin",
"asinh",
"atan",
"atanh",
"cbrt",
"fract",
"round",
"signum",
"sin",
"sinh",
"tan",
"tanh",
"to_degrees",
"to_radians",
];
declare_clippy_lint! {
/// ### What it does
/// Checks for operations where precedence may be unclear
/// and suggests to add parentheses. Currently it catches the following:
/// * mixed usage of arithmetic and bit shifting/combining operators without
/// parentheses
/// * a "negative" numeric literal (which is really a unary `-` followed by a
/// numeric literal)
/// followed by a method call
///
/// ### Why is this bad?
/// Not everyone knows the precedence of those operators by
@ -41,7 +20,6 @@ declare_clippy_lint! {
///
/// ### Example
/// * `1 << 2 + 3` equals 32, while `(1 << 2) + 3` equals 7
/// * `-1i32.abs()` equals -1, while `(-1i32).abs()` equals 1
#[clippy::version = "pre 1.29.0"]
pub PRECEDENCE,
complexity,
@ -104,38 +82,6 @@ impl EarlyLintPass for Precedence {
(false, false) => (),
}
}
if let ExprKind::Unary(UnOp::Neg, operand) = &expr.kind {
let mut arg = operand;
let mut all_odd = true;
while let ExprKind::MethodCall(box MethodCall { seg, receiver, .. }) = &arg.kind {
let seg_str = seg.ident.name.as_str();
all_odd &= ALLOWED_ODD_FUNCTIONS
.iter()
.any(|odd_function| **odd_function == *seg_str);
arg = receiver;
}
if !all_odd
&& let ExprKind::Lit(lit) = &arg.kind
&& let token::LitKind::Integer | token::LitKind::Float = &lit.kind
{
let mut applicability = Applicability::MachineApplicable;
span_lint_and_sugg(
cx,
PRECEDENCE,
expr.span,
"unary minus has lower precedence than method call",
"consider adding parentheses to clarify your intent",
format!(
"-({})",
snippet_with_applicability(cx, operand.span, "..", &mut applicability)
),
applicability,
);
}
}
}
}

View File

@ -20,40 +20,6 @@ fn main() {
1 ^ (1 - 1);
3 | (2 - 1);
3 & (5 - 2);
-(1i32.abs());
-(1f32.abs());
// These should not trigger an error
let _ = (-1i32).abs();
let _ = (-1f32).abs();
let _ = -(1i32).abs();
let _ = -(1f32).abs();
let _ = -(1i32.abs());
let _ = -(1f32.abs());
// Odd functions should not trigger an error
let _ = -1f64.asin();
let _ = -1f64.asinh();
let _ = -1f64.atan();
let _ = -1f64.atanh();
let _ = -1f64.cbrt();
let _ = -1f64.fract();
let _ = -1f64.round();
let _ = -1f64.signum();
let _ = -1f64.sin();
let _ = -1f64.sinh();
let _ = -1f64.tan();
let _ = -1f64.tanh();
let _ = -1f64.to_degrees();
let _ = -1f64.to_radians();
// Chains containing any non-odd function should trigger (issue #5924)
let _ = -(1.0_f64.cos().cos());
let _ = -(1.0_f64.cos().sin());
let _ = -(1.0_f64.sin().cos());
// Chains of odd functions shouldn't trigger
let _ = -1f64.sin().sin();
let b = 3;
trip!(b * 8);

View File

@ -20,40 +20,6 @@ fn main() {
1 ^ 1 - 1;
3 | 2 - 1;
3 & 5 - 2;
-1i32.abs();
-1f32.abs();
// These should not trigger an error
let _ = (-1i32).abs();
let _ = (-1f32).abs();
let _ = -(1i32).abs();
let _ = -(1f32).abs();
let _ = -(1i32.abs());
let _ = -(1f32.abs());
// Odd functions should not trigger an error
let _ = -1f64.asin();
let _ = -1f64.asinh();
let _ = -1f64.atan();
let _ = -1f64.atanh();
let _ = -1f64.cbrt();
let _ = -1f64.fract();
let _ = -1f64.round();
let _ = -1f64.signum();
let _ = -1f64.sin();
let _ = -1f64.sinh();
let _ = -1f64.tan();
let _ = -1f64.tanh();
let _ = -1f64.to_degrees();
let _ = -1f64.to_radians();
// Chains containing any non-odd function should trigger (issue #5924)
let _ = -1.0_f64.cos().cos();
let _ = -1.0_f64.cos().sin();
let _ = -1.0_f64.sin().cos();
// Chains of odd functions shouldn't trigger
let _ = -1f64.sin().sin();
let b = 3;
trip!(b * 8);

View File

@ -43,35 +43,5 @@ error: operator precedence can trip the unwary
LL | 3 & 5 - 2;
| ^^^^^^^^^ help: consider parenthesizing your expression: `3 & (5 - 2)`
error: unary minus has lower precedence than method call
--> tests/ui/precedence.rs:23:5
|
LL | -1i32.abs();
| ^^^^^^^^^^^ help: consider adding parentheses to clarify your intent: `-(1i32.abs())`
error: unary minus has lower precedence than method call
--> tests/ui/precedence.rs:24:5
|
LL | -1f32.abs();
| ^^^^^^^^^^^ help: consider adding parentheses to clarify your intent: `-(1f32.abs())`
error: unary minus has lower precedence than method call
--> tests/ui/precedence.rs:51:13
|
LL | let _ = -1.0_f64.cos().cos();
| ^^^^^^^^^^^^^^^^^^^^ help: consider adding parentheses to clarify your intent: `-(1.0_f64.cos().cos())`
error: unary minus has lower precedence than method call
--> tests/ui/precedence.rs:52:13
|
LL | let _ = -1.0_f64.cos().sin();
| ^^^^^^^^^^^^^^^^^^^^ help: consider adding parentheses to clarify your intent: `-(1.0_f64.cos().sin())`
error: unary minus has lower precedence than method call
--> tests/ui/precedence.rs:53:13
|
LL | let _ = -1.0_f64.sin().cos();
| ^^^^^^^^^^^^^^^^^^^^ help: consider adding parentheses to clarify your intent: `-(1.0_f64.sin().cos())`
error: aborting due to 12 previous errors
error: aborting due to 7 previous errors

View File

@ -206,7 +206,7 @@ mod fixable {
fn issue_9563() {
let _: f64 = (-8.0_f64).exp();
#[allow(clippy::precedence)]
#[allow(ambiguous_negative_literals)]
let _: f64 = -8.0_f64.exp(); // should suggest `-8.0_f64.exp()` here not to change code behavior
}

View File

@ -206,7 +206,7 @@ mod fixable {
fn issue_9563() {
let _: f64 = (-8.0 as f64).exp();
#[allow(clippy::precedence)]
#[allow(ambiguous_negative_literals)]
let _: f64 = -(8.0 as f64).exp(); // should suggest `-8.0_f64.exp()` here not to change code behavior
}

View File

@ -0,0 +1,35 @@
//@ check-fail
fn main() {
let _ = -1i32.abs();
//~^ ERROR `-` has lower precedence than method calls
let _ = -1f32.abs();
//~^ ERROR `-` has lower precedence than method calls
let _ = -1f64.asin();
//~^ ERROR `-` has lower precedence than method calls
let _ = -1f64.asinh();
//~^ ERROR `-` has lower precedence than method calls
let _ = -1f64.tan();
//~^ ERROR `-` has lower precedence than method calls
let _ = -1f64.tanh();
//~^ ERROR `-` has lower precedence than method calls
let _ = -1.0_f64.cos().cos();
//~^ ERROR `-` has lower precedence than method calls
let _ = -1.0_f64.cos().sin();
//~^ ERROR `-` has lower precedence than method calls
let _ = -1.0_f64.sin().cos();
//~^ ERROR `-` has lower precedence than method calls
let _ = -1f64.sin().sin();
//~^ ERROR `-` has lower precedence than method calls
dbg!( -1.0_f32.cos() );
//~^ ERROR `-` has lower precedence than method calls
// should not warn
let _ = (-1i32).abs();
let _ = (-1f32).abs();
let _ = -(1i32).abs();
let _ = -(1f32).abs();
let _ = -(1i32.abs());
let _ = -(1f32.abs());
}

View File

@ -0,0 +1,179 @@
error: `-` has lower precedence than method calls, which might be unexpected
--> $DIR/negative_literals.rs:4:13
|
LL | let _ = -1i32.abs();
| ^^^^^^^^^^^
|
= note: e.g. `-4.abs()` equals `-4`; while `(-4).abs()` equals `4`
= note: `#[deny(ambiguous_negative_literals)]` on by default
help: add parentheses around the `-` and the literal to call the method on a negative literal
|
LL | let _ = (-1i32).abs();
| + +
help: add parentheses around the literal and the method call to keep the current behavior
|
LL | let _ = -(1i32.abs());
| + +
error: `-` has lower precedence than method calls, which might be unexpected
--> $DIR/negative_literals.rs:6:13
|
LL | let _ = -1f32.abs();
| ^^^^^^^^^^^
|
= note: e.g. `-4.abs()` equals `-4`; while `(-4).abs()` equals `4`
help: add parentheses around the `-` and the literal to call the method on a negative literal
|
LL | let _ = (-1f32).abs();
| + +
help: add parentheses around the literal and the method call to keep the current behavior
|
LL | let _ = -(1f32.abs());
| + +
error: `-` has lower precedence than method calls, which might be unexpected
--> $DIR/negative_literals.rs:8:13
|
LL | let _ = -1f64.asin();
| ^^^^^^^^^^^^
|
= note: e.g. `-4.abs()` equals `-4`; while `(-4).abs()` equals `4`
help: add parentheses around the `-` and the literal to call the method on a negative literal
|
LL | let _ = (-1f64).asin();
| + +
help: add parentheses around the literal and the method call to keep the current behavior
|
LL | let _ = -(1f64.asin());
| + +
error: `-` has lower precedence than method calls, which might be unexpected
--> $DIR/negative_literals.rs:10:13
|
LL | let _ = -1f64.asinh();
| ^^^^^^^^^^^^^
|
= note: e.g. `-4.abs()` equals `-4`; while `(-4).abs()` equals `4`
help: add parentheses around the `-` and the literal to call the method on a negative literal
|
LL | let _ = (-1f64).asinh();
| + +
help: add parentheses around the literal and the method call to keep the current behavior
|
LL | let _ = -(1f64.asinh());
| + +
error: `-` has lower precedence than method calls, which might be unexpected
--> $DIR/negative_literals.rs:12:13
|
LL | let _ = -1f64.tan();
| ^^^^^^^^^^^
|
= note: e.g. `-4.abs()` equals `-4`; while `(-4).abs()` equals `4`
help: add parentheses around the `-` and the literal to call the method on a negative literal
|
LL | let _ = (-1f64).tan();
| + +
help: add parentheses around the literal and the method call to keep the current behavior
|
LL | let _ = -(1f64.tan());
| + +
error: `-` has lower precedence than method calls, which might be unexpected
--> $DIR/negative_literals.rs:14:13
|
LL | let _ = -1f64.tanh();
| ^^^^^^^^^^^^
|
= note: e.g. `-4.abs()` equals `-4`; while `(-4).abs()` equals `4`
help: add parentheses around the `-` and the literal to call the method on a negative literal
|
LL | let _ = (-1f64).tanh();
| + +
help: add parentheses around the literal and the method call to keep the current behavior
|
LL | let _ = -(1f64.tanh());
| + +
error: `-` has lower precedence than method calls, which might be unexpected
--> $DIR/negative_literals.rs:16:13
|
LL | let _ = -1.0_f64.cos().cos();
| ^^^^^^^^^^^^^^^^^^^^
|
= note: e.g. `-4.abs()` equals `-4`; while `(-4).abs()` equals `4`
help: add parentheses around the `-` and the literal to call the method on a negative literal
|
LL | let _ = (-1.0_f64).cos().cos();
| + +
help: add parentheses around the literal and the method call to keep the current behavior
|
LL | let _ = -(1.0_f64.cos().cos());
| + +
error: `-` has lower precedence than method calls, which might be unexpected
--> $DIR/negative_literals.rs:18:13
|
LL | let _ = -1.0_f64.cos().sin();
| ^^^^^^^^^^^^^^^^^^^^
|
= note: e.g. `-4.abs()` equals `-4`; while `(-4).abs()` equals `4`
help: add parentheses around the `-` and the literal to call the method on a negative literal
|
LL | let _ = (-1.0_f64).cos().sin();
| + +
help: add parentheses around the literal and the method call to keep the current behavior
|
LL | let _ = -(1.0_f64.cos().sin());
| + +
error: `-` has lower precedence than method calls, which might be unexpected
--> $DIR/negative_literals.rs:20:13
|
LL | let _ = -1.0_f64.sin().cos();
| ^^^^^^^^^^^^^^^^^^^^
|
= note: e.g. `-4.abs()` equals `-4`; while `(-4).abs()` equals `4`
help: add parentheses around the `-` and the literal to call the method on a negative literal
|
LL | let _ = (-1.0_f64).sin().cos();
| + +
help: add parentheses around the literal and the method call to keep the current behavior
|
LL | let _ = -(1.0_f64.sin().cos());
| + +
error: `-` has lower precedence than method calls, which might be unexpected
--> $DIR/negative_literals.rs:22:13
|
LL | let _ = -1f64.sin().sin();
| ^^^^^^^^^^^^^^^^^
|
= note: e.g. `-4.abs()` equals `-4`; while `(-4).abs()` equals `4`
help: add parentheses around the `-` and the literal to call the method on a negative literal
|
LL | let _ = (-1f64).sin().sin();
| + +
help: add parentheses around the literal and the method call to keep the current behavior
|
LL | let _ = -(1f64.sin().sin());
| + +
error: `-` has lower precedence than method calls, which might be unexpected
--> $DIR/negative_literals.rs:25:11
|
LL | dbg!( -1.0_f32.cos() );
| ^^^^^^^^^^^^^^
|
= note: e.g. `-4.abs()` equals `-4`; while `(-4).abs()` equals `4`
help: add parentheses around the `-` and the literal to call the method on a negative literal
|
LL | dbg!( (-1.0_f32).cos() );
| + +
help: add parentheses around the literal and the method call to keep the current behavior
|
LL | dbg!( -(1.0_f32.cos()) );
| + +
error: aborting due to 11 previous errors