Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix FN in needless_return #10110

Merged
merged 3 commits into from
Jan 1, 2023
Merged

Conversation

Niki4tap
Copy link
Contributor

@Niki4tap Niki4tap commented Dec 22, 2022

Fixes #10051

changelog: Enhancement: [needless_return]: Now detects more cases for returns of owned values
#10110

@rustbot
Copy link
Collaborator

rustbot commented Dec 22, 2022

r? @Alexendoo

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 22, 2022
@xFrednet
Copy link
Member

Hey @Niki4tap, I wasn't sure what you meant with the changelog entry. My guess is, that this changes makes sure that not borrowed values are now detected properly? I've updated the changelog entry accordingly. Did I get it right? 🙃

@Niki4tap
Copy link
Contributor Author

Yeah, that's probably it, sorry, not sure what I've meant there myself to be completely honest. I was thinking of something along the lines of "improving the last_statement_borrows function", but more concise, and wrote something that makes no sense apparently 😅

@xFrednet
Copy link
Member

No problem at all, I'm happy that you took the time to think about it and you also responded super quick! :)

@Niki4tap
Copy link
Contributor Author

Oh, and, thanks for the correction ;)

@@ -292,7 +295,7 @@ fn last_statement_borrows<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>)
{
ControlFlow::Break(())
} else {
ControlFlow::Continue(Descend::from(!expr.span.from_expansion()))
ControlFlow::Continue(Descend::from(!e.span.from_expansion()))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed this part and spent far too long wondering how the changes above this fixed the issue 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awwww, I made commits separate specifically for this :v

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah thanks for that, I ended up spotting it because I did a git log and saw the separate fix commit

@Alexendoo
Copy link
Member

Thanks @Niki4tap!

@bors r+

@bors
Copy link
Collaborator

bors commented Jan 1, 2023

📌 Commit 9ff868c has been approved by Alexendoo

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jan 1, 2023

⌛ Testing commit 9ff868c with merge a85e480...

@bors
Copy link
Collaborator

bors commented Jan 1, 2023

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

@bors bors merged commit a85e480 into rust-lang:master Jan 1, 2023
@Niki4tap Niki4tap deleted the needless_anyhow_return branch January 1, 2023 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

needless_return doesn't trigger properly with anyhow! macro
5 participants