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

New lint [needless_return_with_question_mark] #11031

Merged
merged 2 commits into from
Jul 24, 2023

Conversation

Centri3
Copy link
Member

@Centri3 Centri3 commented Jun 26, 2023

Closes #10902

Rather than having a config option, this will just suggest removing the "return"; if try_err is used as well, then it'll be added again but without the ?.

changelog: New lint [needless_return_with_try]

@rustbot
Copy link
Collaborator

rustbot commented Jun 26, 2023

r? @giraffate

(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 Jun 26, 2023
@bors
Copy link
Collaborator

bors commented Jul 2, 2023

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

@giraffate
Copy link
Contributor

I didn't immediately understand try in needless_return_with_try to mean ?. IMO, so how about needless_return_with_question_mark or just needless_return_with_question for the lint naming?

@Centri3
Copy link
Member Author

Centri3 commented Jul 13, 2023

I'm fine with that, _try just felt more concise (after all, ? is the try operator), but I can see how it'd be confusing

@Centri3 Centri3 force-pushed the needless_return branch 2 times, most recently from 97e6924 to 77512cd Compare July 13, 2023 02:54
Copy link
Contributor

@giraffate giraffate left a comment

Choose a reason for hiding this comment

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

Overall looks good, thanks! I made some comments.

clippy_lints/src/returns.rs Outdated Show resolved Hide resolved
/// Ok(())
/// }
/// ```
/// if paired with `try_err`, use instead:
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO it would be better to change suggestions by using is_lint_allowed when warning try_err, but it can be done in another pull request.

Copy link
Member Author

Choose a reason for hiding this comment

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

We should probably move around try_err then, probably to returns instead of matches, that way we can reuse the logic. But I'm ok with this.

@giraffate
Copy link
Contributor

Okay. Then can we update the version #11031 (comment) ?

@Centri3
Copy link
Member Author

Centri3 commented Jul 18, 2023

Yeah, but I'll also move try_err. I'll get back to this soon.

@Centri3
Copy link
Member Author

Centri3 commented Jul 18, 2023

This actually seems too complex to tackle rn, I'll take another look at some point and potentially create a followup PR

@giraffate
Copy link
Contributor

@bors r+

Thanks!

@bors
Copy link
Collaborator

bors commented Jul 24, 2023

📌 Commit 0d59d1d has been approved by giraffate

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jul 24, 2023

⌛ Testing commit 0d59d1d with merge 31f3769...

@bors
Copy link
Collaborator

bors commented Jul 24, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: giraffate
Pushing 31f3769 to master...

@bors bors merged commit 31f3769 into rust-lang:master Jul 24, 2023
7 checks passed
@blyxyas blyxyas changed the title New lint [needless_return_with_try] New lint [needless_return_with_question_mark] Oct 3, 2023
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 with Err and question mark
4 participants