Skip to content

Fix map_entry suggests wrongly for insert with cfg-ed out code #15800

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix map_entry suggests wrongly for insert with cfg-ed out code #15800

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 7 additions & 3 deletions clippy_lints/src/entry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use clippy_utils::ty::is_copy;
use clippy_utils::visitors::for_each_expr;
use clippy_utils::{
SpanlessEq, can_move_expr_to_closure_no_visit, higher, is_expr_final_block_expr, is_expr_used_or_unified,
peel_hir_expr_while,
peel_hir_expr_while, span_contains_non_whitespace,
};
use core::fmt::{self, Write};
use rustc_errors::Applicability;
Expand All @@ -13,7 +13,7 @@ use rustc_hir::intravisit::{Visitor, walk_body, walk_expr};
use rustc_hir::{Block, Expr, ExprKind, HirId, Pat, Stmt, StmtKind, UnOp};
use rustc_lint::{LateContext, LateLintPass};
use rustc_session::declare_lint_pass;
use rustc_span::{DUMMY_SP, Span, SyntaxContext, sym};
use rustc_span::{BytePos, DUMMY_SP, Pos, Span, SyntaxContext, sym};
use std::ops::ControlFlow;

declare_clippy_lint! {
Expand Down Expand Up @@ -166,7 +166,11 @@ impl<'tcx> LateLintPass<'tcx> for HashMapPass {
"if let {}::{entry_kind} = {map_str}.entry({key_str}) {body_str}",
map_ty.entry_path(),
))
} else if let Some(insertion) = then_search.as_single_insertion() {
} else if let Some(insertion) = then_search.as_single_insertion()
&& let span_in_between = then_expr.span.shrink_to_lo().between(insertion.call.span)
&& let span_in_between = span_in_between.with_lo(span_in_between.lo() + BytePos::from_usize(1))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • BytePos::from_usize(1) could be replaced with BytePos(1). In fact, this whole line could be simplified using Span::split_at:
Suggested change
&& let span_in_between = span_in_between.with_lo(span_in_between.lo() + BytePos::from_usize(1))
// remove the opening brace
&& let span_in_between = span_in_between.split_at(1).1
  • I think the same check would be needed for the code after the insert?

Generally, it could be better to do all these checks somewhere inside InsertSearcher? Though I'm not very familiar with visitors, so I'm not sure whether that's actually feasible.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the same check would be needed for the code after the insert?

Uh, I think it's a bit more complicated than that -- if there is cfgd-out code after the insert, then this case isn't even allow_insert_closure anymore. I think this is another argument for doing this check already in the visitor

&& !span_contains_non_whitespace(cx, span_in_between, true)
{
let value_str = snippet_with_context(cx, insertion.value.span, then_expr.span.ctxt(), "..", &mut app).0;
if contains_expr.negated {
if insertion.value.can_have_side_effects() {
Expand Down
10 changes: 10 additions & 0 deletions tests/ui/entry.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,3 +249,13 @@ mod issue14449 {
}

fn main() {}

fn issue15781(m: &mut std::collections::HashMap<i32, i32>, k: i32, v: i32) {
fn very_important_fn() {}
m.entry(k).or_insert_with(|| {
//~^ map_entry
#[cfg(test)]
very_important_fn();
v
});
}
10 changes: 10 additions & 0 deletions tests/ui/entry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,3 +255,13 @@ mod issue14449 {
}

fn main() {}

fn issue15781(m: &mut std::collections::HashMap<i32, i32>, k: i32, v: i32) {
fn very_important_fn() {}
if !m.contains_key(&k) {
//~^ map_entry
#[cfg(test)]
very_important_fn();
m.insert(k, v);
}
}
23 changes: 22 additions & 1 deletion tests/ui/entry.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,5 +246,26 @@ LL + e.insert(42);
LL + }
|

error: aborting due to 11 previous errors
error: usage of `contains_key` followed by `insert` on a `HashMap`
--> tests/ui/entry.rs:261:5
|
LL | / if !m.contains_key(&k) {
LL | |
LL | | #[cfg(test)]
LL | | very_important_fn();
LL | | m.insert(k, v);
LL | | }
| |_____^
|
help: try
|
LL ~ m.entry(k).or_insert_with(|| {
LL +
LL + #[cfg(test)]
LL + very_important_fn();
LL + v
LL + });
|

error: aborting due to 12 previous errors

Loading