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

[option_if_let_else]: suggest .as_ref() if scrutinee is of type &Option<_> #11035

Merged
merged 2 commits into from
Jun 28, 2023

Conversation

y21
Copy link
Member

@y21 y21 commented Jun 26, 2023

Fixes #10729

Option::map_or takes ownership, so if matching on an &Option<_>, we need to suggest .as_ref() before calling map_or to get the same effect and to not cause a borrowck error.

changelog: [option_if_let_else]: suggest .as_ref()/.as_mut() if scrutinee is of type &Option<_>/&mut Option<_>

@rustbot
Copy link
Collaborator

rustbot commented Jun 26, 2023

r? @Jarcho

(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
Copy link
Contributor

@Jarcho Jarcho left a comment

Choose a reason for hiding this comment

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

Just to check, this lint currently doesn't handle patterns like &Some(_), correct?

@@ -239,6 +239,8 @@ fn main() {
Ok(a) => a + 1,
};
let _ = if let Ok(a) = res { a + 1 } else { 5 };
issue10729::reproduce(&None);
issue10729::reproduce2(&mut None);
Copy link
Contributor

Choose a reason for hiding this comment

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

Small nit. There's no need for this as the test code is only compiled, not run.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did that to silence the dead code warning, but I guess I could also #[allow] it.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can also prefix names with underscores as well. That silences unused warnings.

Copy link
Member Author

Choose a reason for hiding this comment

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

Decided to go with #![allow(dead_code)] for that module because it's also used for other unused functions in that test file, hope that's fine too

@y21
Copy link
Member Author

y21 commented Jun 27, 2023

Just to check, this lint currently doesn't handle patterns like &Some(_), correct?

Yes, it doesn't lint in that case: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=790624fb7c9a4ddb11753e60e278ec5b

@Jarcho
Copy link
Contributor

Jarcho commented Jun 28, 2023

Ok. Not a requirement to handle it, just want to make sure that case doesn't end up with a regressions. Once the little nit is handled then this is good.

@Jarcho
Copy link
Contributor

Jarcho commented Jun 28, 2023

Thank you. @bors r+

@bors
Copy link
Collaborator

bors commented Jun 28, 2023

📌 Commit cee4c41 has been approved by Jarcho

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jun 28, 2023

⌛ Testing commit cee4c41 with merge ea4c5c5...

@bors
Copy link
Collaborator

bors commented Jun 28, 2023

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

@bors bors merged commit ea4c5c5 into rust-lang:master Jun 28, 2023
4 checks passed
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.

clippy::option_if_let_else suggestion doesn't compile when passing a matched reference to a function
4 participants