From c209fc9349ff750dc983ecfe23d8e0bb74f002df Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Fri, 5 Oct 2018 09:06:05 -0700 Subject: [PATCH 1/3] Fix string_lit_as_bytes lint for macros 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 --- clippy_lints/src/strings.rs | 39 +++++++++++++++++++++++++++++-------- tests/ui/strings.rs | 8 +++++--- tests/ui/strings.stderr | 12 ------------ 3 files changed, 36 insertions(+), 23 deletions(-) diff --git a/clippy_lints/src/strings.rs b/clippy_lints/src/strings.rs index f4798842205a..9b6478fb9cd6 100644 --- a/clippy_lints/src/strings.rs +++ b/clippy_lints/src/strings.rs @@ -92,7 +92,14 @@ impl LintPass for StringAdd { impl<'a, 'tcx> LateLintPass<'a, 'tcx> for StringAdd { fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, e: &'tcx Expr) { - if let ExprKind::Binary(Spanned { node: BinOpKind::Add, .. }, ref left, _) = e.node { + if let ExprKind::Binary( + Spanned { + node: BinOpKind::Add, .. + }, + ref left, + _, + ) = e.node + { if is_string(cx, left) { if !is_allowed(cx, STRING_ADD_ASSIGN, e.id) { let parent = get_parent_expr(cx, e); @@ -132,13 +139,15 @@ fn is_string(cx: &LateContext<'_, '_>, e: &Expr) -> bool { fn is_add(cx: &LateContext<'_, '_>, src: &Expr, target: &Expr) -> bool { match src.node { - ExprKind::Binary(Spanned { node: BinOpKind::Add, .. }, ref left, _) => SpanlessEq::new(cx).eq_expr(target, left), + ExprKind::Binary( + Spanned { + node: BinOpKind::Add, .. + }, + ref left, + _, + ) => SpanlessEq::new(cx).eq_expr(target, left), ExprKind::Block(ref block, _) => { - block.stmts.is_empty() - && block - .expr - .as_ref() - .map_or(false, |expr| is_add(cx, expr, target)) + block.stmts.is_empty() && block.expr.as_ref().map_or(false, |expr| is_add(cx, expr, target)) }, _ => false, } @@ -162,7 +171,21 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for StringLitAsBytes { if path.ident.name == "as_bytes" { if let ExprKind::Lit(ref lit) = args[0].node { if let LitKind::Str(ref lit_content, _) = lit.node { - if lit_content.as_str().chars().all(|c| c.is_ascii()) && !in_macro(args[0].span) { + let callsite = snippet(cx, args[0].span.source_callsite(), ""); + let expanded = format!("\"{}\"", lit_content.as_str()); + if callsite.starts_with("include_str!") { + span_lint_and_sugg( + cx, + STRING_LIT_AS_BYTES, + e.span, + "calling `as_bytes()` on `include_str!(..)`", + "consider using `include_bytes!(..)` instead", + snippet(cx, args[0].span, r#""foo""#).replacen("include_str", "include_bytes", 1), + ); + } else if callsite == expanded + && lit_content.as_str().chars().all(|c| c.is_ascii()) + && !in_macro(args[0].span) + { span_lint_and_sugg( cx, STRING_LIT_AS_BYTES, diff --git a/tests/ui/strings.rs b/tests/ui/strings.rs index 7bc4e6515f65..6693776a9617 100644 --- a/tests/ui/strings.rs +++ b/tests/ui/strings.rs @@ -10,10 +10,10 @@ - #[warn(clippy::string_add)] #[allow(clippy::string_add_assign)] -fn add_only() { // ignores assignment distinction +fn add_only() { + // ignores assignment distinction let mut x = "".to_owned(); for _ in 1..3 { @@ -63,6 +63,8 @@ fn str_lit_as_bytes() { let ubs = "☃".as_bytes(); let strify = stringify!(foobar).as_bytes(); + + let includestr = include_str!("entry.rs").as_bytes(); } fn main() { @@ -72,6 +74,6 @@ fn main() { // the add is only caught for `String` let mut x = 1; - ; x = x + 1; +; x = x + 1; assert_eq!(2, x); } diff --git a/tests/ui/strings.stderr b/tests/ui/strings.stderr index bcdf91568d27..8a93733732ea 100644 --- a/tests/ui/strings.stderr +++ b/tests/ui/strings.stderr @@ -60,17 +60,5 @@ error: calling `as_bytes()` on a string literal | = note: `-D clippy::string-lit-as-bytes` implied by `-D warnings` -error: calling `as_bytes()` on a string literal - --> $DIR/strings.rs:65:18 - | -65 | let strify = stringify!(foobar).as_bytes(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using a byte string literal instead: `bstringify!(foobar)` - -error: manual implementation of an assign operation - --> $DIR/strings.rs:75:7 - | -75 | ; x = x + 1; - | ^^^^^^^^^ help: replace it with: `x += 1` - error: aborting due to 11 previous errors From f9020bb2dded44e97fd997ab71ab4edf6d88033b Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Wed, 24 Oct 2018 11:49:39 -0400 Subject: [PATCH 2/3] fix: extra semicolon, only create callsite once --- clippy_lints/src/strings.rs | 2 +- tests/ui/strings.rs | 5 ++++- tests/ui/strings.stderr | 2 -- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/strings.rs b/clippy_lints/src/strings.rs index 9b6478fb9cd6..fe3d461ab43a 100644 --- a/clippy_lints/src/strings.rs +++ b/clippy_lints/src/strings.rs @@ -171,7 +171,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for StringLitAsBytes { if path.ident.name == "as_bytes" { if let ExprKind::Lit(ref lit) = args[0].node { if let LitKind::Str(ref lit_content, _) = lit.node { - let callsite = snippet(cx, args[0].span.source_callsite(), ""); + let callsite = snippet(cx, args[0].span.source_callsite(), r#""foo""#); let expanded = format!("\"{}\"", lit_content.as_str()); if callsite.starts_with("include_str!") { span_lint_and_sugg( diff --git a/tests/ui/strings.rs b/tests/ui/strings.rs index 6693776a9617..d2062b356dc0 100644 --- a/tests/ui/strings.rs +++ b/tests/ui/strings.rs @@ -59,6 +59,8 @@ fn both() { fn str_lit_as_bytes() { let bs = "hello there".as_bytes(); + let bs = r###"raw string with three ### in it and some " ""###.as_bytes(); + // no warning, because this cannot be written as a byte string literal: let ubs = "☃".as_bytes(); @@ -67,6 +69,7 @@ fn str_lit_as_bytes() { let includestr = include_str!("entry.rs").as_bytes(); } +#[allow(clippy::assign_op_pattern)] fn main() { add_only(); add_assign_only(); @@ -74,6 +77,6 @@ fn main() { // the add is only caught for `String` let mut x = 1; -; x = x + 1; + x = x + 1; assert_eq!(2, x); } diff --git a/tests/ui/strings.stderr b/tests/ui/strings.stderr index 8a93733732ea..2496270ba0d5 100644 --- a/tests/ui/strings.stderr +++ b/tests/ui/strings.stderr @@ -60,5 +60,3 @@ error: calling `as_bytes()` on a string literal | = note: `-D clippy::string-lit-as-bytes` implied by `-D warnings` -error: aborting due to 11 previous errors - From 19ac2e94c6cb12ae4f9fb410f165e2aa5309e124 Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Fri, 26 Oct 2018 09:10:20 -0700 Subject: [PATCH 3/3] fix: correctly reconstruct raw strings --- clippy_lints/src/strings.rs | 12 ++++++++---- tests/ui/strings.stderr | 14 ++++++++++++++ 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/strings.rs b/clippy_lints/src/strings.rs index fe3d461ab43a..e07b1649a466 100644 --- a/clippy_lints/src/strings.rs +++ b/clippy_lints/src/strings.rs @@ -7,7 +7,6 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. - use crate::rustc::hir::*; use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; use crate::rustc::{declare_tool_lint, lint_array}; @@ -164,15 +163,20 @@ impl LintPass for StringLitAsBytes { impl<'a, 'tcx> LateLintPass<'a, 'tcx> for StringLitAsBytes { fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, e: &'tcx Expr) { - use crate::syntax::ast::LitKind; + use crate::syntax::ast::{LitKind, StrStyle}; use crate::utils::{in_macro, snippet}; if let ExprKind::MethodCall(ref path, _, ref args) = e.node { if path.ident.name == "as_bytes" { if let ExprKind::Lit(ref lit) = args[0].node { - if let LitKind::Str(ref lit_content, _) = lit.node { + if let LitKind::Str(ref lit_content, style) = lit.node { let callsite = snippet(cx, args[0].span.source_callsite(), r#""foo""#); - let expanded = format!("\"{}\"", lit_content.as_str()); + let expanded = if let StrStyle::Raw(n) = style { + let term = (0..n).map(|_| '#').collect::(); + format!("r{0}\"{1}\"{0}", term, lit_content.as_str()) + } else { + format!("\"{}\"", lit_content.as_str()) + }; if callsite.starts_with("include_str!") { span_lint_and_sugg( cx, diff --git a/tests/ui/strings.stderr b/tests/ui/strings.stderr index 2496270ba0d5..21115d8e97ec 100644 --- a/tests/ui/strings.stderr +++ b/tests/ui/strings.stderr @@ -60,3 +60,17 @@ error: calling `as_bytes()` on a string literal | = note: `-D clippy::string-lit-as-bytes` implied by `-D warnings` +error: calling `as_bytes()` on a string literal + --> $DIR/strings.rs:62:14 + | +62 | let bs = r###"raw string with three ### in it and some " ""###.as_bytes(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using a byte string literal instead: `br###"raw string with three ### in it and some " ""###` + +error: calling `as_bytes()` on `include_str!(..)` + --> $DIR/strings.rs:69:22 + | +69 | let includestr = include_str!("entry.rs").as_bytes(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `include_bytes!(..)` instead: `include_bytes!("entry.rs")` + +error: aborting due to 11 previous errors +