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.

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

[joshtriplett]

* Is `todo` the right name for the lint?

One reason we ultimately settled on todo_macro_uses was that we wanted to reserve todos as a potential future lint group that might also include a lint for // TODO comments. (That should not happen here, but we'd invite a future PR providing such a lint.)

* Should the lint warn on code outside the local crate (i.e. from an external macro?)

I think what should happen there, assuming it's trivial, is that we lint at the point where todo!() is written in the macro, not at the point where the macro that calls todo!() is invoked; that way, you get one warning for writing todo!() and not a warning on each invocation.

That said, we can refine it further in the future.

and furthermore should the todo-check also take locality into account (I think so)?

We weren't clear on what you were asking here.

[llogiq]

We weren't clear on what you were asking here.

I was asking about the check to suppress the unreachable_code lint. I think we should only suppress the lint if the macro call originates in local code.

I'm totally on board with the todo_macro_uses name. todo was just a standin anyway.

I'll see if I find the time to write up an RFC, too.

[traviscross]

I was asking about the check to suppress the unreachable_code lint. I think we should only suppress the lint if the macro call originates in local code.

That's an interesting question. E.g., should someone be able to write and publish a wrapper macro and expect for users of it to get the same behavior?

#[macro_export]
macro_rules! dbg_todo {
    ($x:expr) => {
        dbg!($x);
        todo!()
    };
}

Maybe there are plausible use cases. If we didn't do it by default, I could imagine wanting to give macro authors some way to opt-in to it. In any case, this would be a good topic for an RFC on this to discuss in some detail.

[llogiq]

I think that while this may be an option to give macro authors, I feel like this is besides the goal I followed with this PR, that is reducing lint churn while working on code, under the assumption that todo!() is used as a stand-in for code yet to be written, as unlikely as unwanted to remain in production code.

[traviscross]

Yes, makes sense.

[llogiq]

I seem to have a problem getting the todo_macro_uses lint to actually lint something. Probably I'm missing some context action.

[llogiq]

So now I got the lint to do what I want, however the todo_macro symbol is now both declared in rustc and clippy (because it was moved there in a recent PR). I can remove it from clippy here and also submit a clippy PR to sync it back once this PR is merged.

@traviscross is that OK? Do you require further changes?

[rustbot]

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

[llogiq]

Lang RFC: rust-lang/rfcs#3928

[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.

[rust-log-analyzer]

The job x86_64-gnu-miri failed! Check out the build log: (web) (plain enhanced) (plain)

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

tests/pass/shims/x86/rounding-error.rs ... ok
tests/pass/shims/x86/intrinsics-x86-gfni.rs ... ok

FAILED TEST: tests/pass/box-custom-alloc-aliasing.rs (revision `stack`)
command: MIRI_ENV_VAR_TEST="0" MIRI_TEMP="/tmp/miri-uitest-IKJzoj" RUST_BACKTRACE="1" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/release/miri" "--error-format=json" "--sysroot=/checkout/obj/build/x86_64-unknown-linux-gnu/miri-sysroot" "-Dwarnings" "-Dunused" "-Ainternal_features" "-Zui-testing" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/tmp/miri_ui/0/tests/pass" "tests/pass/box-custom-alloc-aliasing.rs" "--edition" "2021" "--cfg=stack" "-Cextra-filename=stack"

error: test got exit status: 1, but expected 0
 = note: compilation failed, but was expected to succeed

error: no output was expected
---
   |
LL |             todo!("Deallocating from another thread");
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `-D todo-macro-uses` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(todo_macro_uses)]`

error: `todo!()` macro used
##[error]  --> tests/pass/box-custom-alloc-aliasing.rs:95:13
   |
LL |             todo!("Deallocating from another thread");
---
   |
LL |             todo!("Deallocating from another thread");
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `-D todo-macro-uses` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(todo_macro_uses)]`

error: `todo!()` macro used
##[error]  --> tests/pass/box-custom-alloc-aliasing.rs:95:13
   |
LL |             todo!("Deallocating from another thread");
---



FAILED TEST: tests/pass/box-custom-alloc-aliasing.rs (revision `tree`)
command: MIRI_ENV_VAR_TEST="0" MIRI_TEMP="/tmp/miri-uitest-IKJzoj" RUST_BACKTRACE="1" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/release/miri" "--error-format=json" "--sysroot=/checkout/obj/build/x86_64-unknown-linux-gnu/miri-sysroot" "-Dwarnings" "-Dunused" "-Ainternal_features" "-Zui-testing" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/tmp/miri_ui/0/tests/pass" "tests/pass/box-custom-alloc-aliasing.rs" "-Zmiri-tree-borrows" "--edition" "2021" "--cfg=tree" "-Cextra-filename=tree"

