Rust-clippy: Lint to remove unimplemented!() macro

[hoodie]

Since production code should seldom contain the unimplemented macro please include a lint that asked to remove it.
Thank you

[hoodie]

@oli-obk actually, just assign me, why not, I always wanted to write a lint :D

[oli-obk]

Awesome!

You can probably hook into the lint in https://github.com/rust-lang-nursery/rust-clippy/blob/master/clippy_lints/src/panic.rs and just add a new if chain for "unreachable" and without all the argument checkery

bonus points for no code duplication

[hoodie]

@oli-obk how are the lints tested?

[oli-obk]

We have ui tests in tests/ui. You can create a new file there. cargo test runs all the tests and fails on those whose .stderr file does not match the expected output. To automatically generate or update the stderr file, run tests/ui/update-all-references.sh after cargo test

[mipli]

@oli-obk I wouldn't mind taking a go at this one. Been looking a bit at it, and got a few questions now.

The issue talks about unimplemented, while you mention unreachable in your post here. Should I implement a lint for each of those?

I was also looking at how to split up the code you pointed at to test for unimplemented and unreachable. It seems like using is_direct_expn_of works. But for unreachable I need to test for if let ExprBlock(_, _) = expr.node as well. I tried reading about what ExprBlock actually does, to figure out why it was needed for unreacable and not for unimplemented. That lead me to https://manishearth.github.io/rust-internals-docs/rustc/hir/enum.Expr_.html#variant.ExprBlock which didn't actually help me much. Could you point me in the right direction for reading more about the expression parsing that's being using in the lint code?

[hoodie]

@mipli please do - I wanted to originally but didn't find the time

[oli-obk]

Sorry, I meant unimplemented of course.

One thing that you can try is the #[clippy::author] attribute on an unimplemented!() call, which should dump most of the code you need to pattern match the call.

[mipli]

I tried to get the #[clippy::author] attribute to work, but it seems like it doesn't trigger. Works fine for a normal line of code, but not for unimplemented!().

Tried a few other things to figure out how to match properly against this, and it seems like the unimplemented! macro disappears very early and is replaced by a ::rt::begin_panic block instead. So I'm not quite sure how to use the AST syntax nodes to match against it.

In the end I got a working lint that you can see here: https://github.com/mipli/rust-clippy/commit/827f731e4968e1eec5296ff8eb57a4e747be99e8#diff-a6c58ff76e670a6f64b8b7b79167341dR30

To keep things a bit tidier while I figure out what I'm doing I'm keeping it separate from the panic lint, but if there's enough overlap in code when I'm done moving it back into that file should be easy.

Tips on how to proceed would be greatly appreciated.

[oli-obk]

Oh :D sorry you ran into this. Well... the easy part is, clippy already has a lint for panic!, so you can just start stealing code from there.

The important part is

https://github.com/rust-lang-nursery/rust-clippy/blob/master/clippy_lints/src/panic.rs#L45

Essentially try to find a call to begin_panic like in https://github.com/rust-lang-nursery/rust-clippy/blob/master/clippy_lints/src/panic.rs#L43 and check whether the span is inside an expansion of "unimplemented".

[mipli]

No problem, been learning a lot as I've been digging around trying to find a good solution for this :)