Adding try_err lint

This commit is contained in:
Joe Frikker 2019-06-18 23:22:51 -04:00
parent c5d1ecd474
commit 60a80849ce
7 changed files with 231 additions and 2 deletions

View file

@ -1134,6 +1134,7 @@ Released 2018-09-13
[`transmuting_null`]: https://rust-lang.github.io/rust-clippy/master/index.html#transmuting_null
[`trivial_regex`]: https://rust-lang.github.io/rust-clippy/master/index.html#trivial_regex
[`trivially_copy_pass_by_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#trivially_copy_pass_by_ref
[`try_err`]: https://rust-lang.github.io/rust-clippy/master/index.html#try_err
[`type_complexity`]: https://rust-lang.github.io/rust-clippy/master/index.html#type_complexity
[`unicode_not_nfc`]: https://rust-lang.github.io/rust-clippy/master/index.html#unicode_not_nfc
[`unimplemented`]: https://rust-lang.github.io/rust-clippy/master/index.html#unimplemented

View file

@ -6,7 +6,7 @@
A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.
[There are 305 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
[There are 306 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you:

View file

@ -263,6 +263,7 @@ pub mod temporary_assignment;
pub mod transmute;
pub mod transmuting_null;
pub mod trivially_copy_pass_by_ref;
pub mod try_err;
pub mod types;
pub mod unicode;
pub mod unsafe_removed_from_name;
@ -546,6 +547,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
reg.register_early_lint_pass(box literal_representation::DecimalLiteralRepresentation::new(
conf.literal_representation_threshold
));
reg.register_late_lint_pass(box try_err::TryErr);
reg.register_late_lint_pass(box use_self::UseSelf);
reg.register_late_lint_pass(box bytecount::ByteCount);
reg.register_late_lint_pass(box infinite_iter::InfiniteIter);
@ -861,6 +863,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
transmute::WRONG_TRANSMUTE,
transmuting_null::TRANSMUTING_NULL,
trivially_copy_pass_by_ref::TRIVIALLY_COPY_PASS_BY_REF,
try_err::TRY_ERR,
types::ABSURD_EXTREME_COMPARISONS,
types::BORROWED_BOX,
types::BOX_VEC,
@ -963,6 +966,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
returns::NEEDLESS_RETURN,
returns::UNUSED_UNIT,
strings::STRING_LIT_AS_BYTES,
try_err::TRY_ERR,
types::FN_TO_NUMERIC_CAST,
types::FN_TO_NUMERIC_CAST_WITH_TRUNCATION,
types::IMPLICIT_HASHER,

120
clippy_lints/src/try_err.rs Normal file
View file

@ -0,0 +1,120 @@
use crate::utils::{match_qpath, paths, snippet, span_lint_and_then};
use if_chain::if_chain;
use rustc::hir::*;
use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
use rustc::ty::Ty;
use rustc::{declare_lint_pass, declare_tool_lint};
use rustc_errors::Applicability;
declare_clippy_lint! {
/// **What it does:** Checks for usages of `Err(x)?`.
///
/// **Why is this bad?** The `?` operator is designed to allow calls that
/// can fail to be easily chained. For example, `foo()?.bar()` or
/// `foo(bar()?)`. Because `Err(x)?` can't be used that way (it will
/// always return), it is more clear to write `return Err(x)`.
///
/// **Known problems:** None.
///
/// **Example:**
///
/// ```rust,ignore
/// // Bad
/// fn foo(fail: bool) -> Result<i32, String> {
/// if fail {
/// Err("failed")?;
/// }
/// Ok(0)
/// }
///
/// // Good
/// fn foo(fail: bool) -> Result<i32, String> {
/// if fail {
/// return Err("failed".into());
/// }
/// Ok(0)
/// }
/// ```
pub TRY_ERR,
style,
"return errors explicitly rather than hiding them behind a `?`"
}
declare_lint_pass!(TryErr => [TRY_ERR]);
impl<'a, 'tcx> LateLintPass<'a, 'tcx> for TryErr {
fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
// Looks for a structure like this:
// match ::std::ops::Try::into_result(Err(5)) {
// ::std::result::Result::Err(err) =>
// #[allow(unreachable_code)]
// return ::std::ops::Try::from_error(::std::convert::From::from(err)),
// ::std::result::Result::Ok(val) =>
// #[allow(unreachable_code)]
// val,
// };
if_chain! {
if let ExprKind::Match(ref match_arg, _, MatchSource::TryDesugar) = expr.node;
if let ExprKind::Call(ref match_fun, ref try_args) = match_arg.node;
if let ExprKind::Path(ref match_fun_path) = match_fun.node;
if match_qpath(match_fun_path, &["std", "ops", "Try", "into_result"]);
if let Some(ref try_arg) = try_args.get(0);
if let ExprKind::Call(ref err_fun, ref err_args) = try_arg.node;
if let Some(ref err_arg) = err_args.get(0);
if let ExprKind::Path(ref err_fun_path) = err_fun.node;
if match_qpath(err_fun_path, &paths::RESULT_ERR);
if let Some(return_type) = find_err_return_type(cx, &expr.node);
then {
let err_type = cx.tables.expr_ty(err_arg);
let suggestion = if err_type == return_type {
format!("return Err({})", snippet(cx, err_arg.span, "_"))
} else {
format!("return Err({}.into())", snippet(cx, err_arg.span, "_"))
};
span_lint_and_then(
cx,
TRY_ERR,
expr.span,
&format!("confusing error return, consider using `{}`", suggestion),
|db| {
db.span_suggestion(
expr.span,
"try this",
suggestion,
Applicability::MaybeIncorrect
);
},
);
}
}
}
}
// In order to determine whether to suggest `.into()` or not, we need to find the error type the
// function returns. To do that, we look for the From::from call (see tree above), and capture
// its output type.
fn find_err_return_type<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx ExprKind) -> Option<Ty<'tcx>> {
if let ExprKind::Match(_, ref arms, MatchSource::TryDesugar) = expr {
arms.iter().filter_map(|ty| find_err_return_type_arm(cx, ty)).nth(0)
} else {
None
}
}
// Check for From::from in one of the match arms.
fn find_err_return_type_arm<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, arm: &'tcx Arm) -> Option<Ty<'tcx>> {
if_chain! {
if let ExprKind::Ret(Some(ref err_ret)) = arm.body.node;
if let ExprKind::Call(ref from_error_path, ref from_error_args) = err_ret.node;
if let ExprKind::Path(ref from_error_fn) = from_error_path.node;
if match_qpath(from_error_fn, &["std", "ops", "Try", "from_error"]);
if let Some(from_error_arg) = from_error_args.get(0);
then {
Some(cx.tables.expr_ty(from_error_arg))
} else {
None
}
}
}

