Skip to content

Commit e6ccc88

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

File tree

1 file changed

+131
-86
lines changed

1 file changed

+131
-86
lines changed

‎clippy_lints/src/vec.rs

+131-86
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;
@@ -20,15 +22,35 @@ use rustc_span::{DesugaringKind, Span, sym};
2022
pub struct UselessVec {
2123
too_large_for_stack: u64,
2224
msrv: Msrv,
23-
span_to_lint_map: BTreeMap<Span, Option<(HirId, SuggestedType, String, Applicability)>>,
25+
/// Maps from a `vec![]` source callsite invocation span to the "state" (i.e., whether we can
26+
/// emit a warning there or not).
27+
///
28+
/// The purpose of this is to buffer lints up until `check_expr_post` so that we can cancel a
29+
/// lint while visiting, because a `vec![]` invocation span can appear multiple times when
30+
/// it is passed as a macro argument, once in a context that doesn't require a `Vec<_>` and
31+
/// another time that does. Consider:
32+
/// ```
33+
/// macro_rules! m {
34+
/// ($v:expr) => {
35+
/// let a = $v;
36+
/// $v.push(3);
37+
/// }
38+
/// }
39+
/// m!(vec![1, 2]);
40+
/// ```
41+
/// The macro invocation expands to two `vec![1, 2]` invocations. If we eagerly suggest changing
42+
/// the first `vec![1, 2]` (which is shared with the other expn) to an array which indeed would work,
43+
/// we get a false positive warning on the `$v.push(3)` which really requires `$v` to be a vector.
44+
span_to_state: BTreeMap<Span, VecState>,
2445
allow_in_test: bool,
2546
}
47+
2648
impl UselessVec {
2749
pub fn new(conf: &'static Conf) -> Self {
2850
Self {
2951
too_large_for_stack: conf.too_large_for_stack,
3052
msrv: conf.msrv,
31-
span_to_lint_map: BTreeMap::new(),
53+
span_to_state: BTreeMap::new(),
3254
allow_in_test: conf.allow_useless_vec_in_tests,
3355
}
3456
}
@@ -62,17 +84,28 @@ declare_clippy_lint! {
6284

6385
impl_lint_pass!(UselessVec => [USELESS_VEC]);
6486

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;
72-
}
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);
87+
/// The "state" of a `vec![]` invocation, indicating whether it can or cannot be changed.
88+
enum VecState {
89+
Change {
90+
suggest_ty: SuggestedType,
91+
vec_snippet: String,
92+
expr_hir_id: HirId,
93+
},
94+
NoChange,
95+
}
7596

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

