Skip to content

Useless vec false positive #11895

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

Merged
merged 1 commit into from
Dec 12, 2023
Merged

Useless vec false positive #11895

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
3 changes: 3 additions & 0 deletions clippy_lints/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ extern crate clippy_utils;
#[macro_use]
extern crate declare_clippy_lint;

use std::collections::BTreeMap;

use rustc_data_structures::fx::FxHashSet;
use rustc_lint::{Lint, LintId};

Expand Down Expand Up @@ -725,6 +727,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
Box::new(vec::UselessVec {
too_large_for_stack,
msrv: msrv(),
span_to_lint_map: BTreeMap::new(),
})
});
store.register_late_pass(|_| Box::new(panic_unimplemented::PanicUnimplemented));
Expand Down
157 changes: 86 additions & 71 deletions clippy_lints/src/vec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,25 +1,27 @@
use std::collections::BTreeMap;
use std::ops::ControlFlow;

use clippy_config::msrvs::{self, Msrv};
use clippy_utils::consts::{constant, Constant};
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::diagnostics::span_lint_hir_and_then;
use clippy_utils::source::snippet_with_applicability;
use clippy_utils::ty::is_copy;
use clippy_utils::visitors::for_each_local_use_after_expr;
use clippy_utils::{get_parent_expr, higher, is_trait_method};
use rustc_errors::Applicability;
use rustc_hir::{BorrowKind, Expr, ExprKind, Mutability, Node, PatKind};
use rustc_hir::{BorrowKind, Expr, ExprKind, HirId, Mutability, Node, PatKind};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::ty::layout::LayoutOf;
use rustc_middle::ty::{self, Ty};
use rustc_session::impl_lint_pass;
use rustc_span::{sym, Span};
use rustc_span::{sym, DesugaringKind, Span};

#[expect(clippy::module_name_repetitions)]
#[derive(Clone)]
pub struct UselessVec {
pub too_large_for_stack: u64,
pub msrv: Msrv,
pub span_to_lint_map: BTreeMap<Span, Option<(HirId, SuggestedType, String, Applicability)>>,
}

