Skip to content
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

Warn for passing Box<&dyn Any> to function taking &dyn std::any::Any #12800

Open
Wumpf opened this issue May 15, 2024 · 3 comments
Open

Warn for passing Box<&dyn Any> to function taking &dyn std::any::Any #12800

Wumpf opened this issue May 15, 2024 · 3 comments
Labels
A-lint Area: New lints

Comments

@Wumpf
Copy link

Wumpf commented May 15, 2024

What it does

Issue a warning if a function takes any of

  • &dyn std::any::Any
  • &mut dyn std::any::Any

The recommendation is to either use an existing trait type or create a "marker trait" to specify what the method can actually process in order to avoid passing (by accident) object references of unexpected types.

EDIT: Only warn when Box<&dyn Any> is passed as-is. Recommend box.as_ref()

Advantage

Beside the obvious advantages of a more concrete type, there's major pitfalls with &dyn Any: Since &Box<Any> itself implements Any, it can easily happen that one accidentally passes the box directly instead of the desired box.as_ref(). This can be the cause of quite hard to debug issues.

Drawbacks

While it's more often than not a shortcut for not defining a trait type, there are likely legitimate usecases for passing &dyn Any (examples?)

Example

Updated suggestion:

fn func(value: &dyn std::any::Any) { ... }

func(box);

Could be written as:

fn func(value: &dyn std::any::Any) { ... }

func(box.as_ref());

Previous suggestion:

fn func(value: &dyn std::any::Any) { ... }

Could be written as:

fn func(value: &dyn MyTrait) { ... }
@Wumpf Wumpf added the A-lint Area: New lints label May 15, 2024
@Wumpf Wumpf changed the title Warn for use of &dyn std::any::Any Warn for use of &dyn std::any::Any as a parameter May 15, 2024
@y21
Copy link
Member

y21 commented May 15, 2024

I don't see an inherent issue with this general pattern. Functions are natural abstractions for splitting up code and sometimes you just have to deal with dyn Any (like, panic payloads are the first thing that came to my mind) and want to refactor something out into its own function. For instance (some random code snippet I found from a github code search): https://github.com/astral-sh/rye/blob/7c4b6991e81e0c6885fac846cc76f8a4425de130/rye/src/utils/panic.rs#L4

That looks like a perfectly fine thing to do and it seems odd to me that clippy should blanket-warn all such functions.

Since &Box<Any> itself implements Any, it can easily happen that one accidentally passes the box directly instead of the desired box.as_ref(). This can be the cause of quite hard to debug issues.

This OTOH makes more sense as a lint to me I think -- passing &Box<dyn Any> to a function accepting &dyn Any. Sounds similar to the sort of mistake that type_id_on_box catches.

@Centri3
Copy link
Member

Centri3 commented May 16, 2024

Yeah I think making it more specific is better, that's a very valid reason to lint these but the general pattern is okay

@Wumpf
Copy link
Author

Wumpf commented May 17, 2024

Makes a lot of sense to me, lint for passing &Box<dyn Any> is a much more general problem and less opinionated/domain specific

@Wumpf Wumpf changed the title Warn for use of &dyn std::any::Any as a parameter Warn for passing Box<&dyn Any> to function taking &dyn std::any::Any May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints
Projects
None yet
Development

No branches or pull requests

3 participants