error: test got exit status: 1, but expected 0
 = note: compilation failed, but was expected to succeed

error: no output was expected
---
   |
LL |             todo!("Deallocating from another thread");
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `-D todo-macro-uses` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(todo_macro_uses)]`

error: `todo!()` macro used
##[error]  --> tests/pass/box-custom-alloc-aliasing.rs:95:13
   |
LL |             todo!("Deallocating from another thread");
---
   |
LL |             todo!("Deallocating from another thread");
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `-D todo-macro-uses` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(todo_macro_uses)]`

error: `todo!()` macro used
##[error]  --> tests/pass/box-custom-alloc-aliasing.rs:95:13
   |
LL |             todo!("Deallocating from another thread");
---
Location:
   /cargo/registry/src/index.crates.io-1949cf8c6b5b557f/ui_test-0.30.3/src/lib.rs:365

  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ BACKTRACE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
   1: <color_eyre[60d19ed43da51bae]::config::EyreHook>::into_eyre_hook::{closure#0}<unknown>
      at <unknown source file>:<unknown line>
   2: <eyre[45a11d7876b850fd]::Report>::from_adhoc::<&str><unknown>
      at <unknown source file>:<unknown line>
   3: ui_test[3f003d28b4763a7a]::run_tests_generic::<ui_test[3f003d28b4763a7a]::default_file_filter, ui[bc9ff0921c91042b]::run_tests::{closure#1}, alloc[de87b3fdd8942b35]::boxed::Box<dyn ui_test[3f003d28b4763a7a]::status_emitter::StatusEmitter>><unknown>
      at <unknown source file>:<unknown line>
   4: ui[bc9ff0921c91042b]::ui<unknown>
      at <unknown source file>:<unknown line>
   5: ui[bc9ff0921c91042b]::main<unknown>
      at <unknown source file>:<unknown line>
   6: std[80ac9c8529bc16af]::sys::backtrace::__rust_begin_short_backtrace::<fn() -> core[d2f919e2a04434db]::result::Result<(), eyre[45a11d7876b850fd]::Report>, core[d2f919e2a04434db]::result::Result<(), eyre[45a11d7876b850fd]::Report>><unknown>
      at <unknown source file>:<unknown line>
   7: std[80ac9c8529bc16af]::rt::lang_start::<core[d2f919e2a04434db]::result::Result<(), eyre[45a11d7876b850fd]::Report>>::{closure#0}<unknown>
      at <unknown source file>:<unknown line>
   8: std[80ac9c8529bc16af]::rt::lang_start_internal<unknown>
      at <unknown source file>:<unknown line>
   9: main<unknown>
      at <unknown source file>:<unknown line>
  10: __libc_start_main<unknown>
      at <unknown source file>:<unknown line>
  11: _start<unknown>
      at <unknown source file>:<unknown line>

Run with COLORBT_SHOW_HIDDEN=1 environment variable to disable frame filtering.
Run with RUST_BACKTRACE=full to include source snippets.
error: test failed, to rerun pass `--test ui`

Caused by:
  process didn't exit successfully: `/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/release/deps/ui-967339ea3dcc1e54` (exit status: 1)
Command `/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo test -Zwarnings --target x86_64-unknown-linux-gnu -Zbinary-dep-depinfo -j 4 -Zroot-dir=/checkout --locked --color=always --profile=release --manifest-path /checkout/src/tools/miri/Cargo.toml -- [workdir=/checkout]` failed with exit code 1
Created at: src/bootstrap/src/core/build_steps/tool.rs:191:21
Executed at: src/bootstrap/src/core/build_steps/test.rs:739:19

Command has failed. Rerun with -v to see more details.
Bootstrap failed while executing `test --stage 2 src/tools/miri src/tools/miri/cargo-miri`
Build completed unsuccessfully in 0:47:25
  local time: Sun Mar 15 15:40:21 UTC 2026
  network time: Sun, 15 Mar 2026 15:40:22 GMT
##[error]Process completed with exit code 1.

[samueltardieu]

@llogiq I don't understand most of the use changes you did to the Clippy side: you don't need to import rustc_span::sym directly, all symbols defined there are visible through clippy_utils::source::sym (which adds symbols). sym::todo_macro will work without rsym::todo_macro.

[llogiq]

@samueltardieu thanks, I didn't know that.