needless_return: Support #[expect] on the return statement

[xFrednet]

A fix for #9361 suppresses clippy::needless_return if there are any attributes on the return statement. This leads to some unexpected behavior, as described in #12998, where adding #[expect(clippy::needless_return)] suppresses the lint, but doesn't fulfill the expectation.

I now decided to manually fulfill any expectations, if they are before the attribute check.

---

Closes: #12998

changelog: none

[rustbot]

r? @Jarcho

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

[Jarcho]

This does mean something like:

fn f() -> i32 {
    #[expect(clippy::needless_return)]
    #[some_other_attribute]
    return 1;
}

will fulfill the expectation even though it technically shouldn't. I don't think this is a problem in practice, but it can be fixed by checking if the only attribute on the statement is #[expect(clippy::needless_return)].

[xFrednet]

I considered parsing the attributes again, just checking for the attribute, might not be enough, since the expectation could also come from a higher level. It would basically end up parsing the entire all attributes again, to see if they are specific to this case. I could do that, but it felt like unnecessary complexity. Do you think I should do that?

[Jarcho]

What's a "higher level" referring to here? Do you mean a lint group or something higher in the AST?

[xFrednet]

With higher level, I meant in the AST. Sorry for not clarifying this. However, the group is also a good example that I didn't consider. I was thinking of this

#[expect(needless_return)]
fn ex() {
    #[expect(lint_a)]
    return n;
}

This would have the expect lint level for needless_return but the expect attribute above the return is not the one that set the level

[Jarcho]

If it's higher in the AST then it should be unfulfilled. This is a variation on my original point.

Since needless_continue doesn't trigger in this:

fn ex() {
    #[expect(lint_a)]
    return n;
}

Adding #[expect(needless_continue)] to the function should fail with an unfulfilled expectation.

---

For the case of lint groups I would want to disallow expect from being able to use them. Although #[expect(warnings)] has a single obvious way to interpret it that interpretation is kind of dumb and defeats the purpose of using expect in the first place.

It's easy enough to work around anyways. If the return has either no attributes; or a single attribute which is #[expect(clippy::needless_return)], #[expect(clippy::style)], #[expect(clippy::all)], or #[expect(warnings)] then trigger the lint as normal.

[xFrednet]

I believe warnings is not a real group as it acts on all warnings and not specific lints.

I've now implemented your suggestion, where I check if there is only one attribute and that expects the either needless_return, style, or all. Let me know if there are any more changes that should be done :D

[Jarcho]

#[expect(warnings)] needs to be handled as well. The expectation is fulfilled if any warning occurs.

Can you add a comment next to the lint category mentioning that any changes to the category need to change the implementation as well? The test should catch this, but it's nicer to be warned up front about it.

[xFrednet]

I have the feeling #[expect] warnings makes no sense. I tried this (playgroud):

pub fn expect_warnings() -> i32 {
    let x = 1;

    if true {
        #[expect(warnings)]
        return x;
    }

    x + 2
}

And the expectation is always fulfilled, no matter what I do. But I've added it now as a test and to the code as well.

[Jarcho]

I guess I should've played with it more. Oh well.

@bors r+

[bors]

📌 Commit 903874d has been approved by Jarcho

It is now in the queue for this repository.

[bors]

⌛ Testing commit 903874d with merge a4bdab3...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Jarcho
Pushing a4bdab3 to master...