Cargo build produces several warning if used with recent toolchain

[lorbax]

I have toolchain with rustc 1.78

~/s/s/protocols (dev) [1]> rustup show
Default host: x86_64-unknown-linux-gnu
rustup home:  /home/lorban/.rustup

installed toolchains
--------------------

stable-x86_64-unknown-linux-gnu (default)
nightly-x86_64-unknown-linux-gnu
1.75.0-x86_64-unknown-linux-gnu
1.75-x86_64-unknown-linux-gnu

active toolchain
----------------

stable-x86_64-unknown-linux-gnu (default)
rustc 1.78.0 (9b00956e5 2024-04-29)

If I compile protocols crate I get the following warnings

warning: trait `Variable` is never used
  --> v2/binary-sv2/no-serde-sv2/codec/src/codec/mod.rs:37:11
   |
37 | pub trait Variable {
   |           ^^^^^^^^
   |
   = note: `#[warn(dead_code)]` on by default

warning: trait `IntoOwned` is never used
 --> v2/binary-sv2/no-serde-sv2/codec/src/datatypes/non_copy_data_types/mod.rs:7:7
  |
7 | trait IntoOwned {
  |       ^^^^^^^^^

warning: `binary_codec_sv2` (lib) generated 2 warnings
warning: `binary_codec_sv2` (lib) generated 2 warnings (2 duplicates)
warning: methods `into_aesg` and `into_chacha` are never used
  --> v2/noise-sv2/src/cipher_state.rs:26:8
   |
7  | pub trait CipherState<Cipher_: AeadCipher>
   |           ----------- methods in this trait
...
26 |     fn into_aesg(mut self) -> Option<Cipher<Aes256Gcm>> {
   |        ^^^^^^^^^
...
33 |     fn into_chacha(mut self) -> Option<Cipher<ChaCha20Poly1305>> {
   |        ^^^^^^^^^^^
   |
   = note: `#[warn(dead_code)]` on by default

warning: associated items `name`, `hkdf_3`, and `ecdh` are never used
   --> v2/noise-sv2/src/handshake.rs:10:8
    |
9   | pub trait HandshakeOp<Cipher: AeadCipher>: CipherState<Cipher> {
    |           ----------- associated items in this trait
10  |     fn name(&self) -> String;
    |        ^^^^
...
69  |     fn hkdf_3(
    |        ^^^^^^
...
109 |     fn ecdh(private: &[u8], public: &[u8]) -> [u8; 32] {
    |        ^^^^

warning: struct `Seq` is never constructed
   --> v2/binary-sv2/serde-sv2/src/de.rs:456:8
    |
456 | struct Seq<'de, 'a> {
    |        ^^^
    |
    = note: `#[warn(dead_code)]` on by default

warning: method `into_static` is never used
 --> v2/binary-sv2/serde-sv2/src/primitives/byte_arrays/mod.rs:8:8
  |
7 | pub trait IntoStatic {
  |           ---------- method in this trait
8 |     fn into_static(self) -> Self;
  |        ^^^^^^^^^^^

warning: `noise_sv2` (lib) generated 2 warnings
warning: `serde_sv2` (lib) generated 2 warnings
warning: struct `ExtendedJobs` is never constructed
  --> v2/roles-logic-sv2/src/job_dispatcher.rs:95:8
   |
95 | struct ExtendedJobs {
   |        ^^^^^^^^^^^^
   |
   = note: `#[warn(dead_code)]` on by default

warning: `roles_logic_sv2` (lib) generated 1 warning
    Finished `dev` profile [optimized + debuginfo] target(s) in 0.08s

Possibly also other crates in the stack are affected. I think that this arose after the removal of rust-version field from crates
#981

We should specify the toolchain every time that is called cargo build or cargo clippy
cargo +1.75 build
or set some sort of override, like setting the RUSTUP_TOOLCHAIN env variable. This has to be done in every script (like in /scripts/clippy-on-all-workspaces.sh or those that triggers MG tests, like this one. I am not practical with the issue, so perhaps there is a better solution.

[jbesraa]

If they are unused shouldn't they be removed?

[lorbax]

If they are unused shouldn't they be removed?

what should be removed?

[jbesraa]

so you get this among other warnings, is Variable used anywhere in the code? if its used under a specific flag then those warning are fine, if its not used at all it should be removed..

warning: trait `Variable` is never used
  --> v2/binary-sv2/no-serde-sv2/codec/src/codec/mod.rs:37:11
   |
37 | pub trait Variable {
   |           ^^^^^^^^
   |
   = note: `#[warn(dead_code)]` on by default

[lorbax]

so you get this among other warnings, is Variable used anywhere in the code? if its used under a specific flag then those warning are fine, if its not used at all it should be removed..

warning: trait `Variable` is never used
  --> v2/binary-sv2/no-serde-sv2/codec/src/codec/mod.rs:37:11
   |
37 | pub trait Variable {
   |           ^^^^^^^^
   |
   = note: `#[warn(dead_code)]` on by default

I get it using rustc 1.78, but I don't get it using rustc 1.75.

[Fi3]

yep cause before 1.75 were enforced by the toml file. Now that is not anymore enforced otherwise we get incompatibility with some editors, we need to specify the version in the script that runs fmt and clippy. Would be also nice to specify it somewhere where new contributors can see it very well I don't want them to waste time in solving "future" clippy error.

[xyephy]

yep cause before 1.75 were enforced by the toml file. Now that is not anymore enforced otherwise we get incompatibility with some editors, we need to specify the version in the script that runs fmt and clippy. Would be also nice to specify it somewhere where new contributors can see it very well I don't want them to waste time in solving "future" clippy error.

does this mean the default toolchain MSRV should be set to 1.75?

[Fi3]

that means that it was, but we removed it cause it was creating issue with some text editors.

[jbesraa]

yep cause before 1.75 were enforced by the toml file. Now that is not anymore enforced otherwise we get incompatibility with some editors, we need to specify the version in the script that runs fmt and clippy. Would be also nice to specify it somewhere where new contributors can see it very well I don't want them to waste time in solving "future" clippy error.

does this mean the default toolchain MSRV should be set to 1.75?

The stratum project should work with any toolchain equal or above 1.75. All the warnings we are seeing should probably be addressed in a PR.

[Fi3]

this will make CI more complicated. We will need to:

1. ensure that project build with MSRV
2. ensure that we have no clippy warning with rust last

It will make also development experience worst, since you will need to run cargo clippy with whatever is rust last on CI.

But I'm not opposed to it. Both approaches are valid: (1) run everything with MSRV and (2) only compile with MSRV and run checks with last.

[jbesraa]

You are right that we should check MSRV, and it was added before removing the tool-chain pining https://github.com/stratum-mining/stratum/actions/workflows/rust-msrv.yaml. Currently it only checks build process, but I guess we should run everything(ie testing as well) with latest toolchain version and with MSRV.

I agree also that we should handle those warnings. currently it is kinda annoying. PRs are more than welcome (:

[jbesraa]

Another note, we are currently running Clippy and Fmt on Nightly, unless we have specific features we need from Nightly for those, I think we can run them on latest toolchain, which I think most of devs are running usually(?)

[Fi3]

Fmt must be runned on Nightly, not sure about clippy

[jbesraa]

@Fi3 why? I think you can run fmt on any toolchain

[Fi3]

cause we use a feature that work only on nightly I don't remeber whcih one but if you try to run it you will see

[Fi3]

here you are:

user@user ~/s/s/protocols (dev)> cargo fmt
Warning: can't set `imports_indent = Block`, unstable features are only available in nightly channel.
Warning: can't set `imports_layout = Mixed`, unstable features are only available in nightly channel.
Warning: can't set `imports_granularity = Crate`, unstable features are only available in nightly channel.
....