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

Detect use of unnecessary dyn in function parameter #13685

Open
tabokie opened this issue Nov 14, 2024 · 2 comments
Open

Detect use of unnecessary dyn in function parameter #13685

tabokie opened this issue Nov 14, 2024 · 2 comments
Labels
A-lint Area: New lints

Comments

@tabokie
Copy link
Contributor

tabokie commented Nov 14, 2024

What it does

Scan for all uses of a function, and check if the &dyn T can be replaced by &impl T.

Advantage

  • Avoid the overhead introduced by dynamic dispatch

Drawbacks

Code bloat and compile overhead.

Example

// A private trait that takes a dyn reference.
trait Trait {
  fn f(writer: &mut dyn std::io::Write);
}

// Using that trait with concrete types.
fn use_trait(obj: impl Trait) {
  obj.f(&mut Vec::new());
}

Could be written as:

trait Trait {
  fn f(writer: &mut impl std::io::Write);
}

fn use_trait(obj: impl Trait) {
  obj.f(&mut Vec::new());
}
@tabokie tabokie added the A-lint Area: New lints label Nov 14, 2024
@y21
Copy link
Member

y21 commented Nov 14, 2024

The downside is that this can introduce a lot of monomorphization bloat if the function is called with lots of different types which can result in increased compile times and bigger code size, so I'm unsure if this should be a lint. Sounds like a tradeoff decision the user should make on a case by case basis

@tabokie
Copy link
Contributor Author

tabokie commented Nov 15, 2024

Indeed. But then again, I think a lot of the "redundant"/"needless"/"useless"/"unnecessary" lints belong to this area of requiring a manual tradeoff. This particular lint is very helpful in my case where latency performance is crucial.

I'm not sure the complexity of this lint though. I'm not looking to add a default-allow lint that is also difficult to implement and maintain.

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

2 participants