Skip to content

Extend UNCONDITIONAL_RECURSION to check for ToString implementations #11980

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

Merged

Conversation

GuillaumeGomez
Copy link
Member

Follow-up of #11938.

r? @llogiq

changelog: Extend UNCONDITIONAL_RECURSION to check for ToString implementations

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 18, 2023
@GuillaumeGomez GuillaumeGomez force-pushed the UNCONDITIONAL_RECURSION-tostring branch from 381531e to efddb33 Compare December 18, 2023 15:58
impl std::string::ToString for S6 {
fn to_string(&self) -> String {
//~^ ERROR: function cannot return without recursing
self.to_string()
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps add another test where self is put into a local before calling its to_string?

And another one calling (self as &Self).to_string() just for extra fun? 😋

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps add another test where self is put into a local before calling its to_string?

For that one, we agree that it's just for cases like:

let x = self;
x.to_string()

and nothing else? Because otherwise it becomes quite tricky to check if self was mutated... However if you know of a way to check it, I'm very interested. :)

And another one calling (self as &Self).to_string() just for extra fun? 😋

Sure. ^^'

Copy link
Contributor

Choose a reason for hiding this comment

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

However if you know of a way to check it, I'm very interested. :)

There's the clippy_utils::expr_or_init function, which should be helpful for that.

@rustbot author

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Dec 25, 2023
@GuillaumeGomez GuillaumeGomez force-pushed the UNCONDITIONAL_RECURSION-tostring branch from efddb33 to d161f3b Compare December 30, 2023 16:12
@GuillaumeGomez
Copy link
Member Author

Updated and added the suggested tests. :)

@llogiq
Copy link
Contributor

llogiq commented Dec 31, 2023

Thank you!

@bors r+

@bors
Copy link
Contributor

bors commented Dec 31, 2023

📌 Commit d161f3b has been approved by llogiq

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Dec 31, 2023

⌛ Testing commit d161f3b with merge 44c99a8...

@bors
Copy link
Contributor

bors commented Dec 31, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 44c99a8 to master...

@bors bors merged commit 44c99a8 into rust-lang:master Dec 31, 2023
@GuillaumeGomez GuillaumeGomez deleted the UNCONDITIONAL_RECURSION-tostring branch December 31, 2023 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants