Make clippy::todo and clippy::unimplemented warn by default.

[jyn514]

Unlike panic, it's almost never correct to have this in a production context.
Make them warn by default to avoid bugs slipping through.

It looks like this is the first time a lint level hasn't been solely determined by the group it's in, but I think it's correct: denying todo! is both a restriction, and should be a loud warning.

This would have caught a bug I introduced in my own codebase last week: cloudflare/wrangler-legacy#2012

changelog: Make [clippy::todo] and [clippy::unimplemented] warn by default.

[rust-highfive]

r? @Manishearth

(rust-highfive has picked a reviewer for you, use r? to override)

[jyn514]

This changed from tt to ident because it made the error message way easier to read. Before it was

error: no rules expected the token `LET_UNDERSCORE_MUST_USE`
  --> clippy_lints/src/let_underscore.rs:30:9
   |
30 |     pub LET_UNDERSCORE_MUST_USE,
   |         ^^^^^^^^^^^^^^^^^^^^^^^ no rules expected this token in macro call
   | 
  ::: clippy_lints/src/lib.rs:98:1
   |
98 | macro_rules! declare_clippy_lint {
   | -------------------------------- when calling this macro

now it's

error: no rules expected the token `LET_UNDERSCORE_MUST_USE`
   --> clippy_lints/src/lib.rs:131:45
    |
98  |   macro_rules! declare_clippy_lint {
    |   -------------------------------- when calling this macro
...
131 |           declare_clippy_lint! { $(#[$attr])* $name, restriction, $description, level = Allow }
    |                                               ^^^^^ no rules expected this token in macro call
    | 
   ::: clippy_lints/src/let_underscore.rs:11:1
    |
11  | / declare_clippy_lint! {
12  | |     /// **What it does:** Checks for `let _ = <expr>`
13  | |     /// where expr is #[must_use]
14  | |     ///
...   |
32  | |     "non-binding let on a `#[must_use]` expression"
33  | | }
    | |_- in this macro invocation
    |
    = note: this error originates in the macro `declare_clippy_lint` (in Nightly builds, run with -Z macro-backtrace for more info)

(the original issue was I forgot pub in front of clippy::$name below)

[jyn514]

The tests are failing because a bunch of lint tests are using unimplemented! - happy to add allow(clippy::unimplemented) on all of those, but I'd like to make sure this change will be accepted first, it will take a while to update them all.

[flip1995]

It looks like this is the first time a lint level hasn't been solely determined by the group it's in

I don't think we should do this. In our README we tie lint groups directly to lint levels. This makes things easy for users to understand them.

Also in the Clippy 1.0 RFC we definied the restriction group as:

Restriction (Allow): Lints for things which are not usually a problem, but may be something specific situations may dictate disallowing.

If those two macros are "almost never correct to have this in a production context", those lints should therefore not be in the restriction group.

---

Also @rust-lang/clippy should we move those lints to a warn-by-default group? I don't have a strong opinion here (about allow- vs warn-by-default).

[jyn514]

Gotcha. I can move them to correctness instead if you like?

[flip1995]

Correctness is deny-by-default. I'm not sure where the best place for those lints is, so I'd like input from others first.

[jyn514]

suspicious seems like a good fit, that's warn-by-default.

[Manishearth]

I don't think this should be warn by default, definitely not deny-by-default.

Unlike panic, it's almost never correct to have this in a production context.

This is a narrow understanding of the users of clippy; in fact the codebases I have worked on professionally in the past have always tolerated todo/unimplemented in certain regions of code. Just because you're running clippy on it doesn't mean it is production ready, it can be in an experimental component or something else.

[jyn514]

This is a narrow understanding of the users of clippy; in fact the codebases I have worked on professionally in the past have always tolerated todo/unimplemented in certain regions of code. Just because you're running clippy on it doesn't mean it is production ready, it can be in an experimental component or something else.

Hmm, your use case makes sense, but it seems strange to me that "todo is ok" is the default. You could always allow(clippy::todo) if it's fine for your codebase but I think it at least makes sense to give a heads up "hey this will panic and it doesn't look intentional".

[camsteffen]

This is a harder line to draw than with other restriction lints. I think the applicability of these lints is too dependent on project state/goals/preferences to be warn-by-default. Perhaps pedantic is better than restriction so that the lints are included in "loud mode". If it were to be warn-by-default, I'd put it in suspicious. We should maintain that the default level is the same per group.

[Manishearth]

Yeah I think it makes sense as either restriction or pedantic. But it varies from project to project.

Hmm, your use case makes sense, but it seems strange to me that "todo is ok" is the default. You could always allow(clippy::todo) if it's fine for your codebase but I think it at least makes sense to give a heads up "hey this will panic and it doesn't look intentional".

To me this is one of those cases where clippy can't supplant code review; todo/unimplemented are pretty obvious in code review already; and it highly depends on the context.

As I said earlier, Clippy's audience is not just people verifying "production code"; it's a far broader one. It's relatively common to check unimplemented/todos in when you're working on a large not-yet-production-ready feature.

[xFrednet]

I think it would make sense to enable this lint shortly before a release to inspect each of these reports. Having them as warn-by-default would be too strong IMO.

This case reminds me of the avoid_breaking_exported_api configuration. The change log entry had a note that stated:

... We recommend to set this configuration option to false before a major release (1.0/2.0/...) to clean up the API #7187

~ source

This is basically the second instance where we say: "You might want to look at these reports shortly before the release, but they are not bad per se". Currently, we basically expect the user to know these cases. What would you all think about adding a collection of check we suggest before releasing? This could be a simple section in the README.md or even a Clippy command if we want to go that far.

[ghost]

We had a suggestion to move all the 'production' lints into their own group - #5755 . It might be a good idea to make a new group and apply the new level to the group as a whole.

[xFrednet]

We had a suggestion to move all the 'production' lints into their own group

This could be a possibility. However, I wouldn't add it as a new standalone lint-group, but instead as an additional collection like clippy::all. This solution would sadly not cover the avoid_breaking_exported_api configuration 🤔

---

@rustbot label +S-needs-discussion

[flip1995]

I don't really want lints to be in multiple groups, if the one group is not a strict subset of another group. I'd be fine to add a new allow-by-default group called production, but only if we move the lints there completely and not get into a state where one lint can live in multiple groups. We should be careful though, that we don't start adding groups for every small use-case, because we aren't really allowed to remove groups again according to the Clippy 1.0 RFC. So the question here is if it is worth to add a whole new group for it? An alternative would be to just document lints that you should enable for production code.

[camsteffen]

The production category doesn't quite carry its weight IMO. The print_stdout lint could be controversial within the group, and the group as a whole feels pretty subjective. Plus it's questionable whether it solves the original problem here since it would be allow-by-default.

An alternative would be to just document lints that you should enable for production code.

I like this idea, if we say something like "depending on the project, some of these lints might be useful to enable at the crate level".

[xFrednet]

I'm also in favor of adding documentation instead of adding a new lint group. 👍