Skip to content

Commit 10d4537

Browse files
y21y21
committed
rework useless_vec lint
1 parent 4c610e3 commit 10d4537

File tree

1 file changed

+144
-101
lines changed

1 file changed

+144
-101
lines changed

‎clippy_lints/src/vec.rs‎

Lines changed: 144 additions & 101 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
11
use std::collections::BTreeMap;
2+
use std::collections::btree_map::Entry;
3+
use std::mem;
24
use std::ops::ControlFlow;
35

46
use clippy_config::Conf;
@@ -17,23 +19,6 @@ use rustc_middle::ty::layout::LayoutOf;
1719
use rustc_session::impl_lint_pass;
1820
use rustc_span::{DesugaringKind, Span, sym};
1921

20-
pub struct UselessVec {
21-
too_large_for_stack: u64,
22-
msrv: Msrv,
23-
span_to_lint_map: BTreeMap<Span, Option<(HirId, SuggestedType, String, Applicability)>>,
24-
allow_in_test: bool,
25-
}
26-
impl UselessVec {
27-
pub fn new(conf: &'static Conf) -> Self {
28-
Self {
29-
too_large_for_stack: conf.too_large_for_stack,
30-
msrv: conf.msrv,
31-
span_to_lint_map: BTreeMap::new(),
32-
allow_in_test: conf.allow_useless_vec_in_tests,
33-
}
34-
}
35-
}
36-
3722
declare_clippy_lint! {
3823
/// ### What it does
3924
/// Checks for usage of `vec![..]` when using `[..]` would
@@ -62,17 +47,62 @@ declare_clippy_lint! {
6247

6348
impl_lint_pass!(UselessVec => [USELESS_VEC]);
6449

65-
impl<'tcx> LateLintPass<'tcx> for UselessVec {
66-
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
67-
let Some(vec_args) = higher::VecArgs::hir(cx, expr.peel_borrows()) else {
68-
return;
69-
};
70-
if self.allow_in_test && is_in_test(cx.tcx, expr.hir_id) {
71-
return;
50+
/// The "state" of a `vec![]` invocation, indicating whether it can or cannot be changed.
51+
#[derive(Debug)]
52+
enum VecState {
53+
Change {
54+
suggest_ty: SuggestedType,
55+
vec_snippet: String,
56+
expr_hir_id: HirId,
57+
},
58+
NoChange,
59+
}
60+
61+
pub struct UselessVec {
62+
too_large_for_stack: u64,
63+
msrv: Msrv,
64+
/// Maps from a `vec![]` source callsite invocation span to the "state" (i.e., whether we can emit a warning there or not).
65+
///
66+
/// The purpose of this is to buffer lints up until `check_expr_post` so that we can cancel a lint while visiting,
67+
/// because a `vec![]` invocation span can appear multiple times when it is passed as a macro argument,
68+
/// once in a context that doesn't require a `Vec<_>` and another time that does. Consider:
69+
/// ```
70+
/// macro_rules! m {
71+
/// ($v:expr) => {
72+
/// let a = $v;
73+
/// $v.push(3);
74+
/// }
75+
/// }
76+
/// m!(vec![1, 2]);
77+
/// ```
78+
/// The macro invocation expands to two `vec![1, 2]` invocations. If we eagerly suggest changing it to an array,
79+
/// we get a false positive warning on the `$v.push(3)` which really requires `$v` to be a vector.
80+
span_to_state: BTreeMap<Span, VecState>,
81+
allow_in_test: bool,
82+
}
83+
84+
impl UselessVec {
85+
pub fn new(conf: &'static Conf) -> Self {
86+
Self {
87+
too_large_for_stack: conf.too_large_for_stack,
88+
msrv: conf.msrv,
89+
span_to_state: BTreeMap::new(),
90+
allow_in_test: conf.allow_useless_vec_in_tests,
7291
}
73-
// the parent callsite of this `vec!` expression, or span to the borrowed one such as `&vec!`
74-
let callsite = expr.span.parent_callsite().unwrap_or(expr.span);
92+
}
93+
}
7594

95+
enum VecToArray {
96+
/// Expression does not need to be a `Vec<_>` and its type can be changed to an array (or slice).
97+
Possible,
98+
/// Expression must be a `Vec<_>`. Type cannot change.
99+
Impossible,
100+
}
101+
102+
impl UselessVec {
103+
/// Checks if the surrounding environment requires this expression to actually be of type
104+
/// `Vec<_>`, or if it can be changed to `&[]`/`[]` without causing type errors.
105+
fn expr_usage_requires_vec<'tcx>(&mut self, cx: &LateContext<'_>, expr: &'tcx Expr<'_>) -> VecToArray {
76106
match cx.tcx.parent_hir_node(expr.hir_id) {
77107
// search for `let foo = vec![_]` expressions where all uses of `foo`
78108
// adjust to slices or call a method that exist on slices (e.g. len)
@@ -100,111 +130,124 @@ impl<'tcx> LateLintPass<'tcx> for UselessVec {
100130
.is_continue();
101131

102132
if only_slice_uses {
103-
self.check_vec_macro(cx, &vec_args, callsite, expr.hir_id, SuggestedType::Array);
133+
VecToArray::Possible
104134
} else {
105-
self.span_to_lint_map.insert(callsite, None);
135+
VecToArray::Impossible
106136
}
107137
},
108138
// if the local pattern has a specified type, do not lint.
109139
Node::LetStmt(LetStmt { ty: Some(_), .. }) if higher::VecArgs::hir(cx, expr).is_some() => {
110-
self.span_to_lint_map.insert(callsite, None);
140+
// FIXME: we can still lint here, it just needs to also change the type annotation
141+
VecToArray::Impossible
111142
},
112143
// search for `for _ in vec![...]`
113144
Node::Expr(Expr { span, .. })
114145
if span.is_desugaring(DesugaringKind::ForLoop) && self.msrv.meets(cx, msrvs::ARRAY_INTO_ITERATOR) =>
115146
{
116-
let suggest_slice = suggest_type(expr);
117-
self.check_vec_macro(cx, &vec_args, callsite, expr.hir_id, suggest_slice);
147+
VecToArray::Possible
118148
},
119149
// search for `&vec![_]` or `vec![_]` expressions where the adjusted type is `&[_]`
120150
_ => {
121-
let suggest_slice = suggest_type(expr);
122-
123151
if adjusts_to_slice(cx, expr) {
124-
self.check_vec_macro(cx, &vec_args, callsite, expr.hir_id, suggest_slice);
152+
VecToArray::Possible
125153
} else {
126-
self.span_to_lint_map.insert(callsite, None);
154+
VecToArray::Impossible
127155
}
128156
},
129157
}
130158
}
159+
}
160+
161+
impl<'tcx> LateLintPass<'tcx> for UselessVec {
162+
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
163+
if let Some(vec_args) = higher::VecArgs::hir(cx, expr.peel_borrows())
164+
// The `vec![]` or `&vec![]` invocation span.
165+
&& let vec_span = expr.span.parent_callsite().unwrap_or(expr.span)
166+
&& !vec_span.from_expansion()
167+
{
168+
if self.allow_in_test && is_in_test(cx.tcx, expr.hir_id) {
169+
return;
170+
}
171+
172+
match self.expr_usage_requires_vec(cx, expr) {
173+
VecToArray::Possible => {
174+
let suggest_ty = suggest_type(expr);
175+
176+
// Size and `Copy` checks don't depend on the enclosing usage of the expression
177+
// and don't need to be inserted into the state map.
178+
let vec_snippet = match vec_args {
179+
higher::VecArgs::Repeat(expr, len) => {
180+
if is_copy(cx, cx.typeck_results().expr_ty(expr))
181+
&& let Some(Constant::Int(length)) = ConstEvalCtxt::new(cx).eval(len)
182+
&& let Ok(length) = u64::try_from(length)
183+
&& size_of(cx, expr)
184+
.checked_mul(length)
185+
.is_some_and(|size| size <= self.too_large_for_stack)
186+
{
187+
suggest_ty.snippet(cx, Some(expr.span), Some(len.span))
188+
} else {
189+
return;
190+
}
191+
},
192+
higher::VecArgs::Vec(args) => {
193+
if let Ok(length) = u64::try_from(args.len())
194+
&& size_of(cx, expr)
195+
.checked_mul(length)
196+
.is_some_and(|size| size <= self.too_large_for_stack)
197+
{
198+
suggest_ty.snippet(
199+
cx,
200+
args.first().zip(args.last()).map(|(first, last)| {
201+
first.span.source_callsite().to(last.span.source_callsite())
202+
}),
203+
None,
204+
)
205+
} else {
206+
return;
207+
}
208+
},
209+
};
210+
211+
if let Entry::Vacant(entry) = self.span_to_state.entry(vec_span) {
212+
entry.insert(VecState::Change {
213+
suggest_ty,
214+
vec_snippet,
215+
expr_hir_id: expr.hir_id,
216+
});
217+
}
218+
},
219+
VecToArray::Impossible => {
220+
self.span_to_state.insert(vec_span, VecState::NoChange);
221+
},
222+
}
223+
}
224+
}
131225

132226
fn check_crate_post(&mut self, cx: &LateContext<'tcx>) {
133-
for (span, lint_opt) in &self.span_to_lint_map {
134-
if let Some((hir_id, suggest_slice, snippet, applicability)) = lint_opt {
135-
let help_msg = format!("you can use {} directly", suggest_slice.desc());
136-
span_lint_hir_and_then(cx, USELESS_VEC, *hir_id, *span, "useless use of `vec!`", |diag| {
137-
// If the `vec!` macro contains comment, better not make the suggestion machine
138-
// applicable as it would remove them.
139-
let applicability = if *applicability != Applicability::Unspecified
140-
&& let source_map = cx.tcx.sess.source_map()
141-
&& span_contains_comment(source_map, *span)
142-
{
227+
for (span, state) in mem::take(&mut self.span_to_state) {
228+
if let VecState::Change {
229+
suggest_ty,
230+
vec_snippet,
231+
expr_hir_id,
232+
} = state
233+
{
234+
let help_msg = format!("you can use {} directly", suggest_ty.desc());
235+
span_lint_hir_and_then(cx, USELESS_VEC, expr_hir_id, span, "useless use of `vec!`", |diag| {
236+
// If the `vec!` macro contains comment, better not make the suggestion machine applicable as it
237+
// would remove them.
238+
let applicability = if span_contains_comment(cx.tcx.sess.source_map(), span) {
143239
Applicability::Unspecified
144240
} else {
145-
*applicability
241+
Applicability::MachineApplicable
146242
};
147-
diag.span_suggestion(*span, help_msg, snippet, applicability);
243+
diag.span_suggestion(span, help_msg, vec_snippet, applicability);
148244
});
149245
}
150246
}
151247
}
152248
}
153249

154-
impl UselessVec {
155-
fn check_vec_macro<'tcx>(
156-
&mut self,
157-
cx: &LateContext<'tcx>,
158-
vec_args: &higher::VecArgs<'tcx>,
159-
span: Span,
160-
hir_id: HirId,
161-
suggest_slice: SuggestedType,
162-
) {
163-
if span.from_expansion() {
164-
return;
165-
}
166-
167-
let snippet = match *vec_args {
168-
higher::VecArgs::Repeat(elem, len) => {
169-
if let Some(Constant::Int(len_constant)) = ConstEvalCtxt::new(cx).eval(len) {
170-
// vec![ty; N] works when ty is Clone, [ty; N] requires it to be Copy also
171-
if !is_copy(cx, cx.typeck_results().expr_ty(elem)) {
172-
return;
173-
}
174-
175-
#[expect(clippy::cast_possible_truncation)]
176-
if len_constant as u64 * size_of(cx, elem) > self.too_large_for_stack {
177-
return;
178-
}
179-
180-
suggest_slice.snippet(cx, Some(elem.span), Some(len.span))
181-
} else {
182-
return;
183-
}
184-
},
185-
higher::VecArgs::Vec(args) => {
186-
let args_span = if let Some(last) = args.iter().last() {
187-
if args.len() as u64 * size_of(cx, last) > self.too_large_for_stack {
188-
return;
189-
}
190-
Some(args[0].span.source_callsite().to(last.span.source_callsite()))
191-
} else {
192-
None
193-
};
194-
suggest_slice.snippet(cx, args_span, None)
195-
},
196-
};
197-
198-
self.span_to_lint_map.entry(span).or_insert(Some((
199-
hir_id,
200-
suggest_slice,
201-
snippet,
202-
Applicability::MachineApplicable,
203-
)));
204-
}
205-
}
206-
207-
#[derive(Copy, Clone)]
250+
#[derive(Copy, Clone, Debug)]
208251
pub(crate) enum SuggestedType {
209252
/// Suggest using a slice `&[..]` / `&mut [..]`
210253
SliceRef(Mutability),

0 commit comments

Comments
 (0)