Bump MSRV to 1.81 + Clippy fixes #1356
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
During development, especially when editing anything in Cargo.toml, I want to verify that Cargo.lock.msrv still works. Doing this manually is tedious, and such helper targets make it easier.
I sometimes want to perform more extensive static checking. This happens if I have some changes that fail in CI, but pass `make ci`. I collected all static checks from CI and put them here.
This allows us to use `expect` lint level.
|
|
Events-related node status is not used for anything, and I don't see any possible uses for it in the future. For that reason I don't think we need this code for anything.
Some checks stopped triggering since we introduced those lines, for one reason or another. There is no need to keep them.
If some check always (independent of configuration) triggers for some code, it is better to use expect because we will be notified when the check stops triggering. Some uses of "allow" remain. We could try to replace them with `cfg_attr` magic, but it would be compplicated in many cases and I feel that it won't bring much benefit.
Cleanup unused dependancies
Turns out clippy has a terrible and shocking default: it won't make suggestions that require introducing breaking changes. This means that e.g. if we introduce a type which has a naming that clippy would normally complain about, it won't if the type is pub! For this reason we have many such types. This commit disables this absurd default, and adds comment and attribute to each place affected by this.
|
I don't understand this failure in MSRV check. Those importa are obviously unused, why is the check not triggered? // The purpose of this test is to verify that the important types are
// correctly re-exported in `scylla` crate.
#[test]
fn test_types_imports() {
#[expect(unused_imports)]
use _scylla::frame::response::result::{CollectionType, ColumnType, NativeType, UserDefinedType};
#[expect(unused_imports)]
use _scylla::value::{
Counter, CqlDate, CqlDecimal, CqlDecimalBorrowed, CqlDuration, CqlTime, CqlTimestamp,
CqlTimeuuid, CqlValue, CqlVarint, CqlVarintBorrowed, MaybeUnset, Row, Unset, ValueOverflow
};
} |
wprzytula
requested changes
May 15, 2025
Comment on lines
+118
to
+137
| .PHONY: static_full | ||
| static_full: | ||
| cargo fmt --all -- --check | ||
| cargo clippy --all-targets | ||
| cargo clippy --all-targets --all-features | ||
| cargo clippy --all-targets -p scylla-cql --features "full-serialization" | ||
| RUSTFLAGS="--cfg cpp_rust_unstable -Dwarnings" cargo clippy --all-targets --all-features | ||
| cargo check --all-targets -p scylla --features "" | ||
| cargo check --all-targets -p scylla --all-features | ||
| cargo check --all-targets -p scylla --features "full-serialization" | ||
| cargo check --all-targets -p scylla --features "metrics" | ||
| cargo check --all-targets -p scylla --features "secrecy-08" | ||
| cargo check --all-targets -p scylla --features "chrono-04" | ||
| cargo check --all-targets -p scylla --features "time-03" | ||
| cargo check --all-targets -p scylla --features "num-bigint-03" | ||
| cargo check --all-targets -p scylla --features "num-bigint-04" | ||
| cargo check --all-targets -p scylla --features "bigdecimal-04" | ||
| cargo check --all-targets -p scylla --features "openssl-010" | ||
| cargo check --all-targets -p scylla --features "rustls-023" | ||
| cargo check --features "openssl-010" --features "rustls-023" |
There was a problem hiding this comment.
💭 Now we need to keep this in sync with the CI. It makes sense to have a notice about this in CI workflows.
Comment on lines
323
to
328
| parse_quote! { | ||
| fn deserialize( | ||
| #[allow(unused_mut)] | ||
| #[expect(unused_mut)] | ||
| mut row: #macro_internal::ColumnIterator<#frame_lifetime, #metadata_lifetime>, | ||
| ) -> ::std::result::Result<Self, #macro_internal::DeserializationError> { | ||
| ::std::result::Result::Ok(Self { |
There was a problem hiding this comment.
🤔 This change looks incorrect. The idea of this lint is that mut is not needed iff there are 0 field finalizers. If there are more, then mut is needed, so expect should break. If it doesn't, then I don't understand something.
Comment on lines
577
to
580
| parse_quote! { | ||
| fn deserialize( | ||
| #[allow(unused_mut)] | ||
| #[expect(unused_mut)] | ||
| mut row: #macro_internal::ColumnIterator<#frame_lifetime, #metadata_lifetime>, |
Comment on lines
+539
to
540
| #[expect(unused_mut)] | ||
| let mut iterator_type: syn::Type = |
There was a problem hiding this comment.
🔧 I don't see what is a this mut even for. This is of syn::Type type; I don't see a way for this variable to mutate.
Comment on lines
62
to
64
| // Currently, this is only used for cloud; but it makes abstract sense to pass endpoint here | ||
| // also for non-cloud cases, so let's just allow(unused). | ||
| // also for non-cloud cases, so let's just expect(unused). | ||
| #[allow(unused)] endpoint: &UntranslatedEndpoint, |
There was a problem hiding this comment.
🔧 You updated the comment, but not the actual occurrence.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We want to use
#[expect(...)]in #1296 which requires Rust 1.81Bumping MSRV and transitioning from allow to expect where possible is big enough change that it imo warrants a separate PR.
I also extracted other smaller changes from #1296 here: Cargo.toml cleanup, Criterion bump, Makefile changes.
I noticed that at some point Clippy stopped checking naming of our pub items. This is because it has an option
avoid-breaking-exported-api, enabled by default.This is an absurd and terrible default because it makes the checks in PRs (when it is perfectly fine to break API) disabled as well.
For this reason we accumulated a lot of wrongly named items which we will need to fix in 2.0.
I added
#[expect(...)]and TODO to each one.Additionally I removed
Node::is_downand all related code. There is no use for it and it was triggering a check, so instead of usingalloworcfg_attrI decied to remove it totally.Pre-review checklist
I added relevant tests for new features and bug fixes.I have provided docstrings for the public items that I want to introduce.I have adjusted the documentation in./docs/source/.I added appropriateFixes:annotations to PR description.