Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Exclude main from exit lint
  • Loading branch information
Licenser committed Nov 7, 2019
commit 9471669e4695921cd8b70c8edaad143d8e84f430
29 changes: 25 additions & 4 deletions clippy_lints/src/exit.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::utils::{match_def_path, paths, qpath_res, span_lint};
use if_chain::if_chain;
use rustc::hir::{Expr, ExprKind};
use rustc::hir::{Expr, ExprKind, Item, ItemKind, Node};
use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
use rustc::{declare_lint_pass, declare_tool_lint};

Expand All @@ -11,8 +11,7 @@ declare_clippy_lint! {
/// **Why is this bad?** Ideally a program is terminated by finishing
/// the main function.
///
/// **Known problems:** This can be valid code in main() to return
/// errors
/// **Known problems:** None.
///
/// **Example:**
/// ```ignore
Expand All @@ -33,7 +32,29 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Exit {
if let Some(def_id) = qpath_res(cx, path, path_expr.hir_id).opt_def_id();
if match_def_path(cx, def_id, &paths::EXIT);
then {
span_lint(cx, EXIT, e.span, "usage of `process::exit`");
let mut parent = cx.tcx.hir().get_parent_item(e.hir_id);
// We have to traverse the parents upwards until we find a function
// otherwise a exit in a let or if in main would still trigger this
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tested this? lets and ifs are expressions, not items. See ItemKind on possible items get_parent_item should find.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some research, it seems get_parent_item will skip non items, which is fine as we don't fare for anything but the first function parent, so if there is a if ... { exit(0) } it will still just skip the if and right go to the function.

That said I'm running in a odd problem and I'm a bit stuck. I added another test to ensure this works with a if un a function, and here it gets odd. Even so the span_lint is hit for both only one error is ever emitted. So I'm a bit stomped. I left the test failing.

A hand with why only one span is emitted would be greatly appreciated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some research, it seems get_parent_item will skip non items

So the loop shouldn't be necessary? The loop will only help, when exit is used in a nested function inside main. IMO we should still lint this though. We only want to bail out, if exit is directly used inside main

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even so the span_lint is hit for both only one error is ever emitted

That is really weird.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I removed the loop, I had the thought that there might something else in the path we need skip but you're right the next item should be the function I removed the loop!

And yes that's really weird o.O

loop{
match cx.tcx.hir().find(parent) {
Some(Node::Item(Item{ident, kind: ItemKind::Fn(..), ..})) => {
// If we found a function we check it's name if it is
// `main` we emit a lint.
if ident.name.as_str() != "main" {
span_lint(cx, EXIT, e.span, "usage of `process::exit`");
}
// We found any kind of function and can end our loop
break;
}
// If we found anything but a funciton we continue with the
// loop and go one parent up
Some(_) => {
cx.tcx.hir().get_parent_item(parent);
},
// If we found nothing we break.
None => break,
}
}
}

}
Expand Down
8 changes: 8 additions & 0 deletions tests/ui/exit.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,12 @@
#[warn(clippy::exit)]
fn not_main() {
std::process::exit(3);
}

fn main() {
if true {
std::process::exit(2);
};
not_main();
std::process::exit(1);
}
2 changes: 1 addition & 1 deletion tests/ui/exit.stderr
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
error: usage of `process::exit`
--> $DIR/exit.rs:3:5
|
LL | std::process::exit(1);
LL | std::process::exit(3);
| ^^^^^^^^^^^^^^^^^^^^^
|
= note: `-D clippy::exit` implied by `-D warnings`
Expand Down