Skip to content

Commit

Permalink
Minor cleanup of map_entry and a few additional tests.
Browse files Browse the repository at this point in the history
  • Loading branch information
@Jarcho
Jarcho committed Apr 15, 2021
1 parent 4de975c commit ff78dd4
Show file tree
Hide file tree
Showing 9 changed files with 207 additions and 185 deletions.
129 changes: 68 additions & 61 deletions clippy_lints/src/entry.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,12 +79,14 @@ impl<'tcx> LateLintPass<'tcx> for HashMapPass {
let sugg = if let Some(else_expr) = else_expr {
// if .. { .. } else { .. }
let else_search = match find_insert_calls(cx, &contains_expr, else_expr) {
Some(search) if !(then_search.edits.is_empty() && search.edits.is_empty()) => search,
_ => return,
Some(search) => search,
None => return,
};

if then_search.edits.is_empty() || else_search.edits.is_empty() {
// if .. { insert } else { .. } or if .. { .. } else { then } of
if then_search.edits.is_empty() && else_search.edits.is_empty() {
return;
} else if then_search.edits.is_empty() || else_search.edits.is_empty() {
// if .. { insert } else { .. } or if .. { .. } else { insert } of
let (then_str, else_str, entry_kind) = if else_search.edits.is_empty() {
if contains_expr.negated {
(
Expand Down Expand Up @@ -184,15 +186,17 @@ impl<'tcx> LateLintPass<'tcx> for HashMapPass {
format!("{}.entry({}).or_insert({});", map_str, key_str, value_str)
}
} else {
// Todo: if let Some(v) = map.get_mut(k)
// TODO: suggest using `if let Some(v) = map.get_mut(k) { .. }` here.
// This would need to be a different lint.
return;
}
} else {
let block_str = then_search.snippet_closure(cx, then_expr.span, &mut app);
if contains_expr.negated {
format!("{}.entry({}).or_insert_with(|| {});", map_str, key_str, block_str)
} else {
// Todo: if let Some(v) = map.get_mut(k)
// TODO: suggest using `if let Some(v) = map.get_mut(k) { .. }` here.
// This would need to be a different lint.
return;
}
}
Expand Down Expand Up @@ -222,7 +226,7 @@ impl MapType {
Self::BTree => "BTreeMap",
}
}
fn entry_path(self) -> &'staic str {
fn entry_path(self) -> &'static str {
match self {
Self::Hash => "std::collections::hash_map::Entry",
Self::BTree => "std::collections::btree_map::Entry",
Expand Down Expand Up @@ -312,15 +316,16 @@ struct Insertion<'tcx> {
value: &'tcx Expr<'tcx>,
}

// This visitor needs to do a multiple things:
// * Find all usages of the map. Only insertions into the map which share the same key are
// permitted. All others will prevent the lint.
// * Determine if the final statement executed is an insertion. This is needed to use `insert_with`.
// * Determine if there's any sub-expression that can't be placed in a closure.
// * Determine if there's only a single insert statement. This is needed to give better suggestions.

/// This visitor needs to do a multiple things:
/// * Find all usages of the map. An insertion can only be made before any other usages of the map.
/// * Determine if there's an insertion using the same key. There's no need for the entry api
/// otherwise.
/// * Determine if the final statement executed is an insertion. This is needed to use
/// `or_insert_with`.
/// * Determine if there's any sub-expression that can't be placed in a closure.
/// * Determine if there's only a single insert statement. `or_insert` can be used in this case.
#[allow(clippy::struct_excessive_bools)]
struct InsertSearcher<'cx, 'i, 'tcx> {
struct InsertSearcher<'cx, 'tcx> {
cx: &'cx LateContext<'tcx>,
/// The map expression used in the contains call.
map: &'tcx Expr<'tcx>,
Expand All @@ -334,13 +339,16 @@ struct InsertSearcher<'cx, 'i, 'tcx> {
can_use_entry: bool,
/// Whether this expression is the final expression in this code path. This may be a statement.
in_tail_pos: bool,
// A single insert expression has a slightly different suggestion.
// Is this expression a single insert. A slightly better suggestion can be made in this case.
is_single_insert: bool,
/// If the visitor has seen the map being used.
is_map_used: bool,
edits: &'i mut Vec<Edit<'tcx>>,
/// The locations where changes need to be made for the suggestion.
edits: Vec<Edit<'tcx>>,
/// A stack of loops the visitor is currently in.
loops: Vec<HirId>,
}
impl<'tcx> InsertSearcher<'_, '_, 'tcx> {
impl<'tcx> InsertSearcher<'_, 'tcx> {
/// Visit the expression as a branch in control flow. Multiple insert calls can be used, but
/// only if they are on separate code paths. This will return whether the map was used in the
/// given expression.
Expand All @@ -363,7 +371,7 @@ impl<'tcx> InsertSearcher<'_, '_, 'tcx> {
self.in_tail_pos = in_tail_pos;
}
}
impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, '_, 'tcx> {
impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, 'tcx> {
type Map = ErasedMap<'tcx>;
fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
NestedVisitorMap::None
Expand Down Expand Up @@ -483,6 +491,7 @@ impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, '_, 'tcx> {
},
ExprKind::Loop(block, ..) => {
self.loops.push(expr.hir_id);
self.is_single_insert = false;
self.allow_insert_closure &= !self.in_tail_pos;
// Don't allow insertions inside of a loop.
let edit_len = self.edits.len();
Expand Down Expand Up @@ -519,7 +528,13 @@ impl InsertSearchResults<'tcx> {
self.is_single_insert.then(|| self.edits[0].as_insertion().unwrap())
}

fn snippet_occupied(&self, cx: &LateContext<'_>, mut span: Span, app: &mut Applicability) -> String {
fn snippet(
&self,
cx: &LateContext<'_>,
mut span: Span,
app: &mut Applicability,
write_wrapped: impl Fn(&mut String, Insertion<'_>, SyntaxContext, &mut Applicability),
) -> String {
let ctxt = span.ctxt();
let mut res = String::new();
for insertion in self.edits.iter().filter_map(|e| e.as_insertion()) {
Expand All @@ -530,56 +545,47 @@ impl InsertSearchResults<'tcx> {
app,
));
if is_expr_used_or_unified(cx.tcx, insertion.call) {
res.push_str("Some(e.insert(");
res.push_str(&snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0);
res.push_str("))");
write_wrapped(&mut res, insertion, ctxt, app);
} else {
res.push_str("e.insert(");
res.push_str(&snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0);
res.push(')');
let _ = write!(
res,
"e.insert({})",
snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0
);
}
span = span.trim_start(insertion.call.span).unwrap_or(DUMMY_SP);
}
res.push_str(&snippet_with_applicability(cx, span, "..", app));
res
}

fn snippet_vacant(&self, cx: &LateContext<'_>, mut span: Span, app: &mut Applicability) -> String {
let ctxt = span.ctxt();
let mut res = String::new();
for insertion in self.edits.iter().filter_map(|e| e.as_insertion()) {
res.push_str(&snippet_with_applicability(
cx,
span.until(insertion.call.span),
"..",
app,
));
if is_expr_used_or_unified(cx.tcx, insertion.call) {
if is_expr_final_block_expr(cx.tcx, insertion.call) {
let _ = write!(
res,
"e.insert({});\n{}None",
snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0,
snippet_indent(cx, insertion.call.span).as_deref().unwrap_or(""),
);
} else {
let _ = write!(
res,
"{{ e.insert({}); None }}",
snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0,
);
}
fn snippet_occupied(&self, cx: &LateContext<'_>, span: Span, app: &mut Applicability) -> String {
self.snippet(cx, span, app, |res, insertion, ctxt, app| {
let _ = write!(
res,
"Some(e.insert({}))",
snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0
);
})
}

fn snippet_vacant(&self, cx: &LateContext<'_>, span: Span, app: &mut Applicability) -> String {
self.snippet(cx, span, app, |res, insertion, ctxt, app| {
let _ = if is_expr_final_block_expr(cx.tcx, insertion.call) {
write!(
res,
"e.insert({});\n{}None",
snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0,
snippet_indent(cx, insertion.call.span).as_deref().unwrap_or(""),
)
} else {
let _ = write!(
write!(
res,
"e.insert({})",
"{{ e.insert({}); None }}",
snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0,
);
}
span = span.trim_start(insertion.call.span).unwrap_or(DUMMY_SP);
}
res.push_str(&snippet_with_applicability(cx, span, "..", app));
res
)
};
})
}

fn snippet_closure(&self, cx: &LateContext<'_>, mut span: Span, app: &mut Applicability) -> String {
Expand Down Expand Up @@ -607,18 +613,18 @@ impl InsertSearchResults<'tcx> {
res
}
}

fn find_insert_calls(
cx: &LateContext<'tcx>,
contains_expr: &ContainsExpr<'tcx>,
expr: &'tcx Expr<'_>,
) -> Option<InsertSearchResults<'tcx>> {
let mut edits = Vec::new();
let mut s = InsertSearcher {
cx,
map: contains_expr.map,
key: contains_expr.key,
ctxt: expr.span.ctxt(),
edits: &mut edits,
edits: Vec::new(),
is_map_used: false,
allow_insert_closure: true,
can_use_entry: true,
Expand All @@ -629,6 +635,7 @@ fn find_insert_calls(
s.visit_expr(expr);
let allow_insert_closure = s.allow_insert_closure;
let is_single_insert = s.is_single_insert;
let edits = s.edits;
s.can_use_entry.then(|| InsertSearchResults {
edits,
allow_insert_closure,
Expand Down
59 changes: 14 additions & 45 deletions clippy_utils/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,8 @@ use rustc_hir::def_id::{DefId, LOCAL_CRATE};
use rustc_hir::intravisit::{self, walk_expr, ErasedMap, NestedVisitorMap, Visitor};
use rustc_hir::{
def, Arm, BindingAnnotation, Block, Body, Constness, Destination, Expr, ExprKind, FnDecl, GenericArgs, HirId, Impl,
ImplItem, ImplItemKind, Item, ItemKind, LangItem, MatchSource, Node, Param, Pat, PatKind, Path, PathSegment, QPath,
Stmt, StmtKind, TraitItem, TraitItemKind, TraitRef, TyKind,
ImplItem, ImplItemKind, Item, ItemKind, LangItem, Local, MatchSource, Node, Param, Pat, PatKind, Path, PathSegment,
QPath, Stmt, StmtKind, TraitItem, TraitItemKind, TraitRef, TyKind,
};
use rustc_lint::{LateContext, Level, Lint, LintContext};
use rustc_middle::hir::exports::Export;
Expand Down Expand Up @@ -1301,48 +1301,7 @@ pub fn is_must_use_func_call(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
did.map_or(false, |did| must_use_attr(&cx.tcx.get_attrs(did)).is_some())
}

pub fn get_expr_use_node(tcx: TyCtxt<'tcx>, expr: &Expr<'_>) -> Option<Node<'tcx>> {
let map = tcx.hir();
let mut child_id = expr.hir_id;
let mut iter = map.parent_iter(child_id);
loop {
match iter.next() {
None => break None,
Some((id, Node::Block(_))) => child_id = id,
Some((id, Node::Arm(arm))) if arm.body.hir_id == child_id => child_id = id,
Some((_, Node::Expr(expr))) => match expr.kind {
ExprKind::Break(
Destination {
target_id: Ok(dest), ..
},
_,
) => {
iter = map.parent_iter(dest);
child_id = dest;
},
ExprKind::DropTemps(_) | ExprKind::Block(..) => child_id = expr.hir_id,
ExprKind::If(control_expr, ..) | ExprKind::Match(control_expr, ..)
if control_expr.hir_id != child_id =>
{
child_id = expr.hir_id
},
_ => break Some(Node::Expr(expr)),
},
Some((_, node)) => break Some(node),
}
}
}

pub fn is_expr_used(tcx: TyCtxt<'_>, expr: &Expr<'_>) -> bool {
!matches!(
get_expr_use_node(tcx, expr),
Some(Node::Stmt(Stmt {
kind: StmtKind::Expr(_) | StmtKind::Semi(_),
..
}))
)
}

/// Gets the node where an expression is either used, or it's type is unified with another branch.
pub fn get_expr_use_or_unification_node(tcx: TyCtxt<'tcx>, expr: &Expr<'_>) -> Option<Node<'tcx>> {
let map = tcx.hir();
let mut child_id = expr.hir_id;
Expand All @@ -1363,16 +1322,26 @@ pub fn get_expr_use_or_unification_node(tcx: TyCtxt<'tcx>, expr: &Expr<'_>) -> O
}
}

/// Checks if the result of an expression is used, or it's type is unified with another branch.
pub fn is_expr_used_or_unified(tcx: TyCtxt<'_>, expr: &Expr<'_>) -> bool {
!matches!(
get_expr_use_or_unification_node(tcx, expr),
None | Some(Node::Stmt(Stmt {
kind: StmtKind::Expr(_) | StmtKind::Semi(_),
kind: StmtKind::Expr(_)
| StmtKind::Semi(_)
| StmtKind::Local(Local {
pat: Pat {
kind: PatKind::Wild,
..
},
..
}),
..
}))
)
}

/// Checks if the expression is the final expression returned from a block.
pub fn is_expr_final_block_expr(tcx: TyCtxt<'_>, expr: &Expr<'_>) -> bool {
matches!(get_parent_node(tcx, expr.hir_id), Some(Node::Block(..)))
}
Expand Down
Loading

0 comments on commit ff78dd4

Please sign in to comment.