Skip to content
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

Only emit useless_vec suggestion if the macro does not contain code comments #13911

Merged
merged 2 commits into from
Jan 4, 2025
Merged

Only emit useless_vec suggestion if the macro does not contain code comments #13911

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1b85ae3
Only emit `useless_vec` suggestion if the macro does not contain code…
GuillaumeGomez Dec 30, 2024
b76e042
Add regression test for `useless_vec` with code comments
GuillaumeGomez Dec 30, 2024
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
16 changes: 13 additions & 3 deletions clippy_lints/src/vec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use clippy_utils::msrvs::{self, Msrv};
use clippy_utils::source::SpanRangeExt;
use clippy_utils::ty::is_copy;
use clippy_utils::visitors::for_each_local_use_after_expr;
use clippy_utils::{get_parent_expr, higher, is_in_test, is_trait_method};
use clippy_utils::{get_parent_expr, higher, is_in_test, is_trait_method, span_contains_comment};
use rustc_errors::Applicability;
use rustc_hir::{BorrowKind, Expr, ExprKind, HirId, LetStmt, Mutability, Node, Pat, PatKind};
use rustc_lint::{LateContext, LateLintPass};
Expand Down Expand Up @@ -132,9 +132,19 @@ impl<'tcx> LateLintPass<'tcx> for UselessVec {
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 {
let help_msg = format!("you can use {} directly", suggest_slice.desc(),);
let help_msg = format!("you can use {} directly", suggest_slice.desc());
span_lint_hir_and_then(cx, USELESS_VEC, *hir_id, *span, "useless use of `vec!`", |diag| {
diag.span_suggestion(*span, help_msg, snippet, *applicability);
// If the `vec!` macro contains comment, better not make the suggestion machine
// applicable as it would remove them.
let applicability = if *applicability != Applicability::Unspecified
&& let source_map = cx.tcx.sess.source_map()
&& span_contains_comment(source_map, *span)
{
Applicability::Unspecified
} else {
*applicability
};
diag.span_suggestion(*span, help_msg, snippet, applicability);
});
}
}
Expand Down
15 changes: 15 additions & 0 deletions tests/ui/useless_vec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
//@no-rustfix: no suggestions

#![warn(clippy::useless_vec)]

// Regression test for <https://github.com/rust-lang/rust-clippy/issues/13692>.
fn foo() {
// There should be no suggestion in this case.
let _some_variable = vec![
//~^ useless_vec
1, 2, // i'm here to stay
3, 4, // but this one going away ;-;
]; // that is life anyways
}

fn main() {}
21 changes: 21 additions & 0 deletions tests/ui/useless_vec.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
error: useless use of `vec!`
--> tests/ui/useless_vec.rs:8:26
|
LL | let _some_variable = vec![
| __________________________^
LL | |
LL | | 1, 2, // i'm here to stay
LL | | 3, 4, // but this one going away ;-;
LL | | ]; // that is life anyways
Copy link
Member

Choose a reason for hiding this comment

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

IMO the help message/suggestion was a fairly important part of the lint because now it's not 100% clear what the lint wants the user to do from the diagnostic alone (use an array instead).

What about just changing the applicability to something else other than MachineApplicable so that --fix won't autofix it but is still shown to the user as a hint to what the fix roughly looks like and they can apply it manually by preserving the comment?

(I guess ideally the suggestion could just include the last comment directly and keep it as MachineApplicable since all other comments except for the one after the last element are already correctly handled, but I can only think of hacky ways to do it like extending the snippet's span to the full vec![] invocation minus 1 (just behind the ]), and am not sure if that will always work so 🤷)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah makes sense. Gonna change the applicability instead.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @y21 that it's better to only change the applicability, like you did now. (Thank you for the good suggestion!)

I would be in favor of expanding the snippet span instead. I don't really see when this should fail. Modulo macro expansion, which will probably make this a lot harder ^^.

I checked the code and don't see a super obvious way to hack it in. I don't think it's worth it to spent too much time on this, since changing the applicability already prevents automatic deletion of the comments.

So I'd say we can merge this once the comment (from my other review comment) has been updated :D

| |_____^
|
= note: `-D clippy::useless-vec` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::useless_vec)]`
help: you can use an array directly
|
LL ~ let _some_variable = [1, 2, // i'm here to stay
LL ~ 3, 4]; // that is life anyways
|

error: aborting due to 1 previous error

Loading