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 <[email protected]> Co-authored-by: Philipp Hansch <[email protected]>
3 parents 6ce5edc + 19ac2e9 + 840e50e commit 14d2700

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(),

tests/ui/single_match.stderr

Lines changed: 32 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -12,38 +12,50 @@ error: you seem to be trying to use match for destructuring a single pattern. Co
1212
error: you seem to be trying to use match for destructuring a single pattern. Consider using `if let`
1313
--> $DIR/single_match.rs:27:5
1414
|
15-
27 | / match z {
16-
28 | | (2...3, 7...9) => dummy(),
17-
29 | | _ => {}
18-
30 | | };
15+
27 | / match x {
16+
28 | | // Note the missing block braces.
17+
29 | | // We suggest `if let Some(y) = x { .. }` because the macro
18+
30 | | // is expanded before we can do anything.
19+
31 | | Some(y) => println!("{:?}", y),
20+
32 | | _ => ()
21+
33 | | }
22+
| |_____^ help: try this: `if let Some(y) = x { println!("{:?}", y) }`
23+
24+
error: you seem to be trying to use match for destructuring a single pattern. Consider using `if let`
25+
--> $DIR/single_match.rs:36:5
26+
|
27+
36 | / match z {
28+
37 | | (2...3, 7...9) => dummy(),
29+
38 | | _ => {}
30+
39 | | };
1931
| |_____^ help: try this: `if let (2...3, 7...9) = z { dummy() }`
2032

2133
error: you seem to be trying to use match for destructuring a single pattern. Consider using `if let`
22-
--> $DIR/single_match.rs:53:5
34+
--> $DIR/single_match.rs:62:5
2335
|
24-
53 | / match x {
25-
54 | | Some(y) => dummy(),
26-
55 | | None => ()
27-
56 | | };
36+
62 | / match x {
37+
63 | | Some(y) => dummy(),
38+
64 | | None => ()
39+
65 | | };
2840
| |_____^ help: try this: `if let Some(y) = x { dummy() }`
2941

3042
error: you seem to be trying to use match for destructuring a single pattern. Consider using `if let`
31-
--> $DIR/single_match.rs:58:5
43+
--> $DIR/single_match.rs:67:5
3244
|
33-
58 | / match y {
34-
59 | | Ok(y) => dummy(),
35-
60 | | Err(..) => ()
36-
61 | | };
45+
67 | / match y {
46+
68 | | Ok(y) => dummy(),
47+
69 | | Err(..) => ()
48+
70 | | };
3749
| |_____^ help: try this: `if let Ok(y) = y { dummy() }`
3850

3951
error: you seem to be trying to use match for destructuring a single pattern. Consider using `if let`
40-
--> $DIR/single_match.rs:65:5
52+
--> $DIR/single_match.rs:74:5
4153
|
42-
65 | / match c {
43-
66 | | Cow::Borrowed(..) => dummy(),
44-
67 | | Cow::Owned(..) => (),
45-
68 | | };
54+
74 | / match c {
55+
75 | | Cow::Borrowed(..) => dummy(),
56+
76 | | Cow::Owned(..) => (),
57+
77 | | };
4658
| |_____^ help: try this: `if let Cow::Borrowed(..) = c { dummy() }`
4759

48-
error: aborting due to 5 previous errors
60+
error: aborting due to 6 previous errors
4961

tests/ui/strings.rs

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,10 +10,10 @@
1010

1111

1212

13-
1413
#[warn(clippy::string_add)]
1514
#[allow(clippy::string_add_assign)]
16-
fn add_only() { // ignores assignment distinction
15+
fn add_only() {
16+
// ignores assignment distinction
1717
let mut x = "".to_owned();
1818

1919
for _ in 1..3 {
@@ -59,19 +59,24 @@ fn both() {
5959
fn str_lit_as_bytes() {
6060
let bs = "hello there".as_bytes();
6161

62+
let bs = r###"raw string with three ### in it and some " ""###.as_bytes();
63+
6264
// no warning, because this cannot be written as a byte string literal:
6365
let ubs = "☃".as_bytes();
6466

6567
let strify = stringify!(foobar).as_bytes();
68+
69+
let includestr = include_str!("entry.rs").as_bytes();
6670
}
6771

72+
#[allow(clippy::assign_op_pattern)]
6873
fn main() {
6974
add_only();
7075
add_assign_only();
7176
both();
7277

7378
// the add is only caught for `String`
7479
let mut x = 1;
75-
; x = x + 1;
80+
x = x + 1;
7681
assert_eq!(2, x);
7782
}

tests/ui/strings.stderr

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -61,16 +61,16 @@ error: calling `as_bytes()` on a string literal
6161
= note: `-D clippy::string-lit-as-bytes` implied by `-D warnings`
6262

6363
error: calling `as_bytes()` on a string literal
64-
--> $DIR/strings.rs:65:18
64+
--> $DIR/strings.rs:62:14
6565
|
66-
65 | let strify = stringify!(foobar).as_bytes();
67-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using a byte string literal instead: `bstringify!(foobar)`
66+
62 | let bs = r###"raw string with three ### in it and some " ""###.as_bytes();
67+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using a byte string literal instead: `br###"raw string with three ### in it and some " ""###`
6868

69-
error: manual implementation of an assign operation
70-
--> $DIR/strings.rs:75:7
69+
error: calling `as_bytes()` on `include_str!(..)`
70+
--> $DIR/strings.rs:69:22
7171
|
72-
75 | ; x = x + 1;
73-
| ^^^^^^^^^ help: replace it with: `x += 1`
72+
69 | let includestr = include_str!("entry.rs").as_bytes();
73+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `include_bytes!(..)` instead: `include_bytes!("entry.rs")`
7474

7575
error: aborting due to 11 previous errors
7676

0 commit comments

Comments
 (0)