From d285389d2f48a17d086b6492fc0521059a9dd595 Mon Sep 17 00:00:00 2001 From: llogiq Date: Fri, 28 Aug 2015 20:06:01 +0200 Subject: [PATCH 01/16] new lint: unicode_not_nfc --- Cargo.toml | 3 +++ README.md | 3 ++- src/lib.rs | 1 + src/unicode.rs | 45 ++++++++++++++++++++++++++++++----- tests/compile-fail/unicode.rs | 4 ++-- 5 files changed, 47 insertions(+), 9 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 28fd03e5d6f0..656efd312b26 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -16,6 +16,9 @@ keywords = ["clippy", "lint", "plugin"] name = "clippy" plugin = true +[dependencies] +unicode-normalization = "*" + [dev-dependencies] compiletest_rs = "*" regex = "*" diff --git a/README.md b/README.md index 478bb25272f9..acb9cf06d6a1 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ A collection of lints that give helpful tips to newbies and catch oversights. ##Lints -There are 49 lints included in this crate: +There are 50 lints included in this crate: name | default | meaning -----------------------------------------------------------------------------------------------------|---------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------- @@ -55,6 +55,7 @@ name [string_to_string](https://github.com/Manishearth/rust-clippy/wiki#string_to_string) | warn | calling `String.to_string()` which is a no-op [toplevel_ref_arg](https://github.com/Manishearth/rust-clippy/wiki#toplevel_ref_arg) | warn | a function argument is declared `ref` (i.e. `fn foo(ref x: u8)`, but not `fn foo((ref x, ref y): (u8, u8))`) [type_complexity](https://github.com/Manishearth/rust-clippy/wiki#type_complexity) | warn | usage of very complex types; recommends factoring out parts into `type` definitions +[unicode_not_nfc](https://github.com/Manishearth/rust-clippy/wiki#unicode_not_nfc) | allow | using a unicode literal not in NFC normal form (see http://www.unicode.org/reports/tr15/ for further information) [unit_cmp](https://github.com/Manishearth/rust-clippy/wiki#unit_cmp) | warn | comparing unit values (which is always `true` or `false`, respectively) [zero_width_space](https://github.com/Manishearth/rust-clippy/wiki#zero_width_space) | deny | using a zero-width space in a string literal, which is confusing diff --git a/src/lib.rs b/src/lib.rs index 32fc953d1c32..8c2ebd456e0a 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -130,6 +130,7 @@ pub fn plugin_registrar(reg: &mut Registry) { types::TYPE_COMPLEXITY, types::UNIT_CMP, unicode::NON_ASCII_LITERAL, + unicode::UNICODE_NOT_NFC, unicode::ZERO_WIDTH_SPACE, ]); } diff --git a/src/unicode.rs b/src/unicode.rs index 8a64f612666b..9364d375a368 100644 --- a/src/unicode.rs +++ b/src/unicode.rs @@ -1,6 +1,10 @@ +extern crate unicode_normalization; + use rustc::lint::*; use syntax::ast::*; use syntax::codemap::{BytePos, Span}; +use self::unicode_normalization::char::canonical_combining_class; +use self::unicode_normalization::UnicodeNormalization; use utils::span_lint; @@ -9,13 +13,16 @@ declare_lint!{ pub ZERO_WIDTH_SPACE, Deny, declare_lint!{ pub NON_ASCII_LITERAL, Allow, "using any literal non-ASCII chars in a string literal; suggests \ using the \\u escape instead" } +declare_lint!{ pub UNICODE_NOT_NFC, Allow, + "using a unicode literal not in NFC normal form (see \ + http://www.unicode.org/reports/tr15/ for further information)" } #[derive(Copy, Clone)] pub struct Unicode; impl LintPass for Unicode { fn get_lints(&self) -> LintArray { - lint_array!(ZERO_WIDTH_SPACE, NON_ASCII_LITERAL) + lint_array!(ZERO_WIDTH_SPACE, NON_ASCII_LITERAL, UNICODE_NOT_NFC) } fn check_expr(&mut self, cx: &Context, expr: &Expr) { @@ -28,22 +35,48 @@ impl LintPass for Unicode { } fn check_str(cx: &Context, string: &str, span: Span) { + let mut ustart = None; + let mut last = None; for (i, c) in string.char_indices() { if c == '\u{200B}' { - str_pos_lint(cx, ZERO_WIDTH_SPACE, span, i, + str_pos_lint(cx, ZERO_WIDTH_SPACE, span, i, Some(i), "zero-width space detected. Consider using `\\u{200B}`"); } if c as u32 > 0x7F { - str_pos_lint(cx, NON_ASCII_LITERAL, span, i, &format!( + str_pos_lint(cx, NON_ASCII_LITERAL, span, i, Some(i), &format!( "literal non-ASCII character detected. Consider using `\\u{{{:X}}}`", c as u32)); } + if canonical_combining_class(c) == 0 { // not a combining char + if let Some(l) = last { + let seq = &string[l..i]; + if seq.nfc().zip(seq.chars()).any(|(a, b)| a != b) { + if ustart.is_none() { ustart = last; } + } else { + if let Some(s) = ustart { + str_pos_lint(cx, UNICODE_NOT_NFC, span, s, Some(i), + &format!("non NFC-normal unicode sequence found. \ + Consider using the normal form instead: '{}'", + &string[s..i].nfc().collect::())); + } + ustart = None; + } + } + last = Some(i); + } + } + if let Some(s) = ustart { + str_pos_lint(cx, UNICODE_NOT_NFC, span, s, None, + &format!("non NFC-normal unicode sequence found. \ + Consider using the normal form instead: '{}'", + &string[s..].nfc().collect::())); } } #[allow(cast_possible_truncation)] -fn str_pos_lint(cx: &Context, lint: &'static Lint, span: Span, index: usize, msg: &str) { +fn str_pos_lint(cx: &Context, lint: &'static Lint, span: Span, index: usize, + end_index: Option, msg: &str) { span_lint(cx, lint, Span { lo: span.lo + BytePos((1 + index) as u32), - hi: span.lo + BytePos((1 + index) as u32), + hi: end_index.map_or(span.hi, + |i| span.lo + BytePos((1 + i) as u32)), expn_id: span.expn_id }, msg); - } diff --git a/tests/compile-fail/unicode.rs b/tests/compile-fail/unicode.rs index e4730f60de85..933b1155d01a 100755 --- a/tests/compile-fail/unicode.rs +++ b/tests/compile-fail/unicode.rs @@ -8,9 +8,9 @@ fn zero() { //~^^ ERROR zero-width space detected. Consider using `\u{200B}` } -//#[deny(unicode_canon)] +#[deny(unicode_not_nfc)] fn canon() { - print!("̀ah?"); //not yet ~ERROR non-canonical unicode sequence detected. Consider using à + print!("̀àh?"); //~ERROR non NFC-normal unicode sequence found. } #[deny(non_ascii_literal)] From 5a68fe59fdf93a162ddc957548d2bf41eda39b5c Mon Sep 17 00:00:00 2001 From: llogiq Date: Fri, 28 Aug 2015 14:35:20 +0200 Subject: [PATCH 02/16] rustup, the ExpnInfo stuff changed --- src/utils.rs | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/utils.rs b/src/utils.rs index 69f5e22c48fe..b6fae89fc31a 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -18,13 +18,17 @@ pub const LL_PATH: [&'static str; 3] = ["collections", "linked_list", "Linke pub fn in_macro(cx: &Context, opt_info: Option<&ExpnInfo>) -> bool { // no ExpnInfo = no macro opt_info.map_or(false, |info| { - if info.callee.format == ExpnFormat::CompilerExpansion { - if info.callee.name == "closure expansion" { - return false; - } - } else if info.callee.format == ExpnFormat::MacroAttribute { - // these are all plugins - return true; + match info.callee.format { + ExpnFormat::CompilerExpansion(..) => { + if info.callee.name() == "closure expansion" { + return false; + } + }, + ExpnFormat::MacroAttribute(..) => { + // these are all plugins + return true; + }, + _ => (), } // no span for the callee = external macro info.callee.span.map_or(true, |span| { From 17808da3cae5d5d33ab4a155671fc403853f6cd0 Mon Sep 17 00:00:00 2001 From: Georg Brandl Date: Sun, 30 Aug 2015 09:57:52 +0200 Subject: [PATCH 03/16] lifetimes lint: take "where" clauses into account (fixes #253) If a where clause is present and has lifetimes mentioned, just bail out. --- src/lifetimes.rs | 30 ++++++++++++++++++++++++------ tests/compile-fail/lifetimes.rs | 6 ++++++ 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/src/lifetimes.rs b/src/lifetimes.rs index 660d68535bd5..bff0db14f7bb 100644 --- a/src/lifetimes.rs +++ b/src/lifetimes.rs @@ -20,21 +20,21 @@ impl LintPass for LifetimePass { fn check_item(&mut self, cx: &Context, item: &Item) { if let ItemFn(ref decl, _, _, _, ref generics, _) = item.node { - check_fn_inner(cx, decl, None, &generics.lifetimes, item.span); + check_fn_inner(cx, decl, None, &generics, item.span); } } fn check_impl_item(&mut self, cx: &Context, item: &ImplItem) { if let MethodImplItem(ref sig, _) = item.node { check_fn_inner(cx, &sig.decl, Some(&sig.explicit_self), - &sig.generics.lifetimes, item.span); + &sig.generics, item.span); } } fn check_trait_item(&mut self, cx: &Context, item: &TraitItem) { if let MethodTraitItem(ref sig, _) = item.node { check_fn_inner(cx, &sig.decl, Some(&sig.explicit_self), - &sig.generics.lifetimes, item.span); + &sig.generics, item.span); } } } @@ -49,11 +49,11 @@ enum RefLt { use self::RefLt::*; fn check_fn_inner(cx: &Context, decl: &FnDecl, slf: Option<&ExplicitSelf>, - named_lts: &[LifetimeDef], span: Span) { - if in_external_macro(cx, span) { + generics: &Generics, span: Span) { + if in_external_macro(cx, span) || has_where_lifetimes(&generics.where_clause) { return; } - if could_use_elision(decl, slf, named_lts) { + if could_use_elision(decl, slf, &generics.lifetimes) { span_lint(cx, NEEDLESS_LIFETIMES, span, "explicit lifetimes given in parameter types where they could be elided"); } @@ -182,3 +182,21 @@ impl<'v> Visitor<'v> for RefVisitor { // for lifetime bounds; the default impl calls visit_lifetime_ref fn visit_lifetime_bound(&mut self, _: &'v Lifetime) { } } + +/// Are any lifetimes mentioned in the `where` clause? If yes, we don't try to +/// reason about elision. +fn has_where_lifetimes(where_clause: &WhereClause) -> bool { + let mut where_visitor = RefVisitor(Vec::new()); + for predicate in &where_clause.predicates { + match *predicate { + WherePredicate::RegionPredicate(..) => return true, + WherePredicate::BoundPredicate(ref pred) => { + walk_ty(&mut where_visitor, &pred.bounded_ty); + } + WherePredicate::EqPredicate(ref pred) => { + walk_ty(&mut where_visitor, &pred.ty); + } + } + } + !where_visitor.into_vec().is_empty() +} diff --git a/tests/compile-fail/lifetimes.rs b/tests/compile-fail/lifetimes.rs index a5597e6478f6..ae115efec043 100755 --- a/tests/compile-fail/lifetimes.rs +++ b/tests/compile-fail/lifetimes.rs @@ -31,6 +31,10 @@ fn deep_reference_2<'a>(x: Result<&'a u8, &'a u8>) -> &'a u8 { x.unwrap() } // n fn deep_reference_3<'a>(x: &'a u8, _y: u8) -> Result<&'a u8, ()> { Ok(x) } //~^ERROR explicit lifetimes given +// where clause, but without lifetimes +fn where_clause_without_lt<'a, T>(x: &'a u8, _y: u8) -> Result<&'a u8, ()> where T: Copy { Ok(x) } +//~^ERROR explicit lifetimes given + type Ref<'r> = &'r u8; fn lifetime_param_1<'a>(_x: Ref<'a>, _y: &'a u8) { } // no error, same lifetime on two params @@ -40,6 +44,8 @@ fn lifetime_param_2<'a, 'b>(_x: Ref<'a>, _y: &'b u8) { } fn lifetime_param_3<'a, 'b: 'a>(_x: Ref<'a>, _y: &'b u8) { } // no error, bounded lifetime +fn lifetime_param_4<'a, 'b>(_x: Ref<'a>, _y: &'b u8) where 'b: 'a { } // no error, bounded lifetime + struct X { x: u8, } From 2f4017766e597d7087443d1c9dcedb0961f213fc Mon Sep 17 00:00:00 2001 From: Georg Brandl Date: Sat, 29 Aug 2015 11:41:06 +0200 Subject: [PATCH 04/16] new lint: loop-match-break, which could be while-let (fixes #118) --- README.md | 1 + src/lib.rs | 1 + src/loops.rs | 65 ++++++++++++++++++++++++++++++-- src/matches.rs | 13 +------ src/utils.rs | 10 +++++ tests/compile-fail/while_loop.rs | 52 +++++++++++++++++++++++++ 6 files changed, 128 insertions(+), 14 deletions(-) create mode 100755 tests/compile-fail/while_loop.rs diff --git a/README.md b/README.md index acb9cf06d6a1..e4a2c6f66857 100644 --- a/README.md +++ b/README.md @@ -57,6 +57,7 @@ name [type_complexity](https://github.com/Manishearth/rust-clippy/wiki#type_complexity) | warn | usage of very complex types; recommends factoring out parts into `type` definitions [unicode_not_nfc](https://github.com/Manishearth/rust-clippy/wiki#unicode_not_nfc) | allow | using a unicode literal not in NFC normal form (see http://www.unicode.org/reports/tr15/ for further information) [unit_cmp](https://github.com/Manishearth/rust-clippy/wiki#unit_cmp) | warn | comparing unit values (which is always `true` or `false`, respectively) +[while_let_loop](https://github.com/Manishearth/rust-clippy/wiki#while_let_loop) | warn | `loop { if let { ... } else break }` can be written as a `while let` loop [zero_width_space](https://github.com/Manishearth/rust-clippy/wiki#zero_width_space) | deny | using a zero-width space in a string literal, which is confusing More to come, please [file an issue](https://github.com/Manishearth/rust-clippy/issues) if you have ideas! diff --git a/src/lib.rs b/src/lib.rs index 8c2ebd456e0a..860abd5af197 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -96,6 +96,7 @@ pub fn plugin_registrar(reg: &mut Registry) { loops::EXPLICIT_ITER_LOOP, loops::ITER_NEXT_LOOP, loops::NEEDLESS_RANGE_LOOP, + loops::WHILE_LET_LOOP, matches::MATCH_REF_PATS, matches::SINGLE_MATCH, methods::OPTION_UNWRAP_USED, diff --git a/src/loops.rs b/src/loops.rs index d12393dba688..eba706ed7e15 100644 --- a/src/loops.rs +++ b/src/loops.rs @@ -4,7 +4,8 @@ use syntax::visit::{Visitor, walk_expr}; use rustc::middle::ty; use std::collections::HashSet; -use utils::{snippet, span_lint, get_parent_expr, match_trait_method, match_type, walk_ptrs_ty}; +use utils::{snippet, span_lint, get_parent_expr, match_trait_method, match_type, walk_ptrs_ty, + in_external_macro, expr_block, span_help_and_lint}; use utils::{VEC_PATH, LL_PATH}; declare_lint!{ pub NEEDLESS_RANGE_LOOP, Warn, @@ -16,12 +17,16 @@ declare_lint!{ pub EXPLICIT_ITER_LOOP, Warn, declare_lint!{ pub ITER_NEXT_LOOP, Warn, "for-looping over `_.next()` which is probably not intended" } +declare_lint!{ pub WHILE_LET_LOOP, Warn, + "`loop { if let { ... } else break }` can be written as a `while let` loop" } + #[derive(Copy, Clone)] pub struct LoopsPass; impl LintPass for LoopsPass { fn get_lints(&self) -> LintArray { - lint_array!(NEEDLESS_RANGE_LOOP, EXPLICIT_ITER_LOOP, ITER_NEXT_LOOP) + lint_array!(NEEDLESS_RANGE_LOOP, EXPLICIT_ITER_LOOP, ITER_NEXT_LOOP, + WHILE_LET_LOOP) } fn check_expr(&mut self, cx: &Context, expr: &Expr) { @@ -36,7 +41,8 @@ impl LintPass for LoopsPass { walk_expr(&mut visitor, body); // linting condition: we only indexed one variable if visitor.indexed.len() == 1 { - let indexed = visitor.indexed.into_iter().next().expect("Len was nonzero, but no contents found"); + let indexed = visitor.indexed.into_iter().next().expect( + "Len was nonzero, but no contents found"); if visitor.nonindex { span_lint(cx, NEEDLESS_RANGE_LOOP, expr.span, &format!( "the loop variable `{}` is used to index `{}`. Consider using \ @@ -77,6 +83,34 @@ impl LintPass for LoopsPass { } } } + // check for `loop { if let {} else break }` that could be `while let` + // (also matches explicit "match" instead of "if let") + if let ExprLoop(ref block, _) = expr.node { + // extract a single expression + if let Some(inner) = extract_single_expr(block) { + if let ExprMatch(ref matchexpr, ref arms, ref source) = inner.node { + // ensure "if let" compatible match structure + match *source { + MatchSource::Normal | MatchSource::IfLetDesugar{..} => if + arms.len() == 2 && + arms[0].pats.len() == 1 && arms[0].guard.is_none() && + arms[1].pats.len() == 1 && arms[1].guard.is_none() && + // finally, check for "break" in the second clause + is_break_expr(&arms[1].body) + { + if in_external_macro(cx, expr.span) { return; } + span_help_and_lint(cx, WHILE_LET_LOOP, expr.span, + "this loop could be written as a `while let` loop", + &format!("try\nwhile let {} = {} {}", + snippet(cx, arms[0].pats[0].span, ".."), + snippet(cx, matchexpr.span, ".."), + expr_block(cx, &arms[0].body, ".."))); + }, + _ => () + } + } + } + } } } @@ -158,3 +192,28 @@ fn is_array(ty: ty::Ty) -> bool { _ => false } } + +/// If block consists of a single expression (with or without semicolon), return it. +fn extract_single_expr(block: &Block) -> Option<&Expr> { + match (&block.stmts.len(), &block.expr) { + (&1, &None) => match block.stmts[0].node { + StmtExpr(ref expr, _) | + StmtSemi(ref expr, _) => Some(expr), + _ => None, + }, + (&0, &Some(ref expr)) => Some(expr), + _ => None + } +} + +/// Return true if expr contains a single break expr (maybe within a block). +fn is_break_expr(expr: &Expr) -> bool { + match expr.node { + ExprBreak(None) => true, + ExprBlock(ref b) => match extract_single_expr(b) { + Some(ref subexpr) => is_break_expr(subexpr), + None => false, + }, + _ => false, + } +} diff --git a/src/matches.rs b/src/matches.rs index 1afb61e9b9e6..3d04c9210ce6 100644 --- a/src/matches.rs +++ b/src/matches.rs @@ -1,10 +1,8 @@ use rustc::lint::*; use syntax::ast; use syntax::ast::*; -use std::borrow::Cow; -use utils::{snippet, snippet_block}; -use utils::{span_lint, span_help_and_lint, in_external_macro}; +use utils::{snippet, span_lint, span_help_and_lint, in_external_macro, expr_block}; declare_lint!(pub SINGLE_MATCH, Warn, "a match statement with a single nontrivial arm (i.e, where the other arm \ @@ -38,12 +36,6 @@ impl LintPass for MatchPass { is_unit_expr(&arms[1].body) { if in_external_macro(cx, expr.span) {return;} - let body_code = snippet_block(cx, arms[0].body.span, ".."); - let body_code = if let ExprBlock(_) = arms[0].body.node { - body_code - } else { - Cow::Owned(format!("{{ {} }}", body_code)) - }; span_help_and_lint(cx, SINGLE_MATCH, expr.span, "you seem to be trying to use match for \ destructuring a single pattern. Did you mean to \ @@ -51,8 +43,7 @@ impl LintPass for MatchPass { &format!("try\nif let {} = {} {}", snippet(cx, arms[0].pats[0].span, ".."), snippet(cx, ex.span, ".."), - body_code) - ); + expr_block(cx, &arms[0].body, ".."))); } // check preconditions for MATCH_REF_PATS diff --git a/src/utils.rs b/src/utils.rs index b6fae89fc31a..f16387f606db 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -103,6 +103,16 @@ pub fn snippet_block<'a>(cx: &Context, span: Span, default: &'a str) -> Cow<'a, trim_multiline(snip, true) } +/// Like snippet_block, but add braces if the expr is not an ExprBlock +pub fn expr_block<'a>(cx: &Context, expr: &Expr, default: &'a str) -> Cow<'a, str> { + let code = snippet_block(cx, expr.span, default); + if let ExprBlock(_) = expr.node { + code + } else { + Cow::Owned(format!("{{ {} }}", code)) + } +} + /// Trim indentation from a multiline string /// with possibility of ignoring the first line pub fn trim_multiline(s: Cow, ignore_first: bool) -> Cow { diff --git a/tests/compile-fail/while_loop.rs b/tests/compile-fail/while_loop.rs new file mode 100755 index 000000000000..bc09168fad03 --- /dev/null +++ b/tests/compile-fail/while_loop.rs @@ -0,0 +1,52 @@ +#![feature(plugin)] +#![plugin(clippy)] + +#[deny(while_let_loop)] +fn main() { + let y = Some(true); + loop { //~ERROR + if let Some(_x) = y { + let _v = 1; + } else { + break; + } + } + loop { //~ERROR + if let Some(_x) = y { + let _v = 1; + } else { + break + } + } + loop { // no error, break is not in else clause + if let Some(_x) = y { + let _v = 1; + } + break; + } + loop { //~ERROR + match y { + Some(_x) => true, + None => break + }; + } + loop { // no error, match is not the only statement + match y { + Some(_x) => true, + None => break + }; + let _x = 1; + } + loop { // no error, else branch does something other than break + match y { + Some(_x) => true, + _ => { + let _z = 1; + break; + } + }; + } + while let Some(x) = y { // no error, obviously + println!("{}", x); + } +} From 5817765e46cd790f480f1e63ffe55538156cb9d3 Mon Sep 17 00:00:00 2001 From: Georg Brandl Date: Sun, 30 Aug 2015 13:10:59 +0200 Subject: [PATCH 05/16] new lint: using collect() to just exhaust an iterator Should use a for loop instead. --- README.md | 3 ++- src/lib.rs | 1 + src/loops.rs | 20 +++++++++++++++++++- tests/compile-fail/for_loop.rs | 5 +++++ 4 files changed, 27 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index e4a2c6f66857..4ab7a119215e 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ A collection of lints that give helpful tips to newbies and catch oversights. ##Lints -There are 50 lints included in this crate: +There are 51 lints included in this crate: name | default | meaning -----------------------------------------------------------------------------------------------------|---------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------- @@ -57,6 +57,7 @@ name [type_complexity](https://github.com/Manishearth/rust-clippy/wiki#type_complexity) | warn | usage of very complex types; recommends factoring out parts into `type` definitions [unicode_not_nfc](https://github.com/Manishearth/rust-clippy/wiki#unicode_not_nfc) | allow | using a unicode literal not in NFC normal form (see http://www.unicode.org/reports/tr15/ for further information) [unit_cmp](https://github.com/Manishearth/rust-clippy/wiki#unit_cmp) | warn | comparing unit values (which is always `true` or `false`, respectively) +[unused_collect](https://github.com/Manishearth/rust-clippy/wiki#unused_collect) | warn | `collect()`ing an iterator without using the result; this is usually better written as a for loop [while_let_loop](https://github.com/Manishearth/rust-clippy/wiki#while_let_loop) | warn | `loop { if let { ... } else break }` can be written as a `while let` loop [zero_width_space](https://github.com/Manishearth/rust-clippy/wiki#zero_width_space) | deny | using a zero-width space in a string literal, which is confusing diff --git a/src/lib.rs b/src/lib.rs index 860abd5af197..410941e3943f 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -96,6 +96,7 @@ pub fn plugin_registrar(reg: &mut Registry) { loops::EXPLICIT_ITER_LOOP, loops::ITER_NEXT_LOOP, loops::NEEDLESS_RANGE_LOOP, + loops::UNUSED_COLLECT, loops::WHILE_LET_LOOP, matches::MATCH_REF_PATS, matches::SINGLE_MATCH, diff --git a/src/loops.rs b/src/loops.rs index eba706ed7e15..fe901794d6cf 100644 --- a/src/loops.rs +++ b/src/loops.rs @@ -20,13 +20,17 @@ declare_lint!{ pub ITER_NEXT_LOOP, Warn, declare_lint!{ pub WHILE_LET_LOOP, Warn, "`loop { if let { ... } else break }` can be written as a `while let` loop" } +declare_lint!{ pub UNUSED_COLLECT, Warn, + "`collect()`ing an iterator without using the result; this is usually better \ + written as a for loop" } + #[derive(Copy, Clone)] pub struct LoopsPass; impl LintPass for LoopsPass { fn get_lints(&self) -> LintArray { lint_array!(NEEDLESS_RANGE_LOOP, EXPLICIT_ITER_LOOP, ITER_NEXT_LOOP, - WHILE_LET_LOOP) + WHILE_LET_LOOP, UNUSED_COLLECT) } fn check_expr(&mut self, cx: &Context, expr: &Expr) { @@ -112,6 +116,20 @@ impl LintPass for LoopsPass { } } } + + fn check_stmt(&mut self, cx: &Context, stmt: &Stmt) { + if let StmtSemi(ref expr, _) = stmt.node { + if let ExprMethodCall(ref method, _, ref args) = expr.node { + if args.len() == 1 && method.node.name == "collect" { + if match_trait_method(cx, expr, &["core", "iter", "Iterator"]) { + span_lint(cx, UNUSED_COLLECT, expr.span, &format!( + "you are collect()ing an iterator and throwing away the result. \ + Consider using an explicit for loop to exhaust the iterator")); + } + } + } + } + } } /// Recover the essential nodes of a desugared for loop: diff --git a/tests/compile-fail/for_loop.rs b/tests/compile-fail/for_loop.rs index eb7667b7fbd8..668386513561 100755 --- a/tests/compile-fail/for_loop.rs +++ b/tests/compile-fail/for_loop.rs @@ -15,6 +15,7 @@ impl Unrelated { } #[deny(needless_range_loop, explicit_iter_loop, iter_next_loop)] +#[deny(unused_collect)] #[allow(linkedlist)] fn main() { let mut vec = vec![1, 2, 3, 4]; @@ -56,4 +57,8 @@ fn main() { let u = Unrelated(vec![]); for _v in u.next() { } // no error for _v in u.iter() { } // no error + + let mut out = vec![]; + vec.iter().map(|x| out.push(x)).collect::>(); //~ERROR you are collect()ing an iterator + let _y = vec.iter().map(|x| out.push(x)).collect::>(); // this is fine } From 59ba2bbae625715a2ba498cd1068f6b47106465b Mon Sep 17 00:00:00 2001 From: Tim Neumann Date: Sun, 30 Aug 2015 17:32:35 +0200 Subject: [PATCH 06/16] add precedence_negative_literal lint --- README.md | 2 +- src/lib.rs | 5 ++- src/misc.rs | 44 --------------------- src/precedence.rs | 65 ++++++++++++++++++++++++++++++++ tests/compile-fail/precedence.rs | 10 +++++ 5 files changed, 79 insertions(+), 47 deletions(-) create mode 100644 src/precedence.rs diff --git a/README.md b/README.md index 4ab7a119215e..8cd3d11c5f68 100644 --- a/README.md +++ b/README.md @@ -39,7 +39,7 @@ name [needless_return](https://github.com/Manishearth/rust-clippy/wiki#needless_return) | warn | using a return statement like `return expr;` where an expression would suffice [non_ascii_literal](https://github.com/Manishearth/rust-clippy/wiki#non_ascii_literal) | allow | using any literal non-ASCII chars in a string literal; suggests using the \\u escape instead [option_unwrap_used](https://github.com/Manishearth/rust-clippy/wiki#option_unwrap_used) | allow | using `Option.unwrap()`, which should at least get a better message using `expect()` -[precedence](https://github.com/Manishearth/rust-clippy/wiki#precedence) | warn | expressions where precedence may trip up the unwary reader of the source; suggests adding parentheses, e.g. `x << 2 + y` will be parsed as `x << (2 + y)` +[precedence](https://github.com/Manishearth/rust-clippy/wiki#precedence) | warn | catches operations where precedence may be unclear. See the wiki for a list of cases caught [ptr_arg](https://github.com/Manishearth/rust-clippy/wiki#ptr_arg) | allow | fn arguments of the type `&Vec<...>` or `&String`, suggesting to use `&[...]` or `&str` instead, respectively [range_step_by_zero](https://github.com/Manishearth/rust-clippy/wiki#range_step_by_zero) | warn | using Range::step_by(0), which produces an infinite iterator [redundant_closure](https://github.com/Manishearth/rust-clippy/wiki#redundant_closure) | warn | using redundant closures, i.e. `|a| foo(a)` (which can be written as just `foo`) diff --git a/src/lib.rs b/src/lib.rs index 410941e3943f..89308593cf8e 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -40,6 +40,7 @@ pub mod lifetimes; pub mod loops; pub mod ranges; pub mod matches; +pub mod precedence; #[plugin_registrar] pub fn plugin_registrar(reg: &mut Registry) { @@ -52,7 +53,7 @@ pub fn plugin_registrar(reg: &mut Registry) { reg.register_lint_pass(box needless_bool::NeedlessBool as LintPassObject); reg.register_lint_pass(box approx_const::ApproxConstant as LintPassObject); reg.register_lint_pass(box misc::FloatCmp as LintPassObject); - reg.register_lint_pass(box misc::Precedence as LintPassObject); + reg.register_lint_pass(box precedence::Precedence as LintPassObject); reg.register_lint_pass(box eta_reduction::EtaPass as LintPassObject); reg.register_lint_pass(box identity_op::IdentityOp as LintPassObject); reg.register_lint_pass(box mut_mut::MutMut as LintPassObject); @@ -109,10 +110,10 @@ pub fn plugin_registrar(reg: &mut Registry) { misc::CMP_OWNED, misc::FLOAT_CMP, misc::MODULO_ONE, - misc::PRECEDENCE, misc::TOPLEVEL_REF_ARG, mut_mut::MUT_MUT, needless_bool::NEEDLESS_BOOL, + precedence::PRECEDENCE, ptr_arg::PTR_ARG, ranges::RANGE_STEP_BY_ZERO, returns::LET_AND_RETURN, diff --git a/src/misc.rs b/src/misc.rs index 6e4384072169..ef9f4248c0c6 100644 --- a/src/misc.rs +++ b/src/misc.rs @@ -109,50 +109,6 @@ fn is_float(cx: &Context, expr: &Expr) -> bool { } } -declare_lint!(pub PRECEDENCE, Warn, - "expressions where precedence may trip up the unwary reader of the source; \ - suggests adding parentheses, e.g. `x << 2 + y` will be parsed as `x << (2 + y)`"); - -#[derive(Copy,Clone)] -pub struct Precedence; - -impl LintPass for Precedence { - fn get_lints(&self) -> LintArray { - lint_array!(PRECEDENCE) - } - - fn check_expr(&mut self, cx: &Context, expr: &Expr) { - if let ExprBinary(Spanned { node: op, ..}, ref left, ref right) = expr.node { - if is_bit_op(op) && (is_arith_expr(left) || is_arith_expr(right)) { - span_lint(cx, PRECEDENCE, expr.span, - "operator precedence can trip the unwary. Consider adding parentheses \ - to the subexpression"); - } - } - } -} - -fn is_arith_expr(expr : &Expr) -> bool { - match expr.node { - ExprBinary(Spanned { node: op, ..}, _, _) => is_arith_op(op), - _ => false - } -} - -fn is_bit_op(op : BinOp_) -> bool { - match op { - BiBitXor | BiBitAnd | BiBitOr | BiShl | BiShr => true, - _ => false - } -} - -fn is_arith_op(op : BinOp_) -> bool { - match op { - BiAdd | BiSub | BiMul | BiDiv | BiRem => true, - _ => false - } -} - declare_lint!(pub CMP_OWNED, Warn, "creating owned instances for comparing with others, e.g. `x == \"foo\".to_string()`"); diff --git a/src/precedence.rs b/src/precedence.rs new file mode 100644 index 000000000000..1d89adf9df87 --- /dev/null +++ b/src/precedence.rs @@ -0,0 +1,65 @@ +use rustc::lint::*; +use syntax::ast::*; +use syntax::codemap::Spanned; + +use utils::span_lint; + +declare_lint!(pub PRECEDENCE, Warn, + "catches operations where precedence may be unclear. See the wiki for a \ + list of cases caught"); + +#[derive(Copy,Clone)] +pub struct Precedence; + +impl LintPass for Precedence { + fn get_lints(&self) -> LintArray { + lint_array!(PRECEDENCE) + } + + fn check_expr(&mut self, cx: &Context, expr: &Expr) { + if let ExprBinary(Spanned { node: op, ..}, ref left, ref right) = expr.node { + if is_bit_op(op) && (is_arith_expr(left) || is_arith_expr(right)) { + span_lint(cx, PRECEDENCE, expr.span, + "operator precedence can trip the unwary. Consider adding parentheses \ + to the subexpression"); + } + } + + if let ExprUnary(UnNeg, ref rhs) = expr.node { + if let ExprMethodCall(_, _, ref args) = rhs.node { + if let Some(slf) = args.first() { + if let ExprLit(ref lit) = slf.node { + match lit.node { + LitInt(..) | LitFloat(..) | LitFloatUnsuffixed(..) => + span_lint(cx, PRECEDENCE, expr.span, + "unary minus has lower precedence than method call. Consider \ + adding parentheses to clarify your intent"), + _ => () + } + } + } + } + } + } +} + +fn is_arith_expr(expr : &Expr) -> bool { + match expr.node { + ExprBinary(Spanned { node: op, ..}, _, _) => is_arith_op(op), + _ => false + } +} + +fn is_bit_op(op : BinOp_) -> bool { + match op { + BiBitXor | BiBitAnd | BiBitOr | BiShl | BiShr => true, + _ => false + } +} + +fn is_arith_op(op : BinOp_) -> bool { + match op { + BiAdd | BiSub | BiMul | BiDiv | BiRem => true, + _ => false + } +} diff --git a/tests/compile-fail/precedence.rs b/tests/compile-fail/precedence.rs index ebb25f61b758..71dcd4930084 100755 --- a/tests/compile-fail/precedence.rs +++ b/tests/compile-fail/precedence.rs @@ -13,4 +13,14 @@ fn main() { format!("{} vs. {}", 3 | 2 - 1, (3 | 2) - 1); //~ERROR operator precedence can trip format!("{} vs. {}", 3 & 5 - 2, (3 & 5) - 2); //~ERROR operator precedence can trip + format!("{} vs. {}", -1i32.abs(), (-1i32).abs()); //~ERROR unary minus has lower precedence + format!("{} vs. {}", -1f32.abs(), (-1f32).abs()); //~ERROR unary minus has lower precedence + + // These should not trigger an error + let _ = (-1i32).abs(); + let _ = (-1f32).abs(); + let _ = -(1i32).abs(); + let _ = -(1f32).abs(); + let _ = -(1i32.abs()); + let _ = -(1f32.abs()); } From bd50565e05ebf0ea1fe1ed7fccc361571a95344e Mon Sep 17 00:00:00 2001 From: llogiq Date: Fri, 28 Aug 2015 20:06:01 +0200 Subject: [PATCH 07/16] new lint: unicode_not_nfc From fb438b52258bb1f5e6f89cdb23f681fe6878cc5e Mon Sep 17 00:00:00 2001 From: llogiq Date: Fri, 28 Aug 2015 20:06:01 +0200 Subject: [PATCH 08/16] new lint: unicode_not_nfc --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 8cd3d11c5f68..21096db43d94 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ A collection of lints that give helpful tips to newbies and catch oversights. ##Lints -There are 51 lints included in this crate: +There are 52 lints included in this crate: name | default | meaning -----------------------------------------------------------------------------------------------------|---------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------- From baaa1dccbb6520505f844c5de8a7ea9058f5d98a Mon Sep 17 00:00:00 2001 From: llogiq Date: Mon, 31 Aug 2015 01:21:09 +0200 Subject: [PATCH 09/16] README updated --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 6cc1c49b8bc8..21096db43d94 100644 --- a/README.md +++ b/README.md @@ -39,7 +39,7 @@ name [needless_return](https://github.com/Manishearth/rust-clippy/wiki#needless_return) | warn | using a return statement like `return expr;` where an expression would suffice [non_ascii_literal](https://github.com/Manishearth/rust-clippy/wiki#non_ascii_literal) | allow | using any literal non-ASCII chars in a string literal; suggests using the \\u escape instead [option_unwrap_used](https://github.com/Manishearth/rust-clippy/wiki#option_unwrap_used) | allow | using `Option.unwrap()`, which should at least get a better message using `expect()` -[precedence](https://github.com/Manishearth/rust-clippy/wiki#precedence) | warn | expressions where precedence may trip up the unwary reader of the source; suggests adding parentheses, e.g. `x << 2 + y` will be parsed as `x << (2 + y)` +[precedence](https://github.com/Manishearth/rust-clippy/wiki#precedence) | warn | catches operations where precedence may be unclear. See the wiki for a list of cases caught [ptr_arg](https://github.com/Manishearth/rust-clippy/wiki#ptr_arg) | allow | fn arguments of the type `&Vec<...>` or `&String`, suggesting to use `&[...]` or `&str` instead, respectively [range_step_by_zero](https://github.com/Manishearth/rust-clippy/wiki#range_step_by_zero) | warn | using Range::step_by(0), which produces an infinite iterator [redundant_closure](https://github.com/Manishearth/rust-clippy/wiki#redundant_closure) | warn | using redundant closures, i.e. `|a| foo(a)` (which can be written as just `foo`) From e00acb483e59a70c5830fb252d45591871b23cdb Mon Sep 17 00:00:00 2001 From: Georg Brandl Date: Mon, 31 Aug 2015 11:11:51 +0200 Subject: [PATCH 10/16] lifetimes lint: walk type bounds as well as types (fixes #253, again) --- src/lifetimes.rs | 48 ++++++++++++++++++++++++--------- tests/compile-fail/lifetimes.rs | 12 +++++++++ 2 files changed, 47 insertions(+), 13 deletions(-) diff --git a/src/lifetimes.rs b/src/lifetimes.rs index bff0db14f7bb..de7a39fdb3b6 100644 --- a/src/lifetimes.rs +++ b/src/lifetimes.rs @@ -1,7 +1,7 @@ use syntax::ast::*; use rustc::lint::*; use syntax::codemap::Span; -use syntax::visit::{Visitor, walk_ty}; +use syntax::visit::{Visitor, walk_ty, walk_ty_param_bound}; use std::collections::HashSet; use utils::{in_external_macro, span_lint}; @@ -68,14 +68,7 @@ fn could_use_elision(func: &FnDecl, slf: Option<&ExplicitSelf>, // level of the current item. // check named LTs - let mut allowed_lts = HashSet::new(); - for lt in named_lts { - if lt.bounds.is_empty() { - allowed_lts.insert(Named(lt.lifetime.name)); - } - } - allowed_lts.insert(Unnamed); - allowed_lts.insert(Static); + let allowed_lts = allowed_lts_from(named_lts); // these will collect all the lifetimes for references in arg/return types let mut input_visitor = RefVisitor(Vec::new()); @@ -142,6 +135,18 @@ fn could_use_elision(func: &FnDecl, slf: Option<&ExplicitSelf>, false } +fn allowed_lts_from(named_lts: &[LifetimeDef]) -> HashSet { + let mut allowed_lts = HashSet::new(); + for lt in named_lts { + if lt.bounds.is_empty() { + allowed_lts.insert(Named(lt.lifetime.name)); + } + } + allowed_lts.insert(Unnamed); + allowed_lts.insert(Static); + allowed_lts +} + /// Number of unique lifetimes in the given vector. fn unique_lifetimes(lts: &[RefLt]) -> usize { lts.iter().collect::>().len() @@ -186,17 +191,34 @@ impl<'v> Visitor<'v> for RefVisitor { /// Are any lifetimes mentioned in the `where` clause? If yes, we don't try to /// reason about elision. fn has_where_lifetimes(where_clause: &WhereClause) -> bool { - let mut where_visitor = RefVisitor(Vec::new()); for predicate in &where_clause.predicates { match *predicate { WherePredicate::RegionPredicate(..) => return true, WherePredicate::BoundPredicate(ref pred) => { - walk_ty(&mut where_visitor, &pred.bounded_ty); + // a predicate like F: Trait or F: for<'a> Trait<'a> + let mut visitor = RefVisitor(Vec::new()); + // walk the type F, it may not contain LT refs + walk_ty(&mut visitor, &pred.bounded_ty); + if !visitor.0.is_empty() { return true; } + // if the bounds define new lifetimes, they are fine to occur + let allowed_lts = allowed_lts_from(&pred.bound_lifetimes); + // now walk the bounds + for bound in pred.bounds.iter() { + walk_ty_param_bound(&mut visitor, bound); + } + // and check that all lifetimes are allowed + for lt in visitor.into_vec() { + if !allowed_lts.contains(<) { + return true; + } + } } WherePredicate::EqPredicate(ref pred) => { - walk_ty(&mut where_visitor, &pred.ty); + let mut visitor = RefVisitor(Vec::new()); + walk_ty(&mut visitor, &pred.ty); + if !visitor.0.is_empty() { return true; } } } } - !where_visitor.into_vec().is_empty() + false } diff --git a/tests/compile-fail/lifetimes.rs b/tests/compile-fail/lifetimes.rs index ae115efec043..0b24ca65241e 100755 --- a/tests/compile-fail/lifetimes.rs +++ b/tests/compile-fail/lifetimes.rs @@ -46,6 +46,18 @@ fn lifetime_param_3<'a, 'b: 'a>(_x: Ref<'a>, _y: &'b u8) { } // no error, bounde fn lifetime_param_4<'a, 'b>(_x: Ref<'a>, _y: &'b u8) where 'b: 'a { } // no error, bounded lifetime +struct Lt<'a, I: 'static> { + x: &'a I +} + +fn fn_bound<'a, F, I>(_m: Lt<'a, I>, _f: F) -> Lt<'a, I> + where F: Fn(Lt<'a, I>) -> Lt<'a, I> // no error, fn bound references 'a +{ unreachable!() } + +fn fn_bound_2<'a, F, I>(_m: Lt<'a, I>, _f: F) -> Lt<'a, I> //~ERROR explicit lifetimes given + where for<'x> F: Fn(Lt<'x, I>) -> Lt<'x, I> +{ unreachable!() } + struct X { x: u8, } From fba6ceb67e9b7c4212bc41fe00ce563b9dfec21b Mon Sep 17 00:00:00 2001 From: Georg Brandl Date: Sun, 30 Aug 2015 19:02:30 +0200 Subject: [PATCH 11/16] new lint: unnecessary patterns (x@_ -> x) --- README.md | 1 + src/lib.rs | 2 ++ src/misc.rs | 21 +++++++++++++++++++++ tests/compile-fail/patterns.rs | 16 ++++++++++++++++ 4 files changed, 40 insertions(+) create mode 100755 tests/compile-fail/patterns.rs diff --git a/README.md b/README.md index 21096db43d94..f765aa62e348 100644 --- a/README.md +++ b/README.md @@ -43,6 +43,7 @@ name [ptr_arg](https://github.com/Manishearth/rust-clippy/wiki#ptr_arg) | allow | fn arguments of the type `&Vec<...>` or `&String`, suggesting to use `&[...]` or `&str` instead, respectively [range_step_by_zero](https://github.com/Manishearth/rust-clippy/wiki#range_step_by_zero) | warn | using Range::step_by(0), which produces an infinite iterator [redundant_closure](https://github.com/Manishearth/rust-clippy/wiki#redundant_closure) | warn | using redundant closures, i.e. `|a| foo(a)` (which can be written as just `foo`) +[redundant_pattern](https://github.com/Manishearth/rust-clippy/wiki#redundant_pattern) | warn | using `name @ _` in a pattern [result_unwrap_used](https://github.com/Manishearth/rust-clippy/wiki#result_unwrap_used) | allow | using `Result.unwrap()`, which might be better handled [shadow_reuse](https://github.com/Manishearth/rust-clippy/wiki#shadow_reuse) | allow | rebinding a name to an expression that re-uses the original value, e.g. `let x = x + 1` [shadow_same](https://github.com/Manishearth/rust-clippy/wiki#shadow_same) | allow | rebinding a name to itself, e.g. `let mut x = &mut x` diff --git a/src/lib.rs b/src/lib.rs index 89308593cf8e..bf22b0fb1883 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -75,6 +75,7 @@ pub fn plugin_registrar(reg: &mut Registry) { reg.register_lint_pass(box types::CastPass as LintPassObject); reg.register_lint_pass(box types::TypeComplexityPass as LintPassObject); reg.register_lint_pass(box matches::MatchPass as LintPassObject); + reg.register_lint_pass(box misc::PatternPass as LintPassObject); reg.register_lint_group("shadow", vec![ shadow::SHADOW_REUSE, @@ -110,6 +111,7 @@ pub fn plugin_registrar(reg: &mut Registry) { misc::CMP_OWNED, misc::FLOAT_CMP, misc::MODULO_ONE, + misc::REDUNDANT_PATTERN misc::TOPLEVEL_REF_ARG, mut_mut::MUT_MUT, needless_bool::NEEDLESS_BOOL, diff --git a/src/misc.rs b/src/misc.rs index ef9f4248c0c6..26493080d873 100644 --- a/src/misc.rs +++ b/src/misc.rs @@ -191,3 +191,24 @@ fn is_lit_one(expr: &Expr) -> bool { } false } + +declare_lint!(pub REDUNDANT_PATTERN, Warn, "using `name @ _` in a pattern"); + +#[derive(Copy,Clone)] +pub struct PatternPass; + +impl LintPass for PatternPass { + fn get_lints(&self) -> LintArray { + lint_array!(REDUNDANT_PATTERN) + } + + fn check_pat(&mut self, cx: &Context, pat: &Pat) { + if let PatIdent(_, ref ident, Some(ref right)) = pat.node { + if right.node == PatWild(PatWildSingle) { + cx.span_lint(REDUNDANT_PATTERN, pat.span, &format!( + "the `{} @ _` pattern can be written as just `{}`", + ident.node.name, ident.node.name)); + } + } + } +} diff --git a/tests/compile-fail/patterns.rs b/tests/compile-fail/patterns.rs new file mode 100755 index 000000000000..62bd2c43cc14 --- /dev/null +++ b/tests/compile-fail/patterns.rs @@ -0,0 +1,16 @@ +#![feature(plugin)] +#![plugin(clippy)] +#![allow(unused)] +#![deny(clippy)] + +fn main() { + let v = Some(true); + match v { + Some(x) => (), + y @ _ => (), //~ERROR the `y @ _` pattern can be written as just `y` + } + match v { + Some(x) => (), + y @ None => (), // no error + } +} From 62dd71655ed8c09412b8b7895d1220b0cd043629 Mon Sep 17 00:00:00 2001 From: llogiq Date: Tue, 1 Sep 2015 19:58:46 +0200 Subject: [PATCH 12/16] changed output to allow multiple ranges --- README.md | 2 +- src/lib.rs | 2 +- src/unicode.rs | 142 +++++++++++++++++++++++++++------- tests/compile-fail/unicode.rs | 8 +- 4 files changed, 118 insertions(+), 36 deletions(-) diff --git a/README.md b/README.md index f765aa62e348..a3de269cd9ce 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ A collection of lints that give helpful tips to newbies and catch oversights. ##Lints -There are 52 lints included in this crate: +There are 53 lints included in this crate: name | default | meaning -----------------------------------------------------------------------------------------------------|---------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------- diff --git a/src/lib.rs b/src/lib.rs index bf22b0fb1883..af0144142f32 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -111,7 +111,7 @@ pub fn plugin_registrar(reg: &mut Registry) { misc::CMP_OWNED, misc::FLOAT_CMP, misc::MODULO_ONE, - misc::REDUNDANT_PATTERN + misc::REDUNDANT_PATTERN, misc::TOPLEVEL_REF_ARG, mut_mut::MUT_MUT, needless_bool::NEEDLESS_BOOL, diff --git a/src/unicode.rs b/src/unicode.rs index 9364d375a368..8e3f798408f4 100644 --- a/src/unicode.rs +++ b/src/unicode.rs @@ -1,8 +1,9 @@ extern crate unicode_normalization; +use std::fmt::Write; use rustc::lint::*; use syntax::ast::*; -use syntax::codemap::{BytePos, Span}; +use syntax::codemap::{Pos, BytePos, CharPos, Span}; use self::unicode_normalization::char::canonical_combining_class; use self::unicode_normalization::UnicodeNormalization; @@ -34,49 +35,132 @@ impl LintPass for Unicode { } } +fn pos(base: BytePos, i: usize) -> BytePos { + if i == 0 { base } else { base + BytePos::from_usize(i + 1) } +} + +#[allow(cast_possible_truncation)] +fn str_pos_lint(cx: &Context, lint: &'static Lint, span: Span, index: usize, + end_index: Option, msg: &str) { + + span_lint(cx, lint, + Span { + lo: pos(span.lo, index), + hi: end_index.map_or(span.hi, |i| pos(span.lo, i)), + expn_id: span.expn_id, + }, + msg); +} + + +fn push_start(from: &mut Option, til: Option, + v: &mut Vec<(usize, Option)>) { + if let Some(s) = from.take() { + v.push((s, til)); + } +} + +fn push_last_and_report(cx: &Context, string: &str, span: Span, + mut from: Option, mut ranges: Vec<(usize, Option)>, + lint: &'static Lint, prefix: &str, multi_fun: F) +where F: Fn(&str) -> String, { + push_start(&mut from, None, &mut ranges); + match ranges.len() { + 0 => (), + 1 => { + let range = ranges[0]; + str_pos_lint(cx, lint, span, range.0, range.1, &format!( + "{} range detected. Consider using `{}`", + prefix, + &if let Some(u) = range.1 { + multi_fun(&string[range.0 .. u]) + } else { + multi_fun(&string[range.0 ..]) + } + )); + }, + x => { + let mut repls = String::new(); + for (from, until) in ranges { + if let Some(u) = until { + write!(&mut repls, "\n{}..{} => {}", + from, u, &multi_fun(&string[from..u])).unwrap(); + } else { + write!(&mut repls, "\n{}.. => {}", + from, &multi_fun(&string[from..])).unwrap(); + } + } + span_lint(cx, lint, span, &format!( + "{} {} ranges detected. Consider the following replacements:{}", + x, prefix, &repls)); + } + } +} + fn check_str(cx: &Context, string: &str, span: Span) { - let mut ustart = None; - let mut last = None; + let mut zero_width_ranges = vec![]; + let mut non_ascii_ranges = vec![]; + let mut non_nfc_ranges = vec![]; + let mut zero_width_start = None; + let mut non_ascii_start = None; + let mut non_nfc_start = None; + let mut last_base_char = None; for (i, c) in string.char_indices() { if c == '\u{200B}' { - str_pos_lint(cx, ZERO_WIDTH_SPACE, span, i, Some(i), - "zero-width space detected. Consider using `\\u{200B}`"); + if zero_width_start.is_none() { + zero_width_start = Some(i); + } + } else { + push_start(&mut zero_width_start, Some(i), &mut zero_width_ranges); } if c as u32 > 0x7F { - str_pos_lint(cx, NON_ASCII_LITERAL, span, i, Some(i), &format!( - "literal non-ASCII character detected. Consider using `\\u{{{:X}}}`", c as u32)); + if non_ascii_start.is_none() { + non_ascii_start = Some(i); + } + } else { + push_start(&mut non_ascii_start, Some(i), &mut non_ascii_ranges); } if canonical_combining_class(c) == 0 { // not a combining char - if let Some(l) = last { + if let Some(l) = last_base_char { let seq = &string[l..i]; if seq.nfc().zip(seq.chars()).any(|(a, b)| a != b) { - if ustart.is_none() { ustart = last; } + if non_nfc_start.is_none() { + non_nfc_start = last_base_char; + } } else { - if let Some(s) = ustart { - str_pos_lint(cx, UNICODE_NOT_NFC, span, s, Some(i), - &format!("non NFC-normal unicode sequence found. \ - Consider using the normal form instead: '{}'", - &string[s..i].nfc().collect::())); + if let Some(nns) = non_nfc_start.take() { + non_nfc_ranges.push((nns, Some(i))); } - ustart = None; } } - last = Some(i); + last_base_char = Some(i); } - } - if let Some(s) = ustart { - str_pos_lint(cx, UNICODE_NOT_NFC, span, s, None, - &format!("non NFC-normal unicode sequence found. \ - Consider using the normal form instead: '{}'", - &string[s..].nfc().collect::())); + } + push_last_and_report(cx, string, span, zero_width_start, zero_width_ranges, + ZERO_WIDTH_SPACE, "zero-width space", zero_width_replacement); + push_last_and_report(cx, string, span, non_ascii_start, non_ascii_ranges, + NON_ASCII_LITERAL, "non-ascii literal", non_ascii_replacement); + if cx.current_level(NON_ASCII_LITERAL) == Level::Allow { + push_last_and_report(cx, string, span, non_nfc_start, non_nfc_ranges, + UNICODE_NOT_NFC, "non-NFC unicode", non_nfc_replacement); + } else { + push_last_and_report(cx, string, span, non_nfc_start, non_nfc_ranges, + UNICODE_NOT_NFC, "non-NFC unicode", non_nfc_ascii_replacement); } } -#[allow(cast_possible_truncation)] -fn str_pos_lint(cx: &Context, lint: &'static Lint, span: Span, index: usize, - end_index: Option, msg: &str) { - span_lint(cx, lint, Span { lo: span.lo + BytePos((1 + index) as u32), - hi: end_index.map_or(span.hi, - |i| span.lo + BytePos((1 + i) as u32)), - expn_id: span.expn_id }, msg); +fn zero_width_replacement(string: &str) -> String { + string.chars().map(|_| "\\u{200B}").collect() +} + +fn non_ascii_replacement(string: &str) -> String { + string.chars().flat_map(char::escape_unicode).collect() +} + +fn non_nfc_replacement(string: &str) -> String { + string.nfc().collect() +} + +fn non_nfc_ascii_replacement(string: &str) -> String { + string.nfc().flat_map(char::escape_unicode).collect() } diff --git a/tests/compile-fail/unicode.rs b/tests/compile-fail/unicode.rs index 933b1155d01a..f73573364c72 100755 --- a/tests/compile-fail/unicode.rs +++ b/tests/compile-fail/unicode.rs @@ -3,19 +3,17 @@ #[deny(zero_width_space)] fn zero() { - print!("Here >​< is a ZWS, and ​another"); - //~^ ERROR zero-width space detected. Consider using `\u{200B}` - //~^^ ERROR zero-width space detected. Consider using `\u{200B}` + print!("Here >​< is a ZWS, and ​another"); //~ ERROR 2 zero-width space ranges detected. } #[deny(unicode_not_nfc)] fn canon() { - print!("̀àh?"); //~ERROR non NFC-normal unicode sequence found. + print!("̀àh?"); //~ERROR non-NFC unicode range detected. Consider using `àh` } #[deny(non_ascii_literal)] fn uni() { - print!("Üben!"); //~ERROR literal non-ASCII character detected. Consider using `\u{DC}` + print!("Üben!"); //~ERROR non-ascii literal range detected. Consider using `\u{dc}` } fn main() { From 2fdb46d23fa60e51df97233b8ce4f701fd69afed Mon Sep 17 00:00:00 2001 From: llogiq Date: Tue, 1 Sep 2015 20:02:09 +0200 Subject: [PATCH 13/16] update_lints --- README.md | 2 +- src/lib.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 51c4ce9cd84f..5b984d8f73c2 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ A collection of lints that give helpful tips to newbies and catch oversights. ##Lints -There are 53 lints included in this crate: +There are 54 lints included in this crate: name | default | meaning -----------------------------------------------------------------------------------------------------|---------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------- diff --git a/src/lib.rs b/src/lib.rs index dca4fd33f717..847c2015b590 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -90,7 +90,7 @@ pub fn plugin_registrar(reg: &mut Registry) { types::CAST_PRECISION_LOSS, types::CAST_SIGN_LOSS, unicode::NON_ASCII_LITERAL, - unicode::UNICODE_NON_NFC, + unicode::UNICODE_NOT_NFC, ]); reg.register_lint_group("clippy", vec![ From 2bbbba68577c5f96b3eaf7a818eae584cdf7cb9b Mon Sep 17 00:00:00 2001 From: llogiq Date: Tue, 1 Sep 2015 20:06:32 +0200 Subject: [PATCH 14/16] dogfooding --- src/unicode.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/unicode.rs b/src/unicode.rs index 8e3f798408f4..dc1ff05028b4 100644 --- a/src/unicode.rs +++ b/src/unicode.rs @@ -3,7 +3,7 @@ extern crate unicode_normalization; use std::fmt::Write; use rustc::lint::*; use syntax::ast::*; -use syntax::codemap::{Pos, BytePos, CharPos, Span}; +use syntax::codemap::{Pos, BytePos, Span}; use self::unicode_normalization::char::canonical_combining_class; use self::unicode_normalization::UnicodeNormalization; @@ -36,7 +36,7 @@ impl LintPass for Unicode { } fn pos(base: BytePos, i: usize) -> BytePos { - if i == 0 { base } else { base + BytePos::from_usize(i + 1) } + if i == 0 { base } else { base + Pos::from_usize(i + 1) } } #[allow(cast_possible_truncation)] @@ -84,10 +84,10 @@ where F: Fn(&str) -> String, { for (from, until) in ranges { if let Some(u) = until { write!(&mut repls, "\n{}..{} => {}", - from, u, &multi_fun(&string[from..u])).unwrap(); + from, u, &multi_fun(&string[from..u])).expect(""); } else { write!(&mut repls, "\n{}.. => {}", - from, &multi_fun(&string[from..])).unwrap(); + from, &multi_fun(&string[from..])).expect(""); } } span_lint(cx, lint, span, &format!( From 30636625250b6256095550016ea3423e96909e8f Mon Sep 17 00:00:00 2001 From: llogiq Date: Tue, 1 Sep 2015 20:11:42 +0200 Subject: [PATCH 15/16] dogfooding 2 --- src/unicode.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/unicode.rs b/src/unicode.rs index dc1ff05028b4..61dc2de9774f 100644 --- a/src/unicode.rs +++ b/src/unicode.rs @@ -82,13 +82,13 @@ where F: Fn(&str) -> String, { x => { let mut repls = String::new(); for (from, until) in ranges { - if let Some(u) = until { + let _ = if let Some(u) = until { write!(&mut repls, "\n{}..{} => {}", - from, u, &multi_fun(&string[from..u])).expect(""); + from, u, &multi_fun(&string[from..u])); } else { write!(&mut repls, "\n{}.. => {}", - from, &multi_fun(&string[from..])).expect(""); - } + from, &multi_fun(&string[from..])); + }; } span_lint(cx, lint, span, &format!( "{} {} ranges detected. Consider the following replacements:{}", From 41f19a887df9ac463dbe7f9f6d55b25882407200 Mon Sep 17 00:00:00 2001 From: llogiq Date: Tue, 1 Sep 2015 21:02:49 +0200 Subject: [PATCH 16/16] removed spurious semicolon --- src/unicode.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/unicode.rs b/src/unicode.rs index 61dc2de9774f..c5532328aebd 100644 --- a/src/unicode.rs +++ b/src/unicode.rs @@ -84,10 +84,10 @@ where F: Fn(&str) -> String, { for (from, until) in ranges { let _ = if let Some(u) = until { write!(&mut repls, "\n{}..{} => {}", - from, u, &multi_fun(&string[from..u])); + from, u, &multi_fun(&string[from..u])) } else { write!(&mut repls, "\n{}.. => {}", - from, &multi_fun(&string[from..])); + from, &multi_fun(&string[from..])) }; } span_lint(cx, lint, span, &format!(