Adding try_err lint #4222
Adding try_err lint #4222
Conversation
clippy_lints/src/try_err.rs
Outdated
| 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"]); |
There was a problem hiding this comment.
Can you add this path to clippy_lints/src/utils/path.rs?
clippy_lints/src/try_err.rs
Outdated
| // 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) |
There was a problem hiding this comment.
Couldn't this be written as
| arms.iter().filter_map(|ty| find_err_return_type_arm(cx, ty)).nth(0) | |
| arms.iter().find_map(|ty| find_err_return_type_arm(cx, ty)) |
If so, this should be added as a lint to methods/mod.rs
clippy_lints/src/try_err.rs
Outdated
| cx, | ||
| TRY_ERR, | ||
| expr.span, | ||
| &format!("confusing error return, consider using `{}`", suggestion), |
There was a problem hiding this comment.
| &format!("confusing error return, consider using `{}`", suggestion), | |
| "returning an `Err(_)` with the `?` operator", |
clippy_lints/src/try_err.rs
Outdated
| expr.span, | ||
| "try this", | ||
| suggestion, | ||
| Applicability::MaybeIncorrect |
There was a problem hiding this comment.
You should be able to use the utils::span_lint_and_sugg function directly.
Also, do you have an example where this lint suggests something incorrect? If not, you can turn this to MachineApplicable and add // run-rustfix to the test file, like here:
There was a problem hiding this comment.
Maybe it's contrived, but a number of the test cases don't compile after applying the fix. For example:
pub fn basic_test() -> Result<i32, i32> {
let err: i32 = 1;
Err(err)?;
Ok(0)
}
becomes
pub fn basic_test() -> Result<i32, i32> {
let err: i32 = 1;
return Err(err);
Ok(0)
}
and the compiler complains about the unreachable Ok(0). I don't expect that to come up much in real life, but the replacement isn't 100% safe.
There was a problem hiding this comment.
But that's just a warning, so it still compiles (except you pass -Dwarnings of course). Also the unreachable_code warning would also apply to the unfixed code, it was just hidden with the weird Err(_)? syntax. So MachineApplicable should be ok here IMO.
Is // run-rustfix tested with -Dwarnings? Otherwise this can be added too.
|
The test file needs formatting. |
|
Thanks! the only thing left to do, before merging this, is to run |
|
Thanks, should be good to go! |
|
@bors r+ |
|
📌 Commit c96bb2c has been approved by |
Adding try_err lint changelog: Adds the "try_err" lint, which catches instances of the following: Err(x)? fixes #4212
|
☀️ Test successful - checks-travis, status-appveyor |
changelog: Adds the "try_err" lint, which catches instances of the following: Err(x)?
fixes #4212