Allow unused variables with todo!

[charles-r-earp]

Fixes #78136

As stated in the issue, todo! should imply that the function will be implemented at some point but isn't yet, and therefore the user shouldn't have to either apply the #[allow(unused_variables)] themselves or prefix the arguments with underscores, since they will at some point replace the todo! with an implementation. Unlike unimplemented, todo! is a placeholder, so the suggested prefixing of the arguments with underscores does not make sense because that would imply that those arguments are not going to be used, when in fact they may be used when the function is implemented. Once the todo! is removed, the lint will return if applicable.

The lint error is emitted in rustc_passes::liveness::Liveness::report_unused. This can be traced back to the <IrMaps as Visiter>::visit_body method. Unfortunately the Body argument here has been fully expanded, so we cannot match on the todo! macro directly. Instead, the idea is to detect a trailing expression, ie the last expression without a semicolon, that is core::panicking::panic (also std). If the panic is called with "not yet implemented", which is what is emitted by todo!, then we return early and don't do any checking for unused variables.

For example:

fn foo(x: i32) -> i32 {
    // some code
    todo!()
}

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @joshtriplett (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[jonas-schievink]

Macros cannot expand to inner attributes at all

[scottmcm]

I think this PR should probably have a ui test to confirm that it's working?

[charles-r-earp]

I think this PR should probably have a ui test to confirm that it's working?

You mean a unit test? Where would that go? In core/tests.rs?

[scottmcm]

No, a ui test. See https://rustc-dev-guide.rust-lang.org/tests/adding.html#guide-to-the-ui-tests

[camelid]

The UI test is failing; I think this is the wrong lang item. Sorry about that! I think we need to change this to catch core::panicking::panic and core::panicking::panic_fmt, but unfortunately there doesn't appear to be a lang item for panic_fmt. So you may need to go back to using string comparisons :/

However, I'm out of my depth here so maybe someone else knows a better way (e.g. @m-ou-se).

If you do end up needing to use the string comparison approach, you may want to use the TyCtxt::def_path_str() method to get a path string for path_def_id. You would use it like:

let path_str = self.tcx.def_path_str(path_def_id);

[camelid]

I would still include something saying that this is a somewhat hacky way to check if it's todo!.

[camelid]

What does this mean? Why did self.tcx.def_path_str not work?

[charles-r-earp]

It errors with:

thread 'rustc' panicked at 'no warnings or errors encountered even though `delayed_good_path_bugs` issued', compiler/rustc_errors/src/lib.rs:974:13

[camelid]

By the way, why does this use [?

[charles-r-earp]

The path_str is like core[76aa]::panicking::panic. I was concerned about matching on some arbitrary crate with a matching path, though it's a bit unlikely. I actually think it should be ~ core[" .. "]::panicking::panic" .. just to ensure that it only triggers for std / core.

[bors]

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

[JohnCSimon]

Ping from triage - @charles-r-earp
can you please resolve the merge conflict? thank you.

@rustbot label: -S-waiting-on-review +S-waiting-on-author

[scottmcm]

I like the thought of todo!() adding a warning instead of the unused variable warning, as one warning for "you have a pending todo" seems more friendly than having a "unused variable" warning for potentially multiple things.

[nikomatsakis]

Something I have often found very useful when developing:

Some way to trigger a warning during development that would be a hard error during ci.

I use this to "make promises" to myself -- but also to my reviewer! -- when doing a patch series. e.g., I might write something broken but include a note that I will get around to fixing it later.

In rustc development, I lean on tidy -- if you write // FIXME (I think?) it gives you an error in the tidy service, or it used to. I found this super useful to leave myself notes that I had to come back to before the code could land.

Maybe todo! could serve that purpose.

[camelid]

In rustc development, I lean on tidy -- if you write // FIXME (I think?) it gives you an error in the tidy service, or it used to. I found this super useful to leave myself notes that I had to come back to before the code could land.

This doesn't seem to be true anymore. I found 1012 FIXMEs in compiler/.

[scottmcm]

IIRC clippy will allow // FIXME but not // TODO.

[nikomatsakis]

@camelid yeah, my bad, this used to be true; but we do still forbid // TODO so that's typically what I use

[crlf0710]

Triage: It's not clear what the next steps should be here. From my understanding the next steps would be convert this comment into an issue describing what's the exact language behavior we want, and closing this PR. Could someone on t-lang confirm with this?

[nikomatsakis]

I am nominating this PR for @rust-lang/lang team discussion.

[nikomatsakis]

We discussed this in our @rust-lang/lang meeting on 2021-05-04. There was definitely a block of us -- maybe even a consensus -- who felt that, early on in the development process, the avalanche of warnings from rustc for "WIP" code could be more annoying than helpful. However, there was generally not a lot of enthusiasm for this particular proposal.

One thing we were thinking about is the proposal from RFC 2383 (tracking issue: #54503) to add a #[expect(lint)] level. This level would silence the given lint within the given scope -- but report an error if the lint does not fire.

The core idea for this was to enable people to silence "expected" lints for WIP things without accidentally leaving those silences lints in production code (a theme which also arose on this PR).

Based on the conversation, I'm going to go and ahead and move to close, but if folks from @rust-lang/lang feel that's the wrong decision, please speak up. In any case, I'd like to encourage people to consider work on the #[expect] idea!

@rfcbot fcp close

[rfcbot]

Team member @nikomatsakis has proposed to close this. The next step is review by the rest of the tagged team members:

• @cramertj
• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[danielhenrymantilla]

I don't know where to discuss about this expect thing, but there is also the possibility for a #[fixme(<lint, e.g.> unused_variables)]-kind of lint-level:

• it'd behave as allow, silencing the associated lint / lint groups;

• if at least one #[fixme] attribute is present in a crate, then exactly one warning pops up warning against the presence of fixme attributes in the crate.

The idea being that exactly one warning is bearable on the dev iteration cycle, but it's still enough of a warning to avoid accidentally releasing it on production (we could even envision an extra bool parameter on action-rs, that would, by default, set a global flag / env var so as to deny that fixme warning, but which would still let the option to opt into just warning about it).

[scottmcm]

@rfcbot reviewed

---

Another possibility that comes to mind here is somehow deemphasizing some of the less important stylistic lints when there are also bigger concerns found. I agree that while in the middle of working on something I tend not to care about, say, having accidentally put parens around my if condition, but I more likely would like to see that I typo'd a constant so one of the match arms is unreachable, or similar. (I don't actually have a suggestion for how to do that, just that I'm sympathetic to the general goal here even if I think closing this specific check is the right way to go.)

[nikomatsakis]

@danielhenrymantilla the RFC in question is old enough that I personally wouldn't object to a fresh RFC in this area. I prefer the expect approach for most of the cases that affect me personally though. I think the first step might be to brainstorm on internals and better identify the "status quo' (what are the things people find annoying), though.

[joshtriplett]

I'd love to have expect exactly as specified in the RFC, and I also like the idea of a version of expect that hides all warnings of a given type and shows one warning that those warnings were hidden. expect would address the case of "this isn't actually an issue in this code, but let me know if the compiler stops complaining about it"; fixme would address the case of "this is an issue, but it's not the current issue, so keep telling me about it but collapse all such warnings to one".

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

The final comment period, with a disposition to close, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

[nikomatsakis]

Going to close. Thanks to all.