102135
if only_slice_uses {
103-
self.check_vec_macro(cx, &vec_args, callsite, expr.hir_id, SuggestedType::Array);
136+
VecToArray::Possible
104137
} else {
105-
self.span_to_lint_map.insert(callsite, None);
138+
VecToArray::Impossible
106139
}
107140
},
108141
// if the local pattern has a specified type, do not lint.
109142
Node::LetStmt(LetStmt { ty: Some(_), .. }) if higher::VecArgs::hir(cx, expr).is_some() => {
110-
self.span_to_lint_map.insert(callsite, None);
143+
VecToArray::Impossible
111144
},
112145
// search for `for _ in vec![...]`
113146
Node::Expr(Expr { span, .. })
114147
if span.is_desugaring(DesugaringKind::ForLoop) && self.msrv.meets(cx, msrvs::ARRAY_INTO_ITERATOR) =>
115148
{
116-
let suggest_slice = suggest_type(expr);
117-
self.check_vec_macro(cx, &vec_args, callsite, expr.hir_id, suggest_slice);
149+
VecToArray::Possible
118150
},
119151
// search for `&vec![_]` or `vec![_]` expressions where the adjusted type is `&[_]`
120152
_ => {
121-
let suggest_slice = suggest_type(expr);
122-
123153
if adjusts_to_slice(cx, expr) {
124-
self.check_vec_macro(cx, &vec_args, callsite, expr.hir_id, suggest_slice);
154+
VecToArray::Possible
125155
} else {
126-
self.span_to_lint_map.insert(callsite, None);
156+
VecToArray::Impossible
127157
}
128158
},
129159
}
130160
}
161+
}
162+
163+
impl<'tcx> LateLintPass<'tcx> for UselessVec {
164+
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
165+
if let Some(vec_args) = higher::VecArgs::hir(cx, expr.peel_borrows())
166+
// The `vec![]` or `&vec![]` invocation span.
167+
&& let vec_span = expr.span.parent_callsite().unwrap_or(expr.span)
168+
&& !vec_span.from_expansion()
169+
{
170+
if self.allow_in_test && is_in_test(cx.tcx, expr.hir_id) {
171+
return;
172+
}
173+
174+
match self.expr_usage_requires_vec(cx, expr) {
175+
VecToArray::Possible => {
176+
let suggest_ty = suggest_type(expr);
177+
178+
// Size and `Copy` checks don't depend on the enclosing usage of the expression
179+
// and don't need to be inserted into the state map.
180+
let vec_snippet = match vec_args {
181+
higher::VecArgs::Repeat(expr, len) => {
182+
if is_copy(cx, cx.typeck_results().expr_ty(expr))
183+
&& let Some(Constant::Int(length)) = ConstEvalCtxt::new(cx).eval(len)
184+
&& let Ok(length) = u64::try_from(length)
185+
&& size_of(cx, expr)
186+
.checked_mul(length)
187+
.is_some_and(|size| size <= self.too_large_for_stack)
188+
{
189+
suggest_ty.snippet(cx, Some(expr.span), Some(len.span))
190+
} else {
191+
return;
192+
}
193+
},
194+
higher::VecArgs::Vec(args) => {
195+
if let Ok(length) = u64::try_from(args.len())
196+
&& size_of(cx, expr)
197+
.checked_mul(length)
198+
.is_some_and(|size| size <= self.too_large_for_stack)
199+
{
200+
suggest_ty.snippet(
201+
cx,
202+
args.first().zip(args.last()).map(|(first, last)| {
203+
first.span.source_callsite().to(last.span.source_callsite())
204+
}),
205+
None,
206+
)
207+
} else {
208+
return;
209+
}
210+
},
211+
};
212+
213+
if let Entry::Vacant(entry) = self.span_to_state.entry(vec_span) {
214+
entry.insert(VecState::Change {
215+
suggest_ty,
216+
vec_snippet,
217+
expr_hir_id: expr.hir_id,
218+
});
219+
}
220+
},
221+
VecToArray::Impossible => {
222+
self.span_to_state.insert(vec_span, VecState::NoChange);
223+
},
224+
}
225+
}
226+
}
131227

132228
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-
{
229+
for (span, state) in mem::take(&mut self.span_to_state) {
230+
if let VecState::Change {
231+
suggest_ty,
232+
vec_snippet,
233+
expr_hir_id,
234+
} = state
235+
{
236+
span_lint_hir_and_then(cx, USELESS_VEC, expr_hir_id, span, "useless use of `vec!`", |diag| {
237+
let help_msg = format!("you can use {} directly", suggest_ty.desc());
238+
// If the `vec!` macro contains comment, better not make the suggestion machine applicable as it
239+
// would remove them.
240+
let applicability = if span_contains_comment(cx.tcx.sess.source_map(), span) {
143241
Applicability::Unspecified
144242
} else {
145-
*applicability
243+
Applicability::MachineApplicable
146244
};
147-
diag.span_suggestion(*span, help_msg, snippet, applicability);
245+
diag.span_suggestion(span, help_msg, vec_snippet, applicability);
148246
});
149247
}
150248
}
151249
}
152250
}
153251

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-
207252
#[derive(Copy, Clone)]
208253
pub(crate) enum SuggestedType {
209254
/// Suggest using a slice `&[..]` / `&mut [..]`

0 commit comments

Comments
 (0)