declare_clippy_lint! {
Expand Down Expand Up @@ -69,72 +71,96 @@ pub fn is_allowed_vec_method(cx: &LateContext<'_>, e: &Expr<'_>) -> bool {

impl<'tcx> LateLintPass<'tcx> for UselessVec {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
// search for `&vec![_]` or `vec![_]` expressions where the adjusted type is `&[_]`
if adjusts_to_slice(cx, expr)
&& let Some(vec_args) = higher::VecArgs::hir(cx, expr.peel_borrows())
{
let (suggest_slice, span) = if let ExprKind::AddrOf(BorrowKind::Ref, mutability, _) = expr.kind {
// `expr` is `&vec![_]`, so suggest `&[_]` (or `&mut[_]` resp.)
(SuggestedType::SliceRef(mutability), expr.span)
} else {
// `expr` is the `vec![_]` expansion, so suggest `[_]`
// and also use the span of the actual `vec![_]` expression
(SuggestedType::Array, expr.span.ctxt().outer_expn_data().call_site)
};

self.check_vec_macro(cx, &vec_args, span, suggest_slice);
}
if let Some(vec_args) = higher::VecArgs::hir(cx, expr.peel_borrows()) {
// search for `let foo = vec![_]` expressions where all uses of `foo`
// adjust to slices or call a method that exist on slices (e.g. len)
if let Node::Local(local) = cx.tcx.hir().get_parent(expr.hir_id)
// for now ignore locals with type annotations.
// this is to avoid compile errors when doing the suggestion here: let _: Vec<_> = vec![..];
&& local.ty.is_none()
&& let PatKind::Binding(_, id, ..) = local.pat.kind
&& is_copy(cx, vec_type(cx.typeck_results().expr_ty_adjusted(expr.peel_borrows())))
{
let only_slice_uses = for_each_local_use_after_expr(cx, id, expr.hir_id, |expr| {
// allow indexing into a vec and some set of allowed method calls that exist on slices, too
if let Some(parent) = get_parent_expr(cx, expr)
&& (adjusts_to_slice(cx, expr)
|| matches!(parent.kind, ExprKind::Index(..))
|| is_allowed_vec_method(cx, parent))
{
ControlFlow::Continue(())
} else {
ControlFlow::Break(())
}
})
.is_continue();

// search for `let foo = vec![_]` expressions where all uses of `foo`
// adjust to slices or call a method that exist on slices (e.g. len)
if let Some(vec_args) = higher::VecArgs::hir(cx, expr)
&& let Node::Local(local) = cx.tcx.hir().get_parent(expr.hir_id)
// for now ignore locals with type annotations.
// this is to avoid compile errors when doing the suggestion here: let _: Vec<_> = vec![..];
&& local.ty.is_none()
&& let PatKind::Binding(_, id, ..) = local.pat.kind
&& is_copy(cx, vec_type(cx.typeck_results().expr_ty_adjusted(expr)))
{
let only_slice_uses = for_each_local_use_after_expr(cx, id, expr.hir_id, |expr| {
// allow indexing into a vec and some set of allowed method calls that exist on slices, too
if let Some(parent) = get_parent_expr(cx, expr)
&& (adjusts_to_slice(cx, expr)
|| matches!(parent.kind, ExprKind::Index(..))
|| is_allowed_vec_method(cx, parent))
{
ControlFlow::Continue(())
let span = expr.span.ctxt().outer_expn_data().call_site;
if only_slice_uses {
self.check_vec_macro(cx, &vec_args, span, expr.hir_id, SuggestedType::Array);
} else {
ControlFlow::Break(())
self.span_to_lint_map.insert(span, None);
}
}
// if the local pattern has a specified type, do not lint.
else if let Some(_) = higher::VecArgs::hir(cx, expr)
&& let Node::Local(local) = cx.tcx.hir().get_parent(expr.hir_id)
&& local.ty.is_some()
{
let span = expr.span.ctxt().outer_expn_data().call_site;
self.span_to_lint_map.insert(span, None);
}
// search for `for _ in vec![...]`
else if let Some(parent) = get_parent_expr(cx, expr)
&& parent.span.is_desugaring(DesugaringKind::ForLoop)
&& self.msrv.meets(msrvs::ARRAY_INTO_ITERATOR)
{
// report the error around the `vec!` not inside `<std macros>:`
let span = expr.span.ctxt().outer_expn_data().call_site;
self.check_vec_macro(cx, &vec_args, span, expr.hir_id, SuggestedType::Array);
}
// search for `&vec![_]` or `vec![_]` expressions where the adjusted type is `&[_]`
else {
let (suggest_slice, span) = if let ExprKind::AddrOf(BorrowKind::Ref, mutability, _) = expr.kind {
// `expr` is `&vec![_]`, so suggest `&[_]` (or `&mut[_]` resp.)
(SuggestedType::SliceRef(mutability), expr.span)
} else {
// `expr` is the `vec![_]` expansion, so suggest `[_]`
// and also use the span of the actual `vec![_]` expression
(SuggestedType::Array, expr.span.ctxt().outer_expn_data().call_site)
};

if adjusts_to_slice(cx, expr) {
self.check_vec_macro(cx, &vec_args, span, expr.hir_id, suggest_slice);
} else {
self.span_to_lint_map.insert(span, None);
}
})
.is_continue();

if only_slice_uses {
self.check_vec_macro(
cx,
&vec_args,
expr.span.ctxt().outer_expn_data().call_site,
SuggestedType::Array,
);
}
}
}

// search for `for _ in vec![…]`
if let Some(higher::ForLoop { arg, .. }) = higher::ForLoop::hir(expr)
&& let Some(vec_args) = higher::VecArgs::hir(cx, arg)
&& self.msrv.meets(msrvs::ARRAY_INTO_ITERATOR)
{
// report the error around the `vec!` not inside `<std macros>:`
let span = arg.span.ctxt().outer_expn_data().call_site;
self.check_vec_macro(cx, &vec_args, span, SuggestedType::Array);
fn check_crate_post(&mut self, cx: &LateContext<'tcx>) {
for (span, lint_opt) in &self.span_to_lint_map {
if let Some((hir_id, suggest_slice, snippet, applicability)) = lint_opt {
Copy link
Member

Choose a reason for hiding this comment

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

This if statement means that in cases like that self.span_to_lint_map.insert(span, None); in line 129, it will not lint. In that case, why insert it in the first case?

If not linting is intended, both that line and the whole lintable_exprs field can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason to insert None when we don't want to lint is to mark the specific span as non-lintable. There are cases where the same span expands to multiple expressions (when a macro expands the vec!) and we want to lint only when all of the expanded expressions are ok for linting.

The reason for lintable_exprs is because the first two checks of the lint will check cases where we do want to emit the lint even when the vec![...] expression doesn't adjust to a slice. I chose to keep track of those expressions separately, although I could have also added a parameter to check_vec_macro indicating whether we want to lint immediately, or wait until the check_crate_post stage to emit lints.

Copy link
Member

Choose a reason for hiding this comment

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

There's no need to gather all lintable expressions in a single collection and then do a crate pass, you can just lint in place.

The infrastructure needed to collect all lintable expressions costs performance and memory, some things that we're trying to improve right now. If you're trying to improve code cleanliness, you can just refactor the span_lint... function into it's own function that takes less arguments. Like a report function or emit. It's done in other lints to avoid typing the same description / notes multiple times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. I'll look into refactoring this weekend, since I am busy with school this week.

let help_msg = format!(
"you can use {} directly",
match suggest_slice {
SuggestedType::SliceRef(_) => "a slice",
SuggestedType::Array => "an array",
}
);
span_lint_hir_and_then(cx, USELESS_VEC, *hir_id, *span, "useless use of `vec!`", |diag| {
diag.span_suggestion(*span, help_msg, snippet, *applicability);
});
}
}
}

extract_msrv_attr!(LateContext);
}

#[derive(Copy, Clone)]
enum SuggestedType {
pub(crate) enum SuggestedType {
/// Suggest using a slice `&[..]` / `&mut [..]`
SliceRef(Mutability),
/// Suggest using an array: `[..]`
Expand All @@ -147,6 +173,7 @@ impl UselessVec {
cx: &LateContext<'tcx>,
vec_args: &higher::VecArgs<'tcx>,
span: Span,
hir_id: HirId,
suggest_slice: SuggestedType,
) {
if span.from_expansion() {
Expand Down Expand Up @@ -204,21 +231,9 @@ impl UselessVec {
},
};

span_lint_and_sugg(
cx,
USELESS_VEC,
span,
"useless use of `vec!`",
&format!(
"you can use {} directly",
match suggest_slice {
SuggestedType::SliceRef(_) => "a slice",
SuggestedType::Array => "an array",
}
),
snippet,
applicability,
);
self.span_to_lint_map
.entry(span)
.or_insert(Some((hir_id, suggest_slice, snippet, applicability)));
}
}

Expand Down
34 changes: 34 additions & 0 deletions tests/ui/vec.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,3 +176,37 @@ fn below() {
let _: String = a;
}
}

fn func_needing_vec(_bar: usize, _baz: Vec<usize>) {}
fn func_not_needing_vec(_bar: usize, _baz: usize) {}

fn issue11861() {
macro_rules! this_macro_needs_vec {
($x:expr) => {{
func_needing_vec($x.iter().sum(), $x);
for _ in $x {}
}};
}
macro_rules! this_macro_doesnt_need_vec {
($x:expr) => {{ func_not_needing_vec($x.iter().sum(), $x.iter().sum()) }};
}

// Do not lint the next line
this_macro_needs_vec!(vec![1]);
this_macro_doesnt_need_vec!([1]); //~ ERROR: useless use of `vec!`

macro_rules! m {
($x:expr) => {
fn f2() {
let _x: Vec<i32> = $x;
}
fn f() {
let _x = $x;
$x.starts_with(&[]);
}
};
}

// should not lint
m!(vec![1]);
}
34 changes: 34 additions & 0 deletions tests/ui/vec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,3 +176,37 @@ fn below() {
let _: String = a;
}
}

fn func_needing_vec(_bar: usize, _baz: Vec<usize>) {}
fn func_not_needing_vec(_bar: usize, _baz: usize) {}

fn issue11861() {
macro_rules! this_macro_needs_vec {
($x:expr) => {{
func_needing_vec($x.iter().sum(), $x);
for _ in $x {}
}};
}
macro_rules! this_macro_doesnt_need_vec {
($x:expr) => {{ func_not_needing_vec($x.iter().sum(), $x.iter().sum()) }};
}

// Do not lint the next line
this_macro_needs_vec!(vec![1]);
this_macro_doesnt_need_vec!(vec![1]); //~ ERROR: useless use of `vec!`

macro_rules! m {
($x:expr) => {
fn f2() {
let _x: Vec<i32> = $x;
}
fn f() {
let _x = $x;
$x.starts_with(&[]);
}
};
}

// should not lint
m!(vec![1]);
}
8 changes: 7 additions & 1 deletion tests/ui/vec.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,5 +115,11 @@ error: useless use of `vec!`
LL | for a in vec![String::new(), String::new()] {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can use an array directly: `[String::new(), String::new()]`

error: aborting due to 19 previous errors
error: useless use of `vec!`
--> $DIR/vec.rs:196:33
|
LL | this_macro_doesnt_need_vec!(vec![1]);
| ^^^^^^^ help: you can use an array directly: `[1]`

error: aborting due to 20 previous errors