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.

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

Add new lint string_as_str #9624

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4377,6 +4377,7 @@ Released 2018-09-13
[`str_to_string`]: https://rust-lang.github.io/rust-clippy/master/index.html#str_to_string
[`string_add`]: https://rust-lang.github.io/rust-clippy/master/index.html#string_add
[`string_add_assign`]: https://rust-lang.github.io/rust-clippy/master/index.html#string_add_assign
[`string_as_str`]: https://rust-lang.github.io/rust-clippy/master/index.html#string_as_str
[`string_extend_chars`]: https://rust-lang.github.io/rust-clippy/master/index.html#string_extend_chars
[`string_from_utf8_as_bytes`]: https://rust-lang.github.io/rust-clippy/master/index.html#string_from_utf8_as_bytes
[`string_lit_as_bytes`]: https://rust-lang.github.io/rust-clippy/master/index.html#string_lit_as_bytes
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/casts/cast_lossless.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ pub(super) fn check(
if should_strip_parens(cast_op, snip) {
&snip[1..snip.len() - 1]
} else {
snip.as_str()
snip
}
},
);
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
crate::methods::SINGLE_CHAR_PATTERN_INFO,
crate::methods::SKIP_WHILE_NEXT_INFO,
crate::methods::STABLE_SORT_PRIMITIVE_INFO,
crate::methods::STRING_AS_STR_INFO,
crate::methods::STRING_EXTEND_CHARS_INFO,
crate::methods::SUSPICIOUS_MAP_INFO,
crate::methods::SUSPICIOUS_SPLITN_INFO,
Expand Down
4 changes: 2 additions & 2 deletions clippy_lints/src/loops/manual_memcpy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ fn build_manual_memcpy_suggestion<'tcx>(
print_offset(apply_offset(&start_str, &idx_expr.idx_offset)).into_sugg(),
print_limit(
end,
end_str.to_string().as_str(),
&end_str.to_string(),
idx_expr.base,
apply_offset(&end_str, &idx_expr.idx_offset),
)
Expand All @@ -159,7 +159,7 @@ fn build_manual_memcpy_suggestion<'tcx>(
print_offset(apply_offset(&counter_start, &idx_expr.idx_offset)).into_sugg(),
print_limit(
end,
end_str.to_string().as_str(),
&end_str.to_string(),
idx_expr.base,
apply_offset(&end_str, &idx_expr.idx_offset) + &counter_start - &start_str,
)
Expand Down
33 changes: 33 additions & 0 deletions clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ mod single_char_push_string;
mod skip_while_next;
mod stable_sort_primitive;
mod str_splitn;
mod string_as_str;
mod string_extend_chars;
mod suspicious_map;
mod suspicious_splitn;
Expand Down Expand Up @@ -3068,6 +3069,37 @@ declare_clippy_lint! {
"iterating on map using `iter` when `keys` or `values` would do"
}

declare_clippy_lint! {
///
/// Checks for `_.as_str()` calls on `String`s, that can be replaced with just `&_`.
///
/// ### Why is this bad?
///
/// `as_str` is an unnecessary method call, that makes the code harder to read.
///
/// ### Example
/// ```rust
/// fn fn_with_str(my_string: &str) {
/// println!("hey {my_string}");
/// }
///
/// let my_string = String::from("hey");
/// fn_with_str(my_string.as_str());
/// ```
/// Use instead:
/// ```rust
/// # fn fn_with_str(my_string: &str) {
/// # println!("hey {my_string}");
/// # }
/// # let my_string = String::from("hey");
/// fn_with_str(&my_string);
/// ```
#[clippy::version = "1.67.0"]
pub STRING_AS_STR,
pedantic,
"finds `as_str()` calls used for `String` when `&` is enough"
}

declare_clippy_lint! {
/// ### What it does
///
Expand Down Expand Up @@ -3480,6 +3512,7 @@ impl Methods {
},
("as_mut", []) => useless_asref::check(cx, expr, "as_mut", recv),
("as_ref", []) => useless_asref::check(cx, expr, "as_ref", recv),
("as_str", []) => string_as_str::check(cx, expr, recv),
("assume_init", []) => uninit_assumed_init::check(cx, expr, recv),
("cloned", []) => cloned_instead_of_copied::check(cx, expr, recv, span, self.msrv),
("collect", []) => match method_call(recv) {
Expand Down
45 changes: 45 additions & 0 deletions clippy_lints/src/methods/string_as_str.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
use clippy_utils::get_parent_expr;
use clippy_utils::sugg::Sugg;
use clippy_utils::{diagnostics::span_lint_and_sugg, ty::is_type_diagnostic_item};
use rustc_ast::BorrowKind;
use rustc_errors::Applicability;
use rustc_hir::{Expr, ExprKind};
use rustc_lint::LateContext;
use rustc_span::sym::String;

use super::STRING_AS_STR;

pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>) {
if is_string(cx, recv) && !is_within_borrowed_call(cx, expr) {
let parent = get_parent_expr(cx, recv).unwrap();
let sugg = format!("&{}", Sugg::hir(cx, recv, "..").maybe_par());
span_lint_and_sugg(
cx,
STRING_AS_STR,
parent.span,
"used `as_str()` for reference",
"replace it with",
sugg,
Applicability::MaybeIncorrect,
);
}
}

fn is_string(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(expr), String)
}

