Fix clippy expect_used complains for derive(FromRow) macro

[Mr-Leshiy]

Description

Disables clippy::expect_used clippy rule for the derive(FromRow) macro.
As right now inside derive(FromRow) definition, as a part of the produced by this macro code expect() is used,
so it also violates and affects the clients code.
For that cases when the consumer of this library enabled clippy::expect_used for their project,
it requires to somehow fix it and disable it for the derive(FromRow) produced code.

Dependent issue

input-output-hk/catalyst-voices#828

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 detected some API incompatibilities in this PR.
Checked commit: c6523b6

See the following report for details:

cargo semver-checks output

./scripts/semver-checks.sh --baseline-rev ab07be6d8816df9ba53529391b65fbd63cc3e59a
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev ab07be6d8816df9ba53529391b65fbd63cc3e59a
     Cloning ab07be6d8816df9ba53529391b65fbd63cc3e59a
     Parsing scylla v0.14.0 (current)
      Parsed [  24.035s] (current)
     Parsing scylla v0.14.0 (baseline)
      Parsed [  20.500s] (baseline)
    Checking scylla v0.14.0 -> v0.14.0 (no change)
     Checked [   0.107s] 89 checks: 88 pass, 1 fail, 0 warn, 0 skip

--- failure enum_variant_missing: pub enum variant removed or renamed ---

Description:
A publicly-visible enum has at least one variant that is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.35.0/src/lints/enum_variant_missing.ron

Failed in:
  variant CqlEventHandlingError::FrameError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-ab07be6d8816df9ba53529391b65fbd63cc3e59a/789b83d096ad7d9a362060a8016a38862072c0e5/scylla/src/transport/errors.rs:552
  variant BrokenConnectionErrorKind::FrameError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-ab07be6d8816df9ba53529391b65fbd63cc3e59a/789b83d096ad7d9a362060a8016a38862072c0e5/scylla/src/transport/errors.rs:499
  variant ConnectionSetupRequestErrorKind::FrameError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-ab07be6d8816df9ba53529391b65fbd63cc3e59a/789b83d096ad7d9a362060a8016a38862072c0e5/scylla/src/transport/errors.rs:386
  variant RequestError::FrameError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-ab07be6d8816df9ba53529391b65fbd63cc3e59a/789b83d096ad7d9a362060a8016a38862072c0e5/scylla/src/transport/errors.rs:624

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [  44.702s] scylla
     Parsing scylla-cql v0.3.0 (current)
      Parsed [  10.381s] (current)
     Parsing scylla-cql v0.3.0 (baseline)
      Parsed [  10.396s] (baseline)
    Checking scylla-cql v0.3.0 -> v0.3.0 (no change)
     Checked [   0.099s] 89 checks: 87 pass, 2 fail, 0 warn, 0 skip

--- failure enum_missing: pub enum removed or renamed ---

Description:
A publicly-visible enum cannot be imported by its prior path. A `pub use` may have been removed, or the enum itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.35.0/src/lints/enum_missing.ron

Failed in:
  enum scylla_cql::frame::frame_errors::FrameError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-ab07be6d8816df9ba53529391b65fbd63cc3e59a/789b83d096ad7d9a362060a8016a38862072c0e5/scylla-cql/src/frame/frame_errors.rs:12
  enum scylla_cql::frame::frame_errors::ParseError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-ab07be6d8816df9ba53529391b65fbd63cc3e59a/789b83d096ad7d9a362060a8016a38862072c0e5/scylla-cql/src/frame/frame_errors.rs:40
  enum scylla_cql::cql_to_rust::CqlTypeError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-ab07be6d8816df9ba53529391b65fbd63cc3e59a/789b83d096ad7d9a362060a8016a38862072c0e5/scylla-cql/src/frame/response/cql_to_rust.rs:20
  enum scylla_cql::frame::response::cql_to_rust::CqlTypeError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-ab07be6d8816df9ba53529391b65fbd63cc3e59a/789b83d096ad7d9a362060a8016a38862072c0e5/scylla-cql/src/frame/response/cql_to_rust.rs:20

