Skip to content

Commit 92783e3

Browse files
borsbors
committed
Auto merge of #6157 - rust-lang:result-unit-err, r=ebroto
New lint: result-unit-err This fixes #1721. - [x] Followed [lint naming conventions][lint_naming] - [x] Added passing UI tests (including committed `.stderr` file) - [x] `cargo test` passes locally - [x] Executed `cargo dev update_lints` - [x] Added lint documentation - [x] Run `cargo dev fmt` [lint_naming]: https://rust-lang.github.io/rfcs/0344-conventions-galore.html#lints --- changelog: new lint: result-unit-err
2 parents 9ead65d + 74ae116 commit 92783e3

File tree

10 files changed

+189
-23
lines changed

10 files changed

+189
-23
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1918,6 +1918,7 @@ Released 2018-09-13
19181918
[`rest_pat_in_fully_bound_structs`]: https://rust-lang.github.io/rust-clippy/master/index.html#rest_pat_in_fully_bound_structs
19191919
[`result_map_or_into_option`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_map_or_into_option
19201920
[`result_map_unit_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_map_unit_fn
1921+
[`result_unit_err`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_unit_err
19211922
[`reversed_empty_ranges`]: https://rust-lang.github.io/rust-clippy/master/index.html#reversed_empty_ranges
19221923
[`same_functions_in_if_condition`]: https://rust-lang.github.io/rust-clippy/master/index.html#same_functions_in_if_condition
19231924
[`same_item_push`]: https://rust-lang.github.io/rust-clippy/master/index.html#same_item_push

‎clippy_lints/src/functions.rs‎

Lines changed: 93 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
11
use crate::utils::{
2-
attr_by_name, attrs::is_proc_macro, is_must_use_ty, is_trait_impl_item, iter_input_pats, match_def_path,
3-
must_use_attr, qpath_res, return_ty, snippet, snippet_opt, span_lint, span_lint_and_help, span_lint_and_then,
4-
trait_ref_of_method, type_is_unsafe_function,
2+
attr_by_name, attrs::is_proc_macro, is_must_use_ty, is_trait_impl_item, is_type_diagnostic_item, iter_input_pats,
3+
last_path_segment, match_def_path, must_use_attr, qpath_res, return_ty, snippet, snippet_opt, span_lint,
4+
span_lint_and_help, span_lint_and_then, trait_ref_of_method, type_is_unsafe_function,
55
};
6+
use if_chain::if_chain;
67
use rustc_ast::ast::Attribute;
78
use rustc_data_structures::fx::FxHashSet;
89
use rustc_errors::Applicability;
@@ -16,6 +17,7 @@ use rustc_middle::ty::{self, Ty};
1617
use rustc_session::{declare_tool_lint, impl_lint_pass};
1718
use rustc_span::source_map::Span;
1819
use rustc_target::spec::abi::Abi;
20+
use rustc_typeck::hir_ty_to_ty;
1921

2022
declare_clippy_lint! {
2123
/// **What it does:** Checks for functions with too many parameters.
@@ -169,6 +171,52 @@ declare_clippy_lint! {
169171
"function or method that could take a `#[must_use]` attribute"
170172
}
171173

174+
declare_clippy_lint! {
175+
/// **What it does:** Checks for public functions that return a `Result`
176+
/// with an `Err` type of `()`. It suggests using a custom type that
177+
/// implements [`std::error::Error`].
178+
///
179+
/// **Why is this bad?** Unit does not implement `Error` and carries no
180+
/// further information about what went wrong.
181+
///
182+
/// **Known problems:** Of course, this lint assumes that `Result` is used
183+
/// for a fallible operation (which is after all the intended use). However
184+
/// code may opt to (mis)use it as a basic two-variant-enum. In that case,
185+
/// the suggestion is misguided, and the code should use a custom enum
186+
/// instead.
187+
///
188+
/// **Examples:**
189+
/// ```rust
190+
/// pub fn read_u8() -> Result<u8, ()> { Err(()) }
191+
/// ```
192+
/// should become
193+
/// ```rust,should_panic
194+
/// use std::fmt;
195+
///
196+
/// #[derive(Debug)]
197+
/// pub struct EndOfStream;
198+
///
199+
/// impl fmt::Display for EndOfStream {
200+
/// fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
201+
/// write!(f, "End of Stream")
202+
/// }
203+
/// }
204+
///
205+
/// impl std::error::Error for EndOfStream { }
206+
///
207+
/// pub fn read_u8() -> Result<u8, EndOfStream> { Err(EndOfStream) }
208+
///# fn main() {
209+
///# read_u8().unwrap();
210+
///# }
211+
/// ```
212+
///
213+
/// Note that there are crates that simplify creating the error type, e.g.
214+
/// [`thiserror`](https://docs.rs/thiserror).
215+
pub RESULT_UNIT_ERR,
216+
style,
217+
"public function returning `Result` with an `Err` type of `()`"
218+
}
219+
172220
#[derive(Copy, Clone)]
173221
pub struct Functions {
174222
threshold: u64,
@@ -188,6 +236,7 @@ impl_lint_pass!(Functions => [
188236
MUST_USE_UNIT,
189237
DOUBLE_MUST_USE,
190238
MUST_USE_CANDIDATE,
239+
RESULT_UNIT_ERR,
191240
]);
192241

193242
impl<'tcx> LateLintPass<'tcx> for Functions {
@@ -233,15 +282,16 @@ impl<'tcx> LateLintPass<'tcx> for Functions {
233282
fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'_>) {
234283
let attr = must_use_attr(&item.attrs);
235284
if let hir::ItemKind::Fn(ref sig, ref _generics, ref body_id) = item.kind {
285+
let is_public = cx.access_levels.is_exported(item.hir_id);
286+
let fn_header_span = item.span.with_hi(sig.decl.output.span().hi());
287+
if is_public {
288+
check_result_unit_err(cx, &sig.decl, item.span, fn_header_span);
289+
}
236290
if let Some(attr) = attr {
237-
let fn_header_span = item.span.with_hi(sig.decl.output.span().hi());
238291
check_needless_must_use(cx, &sig.decl, item.hir_id, item.span, fn_header_span, attr);
239292
return;
240293
}
241-
if cx.access_levels.is_exported(item.hir_id)
242-
&& !is_proc_macro(cx.sess(), &item.attrs)
243-
&& attr_by_name(&item.attrs, "no_mangle").is_none()
244-
{
294+
if is_public && !is_proc_macro(cx.sess(), &item.attrs) && attr_by_name(&item.attrs, "no_mangle").is_none() {
245295
check_must_use_candidate(
246296
cx,
247297
&sig.decl,
@@ -257,11 +307,15 @@ impl<'tcx> LateLintPass<'tcx> for Functions {
257307

258308
fn check_impl_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::ImplItem<'_>) {
259309
if let hir::ImplItemKind::Fn(ref sig, ref body_id) = item.kind {
310+
let is_public = cx.access_levels.is_exported(item.hir_id);
311+
let fn_header_span = item.span.with_hi(sig.decl.output.span().hi());
312+
if is_public && trait_ref_of_method(cx, item.hir_id).is_none() {
313+
check_result_unit_err(cx, &sig.decl, item.span, fn_header_span);
314+
}
260315
let attr = must_use_attr(&item.attrs);
261316
if let Some(attr) = attr {
262-
let fn_header_span = item.span.with_hi(sig.decl.output.span().hi());
263317
check_needless_must_use(cx, &sig.decl, item.hir_id, item.span, fn_header_span, attr);
264-
} else if cx.access_levels.is_exported(item.hir_id)
318+
} else if is_public
265319
&& !is_proc_macro(cx.sess(), &item.attrs)
266320
&& trait_ref_of_method(cx, item.hir_id).is_none()
267321
{
@@ -284,18 +338,21 @@ impl<'tcx> LateLintPass<'tcx> for Functions {
284338
if sig.header.abi == Abi::Rust {
285339
self.check_arg_number(cx, &sig.decl, item.span.with_hi(sig.decl.output.span().hi()));
286340
}
341+
let is_public = cx.access_levels.is_exported(item.hir_id);
342+
let fn_header_span = item.span.with_hi(sig.decl.output.span().hi());
343+
if is_public {
344+
check_result_unit_err(cx, &sig.decl, item.span, fn_header_span);
345+
}
287346

288347
let attr = must_use_attr(&item.attrs);
289348
if let Some(attr) = attr {
290-
let fn_header_span = item.span.with_hi(sig.decl.output.span().hi());
291349
check_needless_must_use(cx, &sig.decl, item.hir_id, item.span, fn_header_span, attr);
292350
}
293351
if let hir::TraitFn::Provided(eid) = *eid {
294352
let body = cx.tcx.hir().body(eid);
295353
Self::check_raw_ptr(cx, sig.header.unsafety, &sig.decl, body, item.hir_id);
296354

297-
if attr.is_none() && cx.access_levels.is_exported(item.hir_id) && !is_proc_macro(cx.sess(), &item.attrs)
298-
{
355+
if attr.is_none() && is_public && !is_proc_macro(cx.sess(), &item.attrs) {
299356
check_must_use_candidate(
300357
cx,
301358
&sig.decl,
@@ -411,6 +468,29 @@ impl<'tcx> Functions {
411468
}
412469
}
413470

471+
fn check_result_unit_err(cx: &LateContext<'_>, decl: &hir::FnDecl<'_>, item_span: Span, fn_header_span: Span) {
472+
if_chain! {
473+
if !in_external_macro(cx.sess(), item_span);
474+
if let hir::FnRetTy::Return(ref ty) = decl.output;
475+
if let hir::TyKind::Path(ref qpath) = ty.kind;
476+
if is_type_diagnostic_item(cx, hir_ty_to_ty(cx.tcx, ty), sym!(result_type));
477+
if let Some(ref args) = last_path_segment(qpath).args;
478+
if let [_, hir::GenericArg::Type(ref err_ty)] = args.args;
479+
if let hir::TyKind::Tup(t) = err_ty.kind;
480+
if t.is_empty();
481+
then {
482+
span_lint_and_help(
483+
cx,
484+
RESULT_UNIT_ERR,
485+
fn_header_span,
486+
"this returns a `Result<_, ()>",
487+
None,
488+
"use a custom Error type instead",
489+
);
490+
}
491+
}
492+
}
493+
414494
fn check_needless_must_use(
415495
cx: &LateContext<'_>,
416496
decl: &hir::FnDecl<'_>,

‎clippy_lints/src/lib.rs‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -582,6 +582,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
582582
&functions::MUST_USE_CANDIDATE,
583583
&functions::MUST_USE_UNIT,
584584
&functions::NOT_UNSAFE_PTR_ARG_DEREF,
585+
&functions::RESULT_UNIT_ERR,
585586
&functions::TOO_MANY_ARGUMENTS,
586587
&functions::TOO_MANY_LINES,
587588
&future_not_send::FUTURE_NOT_SEND,
@@ -1327,6 +1328,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
13271328
LintId::of(&functions::DOUBLE_MUST_USE),
13281329
LintId::of(&functions::MUST_USE_UNIT),
13291330
LintId::of(&functions::NOT_UNSAFE_PTR_ARG_DEREF),
1331+
LintId::of(&functions::RESULT_UNIT_ERR),
13301332
LintId::of(&functions::TOO_MANY_ARGUMENTS),
13311333
LintId::of(&get_last_with_len::GET_LAST_WITH_LEN),
13321334
LintId::of(&identity_op::IDENTITY_OP),
@@ -1558,6 +1560,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
15581560
LintId::of(&formatting::SUSPICIOUS_UNARY_OP_FORMATTING),
15591561
LintId::of(&functions::DOUBLE_MUST_USE),
15601562
LintId::of(&functions::MUST_USE_UNIT),
1563+
LintId::of(&functions::RESULT_UNIT_ERR),
15611564
LintId::of(&if_let_some_result::IF_LET_SOME_RESULT),
15621565
LintId::of(&inherent_to_string::INHERENT_TO_STRING),
15631566
LintId::of(&len_zero::LEN_WITHOUT_IS_EMPTY),

‎src/lintlist/mod.rs‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2005,6 +2005,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
20052005
deprecation: None,
20062006
module: "map_unit_fn",
20072007
},
2008+
Lint {
2009+
name: "result_unit_err",
2010+
group: "style",
2011+
desc: "public function returning `Result` with an `Err` type of `()`",
2012+
deprecation: None,
2013+
module: "functions",
2014+
},
20082015
Lint {
20092016
name: "reversed_empty_ranges",
20102017
group: "correctness",

‎tests/ui/doc_errors.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
// edition:2018
22
#![warn(clippy::missing_errors_doc)]
3+
#![allow(clippy::result_unit_err)]
34

45
use std::io;
56

‎tests/ui/doc_errors.stderr‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error: docs for function returning `Result` missing `# Errors` section
2-
--> $DIR/doc_errors.rs:6:1
2+
--> $DIR/doc_errors.rs:7:1
33
|
44
LL | / pub fn pub_fn_missing_errors_header() -> Result<(), ()> {
55
LL | | unimplemented!();
@@ -9,47 +9,47 @@ LL | | }
99
= note: `-D clippy::missing-errors-doc` implied by `-D warnings`
1010

1111
error: docs for function returning `Result` missing `# Errors` section
12-
--> $DIR/doc_errors.rs:10:1
12+
--> $DIR/doc_errors.rs:11:1
1313
|
1414
LL | / pub async fn async_pub_fn_missing_errors_header() -> Result<(), ()> {
1515
LL | | unimplemented!();
1616
LL | | }
1717
| |_^
1818

1919
error: docs for function returning `Result` missing `# Errors` section
20-
--> $DIR/doc_errors.rs:15:1
20+
--> $DIR/doc_errors.rs:16:1
2121
|
2222
LL | / pub fn pub_fn_returning_io_result() -> io::Result<()> {
2323
LL | | unimplemented!();
2424
LL | | }
2525
| |_^
2626

2727
error: docs for function returning `Result` missing `# Errors` section
28-
--> $DIR/doc_errors.rs:20:1
28+
--> $DIR/doc_errors.rs:21:1
2929
|
3030
LL | / pub async fn async_pub_fn_returning_io_result() -> io::Result<()> {
3131
LL | | unimplemented!();
3232
LL | | }
3333
| |_^
3434

3535
error: docs for function returning `Result` missing `# Errors` section
36-
--> $DIR/doc_errors.rs:50:5
36+
--> $DIR/doc_errors.rs:51:5
3737
|
3838
LL | / pub fn pub_method_missing_errors_header() -> Result<(), ()> {
3939
LL | | unimplemented!();
4040
LL | | }
4141
| |_____^
4242

4343
error: docs for function returning `Result` missing `# Errors` section
44-
--> $DIR/doc_errors.rs:55:5
44+
--> $DIR/doc_errors.rs:56:5
4545
|
4646
LL | / pub async fn async_pub_method_missing_errors_header() -> Result<(), ()> {
4747
LL | | unimplemented!();
4848
LL | | }
4949
| |_____^
5050

5151
error: docs for function returning `Result` missing `# Errors` section
52-
--> $DIR/doc_errors.rs:84:5
52+
--> $DIR/doc_errors.rs:85:5
5353
|
5454
LL | fn trait_method_missing_errors_header() -> Result<(), ()>;
5555
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

‎tests/ui/double_must_use.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
#![warn(clippy::double_must_use)]
2+
#![allow(clippy::result_unit_err)]
23

34
#[must_use]
45
pub fn must_use_result() -> Result<(), ()> {

‎tests/ui/double_must_use.stderr‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error: this function has an empty `#[must_use]` attribute, but returns a type already marked as `#[must_use]`
2-
--> $DIR/double_must_use.rs:4:1
2+
--> $DIR/double_must_use.rs:5:1
33
|
44
LL | pub fn must_use_result() -> Result<(), ()> {
55
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -8,15 +8,15 @@ LL | pub fn must_use_result() -> Result<(), ()> {
88
= help: either add some descriptive text or remove the attribute
99

1010
error: this function has an empty `#[must_use]` attribute, but returns a type already marked as `#[must_use]`
11-
--> $DIR/double_must_use.rs:9:1
11+
--> $DIR/double_must_use.rs:10:1
1212
|
1313
LL | pub fn must_use_tuple() -> (Result<(), ()>, u8) {
1414
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
1515
|
1616
= help: either add some descriptive text or remove the attribute
1717

1818
error: this function has an empty `#[must_use]` attribute, but returns a type already marked as `#[must_use]`
19-
--> $DIR/double_must_use.rs:14:1
19+
--> $DIR/double_must_use.rs:15:1
2020
|
2121
LL | pub fn must_use_array() -> [Result<(), ()>; 1] {
2222
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

‎tests/ui/result_unit_error.rs‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
#[warn(clippy::result_unit_err)]
2+
#[allow(unused)]
3+
4+
pub fn returns_unit_error() -> Result<u32, ()> {
5+
Err(())
6+
}
7+
8+
fn private_unit_errors() -> Result<String, ()> {
9+
Err(())
10+
}
11+
12+
pub trait HasUnitError {
13+
fn get_that_error(&self) -> Result<bool, ()>;
14+
15+
fn get_this_one_too(&self) -> Result<bool, ()> {
16+
Err(())
17+
}
18+
}
19+
20+
impl HasUnitError for () {
21+
fn get_that_error(&self) -> Result<bool, ()> {
22+
Ok(true)
23+
}
24+
}
25+
26+
trait PrivateUnitError {
27+
fn no_problem(&self) -> Result<usize, ()>;
28+
}
29+
30+
pub struct UnitErrorHolder;
31+
32+
impl UnitErrorHolder {
33+
pub fn unit_error(&self) -> Result<usize, ()> {
34+
Ok(0)
35+
}
36+
}
37+
38+
fn main() {}

‎tests/ui/result_unit_error.stderr‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
error: this returns a `Result<_, ()>
2+
--> $DIR/result_unit_error.rs:4:1
3+
|
4+
LL | pub fn returns_unit_error() -> Result<u32, ()> {
5+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
6+
|
7+
= note: `-D clippy::result-unit-err` implied by `-D warnings`
8+
= help: use a custom Error type instead
9+
10+
error: this returns a `Result<_, ()>
11+
--> $DIR/result_unit_error.rs:13:5
12+
|
13+
LL | fn get_that_error(&self) -> Result<bool, ()>;
14+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
15+
|
16+
= help: use a custom Error type instead
17+
18+
error: this returns a `Result<_, ()>
19+
--> $DIR/result_unit_error.rs:15:5
20+
|
21+
LL | fn get_this_one_too(&self) -> Result<bool, ()> {
22+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
23+
|
24+
= help: use a custom Error type instead
25+
26+
error: this returns a `Result<_, ()>
27+
--> $DIR/result_unit_error.rs:33:5
28+
|
29+
LL | pub fn unit_error(&self) -> Result<usize, ()> {
30+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
31+
|
32+
= help: use a custom Error type instead
33+
34+
error: aborting due to 4 previous errors
35+

0 commit comments

Comments
 (0)