Skip to content

[useless_vec]: lint vec! invocations when a slice or an array would do #10901

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

Merged
merged 3 commits into from
Jun 8, 2023
Merged
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
111 changes: 97 additions & 14 deletions clippy_lints/src/vec.rs
Original file line number Diff line number Diff line change
@@ -1,16 +1,21 @@
use std::ops::ControlFlow;

use clippy_utils::consts::{constant, Constant};
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::higher;
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 if_chain::if_chain;
use rustc_ast::BindingAnnotation;
use rustc_errors::Applicability;
use rustc_hir::{BorrowKind, Expr, ExprKind, Mutability};
use rustc_hir::{BorrowKind, Expr, ExprKind, Mutability, Node, PatKind};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::ty::layout::LayoutOf;
use rustc_middle::ty::{self, Ty};
use rustc_session::{declare_tool_lint, impl_lint_pass};
use rustc_span::source_map::Span;
use rustc_span::sym;

#[expect(clippy::module_name_repetitions)]
#[derive(Copy, Clone)]
Expand All @@ -20,7 +25,7 @@ pub struct UselessVec {

declare_clippy_lint! {
/// ### What it does
/// Checks for usage of `&vec![..]` when using `&[..]` would
/// Checks for usage of `vec![..]` when using `[..]` would
/// be possible.
///
/// ### Why is this bad?
Expand All @@ -46,16 +51,70 @@ declare_clippy_lint! {

impl_lint_pass!(UselessVec => [USELESS_VEC]);

fn adjusts_to_slice(cx: &LateContext<'_>, e: &Expr<'_>) -> bool {
if let ty::Ref(_, ty, _) = cx.typeck_results().expr_ty_adjusted(e).kind() {
ty.is_slice()
} else {
false
}
}

/// Checks if the given expression is a method call to a `Vec` method
/// that also exists on slices. If this returns true, it means that
/// this expression does not actually require a `Vec` and could just work with an array.
fn is_allowed_vec_method(cx: &LateContext<'_>, e: &Expr<'_>) -> bool {
const ALLOWED_METHOD_NAMES: &[&str] = &["len", "as_ptr", "is_empty"];
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit curious why len and is_empty are allowed.

Copy link
Member Author

@y21 y21 Jun 7, 2023

Choose a reason for hiding this comment

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

The method names that are checked for here exist on both slices and Vec<T>.
So, given a variable ve of type Vec<i32>, a call expression like ve.is_empty() does not actually call slice::is_empty, but Vec::is_empty, which also means that adjusts_to_slice(ve) would return false.

If we didn't have len and is_empty here, the lint would assume that those are Vec<T> methods that really do require a Vec<T> (like push and pop, for example).
For len and is_empty specifically, that isn't the case. These two methods also exist on slices, and we still want to lint for cases like these:

let ve = vec![1, 2];
dbg!(ve.is_empty()); // Vec::is_empty, but this would also work fine if ve was `[1, 2]`

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I misunderstood then 😅 I thought those were exceptions. Yeah, that sounds fine then.


if let ExprKind::MethodCall(path, ..) = e.kind {
ALLOWED_METHOD_NAMES.contains(&path.ident.name.as_str())
} else {
is_trait_method(cx, e, sym::IntoIterator)
}
}

impl<'tcx> LateLintPass<'tcx> for UselessVec {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
// search for `&vec![_]` expressions where the adjusted type is `&[_]`
if_chain! {
if let ty::Ref(_, ty, _) = cx.typeck_results().expr_ty_adjusted(expr).kind();
if let ty::Slice(..) = ty.kind();
if adjusts_to_slice(cx, expr);
if let ExprKind::AddrOf(BorrowKind::Ref, mutability, addressee) = expr.kind;
if let Some(vec_args) = higher::VecArgs::hir(cx, addressee);
then {
self.check_vec_macro(cx, &vec_args, mutability, expr.span);
self.check_vec_macro(cx, &vec_args, mutability, expr.span, SuggestSlice::Yes);
}
}

// 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(BindingAnnotation(_, mutbl), 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(())
} else {
ControlFlow::Break(())
}
}).is_continue();

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

Expand All @@ -67,21 +126,36 @@ impl<'tcx> LateLintPass<'tcx> for UselessVec {
then {
// 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, Mutability::Not, span);
self.check_vec_macro(cx, &vec_args, Mutability::Not, span, SuggestSlice::Yes);
}
}
}
}

#[derive(Copy, Clone)]
enum SuggestSlice {
/// Suggest using a slice `&[..]` / `&mut [..]`
Yes,
/// Suggest using an array: `[..]`
No,
}

