Skip to content

consider autoderef through user-defined Deref in eager_or_lazy #10896

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
Merged
merged 1 commit into from
Jun 7, 2023
Merged

consider autoderef through user-defined Deref in eager_or_lazy #10896

merged 1 commit into from
Jun 7, 2023

Conversation

y21
Copy link
Member

@y21 y21 commented Jun 5, 2023

Fixes #10462

This PR handles autoderef in the eager_or_lazy util module and stops suggesting to change lazy to eager if autoderef in an expression goes through user defined Deref impls, e.g.

struct S;
impl Deref for S {
  type Target = ();
  fn deref(&self) -> &Self::Target { &() }
}

let _ = Some(()).as_ref().unwrap_or_else(|| &S); // autoderef `&S` -> `&()`

changelog: [unnecessary_lazy_evaluations]: don't suggest changing lazy evaluation to eager if autoderef goes through user-defined Deref

r? @xFrednet (because of the earlier review in #10864, might help for context here)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 5, 2023
@xFrednet
Copy link
Member

xFrednet commented Jun 6, 2023

Thank you for the PR and review request :)

@blyxyas Do you first want to take a look at this PR? You also reviewed the previous one

@blyxyas
Copy link
Member

blyxyas commented Jun 6, 2023

Yep, will take it! 🐱

blyxyas
blyxyas approved these changes Jun 6, 2023
View reviewed changes
Copy link
Member

@blyxyas blyxyas left a comment

Choose a reason for hiding this comment

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

I don't think there's anything wrong with this. Let's hope it doesn't make a big toll on performance (By my benchmarks, it doesn't)! Thanks ❤️!

(Forgot to ping @xFrednet on the other reviews, sorry)

@xFrednet
Copy link
Member

xFrednet commented Jun 7, 2023

Looks good to me as well! Thank you for the update @y21!

Let's hope it doesn't make a big toll on performance (By my benchmarks, it doesn't)!

This change looks pretty simple, I would be surprised if it was even noticeable. Especially, since the condition is nested below other ones. But At the same time, it's good to check.

(Forgot to ping @xFrednet on the other reviews, sorry)

Usually, I still look at the ongoing conversation, in the other PR I just missed it. You're always welcome to ping me, though. Thank you for the review!

Now without further of due: Lord bors, would you mind merging this PR in the name of princess blyxyas and me?

@bors
Copy link
Contributor

bors commented Jun 7, 2023

📌 Commit e70dd55 has been approved by blyxyas,xFrednet

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jun 7, 2023

⌛ Testing commit e70dd55 with merge 7c34ec8...

@bors
Copy link
Contributor

bors commented Jun 7, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: blyxyas,xFrednet
Pushing 7c34ec8 to master...

@bors bors merged commit 7c34ec8 into rust-lang:master Jun 7, 2023
@blyxyas
Copy link
Member

blyxyas commented Jun 7, 2023

princess blyxyas

I'm genuine dying right now
Dies

@flip1995
Copy link
Member

flip1995 commented Jun 7, 2023

Now without further of due: Lord bors, would you mind merging this PR in the name of princess blyxyas and me?

How did you do it this time? How did you trigger bors, you magician? 😄

@xFrednet
Copy link
Member

xFrednet commented Jun 7, 2023

princess blyxyas

I'm genuine dying right now
Dies

Please don't I haven't learned any spells for resurrection. This kingdom still needs you

How did you do it this time?

Should a wizard reveal their power?

(It's the same magic trick I used a few months ago. GitHub still processes pings in markdown doc comments <!-- -->)

@flip1995
Copy link
Member

flip1995 commented Jun 7, 2023

Ah, I already forgot about this trick :D

@xFrednet
Copy link
Member

xFrednet commented Jun 7, 2023 •
edited
Loading

Maybe I can use it again in a few months, to confuse you. Using it is always a lot of fun, and that's one of the reasons we're here, right? ^^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xFrednet xFrednet xFrednet approved these changes

@blyxyas blyxyas blyxyas approved these changes

Assignees

@xFrednet xFrednet

@blyxyas blyxyas

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

unnecessary_lazy_evaluations and once_cell::sync::Lazy
6 participants
@y21 @xFrednet @blyxyas @bors @flip1995 @rustbot