Skip to content

Commit

Permalink
build: fix existing cargo clippy errors and make sure we run cargo cl…
Browse files Browse the repository at this point in the history
…ippy on the whole repository only with relevant lints
  • Loading branch information
rumenov committed Jan 15, 2024
1 parent cd8ad64 commit 879331e
Show file tree
Hide file tree
Showing 16 changed files with 31 additions and 45 deletions.
17 changes: 8 additions & 9 deletions .bazelrc
@@ -1,12 +1,19 @@
# To require no rustfmt issues, pass --config=fmt.
# To require no clippy issues, pass --config=clippy. Without this, warnings will still be generated.
# To enable both of the above, pass --config=lint.

# --config=ci implies --config=lint
build:ci --config=lint
# --config=lint implies both --config=fmt and --config=clippy.
build:lint --config=fmt
build:lint --config=clippy
# rust-clippy
build:clippy --aspects=@rules_rust//rust:defs.bzl%rust_clippy_aspect
build:clippy --output_groups=+clippy_checks
build --@rules_rust//:clippy.toml=//:clippy.toml --@rules_rust//:clippy_flags=-D,warnings,-D,clippy::all,-D,clippy::mem_forget,-A,clippy::too_many_arguments,-C,debug-assertions=off
# rustfmt
build:fmt --aspects=@rules_rust//rust:defs.bzl%rustfmt_aspect
build:fmt --output_groups=+rustfmt_checks
build --@rules_rust//:rustfmt.toml=//:rustfmt.toml

# https://github.com/bazelbuild/rules_rust/pull/2277 breaks our build in CI
# TODO(levsha): localise the problem and report to the upstream
Expand Down Expand Up @@ -61,14 +68,6 @@ build --experimental_repository_downloader_retries=3 # https://bazel.build/refer

build --cxxopt='-std=c++17'

build:clippy --aspects=@rules_rust//rust:defs.bzl%rust_clippy_aspect
build:clippy --output_groups=+clippy_checks
build --@rules_rust//:clippy.toml=//:clippy.toml --@rules_rust//:clippy_flags=-D,warnings,-D,clippy::all,-D,clippy::mem_forget,-A,clippy::manual_clamp,-A,clippy::redundant_closure,-A,clippy::too_many_arguments,-C,debug-assertions=off

build:fmt --aspects=@rules_rust//rust:defs.bzl%rustfmt_aspect
build:fmt --output_groups=+rustfmt_checks
build --@rules_rust//:rustfmt.toml=//:rustfmt.toml

build --flag_alias=ic_version=//bazel:ic_version
build --flag_alias=ic_version_rc_only=//bazel:ic_version_rc_only
build --flag_alias=s3_endpoint=//gitlab-ci/src/artifacts:s3_endpoint
Expand Down
7 changes: 1 addition & 6 deletions gitlab-ci/src/rust_lint/lint.sh
Expand Up @@ -3,12 +3,10 @@ set -xeuo pipefail

cd "$CI_PROJECT_DIR"
cargo fmt -- --check
cargo clippy --locked --all-features --tests --benches -- \
cargo clippy --locked --all-features --workspace --all-targets -- \
-D warnings \
-D clippy::all \
-D clippy::mem_forget \
-A clippy::manual_clamp \
-A clippy::redundant_closure \
-A clippy::too_many_arguments \
-C debug-assertions=off

Expand All @@ -18,6 +16,3 @@ if cargo tree -e features | grep -q 'serde feature "rc"'; then
fi

cargo run -q -p depcheck

cd "$CI_PROJECT_DIR/rs/replica"
cargo check --features malicious_code --locked
9 changes: 3 additions & 6 deletions rs/consensus/src/consensus/proptests.rs
Expand Up @@ -101,10 +101,7 @@ fn prop_ingress_vec(
max_messages: usize,
max_size: usize,
) -> impl Strategy<Value = Vec<SignedIngress>> {
prop::collection::vec(
(0..max_size).prop_map(|size| make_ingress(size)),
1..max_messages,
)
prop::collection::vec((0..max_size).prop_map(make_ingress), 1..max_messages)
}

