Skip to content

consider autoderef through user-defined Deref in eager_or_lazy#10896

Merged
bors merged 1 commit intorust-lang:masterfrom
y21:eager-or-lazy-autoderef
Jun 7, 2023
Merged

consider autoderef through user-defined Deref in eager_or_lazy#10896
bors merged 1 commit intorust-lang:masterfrom
y21:eager-or-lazy-autoderef

Conversation

@y21
Copy link
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
Member

blyxyas commented Jun 6, 2023

Yep, will take it! 🐱

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

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
Copy Markdown
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
Copy Markdown
Contributor

bors commented Jun 7, 2023

⌛ Testing commit e70dd55 with merge 7c34ec8...

@bors
Copy link
Copy Markdown
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
Copy Markdown
Member

blyxyas commented Jun 7, 2023

princess blyxyas

I'm genuine dying right now
Dies

@flip1995
Copy link
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
Member

flip1995 commented Jun 7, 2023

Ah, I already forgot about this trick :D

@xFrednet
Copy link
Copy Markdown
Contributor

xFrednet commented Jun 7, 2023

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

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.

unnecessary_lazy_evaluations and once_cell::sync::Lazy

6 participants