mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Auto merge of - mikerite:lint-5734, r=matthiaskrgr

Browse Source 
Add `manual-strip` lint

Add `manual-strip` lint.

changelog: Add `manual-strip` lint
This commit is contained in:
bors 2020-09-16 18:13:36 +00:00
commit 5e60497fb8
13 changed files with 473 additions and 16 deletions

1
CHANGELOG.md

@ -1672,6 +1672,7 @@ Released 2018-09-13
[`manual_memcpy`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_memcpy
[`manual_non_exhaustive`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_non_exhaustive
[`manual_saturating_arithmetic`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_saturating_arithmetic
[`manual_strip`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_strip
[`manual_swap`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_swap
[`many_single_char_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#many_single_char_names
[`map_clone`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_clone

2
clippy_lints/src/doc.rs

@ -534,7 +534,7 @@ fn check_word(cx: &LateContext<'_>, word: &str, span: Span) {
return false;
}
let s = if s.ends_with('s') { &s[..s.len() - 1] } else { s };
let s = s.strip_suffix('s').unwrap_or(s);
s.chars().all(char::is_alphanumeric)
&& s.chars().filter(|&c| c.is_uppercase()).take(2).count() > 1

5
clippy_lints/src/lib.rs

@ -230,6 +230,7 @@ mod macro_use;
mod main_recursion;
mod manual_async_fn;
mod manual_non_exhaustive;
mod manual_strip;
mod map_clone;
mod map_err_ignore;
mod map_identity;
@ -627,6 +628,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
&main_recursion::MAIN_RECURSION,
&manual_async_fn::MANUAL_ASYNC_FN,
&manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE,
&manual_strip::MANUAL_STRIP,
&map_clone::MAP_CLONE,
&map_err_ignore::MAP_ERR_IGNORE,
&map_identity::MAP_IDENTITY,
@ -1113,6 +1115,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
store.register_late_pass(|| box self_assignment::SelfAssignment);
store.register_late_pass(|| box float_equality_without_abs::FloatEqualityWithoutAbs);
store.register_late_pass(|| box async_yields_async::AsyncYieldsAsync);
store.register_late_pass(|| box manual_strip::ManualStrip);
store.register_late_pass(|| box utils::internal_lints::MatchTypeOnDiagItem);
store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![
@ -1342,6 +1345,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&main_recursion::MAIN_RECURSION),
LintId::of(&manual_async_fn::MANUAL_ASYNC_FN),
LintId::of(&manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE),
LintId::of(&manual_strip::MANUAL_STRIP),
LintId::of(&map_clone::MAP_CLONE),
LintId::of(&map_identity::MAP_IDENTITY),
LintId::of(&map_unit_fn::OPTION_MAP_UNIT_FN),
@ -1633,6 +1637,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&loops::EXPLICIT_COUNTER_LOOP),
LintId::of(&loops::MUT_RANGE_BOUND),
LintId::of(&loops::WHILE_LET_LOOP),
LintId::of(&manual_strip::MANUAL_STRIP),
LintId::of(&map_identity::MAP_IDENTITY),
LintId::of(&map_unit_fn::OPTION_MAP_UNIT_FN),
LintId::of(&map_unit_fn::RESULT_MAP_UNIT_FN),

8
clippy_lints/src/loops.rs

@ -2601,11 +2601,9 @@ fn check_needless_collect_direct_usage<'tcx>(expr: &'tcx Expr<'_>, cx: &LateCont
span,
NEEDLESS_COLLECT_MSG,
|diag| {
let (arg, pred) = if contains_arg.starts_with('&') {
("x", &contains_arg[1..])
} else {
("&x", &*contains_arg)
};
let (arg, pred) = contains_arg
.strip_prefix('&')
.map_or(("&x", &*contains_arg), |s| ("x", s));
diag.span_suggestion(
span,
"replace with",

245
clippy_lints/src/manual_strip.rs Normal file

@ -0,0 +1,245 @@
use crate::consts::{constant, Constant};
use crate::utils::usage::mutated_variables;
use crate::utils::{
eq_expr_value, higher, match_def_path, multispan_sugg, paths, qpath_res, snippet, span_lint_and_then,
};
use if_chain::if_chain;
use rustc_ast::ast::LitKind;
use rustc_hir::def::Res;
use rustc_hir::intravisit::{walk_expr, NestedVisitorMap, Visitor};
use rustc_hir::BinOpKind;
use rustc_hir::{BorrowKind, Expr, ExprKind};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::hir::map::Map;
use rustc_middle::ty;
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::source_map::Spanned;
use rustc_span::Span;
declare_clippy_lint! {
/// **What it does:**
/// Suggests using `strip_{prefix,suffix}` over `str::{starts,ends}_with` and slicing using
/// the pattern's length.
///
/// **Why is this bad?**
/// Using `str:strip_{prefix,suffix}` is safer and may have better performance as there is no
/// slicing which may panic and the compiler does not need to insert this panic code. It is
/// also sometimes more readable as it removes the need for duplicating or storing the pattern
/// used by `str::{starts,ends}_with` and in the slicing.
///
/// **Known problems:**
/// None.
///
/// **Example:**
///
/// ```rust
/// let s = "hello, world!";
/// if s.starts_with("hello, ") {
/// assert_eq!(s["hello, ".len()..].to_uppercase(), "WORLD!");
/// }
/// ```
/// Use instead:
/// ```rust
/// let s = "hello, world!";
/// if let Some(end) = s.strip_prefix("hello, ") {
/// assert_eq!(end.to_uppercase(), "WORLD!");
/// }
/// ```
pub MANUAL_STRIP,
complexity,
"suggests using `strip_{prefix,suffix}` over `str::{starts,ends}_with` and slicing"
}
declare_lint_pass!(ManualStrip => [MANUAL_STRIP]);
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
enum StripKind {
Prefix,
Suffix,
}
impl<'tcx> LateLintPass<'tcx> for ManualStrip {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
if_chain! {
if let Some((cond, then, _)) = higher::if_block(&expr);
if let ExprKind::MethodCall(_, _, [target_arg, pattern], _) = cond.kind;
if let Some(method_def_id) = cx.typeck_results().type_dependent_def_id(cond.hir_id);
if let ExprKind::Path(target_path) = &target_arg.kind;
then {
let strip_kind = if match_def_path(cx, method_def_id, &paths::STR_STARTS_WITH) {
StripKind::Prefix
} else if match_def_path(cx, method_def_id, &paths::STR_ENDS_WITH) {
StripKind::Suffix
} else {
return;
};
let target_res = qpath_res(cx, &target_path, target_arg.hir_id);
if target_res == Res::Err {
return;
};
if_chain! {
if let Res::Local(hir_id) = target_res;
if let Some(used_mutably) = mutated_variables(then, cx);
if used_mutably.contains(&hir_id);
then {
return;
}
}
let strippings = find_stripping(cx, strip_kind, target_res, pattern, then);
if !strippings.is_empty() {
let kind_word = match strip_kind {
StripKind::Prefix => "prefix",
StripKind::Suffix => "suffix",
};
let test_span = expr.span.until(then.span);
span_lint_and_then(cx, MANUAL_STRIP, strippings[0], &format!("stripping a {} manually", kind_word), |diag| {
diag.span_note(test_span, &format!("the {} was tested here", kind_word));
multispan_sugg(
diag,
&format!("try using the `strip_{}` method", kind_word),
vec![(test_span,
format!("if let Some(<stripped>) = {}.strip_{}({}) ",
snippet(cx, target_arg.span, ".."),
kind_word,
snippet(cx, pattern.span, "..")))]
.into_iter().chain(strippings.into_iter().map(|span| (span, "<stripped>".into()))),
)
});
}
}
}
}
}
// Returns `Some(arg)` if `expr` matches `arg.len()` and `None` otherwise.
fn len_arg<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option<&'tcx Expr<'tcx>> {
if_chain! {
if let ExprKind::MethodCall(_, _, [arg], _) = expr.kind;
if let Some(method_def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id);
if match_def_path(cx, method_def_id, &paths::STR_LEN);
then {
Some(arg)
} else {
None
}
}
}
// Returns the length of the `expr` if it's a constant string or char.
fn constant_length(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<u128> {
let (value, _) = constant(cx, cx.typeck_results(), expr)?;
match value {
Constant::Str(value) => Some(value.len() as u128),
Constant::Char(value) => Some(value.len_utf8() as u128),
_ => None,
}
}
// Tests if `expr` equals the length of the pattern.
fn eq_pattern_length<'tcx>(cx: &LateContext<'tcx>, pattern: &Expr<'_>, expr: &'tcx Expr<'_>) -> bool {
if let ExprKind::Lit(Spanned {
node: LitKind::Int(n, _),
..
}) = expr.kind
{
constant_length(cx, pattern).map_or(false, |length| length == n)
} else {
len_arg(cx, expr).map_or(false, |arg| eq_expr_value(cx, pattern, arg))
}
}
// Tests if `expr` is a `&str`.
fn is_ref_str(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
match cx.typeck_results().expr_ty_adjusted(&expr).kind() {
ty::Ref(_, ty, _) => ty.is_str(),
_ => false,
}
}
// Removes the outer `AddrOf` expression if needed.
fn peel_ref<'a>(expr: &'a Expr<'_>) -> &'a Expr<'a> {
if let ExprKind::AddrOf(BorrowKind::Ref, _, unref) = &expr.kind {
unref
} else {
expr
}
}
// Find expressions where `target` is stripped using the length of `pattern`.
// We'll suggest replacing these expressions with the result of the `strip_{prefix,suffix}`
// method.
fn find_stripping<'tcx>(
cx: &LateContext<'tcx>,
strip_kind: StripKind,
target: Res,
pattern: &'tcx Expr<'_>,
expr: &'tcx Expr<'_>,
) -> Vec<Span> {
struct StrippingFinder<'a, 'tcx> {
cx: &'a LateContext<'tcx>,
strip_kind: StripKind,
target: Res,
pattern: &'tcx Expr<'tcx>,
results: Vec<Span>,
}
impl<'a, 'tcx> Visitor<'tcx> for StrippingFinder<'a, 'tcx> {
type Map = Map<'tcx>;
fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
NestedVisitorMap::None
}
fn visit_expr(&mut self, ex: &'tcx Expr<'_>) {
if_chain! {
if is_ref_str(self.cx, ex);
let unref = peel_ref(ex);
if let ExprKind::Index(indexed, index) = &unref.kind;
if let Some(range) = higher::range(index);
if let higher::Range { start, end, .. } = range;
if let ExprKind::Path(path) = &indexed.kind;
if qpath_res(self.cx, path, ex.hir_id) == self.target;
then {
match (self.strip_kind, start, end) {
(StripKind::Prefix, Some(start), None) => {
if eq_pattern_length(self.cx, self.pattern, start) {
self.results.push(ex.span);
return;
}
},
(StripKind::Suffix, None, Some(end)) => {
if_chain! {
if let ExprKind::Binary(Spanned { node: BinOpKind::Sub, .. }, left, right) = end.kind;
if let Some(left_arg) = len_arg(self.cx, left);
if let ExprKind::Path(left_path) = &left_arg.kind;
if qpath_res(self.cx, left_path, left_arg.hir_id) == self.target;
if eq_pattern_length(self.cx, self.pattern, right);
then {
self.results.push(ex.span);
return;
}
}
},
_ => {}
}
}
}
walk_expr(self, ex);
}
}
let mut finder = StrippingFinder {
cx,
strip_kind,
target,
pattern,
results: vec![],
};
walk_expr(&mut finder, expr);
finder.results
}

