Skip to content

Commit a8d417d

Browse files
y21y21
committed
rework useless_vec lint
1 parent 07cc166 commit a8d417d

File tree

1 file changed

+132
-86
lines changed

1 file changed

+132
-86
lines changed

‎clippy_lints/src/vec.rs‎

Lines changed: 132 additions & 86 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;
@@ -20,15 +22,36 @@ use rustc_span::{DesugaringKind, Span};
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
43+
/// work, we get a false positive warning on the `$v.push(3)` which really requires `$v` to
44+
/// be a vector.
45+
span_to_state: BTreeMap<Span, VecState>,
2446
allow_in_test: bool,
2547
}
48+
2649
impl UselessVec {
2750
pub fn new(conf: &'static Conf) -> Self {
2851
Self {
2952
too_large_for_stack: conf.too_large_for_stack,
3053
msrv: conf.msrv,
31-
span_to_lint_map: BTreeMap::new(),
54+
span_to_state: BTreeMap::new(),
3255
allow_in_test: conf.allow_useless_vec_in_tests,
3356
}
3457
}
@@ -62,17 +85,28 @@ declare_clippy_lint! {
6285

6386
impl_lint_pass!(UselessVec => [USELESS_VEC]);
6487

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

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

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

132229
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-
{
230+
for (span, state) in mem::take(&mut self.span_to_state) {
231+
if let VecState::Change {
232+
suggest_ty,
233+
vec_snippet,
234+
expr_hir_id,
235+
} = state
236+
{
237+
span_lint_hir_and_then(cx, USELESS_VEC, expr_hir_id, span, "useless use of `vec!`", |diag| {
238+
let help_msg = format!("you can use {} directly", suggest_ty.desc());
239+
// If the `vec!` macro contains comment, better not make the suggestion machine applicable as it
240+
// would remove them.
241+
let applicability = if span_contains_comment(cx.tcx.sess.source_map(), span) {
143242
Applicability::Unspecified
144243
} else {
145-
*applicability
244+
Applicability::MachineApplicable
146245
};
147-
diag.span_suggestion(*span, help_msg, snippet, applicability);
246+
diag.span_suggestion(span, help_msg, vec_snippet, applicability);
148247
});
149248
}
150249
}
151250
}
152251
}
153252

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

0 commit comments

Comments
 (0)