Don't warn on proc macro generated code in `needless_return` #13464

y21 · 2024-09-26T16:07:23Z

Fixes #13458
Fixes #13457
Fixes #13467
Fixes #13479
Fixes #13481
Fixes #13526
Fixes #13486

The fix is unfortunately a little more convoluted than just simply adding a is_from_proc_macro. That check does fix the issue, however it also introduces a bunch of false negatives in the tests, specifically when the returned expression is in a different syntax context, e.g. return format!(..).

The proc macro check builds up a start and end pattern based on the HIR nodes and compares it to a snippet of the span, however that would currently fail for return format!(..) because we would have the patterns ("return", <something inside of the format macro>), which doesn't compare equal. So we now return an empty string pattern for when it's in a different syntax context.

"Hide whitespace" helps a bit for reviewing the proc macro detection change

changelog: none

rustbot · 2024-09-26T16:07:29Z

r? @Alexendoo

rustbot has assigned @Alexendoo.
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

y21 · 2024-09-26T16:14:32Z

tests/ui/dbg_macro/dbg_macro.rs

@@ -1,5 +1,5 @@
 #![warn(clippy::dbg_macro)]
-#![allow(clippy::unnecessary_operation, clippy::no_effect)]
+#![allow(clippy::unnecessary_operation, clippy::no_effect, clippy::unit_arg)]


It now emits a warning on this line

rust-clippy/tests/ui/dbg_macro/dbg_macro.rs

Line 53 in c771204

bar(dbg!());

which looks like a true positive to me

y21 · 2024-09-26T16:30:39Z

Hmm, lintcheck reports a lot of new warnings. Still going through them, but most of them look like they originate in local macros, so that seems about expected. is_from_proc_macro shouldn't have returned true for them

jieyouxu · 2024-09-28T17:02:07Z

clippy_lints/src/returns.rs

@@ -407,7 +407,7 @@ fn check_final_expr<'tcx>(
                }
            }

