restriction lint for `std::process::exit`#4697

Licenser · 2019-10-18T19:12:43Z

Addition to #4655 - adds the lint checking for std::process::exit

changelog: add restriction lint for std::process::exit

llogiq · 2019-10-19T08:01:12Z

Looks good, but perhaps we can extend the lint to skip main functions automatically? IIRC we can get the parent item with utils::get_parent_item(cx, hir_id) and then we could check if its kind isItemFn named main.

llogiq · 2019-10-19T09:01:13Z

I'd be fine with merging as-is and extending it in a follow-up PR though.

Licenser · 2019-10-19T09:39:06Z

That's a good idea I didn't think about it :) will update!

flip1995

I think, the main check can be simplified.

flip1995 · 2019-10-21T07:46:01Z

clippy_lints/src/exit.rs

+            then {
+                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


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

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.

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

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

That is really weird.

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

clippy_lints/src/exit.rs

bors · 2019-10-23T21:26:47Z

☔ The latest upstream changes (presumably #4680) made this pull request unmergeable. Please resolve the merge conflicts.

clippy_lints/src/lib.rs

Licenser · 2019-10-25T09:00:02Z

heads up I currently can't compile clippy or run tests it fails on some odd things like not finding runstc cratre any more

flip1995 · 2019-10-25T09:10:40Z

I just had the same issue!

The fix is to update the RTIM version:

$ cargo install -Z install-upgrade rustup-toolchain-install-master --bin rustup-toolchain-install-master

and then install the master toolchain with

$ rustup-toolchain-install-master -f -n master -c rustc-dev

(or use the setup-toolchain.sh script)

Licenser · 2019-10-29T06:12:32Z

I'm still getting the following errors when trying to compile/test:

error[E0603]: module `hygiene` is private
  --> clippy_lints/src/utils/mod.rs:48:20
   |
48 | use syntax_expand::hygiene::ExpnKind;
   |                    ^^^^^^^

error[E0603]: module `hygiene` is private
   --> clippy_lints/src/misc.rs:599:24
    |
599 |     use syntax_expand::hygiene::MacroKind;
    |                        ^^^^^^^

error[E0603]: enum `MacroKind` is private
  --> clippy_lints/src/types.rs:22:26
   |
22 | use syntax_expand::base::MacroKind;
   |                          ^^^^^^^^^

error[E0603]: module `hygiene` is private
  --> clippy_lints/src/types.rs:23:20
   |
23 | use syntax_expand::hygiene::ExpnKind;
   |                    ^^^^^^^

error: aborting due to 4 previous errors

For more information about this error, try `rustc --explain E0603`.
error: could not compile `clippy_lints`.

I ran the commands above, cargo clean, and cargo update.

flip1995 · 2019-10-29T09:15:15Z

That changes landed yesterday on the master branch, so you have to rebase

Licenser · 2019-10-29T20:35:40Z

Should be good to go now!

flip1995 · 2019-11-07T13:14:36Z

@bors r+ rollup

bors · 2019-11-07T13:14:37Z

📌 Commit 0bd8527 has been approved by flip1995

bors · 2019-11-07T13:14:38Z

🌲 The tree is currently closed for pull requests below priority 10, this pull request will be tested once the tree is reopened

restriction lint for `std::process::exit` Addition to rust-lang#4655 - adds the lint checking for `std::process::exit` changelog: add restriction lint for `std::process::exit`

bors · 2019-11-07T13:42:02Z

☔ The latest upstream changes (presumably #4788) made this pull request unmergeable. Please resolve the merge conflicts.

Co-Authored-By: Philipp Krones <hello@philkrones.com>

flip1995 · 2019-11-07T17:32:09Z

@bors r+ rollup

bors · 2019-11-07T17:32:10Z

📌 Commit 2f1370d has been approved by flip1995

bors · 2019-11-07T17:32:12Z

🌲 The tree is currently closed for pull requests below priority 10, this pull request will be tested once the tree is reopened

restriction lint for `std::process::exit` Addition to rust-lang#4655 - adds the lint checking for `std::process::exit` changelog: add restriction lint for `std::process::exit`

@ghost

Rollup of 4 pull requests Successful merges: - #4697 (restriction lint for `std::process::exit`) - #4757 (Fix Deprecated lints don't expand) - #4758 (`DecimalLiteralRepresentation` simplification) - #4769 (don't warn on CRLF in `with_newline` lints) Failed merges: r? @ghost

@ghost

Rollup of 4 pull requests Successful merges: - #4697 (restriction lint for `std::process::exit`) - #4757 (Fix Deprecated lints don't expand) - #4758 (`DecimalLiteralRepresentation` simplification) - #4769 (don't warn on CRLF in `with_newline` lints) Failed merges: r? @ghost

restriction lint for `std::process::exit` Addition to #4655 - adds the lint checking for `std::process::exit` changelog: add restriction lint for `std::process::exit`

bors · 2019-11-07T22:05:56Z

⌛ Testing commit 2f1370d with merge 4be144a...

bors · 2019-11-07T22:28:26Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: flip1995
Pushing 4be144a to master...

Licenser · 2019-11-12T21:29:03Z

🎉 👍 thanks! :D

Licenser mentioned this pull request Oct 18, 2019

Some extra pedantic checks? #4655

Closed

flip1995 reviewed Oct 21, 2019

View reviewed changes

Licenser force-pushed the no-exit branch from f343896 to 60ce2bc Compare October 24, 2019 19:17

flip1995 reviewed Oct 25, 2019

View reviewed changes

clippy_lints/src/lib.rs Outdated Show resolved Hide resolved

flip1995 reviewed Oct 25, 2019

View reviewed changes

clippy_lints/src/lib.rs Outdated Show resolved Hide resolved

flip1995 mentioned this pull request Oct 25, 2019

Unconditionally run cargo install RTIM #4731

Closed

Licenser force-pushed the no-exit branch from f2b85f5 to 62483d7 Compare October 29, 2019 18:06

flip1995 mentioned this pull request Nov 7, 2019

Rollup of 4 pull requests #4789

Closed

Licenser and others added 8 commits November 7, 2019 17:10

Add lint for exit

695aa59

Update lints

60c2fdd

Exclude main from exit lint

9471669

Improve function checking

ffcf4be

Simplify dentry point detection

eae6a62

Update clippy_lints/src/lib.rs

a984702

Co-Authored-By: Philipp Krones <hello@philkrones.com>

Update clippy_lints/src/lib.rs

abdf027

Co-Authored-By: Philipp Krones <hello@philkrones.com>

Update tests for exit

5e6017d

Update lints

2f1370d

Licenser force-pushed the no-exit branch from 0bd8527 to 2f1370d Compare November 7, 2019 16:13

flip1995 mentioned this pull request Nov 7, 2019

Rollup of 4 pull requests #4790

Closed

bors added a commit that referenced this pull request Nov 7, 2019

Auto merge of #4697 - Licenser:no-exit, r=flip1995

4be144a

restriction lint for `std::process::exit` Addition to #4655 - adds the lint checking for `std::process::exit` changelog: add restriction lint for `std::process::exit`

bors merged commit 2f1370d into rust-lang:master Nov 7, 2019

bors mentioned this pull request Nov 7, 2019

Add unneeded_try_convert lint #4759

Closed

Conversation

Licenser commented Oct 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llogiq commented Oct 19, 2019

Uh oh!

llogiq commented Oct 19, 2019

Uh oh!

Licenser commented Oct 19, 2019

Uh oh!

flip1995 left a comment

Choose a reason for hiding this comment

Uh oh!

flip1995 Oct 21, 2019

Choose a reason for hiding this comment

Uh oh!

Licenser Oct 21, 2019

Choose a reason for hiding this comment

Uh oh!

flip1995 Oct 24, 2019

Choose a reason for hiding this comment

Uh oh!

flip1995 Oct 24, 2019

Choose a reason for hiding this comment

Uh oh!

Licenser Oct 24, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bors commented Oct 23, 2019

Uh oh!

Uh oh!

Uh oh!

Licenser commented Oct 25, 2019

Uh oh!

flip1995 commented Oct 25, 2019

Uh oh!

Licenser commented Oct 29, 2019

Uh oh!

flip1995 commented Oct 29, 2019

Uh oh!

Licenser commented Oct 29, 2019

Uh oh!

flip1995 commented Nov 7, 2019

Uh oh!

bors commented Nov 7, 2019

Uh oh!

bors commented Nov 7, 2019

Uh oh!

bors commented Nov 7, 2019

Uh oh!

flip1995 commented Nov 7, 2019

Uh oh!

bors commented Nov 7, 2019

Uh oh!

bors commented Nov 7, 2019

Uh oh!

bors commented Nov 7, 2019

Uh oh!

bors commented Nov 7, 2019

Uh oh!

Licenser commented Nov 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Licenser commented Oct 18, 2019 •

edited

Loading