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

Add checking for unnecessary delims in closure body #136906

Open
wants to merge 6 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler/rustc_errors/src/emitter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3455,7 +3455,7 @@ pub fn is_case_difference(sm: &SourceMap, suggested: &str, sp: Span) -> bool {
// All the chars that differ in capitalization are confusable (above):
let confusable = iter::zip(found.chars(), suggested.chars())
.filter(|(f, s)| f != s)
.all(|(f, s)| (ascii_confusables.contains(&f) || ascii_confusables.contains(&s)));
.all(|(f, s)| ascii_confusables.contains(&f) || ascii_confusables.contains(&s));
confusable && found.to_lowercase() == suggested.to_lowercase()
// FIXME: We sometimes suggest the same thing we already have, which is a
// bug, but be defensive against that here.
Expand Down
12 changes: 11 additions & 1 deletion compiler/rustc_lint/src/unused.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use std::iter;

use rustc_ast as ast;
use rustc_ast::util::{classify, parser};
use rustc_ast::{ExprKind, StmtKind};
use rustc_ast::{ExprKind, FnRetTy, StmtKind};
use rustc_errors::{MultiSpan, pluralize};
use rustc_hir::def::{DefKind, Res};
use rustc_hir::def_id::DefId;
Expand Down Expand Up @@ -593,6 +593,7 @@ enum UnusedDelimsCtx {
AnonConst,
MatchArmExpr,
IndexExpr,
ClosureBody,
}

impl From<UnusedDelimsCtx> for &'static str {
Expand All @@ -614,6 +615,7 @@ impl From<UnusedDelimsCtx> for &'static str {
UnusedDelimsCtx::ArrayLenExpr | UnusedDelimsCtx::AnonConst => "const expression",
UnusedDelimsCtx::MatchArmExpr => "match arm expression",
UnusedDelimsCtx::IndexExpr => "index expression",
UnusedDelimsCtx::ClosureBody => "closure body",
}
}
}
Expand Down Expand Up @@ -909,6 +911,14 @@ trait UnusedDelimLint {
let (args_to_check, ctx) = match *call_or_other {
Call(_, ref args) => (&args[..], UnusedDelimsCtx::FunctionArg),
MethodCall(ref call) => (&call.args[..], UnusedDelimsCtx::MethodArg),
Closure(ref closure)
if matches!(closure.fn_decl.output, FnRetTy::Default(_))
// skip `#[core::contracts::requires(...)]` and `#[core::contracts::ensures(...)]`
// which generate closure expr with span that is same as the inner closure body span
&& e.span != closure.body.span =>
{
(&[closure.body.clone()][..], UnusedDelimsCtx::ClosureBody)
}
// actual catch-all arm
_ => {
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,7 @@ impl<'a, Node: Idx> SpantreeBuilder<'a, Node> {
debug_assert!(
span_edges
.iter_enumerated()
.all(|(node, span_edge)| { span_edge.is_some() <= self.is_supernode(node) }),
.all(|(node, span_edge)| span_edge.is_some() <= self.is_supernode(node)),
"only supernodes can have a span edge",
);
debug_assert!(
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir_transform/src/coverage/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ fn create_mappings(extracted_mappings: &ExtractedMappings) -> Vec<Mapping> {
condition_info: _,
true_index: _,
false_index: _,
}| { Mapping { kind: MappingKind::Branch { true_bcb, false_bcb }, span } },
}| Mapping { kind: MappingKind::Branch { true_bcb, false_bcb }, span },
));

for (decision, branches) in mcdc_mappings {
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_mir_transform/src/coverage/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ fn print_mir_graphviz(name: &str, mir_body: &Body<'_>) {
mir_body
.basic_blocks
.successors(bb)
.map(|successor| { format!(" {:?} -> {:?};", bb, successor) })
.map(|successor| format!(" {:?} -> {:?};", bb, successor))
.join("\n")
)
})
Expand All @@ -241,7 +241,7 @@ fn print_coverage_graphviz(name: &str, mir_body: &Body<'_>, graph: &graph::Cover
mir_body[bcb_data.last_bb()].terminator().kind.name(),
graph
.successors(bcb)
.map(|successor| { format!(" {:?} -> {:?};", bcb, successor) })
.map(|successor| format!(" {:?} -> {:?};", bcb, successor))
.join("\n")
)
})
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_parse/src/parser/item.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2186,7 +2186,7 @@ impl<'a> Parser<'a> {

if self.look_ahead(1, |t| *t == token::Not) && self.look_ahead(2, |t| t.is_ident()) {
return IsMacroRulesItem::Yes { has_bang: true };
} else if self.look_ahead(1, |t| (t.is_ident())) {
} else if self.look_ahead(1, |t| t.is_ident()) {
// macro_rules foo
self.dcx().emit_err(errors::MacroRulesMissingBang {
span: macro_rules_span,
Expand Down
3 changes: 1 addition & 2 deletions compiler/rustc_resolve/src/late/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -326,8 +326,7 @@ impl<'ast, 'ra: 'ast, 'tcx> LateResolutionVisitor<'_, 'ast, 'ra, 'tcx> {
let module_did = mod_prefix.as_ref().and_then(Res::mod_def_id);

let mod_prefix =
mod_prefix.map_or_else(String::new, |res| (format!("{} ", res.descr())));

mod_prefix.map_or_else(String::new, |res| format!("{} ", res.descr()));
(mod_prefix, format!("`{}`", Segment::names_to_string(mod_path)), module_did, None)
};

Expand Down
4 changes: 2 additions & 2 deletions library/alloc/src/collections/btree/map/entry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -247,12 +247,12 @@ impl<'a, K: Ord, V, A: Allocator + Clone> Entry<'a, K, V, A> {
/// let mut map: BTreeMap<&str, usize> = BTreeMap::new();
///
/// map.entry("poneyland")
/// .and_modify(|e| { *e += 1 })
/// .and_modify(|e| *e += 1)
/// .or_insert(42);
/// assert_eq!(map["poneyland"], 42);
///
/// map.entry("poneyland")
/// .and_modify(|e| { *e += 1 })
/// .and_modify(|e| *e += 1)
/// .or_insert(42);
/// assert_eq!(map["poneyland"], 43);
/// ```
Expand Down
2 changes: 1 addition & 1 deletion library/alloc/src/vec/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3653,7 +3653,7 @@ impl<T, A: Allocator> Vec<T, A> {
///
/// ```
/// # use std::cmp::min;
/// # let some_predicate = |x: &mut i32| { *x == 2 || *x == 3 || *x == 6 };
/// # let some_predicate = |x: &mut i32| *x == 2 || *x == 3 || *x == 6;
/// # let mut vec = vec![1, 2, 3, 4, 5, 6];
/// # let range = 1..4;
/// let mut i = range.start;
Expand Down
8 changes: 4 additions & 4 deletions library/std/src/collections/hash/map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2029,13 +2029,13 @@ impl<'a, K, V, S> RawEntryMut<'a, K, V, S> {
///
/// map.raw_entry_mut()
/// .from_key("poneyland")
/// .and_modify(|_k, v| { *v += 1 })
/// .and_modify(|_k, v| *v += 1)
/// .or_insert("poneyland", 42);
/// assert_eq!(map["poneyland"], 42);
///
/// map.raw_entry_mut()
/// .from_key("poneyland")
/// .and_modify(|_k, v| { *v += 1 })
/// .and_modify(|_k, v| *v += 1)
/// .or_insert("poneyland", 0);
/// assert_eq!(map["poneyland"], 43);
/// ```
Expand Down Expand Up @@ -2886,12 +2886,12 @@ impl<'a, K, V> Entry<'a, K, V> {
/// let mut map: HashMap<&str, u32> = HashMap::new();
///
/// map.entry("poneyland")
/// .and_modify(|e| { *e += 1 })
/// .and_modify(|e| *e += 1)
/// .or_insert(42);
/// assert_eq!(map["poneyland"], 42);
///
/// map.entry("poneyland")
/// .and_modify(|e| { *e += 1 })
/// .and_modify(|e| *e += 1)
/// .or_insert(42);
/// assert_eq!(map["poneyland"], 43);
/// ```
Expand Down
2 changes: 1 addition & 1 deletion library/std/src/sync/poison/condvar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ impl Condvar {
/// // Wait for the thread to start up.
/// let (lock, cvar) = &*pair;
/// // As long as the value inside the `Mutex<bool>` is `true`, we wait.
/// let _guard = cvar.wait_while(lock.lock().unwrap(), |pending| { *pending }).unwrap();
/// let _guard = cvar.wait_while(lock.lock().unwrap(), |pending| *pending).unwrap();
/// ```
#[stable(feature = "wait_until", since = "1.42.0")]
pub fn wait_while<'a, T, F>(
Expand Down
4 changes: 2 additions & 2 deletions library/std/tests/env_modify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,9 +91,9 @@ fn test_env_set_var() {
unsafe {
set_var(&n, "VALUE");
}
assert!(!e.any(|(k, v)| { &*k == &*n && &*v == "VALUE" }));
assert!(!e.any(|(k, v)| &*k == &*n && &*v == "VALUE"));

assert!(vars_os().any(|(k, v)| { &*k == &*n && &*v == "VALUE" }));
assert!(vars_os().any(|(k, v)| &*k == &*n && &*v == "VALUE"));
}

#[test]
Expand Down
4 changes: 2 additions & 2 deletions src/etc/test-float-parse/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -443,9 +443,9 @@ fn launch_tests(tests: &mut [TestInfo], cfg: &Config) -> Duration {
tests.iter().partition(|t| t.is_huge_test());

// Run the actual tests
normal_tests.par_iter().for_each(|test| ((test.launch)(&tx, test, cfg)));
normal_tests.par_iter().for_each(|test| (test.launch)(&tx, test, cfg));

huge_tests.par_iter().for_each(|test| ((test.launch)(&tx, test, cfg)));
huge_tests.par_iter().for_each(|test| (test.launch)(&tx, test, cfg));
});

start.elapsed()
Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/html/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1342,7 +1342,7 @@ impl clean::FnDecl {
fmt::from_fn(move |f| {
// First, generate the text form of the declaration, with no line wrapping, and count the bytes.
let mut counter = WriteCounter(0);
write!(&mut counter, "{:#}", fmt::from_fn(|f| { self.inner_full_print(None, f, cx) }))
write!(&mut counter, "{:#}", fmt::from_fn(|f| self.inner_full_print(None, f, cx)))
.unwrap();
// If the text form was over 80 characters wide, we will line-wrap our output.
let line_wrapping_indent =
Expand Down
2 changes: 1 addition & 1 deletion src/tools/clippy/clippy_lints/src/unused_async.rs
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ impl<'tcx> LateLintPass<'tcx> for UnusedAsync {
let iter = self
.unused_async_fns
.iter()
.filter(|UnusedAsyncFn { def_id, .. }| (!self.async_fns_as_value.contains(def_id)));
.filter(|UnusedAsyncFn { def_id, .. }| !self.async_fns_as_value.contains(def_id));

for fun in iter {
span_lint_hir_and_then(
Expand Down
2 changes: 1 addition & 1 deletion src/tools/clippy/clippy_utils/src/higher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ impl<'hir> IfLetOrMatch<'hir> {
if_then,
if_else,
let_span,
}| { Self::IfLet(let_expr, let_pat, if_then, if_else, let_span) },
}| Self::IfLet(let_expr, let_pat, if_then, if_else, let_span),
),
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/tools/clippy/clippy_utils/src/ty/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -927,7 +927,7 @@ impl AdtVariantInfo {
.enumerate()
.map(|(i, f)| (i, approx_ty_size(cx, f.ty(cx.tcx, subst))))
.collect::<Vec<_>>();
fields_size.sort_by(|(_, a_size), (_, b_size)| (a_size.cmp(b_size)));
fields_size.sort_by(|(_, a_size), (_, b_size)| a_size.cmp(b_size));

Self {
ind: i,
Expand All @@ -936,7 +936,7 @@ impl AdtVariantInfo {
}
})
.collect::<Vec<_>>();
variants_size.sort_by(|a, b| (b.size.cmp(&a.size)));
variants_size.sort_by(|a, b| b.size.cmp(&a.size));
variants_size
}
}
Expand Down
6 changes: 6 additions & 0 deletions src/tools/compiletest/src/runtest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1713,6 +1713,12 @@ impl<'test> TestCx<'test> {
// Allow tests to use internal features.
rustc.args(&["-A", "internal_features"]);

// Allow tests to have unused parens and braces.
// Add #![deny(unused_parens, unused_braces)] to the test file if you want to
// test that these lints are working.
rustc.args(&["-A", "unused_parens"]);
rustc.args(&["-A", "unused_braces"]);

if self.props.force_host {
self.maybe_add_external_args(&mut rustc, &self.config.host_rustcflags);
if !is_rustdoc {
Expand Down
2 changes: 1 addition & 1 deletion src/tools/miri/src/concurrency/thread.rs
Original file line number Diff line number Diff line change
Expand Up @@ -641,7 +641,7 @@ impl<'tcx> ThreadManager<'tcx> {
assert!(
self.threads
.iter()
.all(|thread| { !thread.state.is_blocked_on(BlockReason::Join(joined_thread_id)) }),
.all(|thread| !thread.state.is_blocked_on(BlockReason::Join(joined_thread_id))),
"this thread already has threads waiting for its termination"
);

Expand Down
1 change: 0 additions & 1 deletion tests/ui/async-await/issues/issue-54752-async-block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,3 @@
//@ pp-exact

fn main() { let _a = (async { }); }
//~^ WARNING unnecessary parentheses around assigned value
15 changes: 0 additions & 15 deletions tests/ui/async-await/issues/issue-54752-async-block.stderr

This file was deleted.

36 changes: 36 additions & 0 deletions tests/ui/lint/unused/closure-body-issue-136741.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
//@ run-rustfix
// ignore-tidy-linelength
#![deny(unused_parens)]
#![deny(unused_braces)]

fn long_expr_that_does_not_require_braces_long_expr_that_does_not_require_braces_long_expr_that_does_not_require_braces()
{}

fn func(f: impl FnOnce()) {
f()
}

pub fn main() {
let _closure = |x: i32, y: i32| x * (x + (y * 2)); //~ ERROR unnecessary braces around closure body
let _ = || 0 == 0; //~ ERROR unnecessary parentheses around closure body
let _ = (0..).find(|n| n % 2 == 0); //~ ERROR unnecessary parentheses around closure body
let _ = (0..).find(|n| n % 2 == 0); //~ ERROR unnecessary braces around closure body

// multiple lines of code will not lint with braces
let _ = (0..).find(|n| {
n % 2 == 0
});

// multiple lines of code will lint with parentheses
let _ = (0..).find(|n| n % 2 == 0);

let _ = || {
_ = 0;
0 == 0 //~ ERROR unnecessary parentheses around block return value
};

// long expressions will not lint with braces
func(|| {
long_expr_that_does_not_require_braces_long_expr_that_does_not_require_braces_long_expr_that_does_not_require_braces()
})
}
38 changes: 38 additions & 0 deletions tests/ui/lint/unused/closure-body-issue-136741.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
//@ run-rustfix
// ignore-tidy-linelength
#![deny(unused_parens)]
#![deny(unused_braces)]

fn long_expr_that_does_not_require_braces_long_expr_that_does_not_require_braces_long_expr_that_does_not_require_braces()
{}

fn func(f: impl FnOnce()) {
f()
}

pub fn main() {
let _closure = |x: i32, y: i32| { x * (x + (y * 2)) }; //~ ERROR unnecessary braces around closure body
let _ = || (0 == 0); //~ ERROR unnecessary parentheses around closure body
let _ = (0..).find(|n| (n % 2 == 0)); //~ ERROR unnecessary parentheses around closure body
let _ = (0..).find(|n| {n % 2 == 0}); //~ ERROR unnecessary braces around closure body

// multiple lines of code will not lint with braces
let _ = (0..).find(|n| {
n % 2 == 0
});

// multiple lines of code will lint with parentheses
let _ = (0..).find(|n| ( //~ ERROR unnecessary parentheses around closure body
n % 2 == 0
));

let _ = || {
_ = 0;
(0 == 0) //~ ERROR unnecessary parentheses around block return value
};

// long expressions will not lint with braces
func(|| {
long_expr_that_does_not_require_braces_long_expr_that_does_not_require_braces_long_expr_that_does_not_require_braces()
})
}
Loading
Loading