From cf96042c65b4c232d0a73f6a8514e59e0358719b Mon Sep 17 00:00:00 2001 From: Georg Brandl Date: Tue, 11 Aug 2015 20:57:21 +0200 Subject: [PATCH 1/2] move walk_ty() to utils module and rename to walk_ptrs_ty --- src/len_zero.rs | 5 ++--- src/misc.rs | 15 ++++----------- src/strings.rs | 5 ++--- src/utils.rs | 8 ++++++++ 4 files changed, 16 insertions(+), 17 deletions(-) diff --git a/src/len_zero.rs b/src/len_zero.rs index 0877fa952383..8aa4c6267607 100644 --- a/src/len_zero.rs +++ b/src/len_zero.rs @@ -9,8 +9,7 @@ use rustc::middle::ty::{self, TypeVariants, TypeAndMut, MethodTraitItemId, ImplO use rustc::middle::def::{DefTy, DefStruct, DefTrait}; use syntax::codemap::{Span, Spanned}; use syntax::ast::*; -use misc::walk_ty; -use utils::span_lint; +use utils::{span_lint, walk_ptrs_ty}; declare_lint!(pub LEN_ZERO, Warn, "Warn when .is_empty() could be used instead of checking .len()"); @@ -136,7 +135,7 @@ fn has_is_empty(cx: &Context, expr: &Expr) -> bool { |iids| iids.iter().any(|i| is_is_empty(cx, i))))) } - let ty = &walk_ty(&cx.tcx.expr_ty(expr)); + let ty = &walk_ptrs_ty(&cx.tcx.expr_ty(expr)); match ty.sty { ty::TyTrait(_) => cx.tcx.trait_item_def_ids.borrow().get( &ty.ty_to_def_id().expect("trait impl not found")).map_or(false, diff --git a/src/misc.rs b/src/misc.rs index 444062a5919f..a1b3dcb32b8a 100644 --- a/src/misc.rs +++ b/src/misc.rs @@ -7,14 +7,7 @@ use rustc::lint::{Context, LintPass, LintArray, Lint, Level}; use rustc::middle::ty; use syntax::codemap::{Span, Spanned}; -use utils::{match_path, snippet, span_lint, span_help_and_lint}; - -pub fn walk_ty<'t>(ty: ty::Ty<'t>) -> ty::Ty<'t> { - match ty.sty { - ty::TyRef(_, ref tm) | ty::TyRawPtr(ref tm) => walk_ty(tm.ty), - _ => ty - } -} +use utils::{match_path, snippet, span_lint, span_help_and_lint, walk_ptrs_ty}; /// Handles uncategorized lints /// Currently handles linting of if-let-able matches @@ -87,7 +80,7 @@ impl LintPass for StrToStringPass { } fn is_str(cx: &Context, expr: &ast::Expr) -> bool { - match walk_ty(cx.tcx.expr_ty(expr)).sty { + match walk_ptrs_ty(cx.tcx.expr_ty(expr)).sty { ty::TyStr => true, _ => false } @@ -175,7 +168,7 @@ impl LintPass for FloatCmp { } fn is_float(cx: &Context, expr: &Expr) -> bool { - if let ty::TyFloat(_) = walk_ty(cx.tcx.expr_ty(expr)).sty { + if let ty::TyFloat(_) = walk_ptrs_ty(cx.tcx.expr_ty(expr)).sty { true } else { false @@ -274,7 +267,7 @@ fn check_to_owned(cx: &Context, expr: &Expr, other_span: Span) { fn is_str_arg(cx: &Context, args: &[P]) -> bool { args.len() == 1 && if let ty::TyStr = - walk_ty(cx.tcx.expr_ty(&*args[0])).sty { true } else { false } + walk_ptrs_ty(cx.tcx.expr_ty(&*args[0])).sty { true } else { false } } declare_lint!(pub NEEDLESS_RETURN, Warn, diff --git a/src/strings.rs b/src/strings.rs index 3384eed8da51..b6bc7654e479 100644 --- a/src/strings.rs +++ b/src/strings.rs @@ -8,9 +8,8 @@ use rustc::middle::ty::TypeVariants::TyStruct; use syntax::ast::*; use syntax::codemap::{Span, Spanned}; use eq_op::is_exp_equal; -use misc::walk_ty; use types::match_ty_unwrap; -use utils::{match_def_path, span_lint}; +use utils::{match_def_path, span_lint, walk_ptrs_ty}; declare_lint! { pub STRING_ADD_ASSIGN, @@ -38,7 +37,7 @@ impl LintPass for StringAdd { } fn is_string(cx: &Context, e: &Expr) -> bool { - if let TyStruct(did, _) = walk_ty(cx.tcx.expr_ty(e)).sty { + if let TyStruct(did, _) = walk_ptrs_ty(cx.tcx.expr_ty(e)).sty { match_def_path(cx, did.did, &["std", "string", "String"]) } else { false } } diff --git a/src/utils.rs b/src/utils.rs index 9b3b94e113e7..107f5c6f99b6 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -84,3 +84,11 @@ pub fn span_help_and_lint(cx: &Context, lint: &'static Lint, span: Span, cx.sess().fileline_help(span, help); } } + +/// return the base type for references and raw pointers +pub fn walk_ptrs_ty<'t>(ty: ty::Ty<'t>) -> ty::Ty<'t> { + match ty.sty { + ty::TyRef(_, ref tm) | ty::TyRawPtr(ref tm) => walk_ptrs_ty(tm.ty), + _ => ty + } +} From 2bcc15188854ff184c85426b5aa60535ed1bd8a8 Mon Sep 17 00:00:00 2001 From: Georg Brandl Date: Tue, 11 Aug 2015 20:53:50 +0200 Subject: [PATCH 2/2] new lint for Option.unwrap() and Result.unwrap() The latter is set to Allow by default (fixes #24) --- src/lib.rs | 4 ++++ src/methods.rs | 39 +++++++++++++++++++++++++++++++++++ tests/compile-fail/methods.rs | 11 ++++++++++ 3 files changed, 54 insertions(+) create mode 100644 src/methods.rs create mode 100755 tests/compile-fail/methods.rs diff --git a/src/lib.rs b/src/lib.rs index 80ba04031fab..4009aa1cf8d7 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -29,6 +29,7 @@ pub mod collapsible_if; pub mod unicode; pub mod utils; pub mod strings; +pub mod methods; #[plugin_registrar] pub fn plugin_registrar(reg: &mut Registry) { @@ -55,6 +56,7 @@ pub fn plugin_registrar(reg: &mut Registry) { reg.register_lint_pass(box unicode::Unicode as LintPassObject); reg.register_lint_pass(box strings::StringAdd as LintPassObject); reg.register_lint_pass(box misc::NeedlessReturn as LintPassObject); + reg.register_lint_pass(box methods::MethodsPass as LintPassObject); reg.register_lint_group("clippy", vec![types::BOX_VEC, types::LINKEDLIST, misc::SINGLE_MATCH, misc::STR_TO_STRING, @@ -77,5 +79,7 @@ pub fn plugin_registrar(reg: &mut Registry) { strings::STRING_ADD_ASSIGN, misc::NEEDLESS_RETURN, misc::MODULO_ONE, + methods::OPTION_UNWRAP_USED, + methods::RESULT_UNWRAP_USED, ]); } diff --git a/src/methods.rs b/src/methods.rs new file mode 100644 index 000000000000..3d9aa8c6ffc1 --- /dev/null +++ b/src/methods.rs @@ -0,0 +1,39 @@ +use syntax::ast::*; +use rustc::lint::{Context, LintPass, LintArray}; +use rustc::middle::ty; + +use utils::{span_lint, match_def_path, walk_ptrs_ty}; + +#[derive(Copy,Clone)] +pub struct MethodsPass; + +declare_lint!(pub OPTION_UNWRAP_USED, Warn, + "Warn on using unwrap() on an Option value"); +declare_lint!(pub RESULT_UNWRAP_USED, Allow, + "Warn on using unwrap() on a Result value"); + +impl LintPass for MethodsPass { + fn get_lints(&self) -> LintArray { + lint_array!(OPTION_UNWRAP_USED, RESULT_UNWRAP_USED) + } + + fn check_expr(&mut self, cx: &Context, expr: &Expr) { + if let ExprMethodCall(ref ident, _, ref args) = expr.node { + if ident.node.name == "unwrap" { + if let ty::TyEnum(did, _) = walk_ptrs_ty(cx.tcx.expr_ty(&*args[0])).sty { + if match_def_path(cx, did.did, &["core", "option", "Option"]) { + span_lint(cx, OPTION_UNWRAP_USED, expr.span, + "used unwrap() on an Option value. If you don't want \ + to handle the None case gracefully, consider using + expect() to provide a better panic message."); + } + else if match_def_path(cx, did.did, &["core", "result", "Result"]) { + span_lint(cx, RESULT_UNWRAP_USED, expr.span, + "used unwrap() on a Result value. Graceful handling \ + of Err values is preferred."); + } + } + } + } + } +} diff --git a/tests/compile-fail/methods.rs b/tests/compile-fail/methods.rs new file mode 100755 index 000000000000..e989dffe5a77 --- /dev/null +++ b/tests/compile-fail/methods.rs @@ -0,0 +1,11 @@ +#![feature(plugin)] +#![plugin(clippy)] + +#[deny(option_unwrap_used, result_unwrap_used)] +fn main() { + let opt = Some(0); + let _ = opt.unwrap(); //~ERROR + + let res: Result = Ok(0); + let _ = res.unwrap(); //~ERROR +}