Reduce unreachable-code churn after todo!()

[llogiq]

View all comments

This was prompted by a rant by /u/matthieum on /r/rust. The idea here is that while writing the code, either rust-analyzer or ourselves may insert todo!() at places we intend to implement later. Unfortunately, that marks all following code as unreachable, which will prompt some lint churn.

So to combat that, I modify the lint to check whether the unreachability stems from a todo!() macro and in this case omit the lint (by setting diverges to Diverges::WarnedAlways instead of Always(..)). Hopefully that makes the unreachable code lint less churn-y during development and improve the developer experience.

In lieu of the unreachable_code lint, this PR also adds a todo_macro_uses lint.

I inserted the check after when we already found some unreachable code, so perf shouldn't suffer too badly.

[rustbot]

r? @fee1-dead

rustbot has assigned @fee1-dead.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[Kivooeo]

Could you please also add a link to the Zulip discussion? It would help provide context.

Also, could you please add a test to demonstrate that it works?

[llogiq]

If we find that this is too heavy on perf (which it would only be in macro-heavy crates where macros produce a lot of exprs of type !), we could first check that the expression has ExprKind::Call(path, [txt]) where path refers to core::panic::panic.

[iago-lito]

Here is the zulip thread.

[llogiq]

Also, could you please add a test to demonstrate that it works?

I added some code to the respective UI test. E.g. current rustc warns on todo!(); let _ = false.

[llogiq]

r? @estebank

[llogiq]

Open question: Should I add some docs to the todo! macro to describe the behavior?

[iago-lito]

I'd personally be happy to read a line about it in the docs, although I would understand that someone steps forward and explains whether/why it'd be off-topic?

[llogiq]

I'm unsure whether this is too cross-cutting a concern, which is why I'm asking.

OTOH, creating another PR just for one line of docs seems wasteful.

[llogiq]

Perhaps something like:

/// The `unreachable_code` lint will ignore code after a `todo!()`. The code will
/// however still be marked as unreachable, which may
/// have effects on type and lifetime checks.

This implies that todo!() could still be in released code without the compiler giving a warning. Perhaps we should upstream clippy's todo lint next and make it warn by default in release mode?

[WaffleLapkin]

r? me

View changes since this review

[WaffleLapkin]

It's a bit hard to discover, but the right directory for the test is tests/ui/reachable

[llogiq]

I'll move the test under a todo.rs there.

[WaffleLapkin]

Can you also add a test for when another macro expands to todo!?

[llogiq]

Sure! 👍

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[WaffleLapkin]

I think having different lints for debug/release is a bad idea, but mentioning this change in todo's docs is good. Either way, this needs T-lang approval, as this is a lint change. Please write a comment explaining the proposal to the language team and nominate this PR for them.

[llogiq]

T-lang nomination

This PR makes the unreachable_code lint ignore code after the todo!() macro. The reasoning behind this is that todo!() is meant as a temporary shortcut while the code is unfinished, and various tools (e.g. rust-analyzer) will insert it at many places as a stand in for code to add, leading to a plethora of unhelpful lint messages, which some perhaps might even consider a false positive. So by avoiding linting unreachable code after todo!(), the user can concentrate on the things they should.

The downside is that a developer won't get any warning on a todo!(). There's a clippy lint against that, though, which we could upstream.

[llogiq]

@rustbot ready

[traviscross]

@rfcbot concern please-rename-lint

(Filing this just to ensure the rename to todo_macro_calls.)

[RalfJung]

Please also update the PR description, it seems to be quite outdated at the moment.

[rustbot]

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred in rustc_ty_utils::consts.rs

cc @BoxyUwU

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

This PR changes a file inside tests/crashes. If a crash was fixed, please move into the corresponding ui subdir and add 'Fixes #' to the PR description to autoclose the issue upon merge.

This PR changes rustc_public

cc @oli-obk, @celinval, @ouz-a, @makai410

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred in compiler/rustc_sanitizers

cc @rcvalle

Some changes occurred in compiler/rustc_builtin_macros/src/autodiff.rs

cc @ZuseZ4

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[llogiq]

I think I fixed most errors in the tests and dogfooding of the lint. I have yet to rename it though.

[rustbot]

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

I removed the crate in question

[rustbot]

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

[rust-log-analyzer]

The job pr-check-1 failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
    ::: src/tools/clippy/clippy_utils/src/ast_utils/mod.rs:1023:65
     |
1023 |         (AttrItemKind::Parsed(_l), AttrItemKind::Parsed(_r)) => todo!(),
     |                                                                 ------- in this macro invocation
     |
     = note: `#[warn(todo_macro_calls)]` on by default

[RUSTC-TIMING] tracing_error test:false 0.199
[RUSTC-TIMING] icu_properties test:false 2.446
    Checking idna_adapter v1.2.1
[RUSTC-TIMING] idna_adapter test:false 0.127

[llogiq]

I'm adding a boatload of allow annotations here, including on subprojects. At this point I wonder whether we should allow the lint in bootstrap somehow...

[rust-bors]

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

[camsteffen]

I'm a bit unsure about the lint being warn by default. I think I would prefer it allow by default and I'd maybe turn it on for CI builds. (And I generally expect dev tools to favor the local development experience by default.) The mere presence of the macro in my code/diff, and the panicking behavior, is signal enough for me. But seeing the warning on every check build can be noisy when I'm working on something else in the moment.

Turning the lint on by default makes the compiler more loud by default since IIUC most todo uses don't have unreachable code following it. On the other hand, allow by default makes the compiler less loud, but I think the delta is smaller.

[llogiq]

Yeah, I believe we settled on warn by default to avoid people running into unwanted panics. But I agree that it probably should be allow by default (I wish we had an option of making "allow in debug, warn on release" standard behavior).