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

Replace From implementation of SwapInterval that could panic with TryFrom #1413

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

antonilol
Copy link
Contributor

fixes #1311

Unfortunately this is a breaking change, Rust automatically implements TryFrom for types that implement From, so just adding the TryFrom implementation will not work.

@Cobrand
Copy link
Member

Cobrand commented Jul 1, 2024

Doesn't seem a very useful breaking change to remove the From, although there is value is adding TryFrom. Could you please re-add From to call TryFrom?

@antonilol
Copy link
Contributor Author

antonilol commented Jul 1, 2024

I can't, (see first comment), all From implementations come with a TryFrom implementation (same conversion, always returning Ok and Error is set to Infallible (empty type)), so they can't coexist (see https://doc.rust-lang.org/src/core/convert/mod.rs.html#802-815)

@Cobrand
Copy link
Member

Cobrand commented Jul 5, 2024

I see, so we can only do that when trait impl specialization is in stable. I think for now we could just add something that acts like try_from but doesn't use the trait TryFrom to avoid conflict, and then when it stabilized without breaking anything we can impl try_from directly.

@antonilol
Copy link
Contributor Author

I see, so we can only do that when trait impl specialization is in stable.

This can take a while, and I don't expect that this will be specializable, because that would allow different logic for From and TryFrom, which to me is counterintuitive.

I think for now we could just add something that acts like try_from but doesn't use the trait TryFrom to avoid conflict, and then when it stabilized without breaking anything we can impl try_from directly.

We can add an inherent function SwapInterval::try_from(value: i32) -> Result<Self, SwapIntervalConversionError> and deprecate the From impl (the deprecation message will guide users to that function). This does sound as the best non-breaking approach to me.

The simplest way is 'just do a breaking change', it can be argued how many people use 1.into() or 0.into() given the context (spelling out SwapInterval::VSync or SwapInterval::Immediate is clearer).

@Cobrand
Copy link
Member

Cobrand commented Jul 6, 2024

We can add an inherent function SwapInterval::try_from(value: i32) -> Result<Self, SwapIntervalConversionError> and deprecate the From impl (the deprecation message will guide users to that function). This does sound as the best non-breaking approach to me.

I would prefer that. I know it's extremely minor but personally I hate when I update a library with a breaking change and it didn't even leave me 1 version of leeway to migrate to a new version using deprecation.

Plus, I really doubt that the trait TryFrom specifically is used by a lot of people, they just want a try_fromand try_into impl, but never are they going to use Box<dyn TryFrom<u32>> or fn example() -> impl TryFrom<u32>, so I don't really feel like there is a rush to implement TryFrom, especially if it breaks stuff.

@antonilol
Copy link
Contributor Author

Apparently you can't deprecate a trait implementation in Rust, so I added a println! there, if you know of any ways to get a proper compile time deprecation warning, let me know!

@Cobrand
Copy link
Member

Cobrand commented Jul 12, 2024

Println in the console is really dirty, I would rather have nothing at runtime and just leave it as-is, but put the deprecation notice in a doc comment

…d a println

Notes:
- Trait implementations can't be deprecated, so had to use some other way.
- try_from is now an inherent function because From and TryFrom can't be
  implemented at the same time.
@antonilol
Copy link
Contributor Author

Rebased this one and my 2 other PRs

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.

From<i32> for SwapInterval should be panic-less TryFrom<i32> implementation
2 participants