Remove Problematic Unwraps In Macros

[Kailokk]

Using unwraps in macros means the code is compiled as part of the end users build.
This therefore means any lints a user uses will check the macro code as well.
These unwraps therefore the clippy lint unwrap_used will throw a warning/error if the user has enabled it.

I wanted to enable this in a project that uses scylla, so i've created this merge request to try and fix this in the least invasive way possible.

I have returned error states where the unwraps were used, but if this feels more in the realm of a panic state, then it can simply be swapped for panic

Pre-review checklist

• I have split my patch into logically separate commits.
• All commit messages clearly explain what they change and why.
• I added relevant tests for new features and bug fixes.
• All commits compile, pass static checks and pass test.
• PR description sums up the changes and reasons why they should be introduced.
• I have provided docstrings for the public items that I want to introduce.
• I have adjusted the documentation in ./docs/source/.
• I added appropriate Fixes: annotations to PR description.

[github-actions]

cargo semver-checks found no API-breaking changes in this PR! 🎉🥳
Checked commit: 70b6e2d

[Lorak-mmk]

As far as I can tell this lint only complains about unwraps, and .exepct() is fine.
Imo if we are sure that .unwraps() can't fail, then they should remain panicking - because triggering this panic means a programming error on our part which should be reported and fixed. Unless @wprzytula disagrees, could you change those unwraps to .expect's with proper error messages?

[Kailokk]

Oh> As far as I can tell this lint only complains about unwraps, and .exepct() is fine.

Imo if we are sure that .unwraps() can't fail, then they should remain panicking - because triggering this panic means a programming error on our part which should be reported and fixed. Unless @wprzytula disagrees, could you change those unwraps to .expect's with proper error messages?

This is true, but it bubbles the problem up to the expect_used lint instead. https://rust-lang.github.io/rust-clippy/master/index.html#/expect_used

This is definitely preferable, but personally i would like to see it removed, just given that this code is expanded into users of the drivers code.

Happy to change it if not panicking feels strange to you, i can absolutely understand the reason you have given, especially seeing as these are both optional lints

[Lorak-mmk]

I think it would be better to change it to expects, but let's wait for @wprzytula 's opinion before doing it.

[wprzytula]

Imo if we are sure that .unwraps() can't fail, then they should remain panicking - because triggering this panic means a programming error on our part which should be reported and fixed.

100%, we use unwrap where a runtime occurence of an error would signify that our assumptions were wrong, and so we need to fix it.

Unless @wprzytula disagrees, could you change those unwraps to .expect's with proper error messages?

I fully agree. expect()s are widely used in library code, too - see tokio's usages of expect(). Also, this is the approach I took in new deserialization macros.

[Kailokk]

@wprzytula

I think that's reasonable. I have one more alternative to suggest:

If we use .unwrap_or_else(|| panic!(...)); in place of expect.
Normally this is equivalent code, but in this case, using .unwrap_or_else will prevent lint warnings in end users code. This is only a problem in my eyes because these are macros.

If you don't find this acceptable, i will implement the .expect calls as discussed.

[Lorak-mmk]

@wprzytula

I think that's reasonable. I have one more alternative to suggest:

If we use .unwrap_or_else(|| panic!(...)); in place of expect. Normally this is equivalent code, but in this case, using .unwrap_or_else will prevent lint warnings in end users code. This is only a problem in my eyes because these are macros.

If someone enables this lint that means they don't want any expect calls in their code - and I suspect they have reasons for that other than the name of this method. Tricking the lint does not seem helpful in this case - it obfuscates the code and prevents the user of the lint from achieving the goal of panic prevention.

If you don't find this acceptable, i will implement the .expect calls as discussed.

I think that would be the best solution.

[Kailokk]

I think that would be the best solution.

Then so it shall be :), will ping you when the change is made.

[Kailokk]

@Lorak-mmk should be okay now.

[Lorak-mmk]

CI fail will be fixed by #1060 so we can ignore the fail here.
I'll squash-merge because of unclean commit history.

[Kailokk]

@Lorak-mmk thank you for your time