6
clippy_lints/src/misc_early.rs

@ -377,8 +377,8 @@ impl EarlyLintPass for MiscEarlyLints {
if let PatKind::Ident(_, ident, None) = arg.pat.kind {
let arg_name = ident.to_string();
if arg_name.starts_with('_') {
if let Some(correspondence) = registered_names.get(&arg_name[1..]) {
if let Some(arg_name) = arg_name.strip_prefix('_') {
if let Some(correspondence) = registered_names.get(arg_name) {
span_lint(
cx,
DUPLICATE_UNDERSCORE_ARGUMENT,
@ -386,7 +386,7 @@ impl EarlyLintPass for MiscEarlyLints {
&format!(
"`{}` already exists, having another argument having almost the same \
name makes code comprehension and documentation more difficult",
arg_name[1..].to_owned()
arg_name
),
);
}

5
clippy_lints/src/redundant_clone.rs

@ -239,10 +239,9 @@ impl<'tcx> LateLintPass<'tcx> for RedundantClone {
);
let mut app = Applicability::MaybeIncorrect;
let mut call_snip = &snip[dot + 1..];
let call_snip = &snip[dot + 1..];
// Machine applicable when `call_snip` looks like `foobar()`
if call_snip.ends_with("()") {
call_snip = call_snip[..call_snip.len()-2].trim();
if let Some(call_snip) = call_snip.strip_suffix("()").map(str::trim) {
if call_snip.as_bytes().iter().all(|b| b.is_ascii_alphabetic() || *b == b'_') {
app = Applicability::MachineApplicable;
}

3
clippy_lints/src/utils/paths.rs

@ -115,6 +115,9 @@ pub const STD_MEM_TRANSMUTE: [&str; 3] = ["std", "mem", "transmute"];
pub const STD_PTR_NULL: [&str; 3] = ["std", "ptr", "null"];
pub const STRING_AS_MUT_STR: [&str; 4] = ["alloc", "string", "String", "as_mut_str"];
pub const STRING_AS_STR: [&str; 4] = ["alloc", "string", "String", "as_str"];
pub const STR_ENDS_WITH: [&str; 4] = ["core", "str", "<impl str>", "ends_with"];
pub const STR_LEN: [&str; 4] = ["core", "str", "<impl str>", "len"];
pub const STR_STARTS_WITH: [&str; 4] = ["core", "str", "<impl str>", "starts_with"];
pub const SYNTAX_CONTEXT: [&str; 3] = ["rustc_span", "hygiene", "SyntaxContext"];
pub const TO_OWNED: [&str; 3] = ["alloc", "borrow", "ToOwned"];
pub const TO_OWNED_METHOD: [&str; 4] = ["alloc", "borrow", "ToOwned", "to_owned"];

7
src/lintlist/mod.rs

@ -1144,6 +1144,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
deprecation: None,
module: "methods",
},
Lint {
name: "manual_strip",
group: "complexity",
desc: "suggests using `strip_{prefix,suffix}` over `str::{starts,ends}_with` and slicing",
deprecation: None,
module: "manual_strip",
},
Lint {
name: "manual_swap",
group: "complexity",

1
tests/ui/let_if_seq.rs

@ -33,6 +33,7 @@ fn issue985_alt() -> i32 {
x
}
#[allow(clippy::manual_strip)]
fn issue975() -> String {
let mut udn = "dummy".to_string();
if udn.starts_with("uuid:") {

8
tests/ui/let_if_seq.stderr

@ -1,5 +1,5 @@
error: `if _ { .. } else { .. }` is an expression
--> $DIR/let_if_seq.rs:63:5
--> $DIR/let_if_seq.rs:64:5
|
LL | / let mut foo = 0;
LL | | if f() {
@ -11,7 +11,7 @@ LL | | }
= note: you might not need `mut` at all
error: `if _ { .. } else { .. }` is an expression
--> $DIR/let_if_seq.rs:68:5
--> $DIR/let_if_seq.rs:69:5
|
LL | / let mut bar = 0;
LL | | if f() {
@ -25,7 +25,7 @@ LL | | }
= note: you might not need `mut` at all
error: `if _ { .. } else { .. }` is an expression
--> $DIR/let_if_seq.rs:76:5
--> $DIR/let_if_seq.rs:77:5
|
LL | / let quz;
LL | | if f() {
@ -36,7 +36,7 @@ LL | | }
| |_____^ help: it is more idiomatic to write: `let quz = if f() { 42 } else { 0 };`
error: `if _ { .. } else { .. }` is an expression
--> $DIR/let_if_seq.rs:105:5
--> $DIR/let_if_seq.rs:106:5
|
LL | / let mut baz = 0;
LL | | if f() {

66
tests/ui/manual_strip.rs Normal file

@ -0,0 +1,66 @@
#![warn(clippy::manual_strip)]
fn main() {
let s = "abc";
if s.starts_with("ab") {
str::to_string(&s["ab".len()..]);
s["ab".len()..].to_string();
str::to_string(&s[2..]);
s[2..].to_string();
}
if s.ends_with("bc") {
str::to_string(&s[..s.len() - "bc".len()]);
s[..s.len() - "bc".len()].to_string();
str::to_string(&s[..s.len() - 2]);
s[..s.len() - 2].to_string();
}
// Character patterns
if s.starts_with('a') {
str::to_string(&s[1..]);
s[1..].to_string();
}
// Variable prefix
let prefix = "ab";
if s.starts_with(prefix) {
str::to_string(&s[prefix.len()..]);
}
// Constant prefix
const PREFIX: &str = "ab";
if s.starts_with(PREFIX) {
str::to_string(&s[PREFIX.len()..]);
str::to_string(&s[2..]);
}
// Constant target
const TARGET: &str = "abc";
if TARGET.starts_with(prefix) {
str::to_string(&TARGET[prefix.len()..]);
}
// String target - not mutated.
let s1: String = "abc".into();
if s1.starts_with("ab") {
s1[2..].to_uppercase();
}
// String target - mutated. (Don't lint.)
let mut s2: String = "abc".into();
if s2.starts_with("ab") {
s2.push('d');
s2[2..].to_uppercase();
}
// Target not stripped. (Don't lint.)
let s3 = String::from("abcd");
let s4 = String::from("efgh");
if s3.starts_with("ab") {
s4[2..].to_string();
}
}

132
tests/ui/manual_strip.stderr Normal file

@ -0,0 +1,132 @@
error: stripping a prefix manually
--> $DIR/manual_strip.rs:7:24
|
LL | str::to_string(&s["ab".len()..]);
| ^^^^^^^^^^^^^^^^
|
= note: `-D clippy::manual-strip` implied by `-D warnings`
note: the prefix was tested here
--> $DIR/manual_strip.rs:6:5
|
LL | if s.starts_with("ab") {
| ^^^^^^^^^^^^^^^^^^^^^^^
help: try using the `strip_prefix` method
|
LL | if let Some(<stripped>) = s.strip_prefix("ab") {
LL | str::to_string(<stripped>);
LL | <stripped>.to_string();
LL |
LL | str::to_string(<stripped>);
LL | <stripped>.to_string();
|
error: stripping a suffix manually
--> $DIR/manual_strip.rs:15:24
|
LL | str::to_string(&s[..s.len() - "bc".len()]);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
|
note: the suffix was tested here
--> $DIR/manual_strip.rs:14:5
|
LL | if s.ends_with("bc") {
| ^^^^^^^^^^^^^^^^^^^^^
help: try using the `strip_suffix` method
|
LL | if let Some(<stripped>) = s.strip_suffix("bc") {
LL | str::to_string(<stripped>);
LL | <stripped>.to_string();
LL |
LL | str::to_string(<stripped>);
LL | <stripped>.to_string();
|
error: stripping a prefix manually
--> $DIR/manual_strip.rs:24:24
|
LL | str::to_string(&s[1..]);
| ^^^^^^^
|
note: the prefix was tested here
--> $DIR/manual_strip.rs:23:5
|
LL | if s.starts_with('a') {
| ^^^^^^^^^^^^^^^^^^^^^^
help: try using the `strip_prefix` method
|
LL | if let Some(<stripped>) = s.strip_prefix('a') {
LL | str::to_string(<stripped>);
LL | <stripped>.to_string();
|
error: stripping a prefix manually
--> $DIR/manual_strip.rs:31:24
|
LL | str::to_string(&s[prefix.len()..]);
| ^^^^^^^^^^^^^^^^^^
|
note: the prefix was tested here
--> $DIR/manual_strip.rs:30:5
|
LL | if s.starts_with(prefix) {
| ^^^^^^^^^^^^^^^^^^^^^^^^^
help: try using the `strip_prefix` method
|
LL | if let Some(<stripped>) = s.strip_prefix(prefix) {
LL | str::to_string(<stripped>);
|
error: stripping a prefix manually
--> $DIR/manual_strip.rs:37:24
|
LL | str::to_string(&s[PREFIX.len()..]);
| ^^^^^^^^^^^^^^^^^^
|
note: the prefix was tested here
--> $DIR/manual_strip.rs:36:5
|
LL | if s.starts_with(PREFIX) {
| ^^^^^^^^^^^^^^^^^^^^^^^^^
help: try using the `strip_prefix` method
|
LL | if let Some(<stripped>) = s.strip_prefix(PREFIX) {
LL | str::to_string(<stripped>);
LL | str::to_string(<stripped>);
|
error: stripping a prefix manually
--> $DIR/manual_strip.rs:44:24
|
LL | str::to_string(&TARGET[prefix.len()..]);
| ^^^^^^^^^^^^^^^^^^^^^^^
|
note: the prefix was tested here
--> $DIR/manual_strip.rs:43:5
|
LL | if TARGET.starts_with(prefix) {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: try using the `strip_prefix` method
|
LL | if let Some(<stripped>) = TARGET.strip_prefix(prefix) {
LL | str::to_string(<stripped>);
|
error: stripping a prefix manually
--> $DIR/manual_strip.rs:50:9
|
LL | s1[2..].to_uppercase();
| ^^^^^^^
|
note: the prefix was tested here
--> $DIR/manual_strip.rs:49:5
|
LL | if s1.starts_with("ab") {
| ^^^^^^^^^^^^^^^^^^^^^^^^
help: try using the `strip_prefix` method
|
LL | if let Some(<stripped>) = s1.strip_prefix("ab") {
LL | <stripped>.to_uppercase();
|
error: aborting due to 7 previous errors