--- failure inherent_method_missing: pub method removed or renamed ---

Description:
A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.35.0/src/lints/inherent_method_missing.ron

Failed in:
  LegacySerializedValues::new_from_frame, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-ab07be6d8816df9ba53529391b65fbd63cc3e59a/789b83d096ad7d9a362060a8016a38862072c0e5/scylla-cql/src/frame/value.rs:803

     Summary semver requires new major version: 2 major and 0 minor checks failed
    Finished [  20.934s] scylla-cql
make: *** [Makefile:61: semver-rev] Error 1

[Lorak-mmk]

There was a recent PR in this area: #1055

expect_used is allowed bu default - so for it to fail a user of the library has to enable it explicitly.
Enabling such lint means that a user does not want expect calls in the code - or at least wants to be warned about them and review them.
If we add such allows in produced code then users will not be warned about those calls, which defeats the purpose of this lint.

[Mr-Leshiy]

@Lorak-mmk I can't agree with you. Have a opposite opinion on this 🙂.
Yes, the macro itself technically expanded and the macro code inserted into the place where this macros is used.
And as such macro defined by the library (as any other primitives like structs, function) from the client code it does not make any sense to know how it works and leaking potential linting problems, because from the client side it is just impossible to fix them.
As a result from the clients code to fulfil the linter as it was enabled (like in our case), it leads to the awful code like this:

#[allow(clippy::expect_used)]
mod result {
    use scylla::FromRow;
    #[derive(FromRow)]
    pub(crate) struct YourStruct {
    }
}
use result:: YourStruct;

In general, for me it seems that the best solution would be eliminate the usage of the .expect() at all, but it will leads to the major refactor of this macro implementation.
If you dont mind, I can try to modify this derive implementation.

But in general I would somehow fix this part, to reduce a headache on the clients side, because right its annoying, I can tell from our team experience 🙂 .

[Lorak-mmk]

@Lorak-mmk I can't agree with you. Have a opposite opinion on this 🙂. Yes, the macro itself technically expanded and the macro code inserted into the place where this macros is used. And as such macro defined by the library (as any other primitives like structs, function) from the client code it does not make any sense to know how it works and leaking potential linting problems, because from the client side it is just impossible to fix them.

If your goal in denying use of expect is to avoid panics, then library code is just as important as your own code (it can panic just as well) so it should be reviewed just as carefully.
It is not impossible to deal with badly behaved library:

• You can contribute the fix
• You can use your own fork
• You can decide to use different library
• You can rewrite part of the library yourself.

As a result from the clients code to fulfil the linter as it was enabled (like in our case), it leads to the awful code like this:

#[allow(clippy::expect_used)]
mod result {
    use scylla::FromRow;
    #[derive(FromRow)]
    pub(crate) struct YourStruct {
    }
}
use result:: YourStruct;

Yeah that's ugly :/ It would be much better if this worked:

    #[derive(Debug, FromRow)]
    #[allow(clippy::expect_used)]
    struct RowData {
    }

It would be good to allow this (unless it turns out to be impossible for some reason). After it is done it would be my recommended fix here instead of putting this allow in macro-generated code.
Why? Someone who is OK with expect in library code can add this directive - the cost is low, just 1 additional line of code.
Someone who is careful about expect in their code and library code (and would like to carefully review each such call) will not be warned about the presence of it at all if macro generated those allow directives - and there is no workaround for that.

In general, for me it seems that the best solution would be eliminate the usage of the .expect() at all, but it will leads to the major refactor of this macro implementation. If you dont mind, I can try to modify this derive implementation.

I agree that removing exepect / unwrap etc to completely avoid the possibility of panic is probably the best option. Is it feasible? I don't know, I don't remember this code that well. I can see that new macros (DeserializeRow / DeserializeValue) also use some unwrap's and expect's. @wprzytula do you think it's possible to avoid them completely?

