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.

Sign up for GitHub

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

Jump to bottom

implicit_return improvements #6951

Merged
merged 4 commits into from
Apr 30, 2021
Merged

implicit_return improvements #6951

merged 4 commits into from
Apr 30, 2021

Conversation

Jarcho
Copy link
Contributor

@Jarcho Jarcho commented Mar 22, 2021

fixes: #6940

changelog: Fix implicit_return suggestion for async functions
changelog: Improve implicit_return suggestions when returning the result of a macro
changelog: Check for break expressions inside a loop which are then implicitly returned
changelog: Allow all diverging functions in implicit_return, not just panic functions

@rust-highfive
Copy link

r? @flip1995

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 22, 2021
@Jarcho Jarcho force-pushed the implicit_return_fp branch 6 times, most recently from 11916da to ffa25ec Compare March 23, 2021 12:32
@bors
Copy link
Contributor

bors commented Mar 25, 2021

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

@Jarcho Jarcho force-pushed the implicit_return_fp branch from ffa25ec to edb76df Compare March 26, 2021 20:44
@flip1995
Copy link
Member

Sorry for taking so long with my reviews recently...

Is there any chance you can split this PR in multiple commits?

@Jarcho Jarcho force-pushed the implicit_return_fp branch from edb76df to 267834e Compare April 2, 2021 21:28
@Jarcho
Copy link
Contributor Author

Jarcho commented Apr 2, 2021

Ended up doing a bit of a rewrite at the same time. The code would be simpler to add a return to the top level expression rather to each individual sub expression (e.g. conditionals get a return added to each branch), but that would change the behaviour of the lint.

@Jarcho Jarcho force-pushed the implicit_return_fp branch from 267834e to f6e7a5a Compare April 2, 2021 21:53
@bors
Copy link
Contributor

bors commented Apr 6, 2021

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

@Jarcho Jarcho force-pushed the implicit_return_fp branch from f6e7a5a to 88dc123 Compare April 6, 2021 16:14
@bors
Copy link
Contributor

bors commented Apr 6, 2021

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

@Jarcho Jarcho force-pushed the implicit_return_fp branch from 88dc123 to 6d6f933 Compare April 6, 2021 20:32
@bors
Copy link
Contributor

bors commented Apr 7, 2021

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

@Jarcho Jarcho force-pushed the implicit_return_fp branch from 6d6f933 to a7ddaf6 Compare April 7, 2021 12:41
@bors
Copy link
Contributor

bors commented Apr 12, 2021

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

flip1995
flip1995 reviewed Apr 12, 2021
View reviewed changes
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

Thanks! This LGTM overall. I still have to double check the lint_implicit_returns function, because I only skimmed it in this review round. I only found two documentation/style NITs.

clippy_lints/src/implicit_return.rs Show resolved Hide resolved
clippy_utils/src/source.rs Show resolved Hide resolved
@flip1995
Copy link
Member

You don't have to keep rebasing your branch. Best thing to do here is to just add commits and rebase/squash it once before merge. It's easier for me to review and you don't have to resolve conflicts every other day.

@flip1995 flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Apr 12, 2021
@flip1995
Copy link
Member

@Jarcho I'd still like to see this change: #6951 (comment) Otherwise this LGTM.

@Jarcho
Copy link
Contributor Author

Jarcho commented Apr 22, 2021

Looks like I forgot to change that.

Jarcho added 2 commits April 22, 2021 09:13
@Jarcho
Improve implicit_return
22f8c13 
Better suggestions when returning macro calls.
Suggest changeing all the break expressions in a loop, not just the final statement.
Don't lint divergent functions.
Don't suggest returning the result of any divergent fuction.
@Jarcho
Fix implicit_return suggestion for async functions
74cf5f2
@Jarcho Jarcho force-pushed the implicit_return_fp branch from 7154e32 to 3d793f3 Compare April 22, 2021 13:38
@flip1995
Copy link
Member

@bors r+

Thanks!

@bors
Copy link
Contributor

bors commented Apr 30, 2021

📌 Commit 3d793f3 has been approved by flip1995

@bors
Copy link
Contributor

bors commented Apr 30, 2021

⌛ Testing commit 3d793f3 with merge 5e49c4b...

@bors
Copy link
Contributor

bors commented Apr 30, 2021

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

@bors bors merged commit 5e49c4b into rust-lang:master Apr 30, 2021
@bors bors mentioned this pull request Apr 30, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flip1995 flip1995 flip1995 left review comments

Assignees

@flip1995 flip1995

Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

implicit_return suggestion is invalid syntax
4 participants
@Jarcho @rust-highfive @bors @flip1995