Skip to content

Commit 39f0719

Browse files
borsbors
committed
Auto merge of #9840 - c410-f3r:arith-2, r=Alexendoo
[`arithmetic-side-effects`]: Consider user-provided pairs Depends on #9592. Because of #9559 (comment), r? `@Alexendoo` ``` changelog: [`arithmetic-side-effects`]: Consider user-provided pairs ```
2 parents 911864d + 1f92f97 commit 39f0719

File tree

9 files changed

+286
-66
lines changed

9 files changed

+286
-66
lines changed

‎clippy_lints/src/lib.rs

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -507,9 +507,20 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
507507
}
508508

509509
let arithmetic_side_effects_allowed = conf.arithmetic_side_effects_allowed.clone();
510+
let arithmetic_side_effects_allowed_binary = conf.arithmetic_side_effects_allowed_binary.clone();
511+
let arithmetic_side_effects_allowed_unary = conf.arithmetic_side_effects_allowed_unary.clone();
510512
store.register_late_pass(move |_| {
511513
Box::new(operators::arithmetic_side_effects::ArithmeticSideEffects::new(
512-
arithmetic_side_effects_allowed.clone(),
514+
arithmetic_side_effects_allowed
515+
.iter()
516+
.flat_map(|el| [[el.clone(), "*".to_string()], ["*".to_string(), el.clone()]])
517+
.chain(arithmetic_side_effects_allowed_binary.clone())
518+
.collect(),
519+
arithmetic_side_effects_allowed
520+
.iter()
521+
.chain(arithmetic_side_effects_allowed_unary.iter())
522+
.cloned()
523+
.collect(),
513524
))
514525
});
515526
store.register_late_pass(|_| Box::new(utils::dump_hir::DumpHir));

‎clippy_lints/src/operators/arithmetic_side_effects.rs

Lines changed: 55 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -5,25 +5,26 @@ use clippy_utils::{
55
peel_hir_expr_refs,
66
};
77
use rustc_ast as ast;
8-
use rustc_data_structures::fx::FxHashSet;
8+
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
99
use rustc_hir as hir;
1010
use rustc_lint::{LateContext, LateLintPass};
1111
use rustc_middle::ty::Ty;
1212
use rustc_session::impl_lint_pass;
1313
use rustc_span::source_map::{Span, Spanned};
1414