fn is_within_borrowed_call(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what this function is supposed to do. It looks like it checks for really specific things in the tests you've written.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't know how to not lint here

Copy link
Member

Choose a reason for hiding this comment

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

Why can't this be linted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because using &snip there are different type on those two branch, one is &str and other &String, don't know if we can change the code in other way

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this is a blocking issue since it's not a machine applicable lint.

This seems like a wrong inference from rustc because explicit typing fixes the issue

let a: Option<&str> = match snip.split_once(" as ") {
        None => Some(&snip),
        Some((import, rename)) => {
            if rename.trim() == name {
                None
            } else {
                Some(import.trim())
            }
        },
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly... Don't sure next step to complete this lint :( 🤔

if let Some(parent) = clippy_utils::get_parent_expr(cx, expr) {
return matches!(
parent.kind,
ExprKind::Closure(..)
| ExprKind::Tup(..)
| ExprKind::Match(_, _, _)
| ExprKind::AddrOf(BorrowKind::Ref, ..)
| ExprKind::MethodCall(_, _, _, _)
);
}

false
}
2 changes: 1 addition & 1 deletion clippy_lints/src/option_if_let_else.rs
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ impl<'tcx> LateLintPass<'tcx> for OptionIfLetElse {
cx,
OPTION_IF_LET_ELSE,
expr.span,
format!("use Option::{} instead of an if let/else", det.method_sugg).as_str(),
&format!("use Option::{} instead of an if let/else", det.method_sugg),
"try",
format!(
"{}.{}({}, {})",
Expand Down
4 changes: 2 additions & 2 deletions clippy_lints/src/unnecessary_wraps.rs
Original file line number Diff line number Diff line change
Expand Up @@ -158,10 +158,10 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryWraps {
)
};

span_lint_and_then(cx, UNNECESSARY_WRAPS, span, lint_msg.as_str(), |diag| {
span_lint_and_then(cx, UNNECESSARY_WRAPS, span, &lint_msg, |diag| {
diag.span_suggestion(
fn_decl.output.span(),
return_type_sugg_msg.as_str(),
&return_type_sugg_msg,
return_type_sugg,
Applicability::MaybeIncorrect,
);
Expand Down
59 changes: 59 additions & 0 deletions tests/ui/string_as_str.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
#![allow(unused)]
#![warn(clippy::string_as_str)]

use clippy_utils::consts::Constant;

fn main() {
let my_string = String::from("hey");
let s = my_string.as_str();
fn_with_str(my_string.as_str());

let fs = FakeString::default();
let ss = fs.as_str();

match my_string.as_str() {
"hello" => (),
"hey" => (),
_ => (),
}

match (my_string.as_str(), my_string.as_str()) {
("hello", ("hello")) => (),
("hey", "hey") => (),
_ => (),
}

const ARRAY: &[&str] = &["hello", "dump_hir"];
let s: String = "hello".to_string();
let exists = ARRAY.contains(&s.as_str());

let a = s.as_str().chars().map(|c| c.is_digit(2)).collect::<Vec<_>>();

let path = vec!["path1".to_string(), "path2".to_string()];
let path: Vec<&str> = path.iter().map(|x| x.as_str()).collect();
let snip = "String".to_owned();
let name = "alice".to_string();
let a: Option<&str> = match snip.split_once(" as ") {
None => Some(snip.as_str()),
Some((import, rename)) => {
if rename.trim() == name {
None
} else {
Some(import.trim())
}
},
};
}

fn fn_with_str(my_string: &str) {
println!("hey {my_string}");
}

#[derive(Default)]
struct FakeString(pub u32);

impl FakeString {
fn as_str(&self) -> String {
self.0.to_string()
}
}
22 changes: 22 additions & 0 deletions tests/ui/string_as_str.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
error: used `as_str()` for reference
--> $DIR/string_as_str.rs:8:13
|
LL | let s = my_string.as_str();
| ^^^^^^^^^^^^^^^^^^ help: replace it with: `&my_string`
|
= note: `-D clippy::string-as-str` implied by `-D warnings`

error: used `as_str()` for reference
--> $DIR/string_as_str.rs:9:17
|
LL | fn_with_str(my_string.as_str());
| ^^^^^^^^^^^^^^^^^^ help: replace it with: `&my_string`

error: used `as_str()` for reference
--> $DIR/string_as_str.rs:37:22
|
LL | None => Some(snip.as_str()),
| ^^^^^^^^^^^^^ help: replace it with: `&snip`

error: aborting due to 3 previous errors