fn make_ingress(size: usize) -> SignedIngress {
Expand All @@ -118,8 +115,8 @@ fn prop_xnet_slice(
max_size: usize,
) -> impl Strategy<Value = BTreeMap<SubnetId, CertifiedStreamSlice>> {
prop::collection::btree_map(
(0..3u64).prop_map(|id| subnet_test_id(id)),
(0..max_size).prop_map(|size| make_xnet_slice(size)),
(0..3u64).prop_map(subnet_test_id),
(0..max_size).prop_map(make_xnet_slice),
1..max_messages,
)
}
Expand Down
2 changes: 1 addition & 1 deletion rs/ethereum/cketh/minter/src/state/transactions/tests.rs
Expand Up @@ -1734,7 +1734,7 @@ pub mod arbitrary {
}

fn arb_address() -> impl Strategy<Value = Address> {
uniform20(any::<u8>()).prop_map(|bytes| Address::new(bytes))
uniform20(any::<u8>()).prop_map(Address::new)
}

fn arb_principal() -> impl Strategy<Value = Principal> {
Expand Down
4 changes: 1 addition & 3 deletions rs/ethereum/cketh/minter/tests/tests.rs
Expand Up @@ -652,9 +652,7 @@ fn should_resubmit_new_transaction_when_price_increased() {
})
.expect_pending_transaction()
.retry_processing_withdrawals()
.retrieve_fee_history(|mock| {
mock.modify_response_for_all(&mut |fee_history| increment_base_fee_per_gas(fee_history))
})
.retrieve_fee_history(|mock| mock.modify_response_for_all(&mut increment_base_fee_per_gas))
.expect_status(RetrieveEthStatus::TxSent(EthTransaction {
transaction_hash: DEFAULT_WITHDRAWAL_TRANSACTION_HASH.to_string(),
}))
Expand Down
8 changes: 4 additions & 4 deletions rs/ethereum/ledger-suite-orchestrator/src/scheduler/mod.rs
Expand Up @@ -62,7 +62,7 @@ async fn create_icrc_canisters_for_erc20(contract: &Erc20Contract) -> Result<(),
//TODO real logic to install canisters, in particular retrying should not necessarily re-create canisters.
let ledger_canister_id = create_canister(100_000_000_000)
.await
.map_err(|e| TaskError::CanisterCreationError(e))?;
.map_err(TaskError::CanisterCreationError)?;
//TODO init args should come from `contract` argument
let ledger_arg = LedgerArgument::Init(LedgerInitArgs {
minting_account: Account {
Expand Down Expand Up @@ -97,11 +97,11 @@ async fn create_icrc_canisters_for_erc20(contract: &Erc20Contract) -> Result<(),
Encode!(&ledger_arg).expect("BUG: failed to encode ledger init arg"),
)
.await
.map_err(|e| TaskError::InstallCodeError(e))?;
.map_err(TaskError::InstallCodeError)?;

let index_canister_id = create_canister(100_000_000_000)
.await
.map_err(|e| TaskError::CanisterCreationError(e))?;
.map_err(TaskError::CanisterCreationError)?;
let index_arg = Encode!(&Some(IndexArg::Init(IndexInitArg {
ledger_id: ledger_canister_id
})))
Expand All @@ -112,7 +112,7 @@ async fn create_icrc_canisters_for_erc20(contract: &Erc20Contract) -> Result<(),
index_arg,
)
.await
.map_err(|e| TaskError::InstallCodeError(e))?;
.map_err(TaskError::InstallCodeError)?;

let created_canisters = Canisters::new(ledger_canister_id, index_canister_id);
mutate_state(|s| s.record_managed_canisters(contract.clone(), created_canisters));
Expand Down
Expand Up @@ -19,7 +19,7 @@ fn run_bench<M: criterion::measurement::Measurement>(
group.bench_function(bench_name, |b| {
b.iter_batched(
// Test setup.
|| setup(),
setup,
// Test measurement.
|(env, test_canister)| {
let result = env.execute_ingress(
Expand Down
6 changes: 3 additions & 3 deletions rs/execution_environment/benches/management_canister/ecdsa.rs
Expand Up @@ -140,21 +140,21 @@ fn sign_with_ecdsa_benchmark(c: &mut Criterion) {
method,
"calls:10/derivation_paths:1/buf_size:1",
(10, 1, 1),
|result| expect_reply(result),
expect_reply,
);
run_bench(
&mut group,
method,
"calls:10/derivation_paths:1/buf_size:2M",
(10, 1, 2_000_000),
|result| expect_reply(result),
expect_reply,
);
run_bench(
&mut group,
method,
"calls:10/derivation_paths:250/buf_size:8k",
(10, 250, 8_000),
|result| expect_reply(result),
expect_reply,
);
run_bench(
&mut group,
Expand Down
Expand Up @@ -62,7 +62,7 @@ fn run_bench<M: criterion::measurement::Measurement>(
group.bench_function(bench_name, |b| {
b.iter_batched(
// Test setup.
|| setup(),
setup,
// Test measurement.
|(env, test_canister)| {
let msg_id = env.send_ingress(
Expand Down
Expand Up @@ -71,7 +71,7 @@ pub fn install_code_benchmark(c: &mut Criterion) {
&mut group,
"canisters:1/wasm:0B/arg:0B",
(1, 0, 0),
|result| expect_reply(result),
expect_reply,
);
run_bench(
&mut group,
Expand Down
Expand Up @@ -69,7 +69,7 @@ pub fn update_settings_benchmark(c: &mut Criterion) {
&mut group,
"canisters:10/controllers:10/reply",
(10, 10),
|result| expect_reply(result),
expect_reply,
);
run_bench(
&mut group,
Expand Down
4 changes: 2 additions & 2 deletions rs/ingress_manager/src/ingress_selector.rs
Expand Up @@ -2004,7 +2004,7 @@ mod tests {
);
let mut small_payloads = vec![(messages_0, canister_0)];

for canister_id in (1..3).map(|i| canister_test_id(i)) {
for canister_id in (1..3).map(canister_test_id) {
let (m, c) = generate_ingress_with_params(
canister_id,
/* msg_count = */ 2,
Expand All @@ -2028,7 +2028,7 @@ mod tests {

// small ingress messages that fall below quota, to generate surplus that
// is later dispersed amongst the first three canisters.
for canister_id in (3..30).map(|i| canister_test_id(i)) {
for canister_id in (3..30).map(canister_test_id) {
small_payloads.push(generate_ingress_with_params(
canister_id,
/* msg_count = */ 1,
Expand Down
5 changes: 1 addition & 4 deletions rs/nervous_system/integration_tests/tests/sns_lifecycle.rs
Expand Up @@ -216,10 +216,7 @@ mod sns {
if proposal_data.executed_timestamp_seconds > 0 {
return Ok(proposal_data);
}
proposal_data
.failure_reason
.clone()
.map_or(Ok(()), |err| Err(err))?;
proposal_data.failure_reason.clone().map_or(Ok(()), Err)?;
last_proposal_data = Some(proposal_data);
pocket_ic.advance_time(Duration::from_millis(100));
}
Expand Down
Expand Up @@ -22,7 +22,7 @@ impl RosettaClient {
Ok(response
.network_identifiers
.into_iter()
.map(|ni| NetworkIdentifier(ni))
.map(NetworkIdentifier)
.collect::<Vec<NetworkIdentifier>>())
}

Expand Down
2 changes: 1 addition & 1 deletion rs/sns/init/src/lib.rs
Expand Up @@ -1570,7 +1570,7 @@ impl SnsInitPayload {
SNS_WASM_CANISTER_ID,
EXCHANGE_RATE_CANISTER_ID,
]
.map(|id| PrincipalId::from(id));
.map(PrincipalId::from);

let nns_canisters_listed_as_dapp = dapp_canisters
.canisters
Expand Down
2 changes: 1 addition & 1 deletion rs/state_machine_tests/src/lib.rs
Expand Up @@ -1377,7 +1377,7 @@ impl StateMachine {
// Run `IngressFilter` on the ingress message.
self.ingress_filter
.should_accept_ingress_message(state, &provisional_whitelist, msg.content())
.map_err(|e| SubmitIngressError::UserError(e))?;
.map_err(SubmitIngressError::UserError)?;

// All checks were successful at this point so we can push the ingress message to the ingress pool.
let message_id = msg.id();
Expand Down

0 comments on commit 879331e

Please sign in to comment.