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

fixes #3561 -- silence new clippy warning #3564

Merged
merged 1 commit into from
Nov 27, 2023
Merged

fixes #3561 -- silence new clippy warning #3564

merged 1 commit into from
Nov 27, 2023

Conversation

alex
Copy link
Contributor

@alex alex commented Nov 7, 2023

No description provided.

@@ -178,6 +178,7 @@ impl SelfType {
.map_err(::std::convert::Into::<_pyo3::PyErr>::into)
.and_then(
#[allow(clippy::useless_conversion)] // In case slf is PyCell<Self>
#[allow(unknown_lints, clippy::unnecessary_fallible_conversions)] // In case slf is Py<Self>
Copy link
Member

Choose a reason for hiding this comment

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

The unknown_lints is necessary for this to work on stable, right?

I am not sure we should commit to doing this until the lint hits stable as lints often change/are demoted before being released to stable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also for any older release, I assume

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we can also silence this lint by moving away from TryFrom to some other implementation, given that's what clippy is complaining about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume (but didn't check in depth) that TryFrom is needed for other receiver types.

Copy link
Member

Choose a reason for hiding this comment

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

It is, but we could replace it all with some other trait or function call

Copy link
Member

Choose a reason for hiding this comment

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

In the end in #3587 we stuck with From, so I think it's correct to stick to TryFrom

@adamreichold
Copy link
Member

What is the status now that 1.74 is released? Is stable Rust affected? If so, from my point of view, I would prefer silencing the now stable lint instead of trying to work around Clippy by using a different trait.

@davidhewitt
Copy link
Member

I think this is now in beta, so we have one more release cycle to solve it 😂

@davidhewitt
Copy link
Member

It struck me that we probably want to fix this before it hits stable, so that users don't have to temporarily silence clippy waiting for a PyO3 upgrade.

@alex
Copy link
Contributor Author

alex commented Nov 25, 2023

Yes, I agree with that. Obviously those of us who test on nightly got this a bit early :-)

@davidhewitt
Copy link
Member

The one alternative which I'd think we could consider is to use feature detection to only emit this on 1.75 and up. I'd slightly prefer that given we already have the machinery in our build.rs for this and then over time we can delete the feature detection, rather than leave it in the unknown_lints allow for some indefinite time.

@alex
Copy link
Contributor Author

alex commented Nov 26, 2023

Hmm, started looking at doing that, but pyo3-macros-backend (where this code is), doesn't depend on pyo3-build-config ATM. Is it better to add a dependency, or just do it this way?

Could also split the baby: Add a cfg to it, and then put it in a comment so we don't forget about it, even though it's not actually used programatically.

@davidhewitt
Copy link
Member

Ah good point. Maybe just add a comment // TODO: remove unknown_lints allow when MSRV above 1.75 or similar, in that case?

@alex
Copy link
Contributor Author

alex commented Nov 27, 2023

Done

Comment on lines 156 to 159

if rustc_minor_version >= 75 {
println!("cargo:rustc-cfg=clippy_unnecessary_fallible_conversions_lint");
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this bit can be reverted, otherwise please merge 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@alex alex enabled auto-merge November 27, 2023 22:07
@alex alex added this pull request to the merge queue Nov 27, 2023
Merged via the queue into main with commit 5080dee Nov 27, 2023
36 checks passed
@alex alex deleted the alex-patch-1 branch November 27, 2023 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants