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

Avoid clippy::useless_conversion lint in macros #4944

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

LilyFoote
Copy link
Contributor

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.

rust-lang/rust-clippy#14272 (comment)

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.

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.

rust-lang/rust-clippy#14272 (comment)
@LilyFoote LilyFoote added refactoring proc-macro CI-skip-changelog Skip checking changelog entry rust Pull requests that update Rust code labels Feb 27, 2025
@LilyFoote LilyFoote self-assigned this Feb 27, 2025
@LilyFoote LilyFoote removed the CI-skip-changelog Skip checking changelog entry label Feb 27, 2025
@LilyFoote LilyFoote force-pushed the fix-useless-conversion-lint branch from 467755d to 05608ae Compare February 27, 2025 13:24
@Icxolu
Copy link
Contributor

Icxolu commented Feb 27, 2025

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?

@LilyFoote
Copy link
Contributor Author

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.

@davidhewitt
Copy link
Member

Will set this to draft in the meanwhile.

@davidhewitt davidhewitt marked this pull request as draft March 17, 2025 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proc-macro refactoring rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants