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 [unnecessary_unwrap_unchecked] #11139

Closed
wants to merge 1 commit into from

Conversation

Centri3
Copy link
Member

@Centri3 Centri3 commented Jul 12, 2023

Closes #11108

Really proud of the solution I came up with for this one ^^ didn't expect there to be enough info to generalize this (i.e., not hardcoding _unchecked functions)

changelog: New lint [unnecessary_unwrap_unchecked]
#11139

@rustbot
Copy link
Collaborator

rustbot commented Jul 12, 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 Jul 12, 2023
&& let Some(unchecked_ident) = unchecked_ident(&last_path_segment(&qpath).ident)
&& let Some(unchecked_def_id) = children
.iter()
.find(|child| child.ident == unchecked_ident)
Copy link
Member Author

Choose a reason for hiding this comment

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

This should probably check if it's a DefKind::Fn as well

@Centri3 Centri3 force-pushed the needless_unwrap_unchecked branch from 52f2545 to e42d365 Compare July 12, 2023 04:47
Comment on lines +3383 to +3388
/// ### What it does
/// Checks for calls to `unwrap_unchecked` when an `_unchecked` variant of the function exists.
///
/// ### Why is this bad?
/// Calling the `_unchecked` variant instead alleviates a check that's entirely redundant if
/// `unwrap_unchecked` is called.
Copy link
Member Author

Choose a reason for hiding this comment

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

This should probably be rewritten...

@bors
Copy link
Contributor

bors commented Jul 17, 2023

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

@xFrednet
Copy link
Member

Hey this is triage, I'm closing this due to inactivity. Currently, @Centri3 sadly doesn't have the time to continue this implementation. If anyone is interested in continuing this PR, you're more than welcome to create a new PR and push it over the finish line. :D

Thank you to @Centri3 and the reviewers for the time, that you already put into this!

@rustbot label +S-inactive-closed -S-waiting-on-author -S-waiting-on-review

@xFrednet xFrednet closed this Mar 31, 2024
@rustbot rustbot added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Mar 31, 2024
@Centri3 Centri3 deleted the needless_unwrap_unchecked branch April 1, 2024 16:52
@Centri3
Copy link
Member Author

Centri3 commented Apr 1, 2024

Triaging my old PRs in case someone wants to pick them up, I don't really think this needs much more work, but we should definitely create utils functions to do this a bit more cleanly.

Also, we definitely need a chapter in the book on DefId traversal, since it can be so crucial yet isn't really mentioned anywhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive-closed Status: Closed due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unnecessary unwrap_unchecked
5 participants