Skip to content

Introduce coerce_container_to_any #14812

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 5, 2025
Merged

Introduce coerce_container_to_any #14812

merged 1 commit into from
Jun 5, 2025

Conversation

Ralith
Copy link
Contributor

@Ralith Ralith commented May 15, 2025 •
edited by llogiq
Loading

This PR introduces a lint which detects when &Box<dyn Any> is coerced to &dyn Any, which is usually a mistake where the intent was to directly pass the dyn Any inside the box without coercion.

cc @llogiq

Remaining work:

  • Documentation
  • Generalize from Box to any implementer of Deref<Target=dyn Any>

changelog: add [coerce_container_to_any] lint

@rustbot
Copy link
Collaborator

rustbot commented May 15, 2025

r? @dswij

rustbot has assigned @dswij.
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 May 15, 2025
@matthiasbeyer
Copy link

r? llogiq

@rustbot rustbot assigned llogiq and unassigned dswij May 15, 2025
@llogiq
Copy link
Contributor

llogiq commented May 16, 2025

This looks good for a start. Let's add the documentation and replace the "default lint description", and then I can start the Final Comment Period for this lint.

@Ralith Ralith force-pushed the coerce_any_ref_to_any branch from 9950e08 to 2363e9d Compare May 17, 2025 13:23
@Ralith Ralith changed the title WIP: Introduce coerce_any_ref_to_any Introduce coerce_any_ref_to_any May 17, 2025
@Ralith
Copy link
Contributor Author

Ralith commented May 17, 2025

Thanks to guidance from @y21 on Zulip, I've got this as smart as I wanted. Seeking feedback particularly on lint name and description wording. I may iterate on the diagnostic a little more.

@Ralith Ralith force-pushed the coerce_any_ref_to_any branch from 2363e9d to 18d86b3 Compare May 17, 2025 13:53
@Ralith
Copy link
Contributor Author

Ralith commented May 17, 2025

Diagnostic is now pretty good. Not sure if there's a better way to format the expression type, e.g. so it shows up as Box rather than std::boxed::Box.

@Ralith Ralith force-pushed the coerce_any_ref_to_any branch 11 times, most recently from 4438e38 to 037945c Compare May 17, 2025 17:40
@Ralith
Copy link
Contributor Author

Ralith commented May 17, 2025

Future work: maybe this should also fire on coercing &Box<dyn Foo> to &dyn Any when Foo: Any?

@rustbot

This comment has been minimized.

Sign in to view
@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels May 17, 2025
@Ralith Ralith force-pushed the coerce_any_ref_to_any branch from fa326e3 to 71bc081 Compare May 17, 2025 19:06
@rustbot rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels May 17, 2025
@Ralith
Copy link
Contributor Author

Ralith commented May 17, 2025

Thanks to @fu5ha for expanded docs!

@Ralith Ralith force-pushed the coerce_any_ref_to_any branch from 71bc081 to eef8a8c Compare May 17, 2025 19:35
jackh726
jackh726 reviewed May 18, 2025
View reviewed changes
sanbox-irl
sanbox-irl reviewed May 18, 2025
View reviewed changes
Copy link

@sanbox-irl sanbox-irl left a comment

Choose a reason for hiding this comment

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

This looks wonderful! I left some bikeshedding comments on the description and what not.

For the name, what about coerce_container_to_any? I think using any_ref to describe &Box<dyn Any> is surprising

@llogiq
Copy link
Contributor

llogiq commented May 18, 2025

Not sure if there's a better way to format the expression type, e.g. so it shows up as Box rather than std::boxed::Box.

That would be path trimming, which we currently don't have. I'm on that, but you don't need to worry about it right now.

@Ralith Ralith force-pushed the coerce_any_ref_to_any branch from eef8a8c to 27eec99 Compare May 19, 2025 01:51
@Ralith
Copy link
Contributor Author

Ralith commented May 19, 2025

Applied all feedback. Thanks for the reviews!

@Ralith Ralith changed the title Introduce coerce_any_ref_to_any Introduce coerce_container_to_any May 19, 2025
@Ralith
Copy link
Contributor Author

Ralith commented May 29, 2025

@llogiq friendly ping

llogiq
llogiq reviewed May 30, 2025
View reviewed changes
Copy link
Contributor

@llogiq llogiq left a comment

Choose a reason for hiding this comment

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

I have one final suggestion regarding the lint message and will start the FCP soon.

@Ralith Ralith force-pushed the coerce_any_ref_to_any branch 2 times, most recently from 7bde47c to be982e8 Compare May 31, 2025 20:11
@llogiq
Copy link
Contributor

llogiq commented May 31, 2025

The final comment period is on.

@Ralith Ralith force-pushed the coerce_any_ref_to_any branch from be982e8 to 7e69dab Compare June 1, 2025 04:58
@Ralith
Copy link
Contributor Author

Ralith commented Jun 1, 2025

Fixed a stray ` in the example.

@rustbot

This comment has been minimized.

Sign in to view
@Ralith Ralith force-pushed the coerce_any_ref_to_any branch from 7e69dab to b51e737 Compare June 2, 2025 18:13
@llogiq
Copy link
Contributor

llogiq commented Jun 5, 2025

Thank you!

@llogiq llogiq added this pull request to the merge queue Jun 5, 2025
Merged via the queue into rust-lang:master with commit b379d54 Jun 5, 2025
11 checks passed
@flip1995 flip1995 mentioned this pull request Jun 13, 2025
Closed
@Ralith Ralith deleted the coerce_any_ref_to_any branch June 14, 2025 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@llogiq llogiq llogiq left review comments

+2 more reviewers

@jackh726 jackh726 jackh726 left review comments

@sanbox-irl sanbox-irl sanbox-irl left review comments

Reviewers whose approvals may not affect merge requirements
Assignees

@llogiq llogiq

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.
7 participants
@Ralith @rustbot @matthiasbeyer @llogiq @jackh726 @sanbox-irl @dswij