-            if ret_span.from_expansion() {
+            if ret_span.from_expansion() || is_from_proc_macro(cx, expr) {


Note: if this is the Span::from_expansion that is the rustc one, then AFAIK in general it is the most robust check for spans from macro expansions that is possible, and I would not have expected is_from_proc_macro detection to not be already covered by from_expansion if the proc macro implements span hygiene correctly.

I suspect that some crates have proc-macros which generate spans that have call-site hygiene instead of mixed-site hygiene, causing from_expansion to return false.

But I suppose it's possible that clippy wants different trade-offs here and try to provide warnings even if the proc-macros do not emit spans with proper hygiene?

See: similar issue I ran into in implementing unit_bindings lint in rustc rust-lang/rust#112380 (comment).

Also see a Rocket PR where I used Span::mixed_site().located_at(span) so the proc-macro produces a Span which will correctly return true for Span::from_expansion because it has distinguishing SyntaxContext.

Yeah, the issue that is_from_proc_macro solves is crates using quote_spanned! with an input span like tokio does in the linked issues which makes it (to my knowledge) completely impossible to detect whether a span is from a proc macro, so this is a big hack that approximates it even further by checking if the code pointed at the span lines up with the AST.

@Alexendoo wrote a good comment about the proc macro situation in a zulip thread: https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/report_in_external_macro.20no.20longer.20configurable.3F/near/448135182

To be clear, I'd also love to get rid of this, but afaik we're kind of stuck with this hack for now and add such checks whenever needed because proc macro tooling like quote::quote_spanned! makes it incredibly easy to fool clippy and we get a lot of bug reports for it because a lot of proc macros do this

Right, that's entirely reasonable for clippy. I opened a zulip topic for T-compiler (and T-lang) to discuss the proc-macro span ~~hygiene~~ syntax context situation and if we can somehow make more people aware of the proc-macro span ~~hygiene~~ syntax context issues (because rustc lints/diagnostics also gets bitten by this quite often): https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/proc-macro.20span.20hygiene.

It's really not ideal when many, many crate authors get proc-macro ~~hygiene~~ syntax context wrong because there's not much awareness or guidance for it. This in turn causes diagnostic and lint issues in rustc and clippy.

EDIT: and an issue rust-lang/rust#130995
EDIT 2: hygiene -> syntax context, I am one of the people confused myself

clippy_utils/src/check_proc_macro.rs

…macro

track: rust-lang/rust-clippy#13464

rust-lang/rust-clippy#13464

y21 · 2024-10-08T07:56:50Z

Beta branches in 3-4 days, so I don't think this has a chance of making it into the same release as the change that caused this "regression".
It looks like the bug also has a huge impact given that #13458 managed to get the most amount of reactions out of all open issues on the repo in 2 weeks, so:

@rustbot label +beta-nominated

AaronKutch · 2024-10-08T22:13:51Z

Many repos have a CI that pulls in nightly clippy (so that people with varying versions of clippy don't have conflicts trying to run clippy on the repo), and the bug is making it unusable (usually when clippy has a problem I can just sprinkle #[allow in a few places, but this affects almost every subcrate). Can we just disable needless_return for now or is this going to be fixed very soon?

flip1995

That check does fix the issue, however it also introduces a bunch of false negatives in the tests

I couldn't find any FNs in the tests. Is this, because we just don't have any tests for this?

LGTM and I'd like to pre-backport this today, so that this bug doesn't even get into beta. (and this is the only nominated PR)

flip1995 · 2024-10-10T08:51:50Z

Lintcheck diff is mostly restriction lints, that all look like true positives and one instance of needless_return that's actually fixed by this PR.

y21 · 2024-10-10T08:54:04Z

I couldn't find any FNs in the tests. Is this, because we just don't have any tests for this?

I realize now I phrased that a bit poorly. With this I mean that the usual fix for these kinds of proc macro issues, adding a simple is_from_proc_macro() check as done in the first commit, would add false negatives. But I fixed those false negatives in the second commit.
If you remove the changes in the check_proc_macro.rs file and you bless the tests then you would see the removed warnings in the test file.

So, this PR doesn't add any FNs (as far as I'm aware) with both of the commits.

Also, I'm not sure exactly if this needs a beta backport if this got into the rust-lang/rust repo right now. I only added it prematurely because the next clippy sync is the 17th oct (I think) which would definitely be after the beta cutoff.

The PR that "introduced" the regression (well, it didn't really regress, it always warned on proc macros, but it specifically recognized the tokio::main macro output), was #13214, which isn't in beta right now I think.

flip1995 · 2024-10-10T09:16:10Z

Thanks for the clarification!

It doesn't need a beta backport, but, as you said, we can cherry-pick this PR over to rustc master, so it will get into beta tomorrow. So it needs a "master-backport".

I will do that, after this is merged.

@bors r+

bors · 2024-10-10T09:16:13Z

📌 Commit 55834a3 has been approved by flip1995

It is now in the queue for this repository.

bors · 2024-10-10T09:17:22Z

⌛ Testing commit 55834a3 with merge 8e60f14...

bors · 2024-10-10T09:28:10Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 8e60f14 to master...

flip1995 · 2024-10-10T09:41:14Z

rust-lang/rust#131492

This wasn't really a beta backport, but a master backport. So for changelog writing, this PR probably doesn't matter, as it fixes a bug/FP that was introduced in this cycle.

…atthiaskrgr Clippy: Backport `needless_return` fix r? `@Manishearth` This cherry-picks rust-lang/rust-clippy#13464, so that it gets into master and with that into `beta` tomorrow, so that the bug in this lint doesn't hit `beta`. Changes look quite big, but most of them are whitespace changes because of the introduction of an `_inner` function. In reality it only adds 2 checks.

…=matthiaskrgr Clippy: Backport `needless_return` fix r? `@Manishearth` This cherry-picks rust-lang/rust-clippy#13464, so that it gets into master and with that into `beta` tomorrow, so that the bug in this lint doesn't hit `beta`. Changes look quite big, but most of them are whitespace changes because of the introduction of an `_inner` function. In reality it only adds 2 checks.

…=matthiaskrgr Clippy: Backport `needless_return` fix r? ``@Manishearth`` This cherry-picks rust-lang/rust-clippy#13464, so that it gets into master and with that into `beta` tomorrow, so that the bug in this lint doesn't hit `beta`. Changes look quite big, but most of them are whitespace changes because of the introduction of an `_inner` function. In reality it only adds 2 checks.

…atthiaskrgr Clippy: Backport `needless_return` fix r? `@Manishearth` This cherry-picks rust-lang/rust-clippy#13464, so that it gets into master and with that into `beta` tomorrow, so that the bug in this lint doesn't hit `beta`. Changes look quite big, but most of them are whitespace changes because of the introduction of an `_inner` function. In reality it only adds 2 checks.

add proc macro check to needless_return

c8725f7

rustbot assigned Alexendoo Sep 26, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 26, 2024

y21 commented Sep 26, 2024

View reviewed changes

y21 force-pushed the issue13458 branch from 9cc0e57 to 99d85f8 Compare September 26, 2024 16:46

jieyouxu reviewed Sep 28, 2024

View reviewed changes

y21 mentioned this pull request Sep 28, 2024

Clippy warns "unneeded return statement" for #[tokio::{main,test}] fns on latest nightly tokio-rs/tokio#6869

Closed

Alexendoo reviewed Sep 29, 2024

View reviewed changes

clippy_utils/src/check_proc_macro.rs Outdated Show resolved Hide resolved

deal with differing syntax contexts for subexpressions in check_proc_…

55834a3

…macro

y21 force-pushed the issue13458 branch from 99d85f8 to 55834a3 Compare September 29, 2024 13:59

This was referenced Sep 30, 2024

rustc 1.83.0-nightly clippy wrongly report needless_return #13479

Closed

needless_return triggered without a return statement #13481

Closed

ralexstokes added a commit to ralexstokes/mev-rs that referenced this pull request Oct 1, 2024

silence clippy warning (should be temporary)

f8696b4

track: rust-lang/rust-clippy#13464

y21 mentioned this pull request Oct 1, 2024

False needless_return trigger + invalid suggestion #13486

Closed

coderabbitai bot mentioned this pull request Oct 7, 2024

Feat: support filer helyim/helyim#127

Open

Nugine added a commit to Nugine/s3s that referenced this pull request Oct 8, 2024

style: temporarily allow clippy::needless_return

6604fdd

rust-lang/rust-clippy#13464

MrCroxx mentioned this pull request Oct 8, 2024

refactor: make clippy happy with nightly-20241008 fast/fastrace#46

Merged

rustbot added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 8, 2024

y21 mentioned this pull request Oct 10, 2024

False positive for what Clippy thinks are return statements, but which aren't. #13526

Closed

flip1995 approved these changes Oct 10, 2024

View reviewed changes

bors merged commit 8e60f14 into rust-lang:master Oct 10, 2024
8 checks passed

flip1995 mentioned this pull request Oct 10, 2024

Clippy: Backport needless_return fix rust-lang/rust#131492

Merged

flip1995 added beta-accepted Accepted for backporting to the compiler in the beta channel. and removed beta-nominated Nominated for backporting to the compiler in the beta channel. labels Oct 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't warn on proc macro generated code in `needless_return` #13464

Don't warn on proc macro generated code in `needless_return` #13464

y21 commented Sep 26, 2024 •

edited

Loading

rustbot commented Sep 26, 2024

y21 Sep 26, 2024

y21 commented Sep 26, 2024

jieyouxu Sep 28, 2024 •

edited

Loading

y21 Sep 28, 2024

jieyouxu Sep 28, 2024 •

edited

Loading

y21 commented Oct 8, 2024 •

edited

Loading

AaronKutch commented Oct 8, 2024

flip1995 left a comment

flip1995 commented Oct 10, 2024

y21 commented Oct 10, 2024 •

edited

Loading

flip1995 commented Oct 10, 2024

bors commented Oct 10, 2024

bors commented Oct 10, 2024

bors commented Oct 10, 2024

flip1995 commented Oct 10, 2024

Don't warn on proc macro generated code in needless_return #13464

Don't warn on proc macro generated code in needless_return #13464

Conversation

y21 commented Sep 26, 2024 • edited Loading

rustbot commented Sep 26, 2024

y21 Sep 26, 2024

Choose a reason for hiding this comment

y21 commented Sep 26, 2024

jieyouxu Sep 28, 2024 • edited Loading

Choose a reason for hiding this comment

y21 Sep 28, 2024

Choose a reason for hiding this comment

jieyouxu Sep 28, 2024 • edited Loading

Choose a reason for hiding this comment

y21 commented Oct 8, 2024 • edited Loading

AaronKutch commented Oct 8, 2024

flip1995 left a comment

Choose a reason for hiding this comment

flip1995 commented Oct 10, 2024

y21 commented Oct 10, 2024 • edited Loading

flip1995 commented Oct 10, 2024

bors commented Oct 10, 2024

bors commented Oct 10, 2024

bors commented Oct 10, 2024

flip1995 commented Oct 10, 2024

Don't warn on proc macro generated code in `needless_return` #13464

Don't warn on proc macro generated code in `needless_return` #13464

y21 commented Sep 26, 2024 •

edited

Loading

jieyouxu Sep 28, 2024 •

edited

Loading

jieyouxu Sep 28, 2024 •

edited

Loading

y21 commented Oct 8, 2024 •

edited

Loading

y21 commented Oct 10, 2024 •

edited

Loading