pyo3 icon indicating copy to clipboard operation
pyo3 copied to clipboard

Avoid clippy::useless_conversion lint in macros

Open LilyFirefly opened this issue 10 months ago • 3 comments

Using #[allow(clippy::useless_conversion)] still causes failures if the PyO3 user uses #[forbid(clippy::useless_conversion)]. Instead we can ensure the into call is not within a quote_spanned! block, allowing clippy to know this is macro code.

https://github.com/rust-lang/rust-clippy/issues/14272#issuecomment-2686661392

I tried adding a UI test for this, but I couldn't get it to fail (before implementing the fix). I can confirm that this patch resolves the error in my project.

LilyFirefly avatar Feb 27 '25 13:02 LilyFirefly

I believe the reason for the original implementation is that the Into::into call can output diagnostics if the type does implement it. If it does not have a Span attached these diagnostics will be emitted on the macro call site, which is not so nice for UX and can be pretty confusing at times. All of the additions to the UI tests are not great IMO. (I believe this will also cause rust-analyzer to mark the entire body, which is also not nice, but I might be wrong there)

I'm inclined to say the the trade off is not worth it, but others might have a different opinion.


Potential alternatives I can think of:

  • for the BoundRef changes, we could maybe add a generic method on it, which does the conversion instead of calling into directly
  • for the PyErr change, we could add a function inside impl_ to do that, but that does sound a bit stupid, maybe there is something smarter.

But I'm still not sure if it would be worth the extra complexity.


Another thought: We can't just change the span of the attribute, can we?

Icxolu avatar Feb 27 '25 18:02 Icxolu

Yeah, I'm also not sure the tradeoffs are worth it here - probably no-one is setting #[forbid(clippy::useless_conversion). Thanks for explaining the prior thinking though, that's interesting context. This PR was pretty much showing where I'd got to and then seeing if anyone had any better ideas. I'll have a think about those potential alternatives.

LilyFirefly avatar Feb 27 '25 19:02 LilyFirefly

Will set this to draft in the meanwhile.

davidhewitt avatar Mar 17 '25 21:03 davidhewitt