impl UselessVec {
fn check_vec_macro<'tcx>(
self,
cx: &LateContext<'tcx>,
vec_args: &higher::VecArgs<'tcx>,
mutability: Mutability,
span: Span,
suggest_slice: SuggestSlice,
) {
let mut applicability = Applicability::MachineApplicable;

let (borrow_prefix_mut, borrow_prefix) = match suggest_slice {
SuggestSlice::Yes => ("&mut ", "&"),
SuggestSlice::No => ("", ""),
};

let snippet = match *vec_args {
higher::VecArgs::Repeat(elem, len) => {
if let Some(Constant::Int(len_constant)) = constant(cx, cx.typeck_results(), len) {
Expand All @@ -93,14 +167,14 @@ impl UselessVec {
match mutability {
Mutability::Mut => {
format!(
"&mut [{}; {}]",
"{borrow_prefix_mut}[{}; {}]",
snippet_with_applicability(cx, elem.span, "elem", &mut applicability),
snippet_with_applicability(cx, len.span, "len", &mut applicability)
)
},
Mutability::Not => {
format!(
"&[{}; {}]",
"{borrow_prefix}[{}; {}]",
snippet_with_applicability(cx, elem.span, "elem", &mut applicability),
snippet_with_applicability(cx, len.span, "len", &mut applicability)
)
Expand All @@ -120,18 +194,21 @@ impl UselessVec {
match mutability {
Mutability::Mut => {
format!(
"&mut [{}]",
"{borrow_prefix_mut}[{}]",
snippet_with_applicability(cx, span, "..", &mut applicability)
)
},
Mutability::Not => {
format!("&[{}]", snippet_with_applicability(cx, span, "..", &mut applicability))
format!(
"{borrow_prefix}[{}]",
snippet_with_applicability(cx, span, "..", &mut applicability)
)
},
}
} else {
match mutability {
Mutability::Mut => "&mut []".into(),
Mutability::Not => "&[]".into(),
Mutability::Mut => format!("{borrow_prefix_mut}[]"),
Mutability::Not => format!("{borrow_prefix}[]"),
}
}
},
Expand All @@ -142,7 +219,13 @@ impl UselessVec {
USELESS_VEC,
span,
"useless use of `vec!`",
"you can use a slice directly",
&format!(
"you can use {} directly",
match suggest_slice {
SuggestSlice::Yes => "a slice",
SuggestSlice::No => "an array",
}
),
snippet,
applicability,
);
Expand Down
7 changes: 6 additions & 1 deletion tests/ui-toml/suppress_lint_in_const/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,12 @@
// We also check the out_of_bounds_indexing lint here, because it lints similar things and
// we want to avoid false positives.
#![warn(clippy::out_of_bounds_indexing)]
#![allow(unconditional_panic, clippy::no_effect, clippy::unnecessary_operation)]
#![allow(
unconditional_panic,
clippy::no_effect,
clippy::unnecessary_operation,
clippy::useless_vec
)]

const ARR: [i32; 2] = [1, 2];
const REF: &i32 = &ARR[idx()]; // Ok, should not produce stderr, since `suppress-restriction-lint-in-const` is set true.
Expand Down
18 changes: 9 additions & 9 deletions tests/ui-toml/suppress_lint_in_const/test.stderr
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
error[E0080]: evaluation of `main::{constant#3}` failed
--> $DIR/test.rs:31:14
--> $DIR/test.rs:36:14
|
LL | const { &ARR[idx4()] }; // Ok, should not produce stderr, since `suppress-restriction-lint-in-const` is set true.
| ^^^^^^^^^^^ index out of bounds: the length is 2 but the index is 4

note: erroneous constant used
--> $DIR/test.rs:31:5
--> $DIR/test.rs:36:5
|
LL | const { &ARR[idx4()] }; // Ok, should not produce stderr, since `suppress-restriction-lint-in-const` is set true.
| ^^^^^^^^^^^^^^^^^^^^^^

error: indexing may panic
--> $DIR/test.rs:22:5
--> $DIR/test.rs:27:5
|
LL | x[index];
| ^^^^^^^^
Expand All @@ -20,47 +20,47 @@ LL | x[index];
= note: `-D clippy::indexing-slicing` implied by `-D warnings`

error: indexing may panic
--> $DIR/test.rs:38:5
--> $DIR/test.rs:43:5
|
LL | v[0];
| ^^^^
|
= help: consider using `.get(n)` or `.get_mut(n)` instead

error: indexing may panic
--> $DIR/test.rs:39:5
--> $DIR/test.rs:44:5
|
LL | v[10];
| ^^^^^
|
= help: consider using `.get(n)` or `.get_mut(n)` instead

error: indexing may panic
--> $DIR/test.rs:40:5
--> $DIR/test.rs:45:5
|
LL | v[1 << 3];
| ^^^^^^^^^
|
= help: consider using `.get(n)` or `.get_mut(n)` instead

error: indexing may panic
--> $DIR/test.rs:46:5
--> $DIR/test.rs:51:5
|
LL | v[N];
| ^^^^
|
= help: consider using `.get(n)` or `.get_mut(n)` instead

error: indexing may panic
--> $DIR/test.rs:47:5
--> $DIR/test.rs:52:5
|
LL | v[M];
| ^^^^
|
= help: consider using `.get(n)` or `.get_mut(n)` instead

error[E0080]: evaluation of constant value failed
--> $DIR/test.rs:10:24
--> $DIR/test.rs:15:24
|
LL | const REF_ERR: &i32 = &ARR[idx4()]; // Ok, let rustc handle const contexts.
| ^^^^^^^^^^^ index out of bounds: the length is 2 but the index is 4
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
//@compile-flags: --crate-name conf_disallowed_methods

#![warn(clippy::disallowed_methods)]
#![allow(clippy::useless_vec)]

extern crate futures;
extern crate regex;
Expand Down
Loading