Skip to content

Conversation

@apoisternex
Copy link
Contributor

Fixes #12907

changelog: Fix [needless_return] false negative when returned expression borrows a value.

…ws a value

Fixes rust-lang#12907

changelog: Fix [`needless_return`] false negative when returned expression borrows a value
@rustbot
Copy link
Collaborator

rustbot commented Aug 4, 2024

r? @Centri3

rustbot has assigned @Centri3.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Aug 4, 2024
@GuillaumeGomez
Copy link
Member

Looks good to me, thanks! Let's just wait for the assigned reviewer to take a look before approving.

fn issue12907() -> String {
return "".split("").next().unwrap().to_string();
}

Copy link
Member

@Centri3 Centri3 Aug 29, 2024

Choose a reason for hiding this comment

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

I think this just needs a test for the mentioned RefCell or any other type that fails to compile. But I think the significant drop check is adequate

There is the rustc_insigificant_dtor attribute. (I didn't even know this existed before checking.) This is applied to certain types like Rc, HashMap, and array::IntoIter. Not sure whether this attribute changes the behavior (i.e., compiling would succeed) but it'd be a good idea to check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this just needs a test for the mentioned RefCell or any other type that fails to compile. But I think the significant drop check is adequate

So should i delete the test then?

Not sure whether this attribute changes the behavior (i.e., compiling would succeed) but it'd be a good idea to check.

Yes, it compiles with the rustc_insigificant_dtor attribute and the lint is triggered.

Copy link
Member

Choose a reason for hiding this comment

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

No I meant the only change it needs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is already a very similar test to the one mentioned:

fn temporary_outlives_local() -> String {
    let x = RefCell::<String>::default();
    return x.borrow().clone();
}

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that does the same thing 👍 missed that before

@Centri3
Copy link
Member

Centri3 commented Sep 7, 2024

Thank you! @bors r+

@bors
Copy link
Contributor

bors commented Sep 7, 2024

📌 Commit 66283bf has been approved by Centri3

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Sep 7, 2024

⌛ Testing commit 66283bf with merge 41dc86d...

@bors
Copy link
Contributor

bors commented Sep 7, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Centri3
Pushing 41dc86d to master...

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.

Removing return not sugggested by needless_return when calling some chained string methods

5 participants