View file

@ -6,7 +6,7 @@ pub use lint::Lint;
pub use lint::LINT_LEVELS;
// begin lint list, do not remove this comment, its used in `update_lints`
pub const ALL_LINTS: [Lint; 305] = [
pub const ALL_LINTS: [Lint; 306] = [
Lint {
name: "absurd_extreme_comparisons",
group: "correctness",
@ -1820,6 +1820,13 @@ pub const ALL_LINTS: [Lint; 305] = [
deprecation: None,
module: "trivially_copy_pass_by_ref",
},
Lint {
name: "try_err",
group: "style",
desc: "TODO",
deprecation: None,
module: "try_err",
},
Lint {
name: "type_complexity",
group: "complexity",

65
tests/ui/try_err.rs Normal file
View file

@ -0,0 +1,65 @@
#![deny(clippy::try_err)]
// Tests that a simple case works
// Should flag `Err(err)?`
pub fn basic_test() -> Result<i32, i32> {
let err: i32 = 1;
Err(err)?;
Ok(0)
}
// Tests that `.into()` is added when appropriate
pub fn into_test() -> Result<i32, i32> {
let err: u8 = 1;
Err(err)?;
Ok(0)
}
// Tests that tries in general don't trigger the error
pub fn negative_test() -> Result<i32, i32> {
Ok(nested_error()? + 1)
}
// Tests that `.into()` isn't added when the error type
// matches the surrounding closure's return type, even
// when it doesn't match the surrounding function's.
pub fn closure_matches_test() -> Result<i32, i32> {
let res: Result<i32, i8> = Some(1).into_iter()
.map(|i| {
let err: i8 = 1;
Err(err)?;
Ok(i)
})
.next()
.unwrap();
Ok(res?)
}
// Tests that `.into()` isn't added when the error type
// doesn't match the surrounding closure's return type.
pub fn closure_into_test() -> Result<i32, i32> {
let res: Result<i32, i16> = Some(1).into_iter()
.map(|i| {
let err: i8 = 1;
Err(err)?;
Ok(i)
})
.next()
.unwrap();
Ok(res?)
}
fn nested_error() -> Result<i32, i32> {
Ok(1)
}
fn main() {
basic_test().unwrap();
into_test().unwrap();
negative_test().unwrap();
closure_matches_test().unwrap();
closure_into_test().unwrap();
}

32
tests/ui/try_err.stderr Normal file
View file

@ -0,0 +1,32 @@
error: confusing error return, consider using `return Err(err)`
--> $DIR/try_err.rs:7:5
|
LL | Err(err)?;
| ^^^^^^^^^ help: try this: `return Err(err)`
|
note: lint level defined here
--> $DIR/try_err.rs:1:9
|
LL | #![deny(clippy::try_err)]
| ^^^^^^^^^^^^^^^
error: confusing error return, consider using `return Err(err.into())`
--> $DIR/try_err.rs:14:5
|
LL | Err(err)?;
| ^^^^^^^^^ help: try this: `return Err(err.into())`
error: confusing error return, consider using `return Err(err)`
--> $DIR/try_err.rs:31:13
|
LL | Err(err)?;
| ^^^^^^^^^ help: try this: `return Err(err)`
error: confusing error return, consider using `return Err(err.into())`
--> $DIR/try_err.rs:46:13
|
LL | Err(err)?;
| ^^^^^^^^^ help: try this: `return Err(err.into())`
error: aborting due to 4 previous errors