15-
const HARD_CODED_ALLOWED: &[&str] = &[
16-
"&str",
17-
"f32",
18-
"f64",
19-
"std::num::Saturating",
20-
"std::num::Wrapping",
21-
"std::string::String",
15+
const HARD_CODED_ALLOWED_BINARY: &[[&str; 2]] = &[
16+
["f32", "f32"],
17+
["f64", "f64"],
18+
["std::num::Saturating", "std::num::Saturating"],
19+
["std::num::Wrapping", "std::num::Wrapping"],
20+
["std::string::String", "&str"],
2221
];
22+
const HARD_CODED_ALLOWED_UNARY: &[&str] = &["f32", "f64", "std::num::Saturating", "std::num::Wrapping"];
2323

2424
#[derive(Debug)]
2525
pub struct ArithmeticSideEffects {
26-
allowed: FxHashSet<String>,
26+
allowed_binary: FxHashMap<String, FxHashSet<String>>,
27+
allowed_unary: FxHashSet<String>,
2728
// Used to check whether expressions are constants, such as in enum discriminants and consts
2829
const_span: Option<Span>,
2930
expr_span: Option<Span>,
@@ -33,19 +34,55 @@ impl_lint_pass!(ArithmeticSideEffects => [ARITHMETIC_SIDE_EFFECTS]);
3334

3435
impl ArithmeticSideEffects {
3536
#[must_use]
36-
pub fn new(mut allowed: FxHashSet<String>) -> Self {
37-
allowed.extend(HARD_CODED_ALLOWED.iter().copied().map(String::from));
37+
pub fn new(user_allowed_binary: Vec<[String; 2]>, user_allowed_unary: Vec<String>) -> Self {
38+
let mut allowed_binary: FxHashMap<String, FxHashSet<String>> = <_>::default();
39+
for [lhs, rhs] in user_allowed_binary.into_iter().chain(
40+
HARD_CODED_ALLOWED_BINARY
41+
.iter()
42+
.copied()
43+
.map(|[lhs, rhs]| [lhs.to_string(), rhs.to_string()]),
44+
) {
45+
allowed_binary.entry(lhs).or_default().insert(rhs);
46+
}
47+
let allowed_unary = user_allowed_unary
48+
.into_iter()
49+
.chain(HARD_CODED_ALLOWED_UNARY.iter().copied().map(String::from))
50+
.collect();
3851
Self {
39-
allowed,
52+
allowed_binary,
53+
allowed_unary,
4054
const_span: None,
4155
expr_span: None,
4256
}
4357
}
4458

45-
/// Checks if the given `expr` has any of the inner `allowed` elements.
46-
fn is_allowed_ty(&self, ty: Ty<'_>) -> bool {
47-
self.allowed
48-
.contains(ty.to_string().split('<').next().unwrap_or_default())
59+
/// Checks if the lhs and the rhs types of a binary operation like "addition" or
60+
/// "multiplication" are present in the inner set of allowed types.
61+
fn has_allowed_binary(&self, lhs_ty: Ty<'_>, rhs_ty: Ty<'_>) -> bool {
62+
let lhs_ty_string = lhs_ty.to_string();
63+
let lhs_ty_string_elem = lhs_ty_string.split('<').next().unwrap_or_default();
64+
let rhs_ty_string = rhs_ty.to_string();
65+
let rhs_ty_string_elem = rhs_ty_string.split('<').next().unwrap_or_default();
66+
if let Some(rhs_from_specific) = self.allowed_binary.get(lhs_ty_string_elem)
67+
&& {
68+
let rhs_has_allowed_ty = rhs_from_specific.contains(rhs_ty_string_elem);
69+
rhs_has_allowed_ty || rhs_from_specific.contains("*")
70+
}
71+
{
72+
true
73+
} else if let Some(rhs_from_glob) = self.allowed_binary.get("*") {
74+
rhs_from_glob.contains(rhs_ty_string_elem)
75+
} else {
76+
false
77+
}
78+
}
79+
80+
/// Checks if the type of an unary operation like "negation" is present in the inner set of
81+
/// allowed types.
82+
fn has_allowed_unary(&self, ty: Ty<'_>) -> bool {
83+
let ty_string = ty.to_string();
84+
let ty_string_elem = ty_string.split('<').next().unwrap_or_default();
85+
self.allowed_unary.contains(ty_string_elem)
4986
}
5087

5188
// For example, 8i32 or &i64::MAX.
@@ -97,8 +134,7 @@ impl ArithmeticSideEffects {
97134
};
98135
let lhs_ty = cx.typeck_results().expr_ty(lhs);
99136
let rhs_ty = cx.typeck_results().expr_ty(rhs);
100-
let lhs_and_rhs_have_the_same_ty = lhs_ty == rhs_ty;
101-
if lhs_and_rhs_have_the_same_ty && self.is_allowed_ty(lhs_ty) && self.is_allowed_ty(rhs_ty) {
137+
if self.has_allowed_binary(lhs_ty, rhs_ty) {
102138
return;
103139
}
104140
let has_valid_op = if Self::is_integral(lhs_ty) && Self::is_integral(rhs_ty) {
@@ -137,7 +173,7 @@ impl ArithmeticSideEffects {
137173
return;
138174
}
139175
let ty = cx.typeck_results().expr_ty(expr).peel_refs();
140-
if self.is_allowed_ty(ty) {
176+
if self.has_allowed_unary(ty) {
141177
return;
142178
}
143179
let actual_un_expr = peel_hir_expr_refs(un_expr).0;

‎clippy_lints/src/operators/mod.rs

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -90,9 +90,6 @@ declare_clippy_lint! {
9090
/// use rust_decimal::Decimal;
9191
/// let _n = Decimal::MAX + Decimal::MAX;
9292
/// ```
93-
///
94-
/// ### Allowed types
95-
/// Custom allowed types can be specified through the "arithmetic-side-effects-allowed" filter.
9693
#[clippy::version = "1.64.0"]
9794
pub ARITHMETIC_SIDE_EFFECTS,
9895
restriction,

‎clippy_lints/src/utils/conf.rs

Lines changed: 41 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -205,10 +205,49 @@ macro_rules! define_Conf {
205205
}
206206

207207
define_Conf! {
208-
/// Lint: Arithmetic.
208+
/// Lint: ARITHMETIC_SIDE_EFFECTS.
209209
///
210-
/// Suppress checking of the passed type names.
210+
/// Suppress checking of the passed type names in all types of operations.
211+
///
212+
/// If a specific operation is desired, consider using `arithmetic_side_effects_allowed_binary` or `arithmetic_side_effects_allowed_unary` instead.
213+
///
214+
/// #### Example
215+
///
216+
/// ```toml
217+
/// arithmetic-side-effects-allowed = ["SomeType", "AnotherType"]
218+
/// ```
219+
///
220+
/// #### Noteworthy
221+
///
222+
/// A type, say `SomeType`, listed in this configuration has the same behavior of `["SomeType" , "*"], ["*", "SomeType"]` in `arithmetic_side_effects_allowed_binary`.
211223
(arithmetic_side_effects_allowed: rustc_data_structures::fx::FxHashSet<String> = <_>::default()),
224+
/// Lint: ARITHMETIC_SIDE_EFFECTS.
225+
///
226+
/// Suppress checking of the passed type pair names in binary operations like addition or
227+
/// multiplication.
228+
///
229+
/// Supports the "*" wildcard to indicate that a certain type won't trigger the lint regardless
230+
/// of the involved counterpart. For example, `["SomeType", "*"]` or `["*", "AnotherType"]`.
231+
///
232+
/// Pairs are asymmetric, which means that `["SomeType", "AnotherType"]` is not the same as
233+
/// `["AnotherType", "SomeType"]`.
234+
///
235+
/// #### Example
236+
///
237+
/// ```toml
238+
/// arithmetic-side-effects-allowed-binary = [["SomeType" , "f32"], ["AnotherType", "*"]]
239+
/// ```
240+
(arithmetic_side_effects_allowed_binary: Vec<[String; 2]> = <_>::default()),
241+
/// Lint: ARITHMETIC_SIDE_EFFECTS.
242+
///
243+
/// Suppress checking of the passed type names in unary operations like "negation" (`-`).
244+
///
245+
/// #### Example
246+
///
247+
/// ```toml
248+
/// arithmetic-side-effects-allowed-unary = ["SomeType", "AnotherType"]
249+
/// ```
250+
(arithmetic_side_effects_allowed_unary: rustc_data_structures::fx::FxHashSet<String> = <_>::default()),
212251
/// Lint: ENUM_VARIANT_NAMES, LARGE_TYPES_PASSED_BY_VALUE, TRIVIALLY_COPY_PASS_BY_REF, UNNECESSARY_WRAPS, UNUSED_SELF, UPPER_CASE_ACRONYMS, WRONG_SELF_CONVENTION, BOX_COLLECTION, REDUNDANT_ALLOCATION, RC_BUFFER, VEC_BOX, OPTION_OPTION, LINKEDLIST, RC_MUTEX.
213252
///
214253
/// Suppress lints whenever the suggested change would cause breakage for other crates.

‎tests/ui-toml/arithmetic_side_effects_allowed/arithmetic_side_effects_allowed.rs

Lines changed: 104 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -2,32 +2,117 @@
22

33
use core::ops::{Add, Neg};
44

5-
#[derive(Clone, Copy)]
6-
struct Point {
7-
x: i32,
8-
y: i32,
5+
macro_rules! create {
6+
($name:ident) => {
7+
#[allow(clippy::arithmetic_side_effects)]
8+
#[derive(Clone, Copy)]
9+
struct $name;
10+
11+
impl Add<$name> for $name {
12+
type Output = $name;
13+
fn add(self, other: $name) -> Self::Output {
14+
todo!()
15+
}
16+
}
17+
18+
impl Add<i32> for $name {
19+
type Output = $name;
20+
fn add(self, other: i32) -> Self::Output {
21+
todo!()
22+
}
23+
}
24+
25+
impl Add<$name> for i32 {
26+
type Output = $name;
27+
fn add(self, other: $name) -> Self::Output {
28+
todo!()
29+
}
30+
}
31+
32+
impl Add<i64> for $name {
33+
type Output = $name;
34+
fn add(self, other: i64) -> Self::Output {
35+
todo!()
36+
}
37+
}
38+
39+
impl Add<$name> for i64 {
40+
type Output = $name;
41+
fn add(self, other: $name) -> Self::Output {
42+
todo!()
43+
}
44+
}
45+
46+
impl Neg for $name {
47+
type Output = $name;
48+
fn neg(self) -> Self::Output {
49+
todo!()
50+
}
51+
}
52+
};
953
}
1054

11-
impl Add for Point {
12-
type Output = Self;
55+
create!(Foo);
56+
create!(Bar);
57+
create!(Baz);
58+
create!(OutOfNames);
1359

14-
fn add(self, other: Self) -> Self {
15-
todo!()
16-
}
60+
fn lhs_and_rhs_are_equal() {
61+
// is explicitly on the list
62+
let _ = OutOfNames + OutOfNames;
63+
// is explicitly on the list
64+
let _ = Foo + Foo;
65+
// is implicitly on the list
66+
let _ = Bar + Bar;
67+
// not on the list
68+
let _ = Baz + Baz;
1769
}
1870

19-
impl Neg for Point {
20-
type Output = Self;
71+
fn lhs_is_different() {
72+
// is explicitly on the list
73+
let _ = 1i32 + OutOfNames;
74+
// is explicitly on the list
75+
let _ = 1i32 + Foo;
76+
// is implicitly on the list
77+
let _ = 1i32 + Bar;
78+
// not on the list
79+
let _ = 1i32 + Baz;
2180

22-
fn neg(self) -> Self::Output {
23-
todo!()
24-
}
81+
// not on the list
82+
let _ = 1i64 + Foo;
83+
// is implicitly on the list
84+
let _ = 1i64 + Bar;
85+
// not on the list
86+
let _ = 1i64 + Baz;
2587
}
2688

27-
fn main() {
28-
let _ = Point { x: 1, y: 0 } + Point { x: 2, y: 3 };
89+
fn rhs_is_different() {
90+
// is explicitly on the list
91+
let _ = OutOfNames + 1i32;
92+
// is explicitly on the list
93+
let _ = Foo + 1i32;
94+
// is implicitly on the list
95+
let _ = Bar + 1i32;
96+
// not on the list
97+
let _ = Baz + 1i32;
98+
99+
// not on the list
100+
let _ = Foo + 1i64;
101+
// is implicitly on the list
102+
let _ = Bar + 1i64;
103+
// not on the list
104+
let _ = Baz + 1i64;
105+
}
29106

30-
let point: Point = Point { x: 1, y: 0 };
31-
let _ = point + point;
32-
let _ = -point;
107+
fn unary() {
108+
// is explicitly on the list
109+
let _ = -OutOfNames;
110+
// is specifically on the list
111+
let _ = -Foo;
112+
// not on the list
113+
let _ = -Bar;
114+
// not on the list
115+
let _ = -Baz;
33116
}
117+
118+
fn main() {}

‎tests/ui-toml/arithmetic_side_effects_allowed/arithmetic_side_effects_allowed.stderr

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
error: arithmetic operation that can potentially result in unexpected side-effects
2+
--> $DIR/arithmetic_side_effects_allowed.rs:68:13
3+
|
4+
LL | let _ = Baz + Baz;
5+
| ^^^^^^^^^
6+
|
7+
= note: `-D clippy::arithmetic-side-effects` implied by `-D warnings`
8+
9+
error: arithmetic operation that can potentially result in unexpected side-effects
10+
--> $DIR/arithmetic_side_effects_allowed.rs:79:13
11+
|
12+
LL | let _ = 1i32 + Baz;
13+
| ^^^^^^^^^^
14+
15+
error: arithmetic operation that can potentially result in unexpected side-effects
16+
--> $DIR/arithmetic_side_effects_allowed.rs:82:13
17+
|
18+
LL | let _ = 1i64 + Foo;
19+
| ^^^^^^^^^^
20+
21+
error: arithmetic operation that can potentially result in unexpected side-effects
22+
--> $DIR/arithmetic_side_effects_allowed.rs:86:13
23+
|
24+
LL | let _ = 1i64 + Baz;
25+
| ^^^^^^^^^^
26+
27+
error: arithmetic operation that can potentially result in unexpected side-effects
28+
--> $DIR/arithmetic_side_effects_allowed.rs:97:13
29+
|
30+
LL | let _ = Baz + 1i32;
31+
| ^^^^^^^^^^
32+
33+
error: arithmetic operation that can potentially result in unexpected side-effects
34+
--> $DIR/arithmetic_side_effects_allowed.rs:100:13
35+
|
36+
LL | let _ = Foo + 1i64;
37+
| ^^^^^^^^^^
38+
39+
error: arithmetic operation that can potentially result in unexpected side-effects
40+
--> $DIR/arithmetic_side_effects_allowed.rs:104:13
41+
|
42+
LL | let _ = Baz + 1i64;
43+
| ^^^^^^^^^^
44+
45+
error: arithmetic operation that can potentially result in unexpected side-effects
46+
--> $DIR/arithmetic_side_effects_allowed.rs:113:13
47+
|
48+
LL | let _ = -Bar;
49+
| ^^^^
50+
51+
error: arithmetic operation that can potentially result in unexpected side-effects
52+
--> $DIR/arithmetic_side_effects_allowed.rs:115:13
53+
|
54+
LL | let _ = -Baz;
55+
| ^^^^
56+
57+
error: aborting due to 9 previous errors
58+

0 commit comments

Comments
 (0)