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

[unnecessary_literal_unwrap]: Fix ICE on None.unwrap_or_default() #11106

Merged
merged 2 commits into from
Jul 20, 2023
Merged

[unnecessary_literal_unwrap]: Fix ICE on None.unwrap_or_default() #11106

merged 2 commits into from
Jul 20, 2023

Conversation

syvb
Copy link
Contributor

@syvb syvb commented Jul 4, 2023 •
edited by dswij
Loading

Fixes #11099
Fixes #11064

I'm running into #11099 (cc @y21) on my Rust codebase. Clippy ICEs on this code when evaluating the unnecessary_literal_unwrap lint:

fn main() {
    let val1: u8 = None.unwrap_or_default();
}

This fixes that ICE and adds an message specifically for that case:

error: used `unwrap_or_default()` on `None` value
  --> $DIR/unnecessary_literal_unwrap.rs:26:5
   |
LL |     None::<String>.unwrap_or_default();
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove the `None` and `unwrap_or_default()`: `String::default()`

This PR also fixes the same ICE with None.unwrap_or_else (by giving the generic error message for the lint in that case).

changelog: Fix ICE in unnecessary_literal_unwrap on None.unwrap_or_default()

@rustbot
Copy link
Collaborator

rustbot commented Jul 4, 2023

r? @dswij

(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 4, 2023
@y21
Copy link
Member

y21 commented Jul 4, 2023

Nice, I was about to look into this one as well! ^^
This ICE also happens for unwrap_or and unwrap_or_else (forgot to mention it in the issue): https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=12873182c7a7d7ca7a9fb869b2a2b565

Can you handle these also and add tests for them?

@syvb syvb force-pushed the literal_unwrap_ice branch 5 times, most recently from 0d94bbf to a80778c Compare July 7, 2023 14:14
@syvb
Copy link
Contributor Author

syvb commented Jul 8, 2023

I updated this to add specific error messages for unwrap_or and unwrap_or_else.

@bors
Copy link
Contributor

bors commented Jul 12, 2023

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

@bors
Copy link
Contributor

bors commented Jul 12, 2023

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

@syvb syvb force-pushed the literal_unwrap_ice branch from a80778c to 8d258c1 Compare July 13, 2023 17:06
@Centri3
Copy link
Member

Centri3 commented Jul 20, 2023 •
edited
Loading

This also seems to fix #11064 as well (it doesn't ICE anymore, but the suggestion is wrong).

Can you link the two by adding "Fixes #11064, fixes #11099" to the PR description? That way they're closed when this is merged.

dswij
dswij approved these changes Jul 20, 2023
View reviewed changes
Copy link
Member

@dswij dswij left a comment

Choose a reason for hiding this comment

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

Thanks for this! Sorry for the slow review.

@dswij
Copy link
Member

dswij commented Jul 20, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Jul 20, 2023

📌 Commit 8d258c1 has been approved by dswij

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jul 20, 2023

⌛ Testing commit 8d258c1 with merge fca1f9a...

@bors
Copy link
Contributor

bors commented Jul 20, 2023

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

@bors bors merged commit fca1f9a into rust-lang:master Jul 20, 2023
@flip1995 flip1995 added beta-nominated Nominated for backporting to the compiler in the beta channel. beta-accepted Accepted for backporting to the compiler in the beta channel. and removed beta-nominated Nominated for backporting to the compiler in the beta channel. labels Aug 14, 2023
@flip1995 flip1995 mentioned this pull request Aug 17, 2023
Merged
@flip1995
Copy link
Member

rust-lang/rust#114937

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 17, 2023
@bors
Auto merge of rust-lang#114937 - flip1995:clippy_beta_backport, r=Mar…
8c60295 
…k-Simulacrum

[beta] Clippy backports for ICE fixes

This backports PRs to beta, that fix ICEs, some lint grouping and FP fixes. Namely:

- rust-lang/rust-clippy#11191
- rust-lang/rust-clippy#11172
- rust-lang/rust-clippy#11130
- rust-lang/rust-clippy#11106
- rust-lang/rust-clippy#11104
- rust-lang/rust-clippy#11077
- rust-lang/rust-clippy#11070 (This PR is not synced to the Rust repo yet, but I will open a separate PR to get it into `master`, before beta is branched: rust-lang#114938)
- rust-lang/rust-clippy#11069

Kind of a big backport, but most of it is tests.

r? `@Mark-Simulacrum`

cc `@Manishearth`
@xFrednet xFrednet removed the beta-accepted Accepted for backporting to the compiler in the beta channel. label Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dswij dswij dswij approved these changes

Assignees

@dswij dswij

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
8 participants
@syvb @rustbot @y21 @bors @Centri3 @dswij @flip1995 @xFrednet