Skip to content

Commit 14d2700

Browse files
bors[bot]yaahcphansch
committed
Merge #3217 #3366
3217: Fix string_lit_as_bytes lint for macros r=phansch a=yaahallo Prior to this change, string_lit_as_bytes would trigger for constructs like `include_str!("filename").as_bytes()` and would recommend fixing it by rewriting as `binclude_str!("filename")`. This change updates the lint to act as an EarlyLintPass lint. It then differentiates between string literals and macros that have bytes yielding alternatives. Closes #3205 3366: Don't expand macros in some suggestions r=oli-obk a=phansch Fixes #1148 Fixes #1628 Fixes #2455 Fixes #3023 Fixes #3333 Fixes #3360 Co-authored-by: Jane Lusby <jlusby42@gmail.com> Co-authored-by: Philipp Hansch <dev@phansch.net>
3 parents 6ce5edc + 19ac2e9 + 840e50e commit 14d2700

File tree

13 files changed

+123
-52
lines changed

13 files changed

+123
-52
lines changed

‎clippy_lints/src/identity_conversion.rs‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
1212
use crate::rustc::{declare_tool_lint, lint_array};
1313
use crate::rustc::hir::*;
1414
use crate::syntax::ast::NodeId;
15-
use crate::utils::{in_macro, match_def_path, match_trait_method, same_tys, snippet, span_lint_and_then};
15+
use crate::utils::{in_macro, match_def_path, match_trait_method, same_tys, snippet, snippet_with_macro_callsite, span_lint_and_then};
1616
use crate::utils::{opt_def_id, paths, resolve_node};
1717
use crate::rustc_errors::Applicability;
1818