But in general I would somehow fix this part, to reduce a headache on the clients side, because right its annoying, I can tell from our team experience 🙂 .

[wprzytula]

Yeah that's ugly :/ It would be much better if this worked:

    #[derive(Debug, FromRow)]
    #[allow(clippy::expect_used)]
    struct RowData {
    }

It would be good to allow this (unless it turns out to be impossible for some reason). After it is done it would be my recommended fix here instead of putting this allow in macro-generated code.

Agreed. I support this, especially that it would allow solving other similar problems arising from macro expansion, too, by adding corresponding #[allow] attributes.
So what could be done (preferably by external contributors, as the maintainers are busy with higher-priority items now) is modifying FromRow, FromCqlVal, DeserializeRow, DeserializeValueso that those macros put decorated struct definitions into a new module and decorate the module with #[allow] attributes specified after[derive]`.

[Mr-Leshiy]

@Lorak-mmk , @wprzytula yea, sorry I see your point. just adding #[allow(clippy::expect_used)] is not an option, its just move responsibility to fix it in future 😔

Seems I've found one possible solution how to eliminate the usage of .expect.
Check it out !

[wprzytula]

expect (a method that may panic) is replaced with indexing (which may panic as well). This defeats the purpose of expect_used lint, which is to keep the code free of possible panics.

[Mr-Leshiy]

I've used an array instead of iterator let mut row_columns: [_; #fields_count], which allows for Rust compiler to detect such index out of range errors during the compile time.
So technically speaking if you have not disabled unconditional panic lint (which you definitely not), this code is safe.
And if in this code a bug will be appears it will be 99.9% (0.1% stand when unconditional panic will be disabled, and I dont why someone really want to do that, its like to disable a borrow checker) catches by the compiler, with the detailed explanation what was incorrect.

E.g. I've modified this line of the code to

let col_value = ::std::mem::take(&mut row_columns[#col_ix + 1]);

And rustc immediately complains

error: this operation will panic at runtime
    --> scylla-cql/src/frame/response/cql_to_rust.rs:1014:43
     |
1014 |         struct MyRow(i32, Option<String>, Option<Vec<i32>>);
     |                                           ^^^^^^ index out of bounds: the length is 3 but the index is 3
     |
     = note: `#[deny(unconditional_panic)]` on by default

error: could not compile `scylla-cql` (lib test) due to 1 previous error

https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/builtin/static.UNCONDITIONAL_PANIC.html

[Mr-Leshiy]

So to sum up, this code allows to catch an error on the compile time, using .expect does not allow to do that and will return it during the runtime only

[wprzytula]

You're right, it seems I have missed that you made row_columns an array. In such case indexing can be indeed checked by the compiler, with no panic risk in runtime if no warning is emitted about that.
Could you please write in comments why the array is used (to preserve length info in the type, to avoid panics) and why indexing won't panic (because array has a compile-time known length, and the index is a compile-time const, too)?

[Mr-Leshiy]

yea sure, will do that !

[Mr-Leshiy]

@wprzytula done

[wprzytula]

@Mr-Leshiy Note that the new deserialization framework is going to soon replace the old one, with DeserializeRow/Value replacing FromRow/CqlVal. Derive macros for the former ones are going to need your refactor, too; I encourage you to try to get rid of panicking there, too, in a follow-up.

[Mr-Leshiy]

@wprzytula Yea, would be happy to help there as well.
But I would do that in the next PR if you dont mind

[wprzytula]

Why did you delete this?

[Mr-Leshiy]

Following our discussion, I agreed with the value of such possible linting errors which could appear under the macro definition.
So as we dont want to prevent using expect or unwrap under the macro definition, this line of code just does not make sense.

[wprzytula]

So as we dont want to prevent using expect or unwrap under the macro definition

I'm confused. I thought the purpose of this PR, that is, the changes I've just reviewed, is to remove use of expect or unwrap from the macro definition, so that the lint can be used with no exception of the macro-generated code.

[Mr-Leshiy]

🤔
yea, agree will add it back, make sense