diff --git a/clippy_lints/src/double_parens.rs b/clippy_lints/src/double_parens.rs index bddf4702fb34..8defbeeaa5f2 100644 --- a/clippy_lints/src/double_parens.rs +++ b/clippy_lints/src/double_parens.rs @@ -1,5 +1,5 @@ use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::source::{HasSession, snippet_with_applicability, snippet_with_context}; +use clippy_utils::source::{HasSession, SpanRangeExt, snippet_with_applicability, snippet_with_context}; use rustc_ast::ast::{Expr, ExprKind, MethodCall}; use rustc_errors::Applicability; use rustc_lint::{EarlyContext, EarlyLintPass}; @@ -47,8 +47,12 @@ impl EarlyLintPass for DoubleParens { // ^^^^^^ expr // ^^^^ inner ExprKind::Paren(inner) if matches!(inner.kind, ExprKind::Paren(_) | ExprKind::Tup(_)) => { - // suggest removing the outer parens - if expr.span.eq_ctxt(inner.span) { + if expr.span.eq_ctxt(inner.span) + && !expr.span.in_external_macro(cx.sess().source_map()) + && check_source(cx, inner) + { + // suggest removing the outer parens + let mut applicability = Applicability::MachineApplicable; // We don't need to use `snippet_with_context` here, because: // - if `inner`'s `ctxt` is from macro, we don't lint in the first place (see the check above) @@ -74,8 +78,12 @@ impl EarlyLintPass for DoubleParens { if let [arg] = &**args && let ExprKind::Paren(inner) = &arg.kind => { - // suggest removing the inner parens - if expr.span.eq_ctxt(arg.span) { + if expr.span.eq_ctxt(arg.span) + && !arg.span.in_external_macro(cx.sess().source_map()) + && check_source(cx, arg) + { + // suggest removing the inner parens + let mut applicability = Applicability::MachineApplicable; let sugg = snippet_with_context(cx.sess(), inner.span, arg.span.ctxt(), "_", &mut applicability).0; span_lint_and_sugg( @@ -93,3 +101,22 @@ impl EarlyLintPass for DoubleParens { } } } + +/// Check that the span does indeed look like `( (..) )` +fn check_source(cx: &EarlyContext<'_>, inner: &Expr) -> bool { + if let Some(sfr) = inner.span.get_source_range(cx) + // this is the same as `SourceFileRange::as_str`, but doesn't apply the range right away, because + // we're interested in the source code outside it + && let Some(src) = sfr.sf.src.as_ref().map(|src| src.as_str()) + && let Some((start, outer_after_inner)) = src.split_at_checked(sfr.range.end) + && let Some((outer_before_inner, inner)) = start.split_at_checked(sfr.range.start) + && outer_before_inner.trim_end().ends_with('(') + && inner.starts_with('(') + && inner.ends_with(')') + && outer_after_inner.trim_start().starts_with(')') + { + true + } else { + false + } +} diff --git a/tests/ui/auxiliary/macro_rules.rs b/tests/ui/auxiliary/macro_rules.rs index 9efbb3908497..1f1afe4ea3a5 100644 --- a/tests/ui/auxiliary/macro_rules.rs +++ b/tests/ui/auxiliary/macro_rules.rs @@ -57,3 +57,15 @@ macro_rules! bad_transmute { std::mem::transmute($e) }; } + +#[macro_export] +#[rustfmt::skip] +macro_rules! double_parens { + ($a:expr, $b:expr, $c:expr, $d:expr) => {{ + let a = ($a); + let a = (()); + let b = ((5)); + let c = std::convert::identity((5)); + InterruptMask((($a.union($b).union($c).union($d)).into_bits()) as u32) + }}; +} diff --git a/tests/ui/auxiliary/proc_macro_derive.rs b/tests/ui/auxiliary/proc_macro_derive.rs index 546509228717..629a500ff6f5 100644 --- a/tests/ui/auxiliary/proc_macro_derive.rs +++ b/tests/ui/auxiliary/proc_macro_derive.rs @@ -230,3 +230,14 @@ pub fn allow_lint_same_span_derive(input: TokenStream) -> TokenStream { span_help(Group::new(Delimiter::Brace, TokenStream::new()).into()), ]) } + +#[proc_macro_derive(DoubleParens)] +pub fn derive_double_parens(_: TokenStream) -> TokenStream { + quote! { + fn foo() { + let a = (()); + let b = ((5)); + let c = std::convert::identity((5)); + } + } +} diff --git a/tests/ui/double_parens.fixed b/tests/ui/double_parens.fixed index dedc513438d1..024af6840132 100644 --- a/tests/ui/double_parens.fixed +++ b/tests/ui/double_parens.fixed @@ -1,8 +1,13 @@ +//@aux-build:proc_macros.rs +//@aux-build:proc_macro_derive.rs +//@aux-build:macro_rules.rs #![warn(clippy::double_parens)] #![expect(clippy::eq_op, clippy::no_effect)] #![feature(custom_inner_attributes)] #![rustfmt::skip] +use proc_macros::{external, with_span}; + fn dummy_fn(_: T) {} struct DummyStruct; @@ -96,4 +101,64 @@ fn issue9000(x: DummyStruct) { //~^ double_parens } +fn issue15892() { + use macro_rules::double_parens as double_parens_external; + + macro_rules! double_parens{ + ($a:expr, $b:expr, $c:expr, $d:expr) => {{ + let a = ($a); + let a = (); + //~^ double_parens + let b = (5); + //~^ double_parens + let c = std::convert::identity(5); + //~^ double_parens + InterruptMask((($a.union($b).union($c).union($d)).into_bits()) as u32) + }}; + } + + // Don't lint: external macro + (external!((5))); + external!(((5))); + + #[repr(transparent)] + #[derive(Clone, Copy, PartialEq, Eq)] + pub struct InterruptMask(u32); + + impl InterruptMask { + pub const OE: InterruptMask = InterruptMask(1 << 10); + pub const BE: InterruptMask = InterruptMask(1 << 9); + pub const PE: InterruptMask = InterruptMask(1 << 8); + pub const FE: InterruptMask = InterruptMask(1 << 7); + // Lint: internal macro + pub const E: InterruptMask = double_parens!((Self::OE), Self::BE, Self::PE, Self::FE); + // Don't lint: external macro + pub const F: InterruptMask = double_parens_external!((Self::OE), Self::BE, Self::PE, Self::FE); + #[allow(clippy::unnecessary_cast)] + pub const G: InterruptMask = external!( + InterruptMask((((Self::OE.union(Self::BE).union(Self::PE).union(Self::FE))).into_bits()) as u32) + ); + #[allow(clippy::unnecessary_cast)] + // Don't lint: external proc-macro + pub const H: InterruptMask = with_span!(span + InterruptMask((((Self::OE.union(Self::BE).union(Self::PE).union(Self::FE))).into_bits()) as u32) + ); + pub const fn into_bits(self) -> u32 { + self.0 + } + #[must_use] + pub const fn union(self, rhs: Self) -> Self { + InterruptMask(self.0 | rhs.0) + } + } +} + +fn issue15940() { + use proc_macro_derive::DoubleParens; + + #[derive(DoubleParens)] + // Don't lint: external derive macro + pub struct Person; +} + fn main() {} diff --git a/tests/ui/double_parens.rs b/tests/ui/double_parens.rs index 27f252485b71..8a76f2837f35 100644 --- a/tests/ui/double_parens.rs +++ b/tests/ui/double_parens.rs @@ -1,8 +1,13 @@ +//@aux-build:proc_macros.rs +//@aux-build:proc_macro_derive.rs +//@aux-build:macro_rules.rs #![warn(clippy::double_parens)] #![expect(clippy::eq_op, clippy::no_effect)] #![feature(custom_inner_attributes)] #![rustfmt::skip] +use proc_macros::{external, with_span}; + fn dummy_fn(_: T) {} struct DummyStruct; @@ -96,4 +101,64 @@ fn issue9000(x: DummyStruct) { //~^ double_parens } +fn issue15892() { + use macro_rules::double_parens as double_parens_external; + + macro_rules! double_parens{ + ($a:expr, $b:expr, $c:expr, $d:expr) => {{ + let a = ($a); + let a = (()); + //~^ double_parens + let b = ((5)); + //~^ double_parens + let c = std::convert::identity((5)); + //~^ double_parens + InterruptMask((($a.union($b).union($c).union($d)).into_bits()) as u32) + }}; + } + + // Don't lint: external macro + (external!((5))); + external!(((5))); + + #[repr(transparent)] + #[derive(Clone, Copy, PartialEq, Eq)] + pub struct InterruptMask(u32); + + impl InterruptMask { + pub const OE: InterruptMask = InterruptMask(1 << 10); + pub const BE: InterruptMask = InterruptMask(1 << 9); + pub const PE: InterruptMask = InterruptMask(1 << 8); + pub const FE: InterruptMask = InterruptMask(1 << 7); + // Lint: internal macro + pub const E: InterruptMask = double_parens!((Self::OE), Self::BE, Self::PE, Self::FE); + // Don't lint: external macro + pub const F: InterruptMask = double_parens_external!((Self::OE), Self::BE, Self::PE, Self::FE); + #[allow(clippy::unnecessary_cast)] + pub const G: InterruptMask = external!( + InterruptMask((((Self::OE.union(Self::BE).union(Self::PE).union(Self::FE))).into_bits()) as u32) + ); + #[allow(clippy::unnecessary_cast)] + // Don't lint: external proc-macro + pub const H: InterruptMask = with_span!(span + InterruptMask((((Self::OE.union(Self::BE).union(Self::PE).union(Self::FE))).into_bits()) as u32) + ); + pub const fn into_bits(self) -> u32 { + self.0 + } + #[must_use] + pub const fn union(self, rhs: Self) -> Self { + InterruptMask(self.0 | rhs.0) + } + } +} + +fn issue15940() { + use proc_macro_derive::DoubleParens; + + #[derive(DoubleParens)] + // Don't lint: external derive macro + pub struct Person; +} + fn main() {} diff --git a/tests/ui/double_parens.stderr b/tests/ui/double_parens.stderr index 3a740e44cacf..51b5c6279b21 100644 --- a/tests/ui/double_parens.stderr +++ b/tests/ui/double_parens.stderr @@ -1,5 +1,5 @@ error: unnecessary parentheses - --> tests/ui/double_parens.rs:15:5 + --> tests/ui/double_parens.rs:20:5 | LL | ((0)) | ^^^^^ help: remove them: `(0)` @@ -8,37 +8,37 @@ LL | ((0)) = help: to override `-D warnings` add `#[allow(clippy::double_parens)]` error: unnecessary parentheses - --> tests/ui/double_parens.rs:20:14 + --> tests/ui/double_parens.rs:25:14 | LL | dummy_fn((0)); | ^^^ help: remove them: `0` error: unnecessary parentheses - --> tests/ui/double_parens.rs:25:20 + --> tests/ui/double_parens.rs:30:20 | LL | x.dummy_method((0)); | ^^^ help: remove them: `0` error: unnecessary parentheses - --> tests/ui/double_parens.rs:30:5 + --> tests/ui/double_parens.rs:35:5 | LL | ((1, 2)) | ^^^^^^^^ help: remove them: `(1, 2)` error: unnecessary parentheses - --> tests/ui/double_parens.rs:36:5 + --> tests/ui/double_parens.rs:41:5 | LL | (()) | ^^^^ help: remove them: `()` error: unnecessary parentheses - --> tests/ui/double_parens.rs:59:16 + --> tests/ui/double_parens.rs:64:16 | LL | assert_eq!(((1, 2)), (1, 2), "Error"); | ^^^^^^^^ help: remove them: `(1, 2)` error: unnecessary parentheses - --> tests/ui/double_parens.rs:84:16 + --> tests/ui/double_parens.rs:89:16 | LL | () => {((100))} | ^^^^^^^ help: remove them: `(100)` @@ -49,22 +49,55 @@ LL | bar!(); = note: this error originates in the macro `bar` (in Nightly builds, run with -Z macro-backtrace for more info) error: unnecessary parentheses - --> tests/ui/double_parens.rs:91:5 + --> tests/ui/double_parens.rs:96:5 | LL | ((vec![1, 2])); | ^^^^^^^^^^^^^^ help: remove them: `(vec![1, 2])` error: unnecessary parentheses - --> tests/ui/double_parens.rs:93:14 + --> tests/ui/double_parens.rs:98:14 | LL | dummy_fn((vec![1, 2])); | ^^^^^^^^^^^^ help: remove them: `vec![1, 2]` error: unnecessary parentheses - --> tests/ui/double_parens.rs:95:20 + --> tests/ui/double_parens.rs:100:20 | LL | x.dummy_method((vec![1, 2])); | ^^^^^^^^^^^^ help: remove them: `vec![1, 2]` -error: aborting due to 10 previous errors +error: unnecessary parentheses + --> tests/ui/double_parens.rs:110:21 + | +LL | let a = (()); + | ^^^^ help: remove them: `()` +... +LL | pub const E: InterruptMask = double_parens!((Self::OE), Self::BE, Self::PE, Self::FE); + | -------------------------------------------------------- in this macro invocation + | + = note: this error originates in the macro `double_parens` (in Nightly builds, run with -Z macro-backtrace for more info) + +error: unnecessary parentheses + --> tests/ui/double_parens.rs:112:21 + | +LL | let b = ((5)); + | ^^^^^ help: remove them: `(5)` +... +LL | pub const E: InterruptMask = double_parens!((Self::OE), Self::BE, Self::PE, Self::FE); + | -------------------------------------------------------- in this macro invocation + | + = note: this error originates in the macro `double_parens` (in Nightly builds, run with -Z macro-backtrace for more info) + +error: unnecessary parentheses + --> tests/ui/double_parens.rs:114:44 + | +LL | let c = std::convert::identity((5)); + | ^^^ help: remove them: `5` +... +LL | pub const E: InterruptMask = double_parens!((Self::OE), Self::BE, Self::PE, Self::FE); + | -------------------------------------------------------- in this macro invocation + | + = note: this error originates in the macro `double_parens` (in Nightly builds, run with -Z macro-backtrace for more info) + +error: aborting due to 13 previous errors