@@ -72,7 +72,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IdentityConversion {
7272
let a = cx.tables.expr_ty(e);
7373
let b = cx.tables.expr_ty(&args[0]);
7474
if same_tys(cx, a, b) {
75-
let sugg = snippet(cx, args[0].span, "<expr>").into_owned();
75+
let sugg = snippet_with_macro_callsite(cx, args[0].span, "<expr>").to_string();
76+
7677
span_lint_and_then(cx, IDENTITY_CONVERSION, e.span, "identical conversion", |db| {
7778
db.span_suggestion_with_applicability(
7879
e.span,

‎clippy_lints/src/matches.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,8 @@ use crate::syntax::ast::LitKind;
1919
use crate::syntax::source_map::Span;
2020
use crate::utils::paths;
2121
use crate::utils::{expr_block, is_allowed, is_expn_of, match_qpath, match_type, multispan_sugg,
22-
remove_blocks, snippet, span_lint_and_sugg, span_lint_and_then, span_note_and_lint, walk_ptrs_ty};
22+
remove_blocks, snippet, span_lint_and_sugg, span_lint_and_then,
23+
span_note_and_lint, walk_ptrs_ty};
2324
use crate::utils::sugg::Sugg;
2425
use crate::consts::{constant, Constant};
2526
use crate::rustc_errors::Applicability;

‎clippy_lints/src/methods/mod.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ use crate::utils::sugg;
2121
use crate::utils::{
2222
get_arg_name, get_trait_def_id, implements_trait, in_macro, is_copy, is_expn_of, is_self, is_self_ty,
2323
iter_input_pats, last_path_segment, match_def_path, match_path, match_qpath, match_trait_method, match_type,
24-
match_var, method_chain_args, remove_blocks, return_ty, same_tys, single_segment_path, snippet, span_lint,
24+
match_var, method_chain_args, remove_blocks, return_ty, same_tys, single_segment_path, snippet, snippet_with_macro_callsite, span_lint,
2525
span_lint_and_sugg, span_lint_and_then, span_note_and_lint, walk_ptrs_ty, walk_ptrs_ty_depth, SpanlessEq,
2626
};
2727
use if_chain::if_chain;
@@ -1062,9 +1062,9 @@ fn lint_or_fun_call(cx: &LateContext<'_, '_>, expr: &hir::Expr, method_span: Spa
10621062
}
10631063

10641064
let sugg: Cow<'_, _> = match (fn_has_arguments, !or_has_args) {
1065-
(true, _) => format!("|_| {}", snippet(cx, arg.span, "..")).into(),
1066-
(false, false) => format!("|| {}", snippet(cx, arg.span, "..")).into(),
1067-
(false, true) => snippet(cx, fun_span, ".."),
1065+
(true, _) => format!("|_| {}", snippet_with_macro_callsite(cx, arg.span, "..")).into(),
1066+
(false, false) => format!("|| {}", snippet_with_macro_callsite(cx, arg.span, "..")).into(),
1067+
(false, true) => snippet_with_macro_callsite(cx, fun_span, ".."),
10681068
};
10691069
let span_replace_word = method_span.with_hi(span.hi());
10701070
span_lint_and_sugg(

‎clippy_lints/src/strings.rs‎

Lines changed: 38 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@
77
// option. This file may not be copied, modified, or distributed
88
// except according to those terms.
99

10-
1110
use crate::rustc::hir::*;
1211
use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
1312
use crate::rustc::{declare_tool_lint, lint_array};
@@ -92,7 +91,14 @@ impl LintPass for StringAdd {
9291

9392
impl<'a, 'tcx> LateLintPass<'a, 'tcx> for StringAdd {
9493
fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, e: &'tcx Expr) {
95-
if let ExprKind::Binary(Spanned { node: BinOpKind::Add, .. }, ref left, _) = e.node {
94+
if let ExprKind::Binary(
95+
Spanned {
96+
node: BinOpKind::Add, ..
97+
},
98+
ref left,
99+
_,
100+
) = e.node
101+
{
96102
if is_string(cx, left) {
97103
if !is_allowed(cx, STRING_ADD_ASSIGN, e.id) {
98104
let parent = get_parent_expr(cx, e);
@@ -132,13 +138,15 @@ fn is_string(cx: &LateContext<'_, '_>, e: &Expr) -> bool {
132138

133139
fn is_add(cx: &LateContext<'_, '_>, src: &Expr, target: &Expr) -> bool {
134140
match src.node {
135-
ExprKind::Binary(Spanned { node: BinOpKind::Add, .. }, ref left, _) => SpanlessEq::new(cx).eq_expr(target, left),
141+
ExprKind::Binary(
142+
Spanned {
143+
node: BinOpKind::Add, ..
144+
},
145+
ref left,
146+
_,
147+
) => SpanlessEq::new(cx).eq_expr(target, left),
136148
ExprKind::Block(ref block, _) => {
137-
block.stmts.is_empty()
138-
&& block
139-
.expr
140-
.as_ref()
141-
.map_or(false, |expr| is_add(cx, expr, target))
149+
block.stmts.is_empty() && block.expr.as_ref().map_or(false, |expr| is_add(cx, expr, target))
142150
},
143151
_ => false,
144152
}
@@ -155,14 +163,33 @@ impl LintPass for StringLitAsBytes {
155163

156164
impl<'a, 'tcx> LateLintPass<'a, 'tcx> for StringLitAsBytes {
157165
fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, e: &'tcx Expr) {
158-
use crate::syntax::ast::LitKind;
166+
use crate::syntax::ast::{LitKind, StrStyle};
159167
use crate::utils::{in_macro, snippet};
160168

161169
if let ExprKind::MethodCall(ref path, _, ref args) = e.node {
162170
if path.ident.name == "as_bytes" {
163171
if let ExprKind::Lit(ref lit) = args[0].node {
164-
if let LitKind::Str(ref lit_content, _) = lit.node {
165-
if lit_content.as_str().chars().all(|c| c.is_ascii()) && !in_macro(args[0].span) {
172+
if let LitKind::Str(ref lit_content, style) = lit.node {
173+
let callsite = snippet(cx, args[0].span.source_callsite(), r#""foo""#);
174+
let expanded = if let StrStyle::Raw(n) = style {
175+
let term = (0..n).map(|_| '#').collect::<String>();
176+
format!("r{0}\"{1}\"{0}", term, lit_content.as_str())
177+
} else {
178+
format!("\"{}\"", lit_content.as_str())
179+
};
180+
if callsite.starts_with("include_str!") {
181+
span_lint_and_sugg(
182+
cx,
183+
STRING_LIT_AS_BYTES,
184+
e.span,
185+
"calling `as_bytes()` on `include_str!(..)`",
186+
"consider using `include_bytes!(..)` instead",
187+
snippet(cx, args[0].span, r#""foo""#).replacen("include_str", "include_bytes", 1),
188+
);
189+
} else if callsite == expanded
190+
&& lit_content.as_str().chars().all(|c| c.is_ascii())
191+
&& !in_macro(args[0].span)
192+
{
166193
span_lint_and_sugg(
167194
cx,
168195
STRING_LIT_AS_BYTES,

‎clippy_lints/src/utils/mod.rs‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -362,6 +362,12 @@ pub fn snippet<'a, 'b, T: LintContext<'b>>(cx: &T, span: Span, default: &'a str)
362362
snippet_opt(cx, span).map_or_else(|| Cow::Borrowed(default), From::from)
363363
}
364364

365+
/// Same as `snippet`, but should only be used when it's clear that the input span is
366+
/// not a macro argument.
367+
pub fn snippet_with_macro_callsite<'a, 'b, T: LintContext<'b>>(cx: &T, span: Span, default: &'a str) -> Cow<'a, str> {
368+
snippet(cx, span.source_callsite(), default)
369+
}
370+
365371
/// Convert a span to a code snippet. Returns `None` if not available.
366372
pub fn snippet_opt<'a, T: LintContext<'a>>(cx: &T, span: Span) -> Option<String> {
367373
cx.sess().source_map().span_to_snippet(span).ok()
@@ -400,7 +406,10 @@ pub fn expr_block<'a, 'b, T: LintContext<'b>>(
400406
) -> Cow<'a, str> {
401407
let code = snippet_block(cx, expr.span, default);
402408
let string = option.unwrap_or_default();
403-
if let ExprKind::Block(_, _) = expr.node {
409+
if in_macro(expr.span) {
410+
Cow::Owned(format!("{{ {} }}", snippet_with_macro_callsite(cx, expr.span, default)))
411+
}
412+
else if let ExprKind::Block(_, _) = expr.node {
404413
Cow::Owned(format!("{}{}", code, string))
405414
} else if string.is_empty() {
406415
Cow::Owned(format!("{{ {} }}", code))

‎tests/ui/identity_conversion.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,4 +53,5 @@ fn main() {
5353
let _ = String::from(format!("A: {:04}", 123));
5454
let _ = "".lines().into_iter();
5555
let _ = vec![1, 2, 3].into_iter().into_iter();
56+
let _: String = format!("Hello {}", "world").into();
5657
}

‎tests/ui/identity_conversion.stderr‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,5 +58,11 @@ error: identical conversion
5858
55 | let _ = vec![1, 2, 3].into_iter().into_iter();
5959
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `.into_iter()`: `vec![1, 2, 3].into_iter()`
6060

61-
error: aborting due to 9 previous errors
61+
error: identical conversion
62+
--> $DIR/identity_conversion.rs:56:21
63+
|
64+
56 | let _: String = format!("Hello {}", "world").into();
65+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `.into()`: `format!("Hello {}", "world")`
66+
67+
error: aborting due to 10 previous errors
6268

‎tests/ui/matches.stderr‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ error: you seem to be trying to use match for destructuring a single pattern. Co
3333
51 | | &(v, 1) => println!("{}", v),
3434
52 | | _ => println!("none"),
3535
53 | | }
36-
| |_____^ help: try this: `if let &(v, 1) = tup { $ crate :: io :: _print ( format_args_nl ! ( $ ( $ arg ) * ) ) ; } else { $ crate :: io :: _print ( format_args_nl ! ( $ ( $ arg ) * ) ) ; }`
36+
| |_____^ help: try this: `if let &(v, 1) = tup { println!("{}", v) } else { println!("none") }`
3737

3838
error: you don't need to add `&` to all patterns
3939
--> $DIR/matches.rs:50:5

‎tests/ui/methods.stderr‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -297,7 +297,7 @@ error: use of `unwrap_or` followed by a function call
297297
--> $DIR/methods.rs:339:14
298298
|
299299
339 | with_vec.unwrap_or(vec![]);
300-
| ^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| < [ _ ] > :: into_vec ( box [ $ ( $ x ) , * ] ))`
300+
| ^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| vec![])`
301301

302302
error: use of `unwrap_or` followed by a function call
303303
--> $DIR/methods.rs:344:21

‎tests/ui/single_match.rs‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,15 @@ fn single_match(){
2323
_ => ()
2424
};
2525

26+
let x = Some(1u8);
27+
match x {
28+
// Note the missing block braces.
29+
// We suggest `if let Some(y) = x { .. }` because the macro
30+
// is expanded before we can do anything.
31+
Some(y) => println!("{:?}", y),
32+
_ => ()
33+
}
34+
2635
let z = (1u8,1u8);
2736
match z {
2837
(2...3, 7...9) => dummy(),

0 commit comments

Comments
 (0)