Clippy: Remove let unit

[matthiasbeyer]

Proposed changes

It seems that clippy failures were merged at some point... I don't know.

This fixes a clippy lint over all crates in the repository, namingly the let-unit-value lint.

Nothing more was done in this patchset.

Types of changes

• [ ] Bugfix (non-breaking change which fixes an issue)
• [ ] New feature (non-breaking change which adds functionality)
• [x] Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
• [ ] Documentation Update (if none of the other choices apply)
• [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

• [x] I have read the CONTRIBUTING doc
• [x] I have signed the CLA (in all commits with git commit -s)
• [x] I ran cargo fmt as mentioned in CODING_GUIDELINES
• [x] I used cargo clippy as mentioned in CODING_GUIDELINES
• [ ] I have added tests that prove my fix is effective or that my feature works
• [ ] I have added necessary documentation (if appropriate)

[didier-wenzek]

It seems that clippy failures were merged at some point... I don't know.

This more likely a new clippy check. We are using this idiom since quit a long time. The motivation is that the following highlight that we are not ignoring something:

let () = do_stuff();

Thanks to the #[must_use] attribute of Result this style is indeed not required.

For me both styles are valid.

[matthiasbeyer]

Why is our workflow is not catching these in the cargo clippy job?

That is indeed a good question :laughing:

~~As @didier-wenzek indicated, this might be a new check and maybe because we're running rust 1.56 in CI, which is kinda old (current is 1.62) the old clippy may not have the check yet.~~

Edit: Well, no, that clippy lint was added before 1.29.0 (it says so on the site of the lint)... so I honestly don't know...

If we update our CI to recent rust, we might experience some more failures even (although locally I didn't see any)!

[cmosd]

Edit: Well, no, that clippy lint was added before 1.29.0 (it says so on the site of the lint)... so I honestly don't know...

So I have both 1.62 and 1.58. On 1.62 if I run cargo clippy --all-features I too see this omit let warning. But on 1.58 I do not. However, if I run cargo clippy --all-features -- -W clippy::pedantic on 1.58 I start seeing the omit let warning.

[matthiasbeyer]

Interesting! Maybe it got promoted recently! :smile:

[rina23q]

We can merge it, however, the current cargo clippy in our workflow doesn't detect the warning, that means, in the future, someone can add let unit.

Which one should we go? Update our Rust version to 1.62.0 or keep 1.58.0 add -W clippy::pedantic option?

[matthiasbeyer]

IMO updating rust is the way to go, as clippy::pedantic is rather strict and also might have false positives. From its documentation:

lints which are rather strict or might have false positives

[cmosd]

@rina23q, @matthiasbeyer does it make sense to build for 1.58.1 (our MSRV) but check linting for 1.62.0?

[matthiasbeyer]

Yes I think that sounds like a good idea, IMO.

[matthiasbeyer]

After thinking again, I think that updating rust to 1.62.0 is the way to go. I mean for the whole project. Because there's no reason to keep it at an old version and having two different versions just introduces complexity in maintaining the CI ecosystem.

I'll file a PR to update it.

[matthiasbeyer]

:thinking: Are we gonna go forward with this?

[didier-wenzek]

🤔 Are we gonna go forward with this?

Yes, I will approve even if I'm okay with the let () = ... syntax. This will let us move freely with new version of clippy.

[matthiasbeyer]

Rebased and resolved merge conflicts.