Unverified Commit 4b909706 authored by Michael Müller's avatar Michael Müller Committed by GitHub
Browse files

Run clippy with `--all-targets` (#884)

* Add clippy flag `--all-targets`

* Fix `redundant_closure`

* Fix `redundant_clone`

* Fix `clone_on_copy`

* Fix `needless_borrow`

* Fix `bool_assert_comparison`

* Fix `len_zero`

* Fix `stable_sort_primitive`

* Forward feature

* Allow `type_complexity`

* Fix `unnecessary_mut_passed`

* Fix `manual_map`

* Fix `match_like_matches_macro`

* Remove duplicate test

* Fix `new_without_default`

* Fix `match_ref_pats`

* Allow clippy rules for  tests

* Fix `unit_arg`

* Apply `cargo fmt`

* Add explanatory comments

* Remove `--all-targets` for Wasm tests

* Fix `or_fun_call`

* Fix `bool_comparison`

* Fix `needless_collect`

* Revert "Forward feature"

This reverts commit 472c50ed.

* Fix `bool_assert_comparison`

* Deactivate test because of feature issue with dev-dependencies

* Add flag

* Fix `unique_topics` tests

* Allow `clippy:bool_assert_comparison`

* Revert "Fix `bool_assert_comparison`"

This reverts commit bceb2dc2.

* Revert "Fix `bool_comparison`"

This reverts commit 02960f98.

* Fix flags positioning

* Add comment for clarification

* Fix comparison

* Do not check all targets for target = wasm32

* Adapt `check-workspace.sh` with clippy flag

* Add clarification comment

* Ignore test

* Revert `CLIPPY_FLAGS` hack

* Link ink! issue

* Fix `assert`'s

* Fix `assert`'s
parent 0e9a4251
Pipeline #151237 failed with stages
in 6 minutes and 36 seconds
......@@ -24,7 +24,7 @@ variables:
# this var is changed to "-:staging" when the CI image gets rebuilt
# read more https://github.com/paritytech/scripts/pull/244
ALL_CRATES: "${PURELY_STD_CRATES} ${ALSO_WASM_CRATES}"
DELEGATOR_SUBCONTRACTS: "accumulator adder subber"
DELEGATOR_SUBCONTRACTS: "accumulator adder subber"
workflow:
rules:
......@@ -191,7 +191,7 @@ clippy-std:
artifacts: false
script:
- for crate in ${ALL_CRATES}; do
cargo clippy --verbose --all-features --manifest-path crates/${crate}/Cargo.toml -- -D warnings;
cargo clippy --verbose --all-targets --all-features --manifest-path crates/${crate}/Cargo.toml -- -D warnings;
done
clippy-wasm:
......@@ -269,10 +269,10 @@ examples-clippy-std:
artifacts: false
script:
- for example in examples/*/; do
cargo clippy --verbose --manifest-path ${example}/Cargo.toml -- -D warnings;
cargo clippy --verbose --all-targets --manifest-path ${example}/Cargo.toml -- -D warnings;
done
- for contract in ${DELEGATOR_SUBCONTRACTS}; do
cargo clippy --verbose --manifest-path examples/delegator/${contract}/Cargo.toml -- -D warnings;
cargo clippy --verbose --all-targets --manifest-path examples/delegator/${contract}/Cargo.toml -- -D warnings;
done
examples-clippy-wasm:
......
......@@ -24,8 +24,7 @@ fn simple_storage_works() {
}
};
assert!(matches!(
<ir::Item as TryFrom<_>>::try_from(storage_struct.clone())
.map_err(|err| err.to_string()),
<ir::Item as TryFrom<_>>::try_from(storage_struct).map_err(|err| err.to_string()),
Ok(ir::Item::Ink(ir::InkItem::Storage(_)))
))
}
......@@ -41,8 +40,7 @@ fn simple_event_works() {
}
};
assert!(matches!(
<ir::Item as TryFrom<_>>::try_from(event_struct.clone())
.map_err(|err| err.to_string()),
<ir::Item as TryFrom<_>>::try_from(event_struct).map_err(|err| err.to_string()),
Ok(ir::Item::Ink(ir::InkItem::Event(_)))
))
}
......
......@@ -283,7 +283,7 @@ mod tests {
.unwrap()
.inputs()
.cloned()
.map(|pat_type| syn::FnArg::Typed(pat_type))
.map(syn::FnArg::Typed)
.collect::<Vec<_>>();
assert_eq!(actual_inputs, expected_inputs);
}
......
......@@ -366,7 +366,7 @@ mod tests {
.unwrap()
.inputs()
.cloned()
.map(|pat_type| syn::FnArg::Typed(pat_type))
.map(syn::FnArg::Typed)
.collect::<Vec<_>>();
assert_eq!(actual_inputs, expected_inputs);
}
......
......@@ -44,4 +44,10 @@ std = [
"ink_lang_ir/std",
"ink_primitives/std",
]
# Due to https://github.com/rust-lang/cargo/issues/6915 features that affect a dev-dependency
# currently can't be enabled by a parent crate, hence `["ink_env/ink-experimental-engine"]` does
# not work.
# After we switch to the new off-chain environment with https://github.com/paritytech/ink/issues/565
# we can remove the feature altogether.
ink-experimental-engine = []
......@@ -37,9 +37,9 @@ mod flipper {
#[ink::test]
fn it_works() {
let mut flipper = Flipper::new(false);
assert_eq!(flipper.get(), false);
assert!(!flipper.get());
flipper.flip();
assert_eq!(flipper.get(), true);
assert!(flipper.get());
}
}
}
......
......@@ -460,7 +460,7 @@ mod erc721 {
assert_eq!(erc721.owner_of(2), Some(accounts.alice));
// Get contract address
let callee = ink_env::account_id::<ink_env::DefaultEnvironment>()
.unwrap_or([0x0; 32].into());
.unwrap_or_else(|_| [0x0; 32].into());
// Create call
let mut data =
ink_env::test::CallData::new(ink_env::call::Selector::new([0x00; 4])); // balance_of
......@@ -492,7 +492,7 @@ mod erc721 {
assert_eq!(erc721.approve(accounts.bob, 1), Ok(()));
// Get contract address.
let callee = ink_env::account_id::<ink_env::DefaultEnvironment>()
.unwrap_or([0x0; 32].into());
.unwrap_or_else(|_| [0x0; 32].into());
// Create call
let mut data =
ink_env::test::CallData::new(ink_env::call::Selector::new([0x00; 4])); // balance_of
......@@ -536,13 +536,10 @@ mod erc721 {
// Approve token Id 1 transfer for Bob on behalf of Alice.
assert_eq!(erc721.set_approval_for_all(accounts.bob, true), Ok(()));
// Bob is an approved operator for Alice
assert_eq!(
erc721.is_approved_for_all(accounts.alice, accounts.bob),
true
);
assert!(erc721.is_approved_for_all(accounts.alice, accounts.bob));
// Get contract address.
let callee = ink_env::account_id::<ink_env::DefaultEnvironment>()
.unwrap_or([0x0; 32].into());
.unwrap_or_else(|_| [0x0; 32].into());
// Create call
let mut data =
ink_env::test::CallData::new(ink_env::call::Selector::new([0x00; 4])); // balance_of
......@@ -578,10 +575,7 @@ mod erc721 {
// Remove operator approval for Bob on behalf of Alice.
assert_eq!(erc721.set_approval_for_all(accounts.bob, false), Ok(()));
// Bob is not an approved operator for Alice.
assert_eq!(
erc721.is_approved_for_all(accounts.alice, accounts.bob),
false
);
assert!(!erc721.is_approved_for_all(accounts.alice, accounts.bob));
}
#[ink::test]
......@@ -601,7 +595,7 @@ mod erc721 {
assert_eq!(erc721.balance_of(accounts.eve), 0);
// Get contract address.
let callee = ink_env::account_id::<ink_env::DefaultEnvironment>()
.unwrap_or([0x0; 32].into());
.unwrap_or_else(|_| [0x0; 32].into());
// Create call
let mut data =
ink_env::test::CallData::new(ink_env::call::Selector::new([0x00; 4])); // balance_of
......@@ -672,7 +666,7 @@ mod erc721 {
fn set_sender(sender: AccountId) {
let callee = ink_env::account_id::<ink_env::DefaultEnvironment>()
.unwrap_or([0x0; 32].into());
.unwrap_or_else(|_| [0x0; 32].into());
test::push_execution_context::<Environment>(
sender,
callee,
......
......@@ -53,12 +53,23 @@ mod my_contract {
}
}
impl Default for MyContract {
fn default() -> Self {
Self::new()
}
}
#[cfg(not(feature = "ink-experimental-engine"))]
#[cfg(test)]
mod tests {
use super::*;
use ink_env::test::EmittedEvent;
use ink_lang as ink;
// The following test unfortunately has to be ignored until we make
// `ink-experimental-engine` the default with https://github.com/paritytech/ink/issues/565.
// See the issue for details.
#[ignore]
#[ink::test]
#[cfg(feature = "ink-experimental-engine")]
fn event_must_have_unique_topics() {
......@@ -81,7 +92,6 @@ mod my_contract {
}
#[ink::test]
#[cfg(not(feature = "ink-experimental-engine"))]
fn event_must_have_unique_topics() {
// given
let my_contract = MyContract::new();
......@@ -100,21 +110,21 @@ mod my_contract {
.collect();
assert!(!has_duplicates(&mut encoded_topics));
}
}
/// Finds duplicates in a given vector.
///
/// This function has complexity of `O(n * log n)` and no additional memory
/// is required, although the order of items is not preserved.
fn has_duplicates<T: PartialEq + AsRef<[u8]>>(items: &mut Vec<T>) -> bool {
// Sort the vector
items.sort_by(|a, b| Ord::cmp(a.as_ref(), b.as_ref()));
// And then find any two consecutive equal elements.
items.windows(2).any(|w| {
match w {
&[ref a, ref b] => a == b,
_ => false,
}
})
/// Finds duplicates in a given vector.
///
/// This function has complexity of `O(n * log n)` and no additional memory
/// is required, although the order of items is not preserved.
fn has_duplicates<T: PartialEq + AsRef<[u8]>>(items: &mut Vec<T>) -> bool {
// Sort the vector
items.sort_by(|a, b| Ord::cmp(a.as_ref(), b.as_ref()));
// And then find any two consecutive equal elements.
items.windows(2).any(|w| {
match w {
[ref a, ref b] => a == b,
_ => false,
}
})
}
}
}
......@@ -189,7 +189,7 @@ fn mixed_enum_layout(key_ptr: &mut KeyPtr) -> Layout {
vec![
(Discriminant(0), StructLayout::new(vec![])),
{
let mut variant_key_ptr = key_ptr.clone();
let mut variant_key_ptr = *key_ptr;
(
Discriminant(1),
StructLayout::new(vec![
......@@ -209,7 +209,7 @@ fn mixed_enum_layout(key_ptr: &mut KeyPtr) -> Layout {
)
},
{
let mut variant_key_ptr = key_ptr.clone();
let mut variant_key_ptr = *key_ptr;
(
Discriminant(2),
StructLayout::new(vec![
......
......@@ -43,7 +43,7 @@ fn bench_key_add_assign_u64(c: &mut Criterion) {
c.bench_function("Key2::add_assign(u64)", |b| {
b.iter(|| {
let mut copy = black_box(key);
let _ = black_box(copy += 1u64);
let _ = black_box(|| copy += 1u64);
})
});
}
......@@ -57,7 +57,7 @@ fn bench_key_add_assign_u64_one_ofvl(c: &mut Criterion) {
c.bench_function("Key2::add_assign(u64) - 1 ofvl", |b| {
b.iter(|| {
let mut copy = black_box(key);
let _ = black_box(copy += 1u64);
let _ = black_box(|| copy += 1u64);
})
});
}
......@@ -71,7 +71,7 @@ fn bench_key_add_assign_u64_two_ofvls(c: &mut Criterion) {
c.bench_function("Key2::add_assign(u64) - 2 ofvls", |b| {
b.iter(|| {
let mut copy = black_box(key);
let _ = black_box(copy += 1u64);
let _ = black_box(|| copy += 1u64);
})
});
}
......@@ -85,7 +85,7 @@ fn bench_key_add_assign_u64_three_ofvls(c: &mut Criterion) {
c.bench_function("Key2::add_assign(u64) - 3 ofvls", |b| {
b.iter(|| {
let mut copy = black_box(key);
let _ = black_box(copy += 1u64);
let _ = black_box(|| copy += 1u64);
})
});
}
......@@ -95,7 +95,7 @@ fn bench_key_add_assign_u64_wrap(c: &mut Criterion) {
c.bench_function("Key2::add_assign(u64) - wrap", |b| {
b.iter(|| {
let mut copy = black_box(key);
let _ = black_box(copy += 1u64);
let _ = black_box(|| copy += 1u64);
})
});
}
......@@ -104,7 +104,7 @@ fn bench_key_ptr_advance_by(c: &mut Criterion) {
let key = Key::from([0x00; 32]);
c.bench_function("KeyPtr2::advance_by copy", |b| {
b.iter(|| {
let mut key_ptr = KeyPtr::from(key.clone());
let mut key_ptr = KeyPtr::from(key);
let _ = black_box(key_ptr.advance_by(1));
})
});
......@@ -112,7 +112,7 @@ fn bench_key_ptr_advance_by(c: &mut Criterion) {
fn bench_key_ptr_advance_by_repeat(c: &mut Criterion) {
let key = Key::from([0x00; 32]);
let mut key_ptr = KeyPtr::from(key.clone());
let mut key_ptr = KeyPtr::from(key);
c.bench_function("KeyPtr2::advance_by reuse", |b| {
b.iter(|| {
let _ = black_box(key_ptr.advance_by(1));
......
......@@ -82,12 +82,10 @@ mod populated_cache {
fn bench_populated_cache(c: &mut Criterion) {
let mut group = c.benchmark_group("Bench: populated cache");
group.bench_function("fill_bitstash", |b| {
b.iter(|| populated_cache::fill_bitstash())
});
group.bench_function("fill_bitstash", |b| b.iter(populated_cache::fill_bitstash));
group.bench_function("one_put", |b| {
b.iter_batched_ref(
|| create_large_stash(),
create_large_stash,
|stash| one_put(stash),
BatchSize::SmallInput,
)
......@@ -114,8 +112,7 @@ mod empty_cache {
fn bench_empty_cache(c: &mut Criterion) {
let _ = ink_env::test::run_test::<ink_env::DefaultEnvironment, _>(|_| {
let mut group = c.benchmark_group("Bench: empty cache");
group
.bench_function("fill_bitstash", |b| b.iter(|| empty_cache::fill_bitstash()));
group.bench_function("fill_bitstash", |b| b.iter(empty_cache::fill_bitstash));
group.bench_function("one_put", |b| {
b.iter_batched_ref(
|| {
......
......@@ -131,14 +131,14 @@ macro_rules! gen_tests_for_backend {
);
group.bench_function("insert_and_inc", |b| {
b.iter_batched_ref(
|| setup_hashmap(),
setup_hashmap,
|hmap| insert_and_inc(hmap),
BatchSize::SmallInput,
)
});
group.bench_function("insert_and_inc_entry_api", |b| {
b.iter_batched_ref(
|| setup_hashmap(),
setup_hashmap,
|hmap| insert_and_inc_entry_api(hmap),
BatchSize::SmallInput,
)
......@@ -153,14 +153,14 @@ macro_rules! gen_tests_for_backend {
);
group.bench_function("remove", |b| {
b.iter_batched_ref(
|| setup_hashmap(),
setup_hashmap,
|hmap| remove(hmap),
BatchSize::SmallInput,
)
});
group.bench_function("remove_entry_api", |b| {
b.iter_batched_ref(
|| setup_hashmap(),
setup_hashmap,
|hmap| remove_entry_api(hmap),
BatchSize::SmallInput,
)
......
......@@ -39,23 +39,21 @@ mod populated_cache {
pub fn set() {
let mut lazy = <Lazy<i32>>::new(1);
let lazy_mut = black_box(&mut lazy);
black_box(Lazy::set(lazy_mut, 17));
let _ = black_box(|| Lazy::set(lazy_mut, 17));
}
pub fn deref_mut() {
let mut lazy = <Lazy<i32>>::new(1);
let i32_mut = black_box(lazy.deref_mut());
black_box(*i32_mut = 17);
let _ = black_box(|| *i32_mut = 17);
}
}
fn bench_set_populated_cache(c: &mut Criterion) {
let mut group = c.benchmark_group("Compare: `set` and `deref_mut` (populated cache)");
group.bench_function(BenchmarkId::new("set", 0), |b| {
b.iter(|| populated_cache::set())
});
group.bench_function(BenchmarkId::new("set", 0), |b| b.iter(populated_cache::set));
group.bench_function(BenchmarkId::new("deref_mut", 0), |b| {
b.iter(|| populated_cache::deref_mut())
b.iter(populated_cache::deref_mut)
});
group.finish();
}
......@@ -72,23 +70,21 @@ mod empty_cache {
pub fn set() {
let mut lazy = push_storage_lazy(1);
black_box(Lazy::set(&mut lazy, 13));
let _ = black_box(|| Lazy::set(&mut lazy, 13));
}
pub fn deref_mut() {
let mut lazy = push_storage_lazy(1);
black_box(*lazy = 13);
let _ = black_box(|| *lazy = 13);
}
}
fn bench_set_empty_cache(c: &mut Criterion) {
let _ = ink_env::test::run_test::<ink_env::DefaultEnvironment, _>(|_| {
let mut group = c.benchmark_group("Compare: `set` and `deref_mut` (empty cache)");
group.bench_function(BenchmarkId::new("set", 0), |b| {
b.iter(|| empty_cache::set())
});
group.bench_function(BenchmarkId::new("set", 0), |b| b.iter(empty_cache::set));
group.bench_function(BenchmarkId::new("deref_mut", 0), |b| {
b.iter(|| empty_cache::deref_mut())
b.iter(empty_cache::deref_mut)
});
group.finish();
Ok(())
......
......@@ -124,9 +124,9 @@ fn bench_remove_occupied_empty_cache(c: &mut Criterion) {
"Compare: `remove_occupied_all` and `take_all` (empty cache)",
);
group.bench_function("remove_occupied_all", |b| {
b.iter(|| empty_cache::remove_occupied_all())
b.iter(empty_cache::remove_occupied_all)
});
group.bench_function("take_all", |b| b.iter(|| empty_cache::take_all()));
group.bench_function("take_all", |b| b.iter(empty_cache::take_all));
group.finish();
Ok(())
})
......
......@@ -67,26 +67,26 @@ mod populated_cache {
use super::*;
pub fn clear(test_values: &[u8]) {
let mut vec = storage_vec_from_slice(&test_values);
black_box(vec.clear());
let mut vec = storage_vec_from_slice(test_values);
let _ = black_box(|| vec.clear());
}
pub fn pop_all(test_values: &[u8]) {
let mut vec = storage_vec_from_slice(&test_values);
let mut vec = storage_vec_from_slice(test_values);
while let Some(ignored) = black_box(vec.pop()) {
black_box(ignored);
}
}
pub fn set(test_values: &[u8]) {
let mut vec = storage_vec_from_slice(&test_values);
let mut vec = storage_vec_from_slice(test_values);
for (index, _value) in test_values.iter().enumerate() {
let _ = black_box(vec.set(index as u32, b'X'));
}
}
pub fn get_mut(test_values: &[u8]) {
let mut vec = storage_vec_from_slice(&test_values);
let mut vec = storage_vec_from_slice(test_values);
for (index, _value) in test_values.iter().enumerate() {
*black_box(vec.get_mut(index as u32).unwrap()) = b'X';
}
......@@ -124,7 +124,7 @@ mod empty_cache {
pub fn clear() {
push_storage_vec();
let mut vec = pull_storage_vec();
black_box(vec.clear());
let _ = black_box(|| vec.clear());
}
/// In this case we lazily load the vec from storage using `pull_spread`.
......@@ -167,8 +167,8 @@ mod empty_cache {
fn bench_clear_empty_cache(c: &mut Criterion) {
let _ = ink_env::test::run_test::<ink_env::DefaultEnvironment, _>(|_| {
let mut group = c.benchmark_group("Compare: `clear` and `pop_all` (empty cache)");
group.bench_function("clear", |b| b.iter(|| empty_cache::clear()));
group.bench_function("pop_all", |b| b.iter(|| empty_cache::pop_all()));
group.bench_function("clear", |b| b.iter(empty_cache::clear));
group.bench_function("pop_all", |b| b.iter(empty_cache::pop_all));
group.finish();
Ok(())
})
......@@ -181,8 +181,8 @@ fn bench_clear_empty_cache(c: &mut Criterion) {
fn bench_put_empty_cache(c: &mut Criterion) {
let _ = ink_env::test::run_test::<ink_env::DefaultEnvironment, _>(|_| {
let mut group = c.benchmark_group("Compare: `set` and `get_mut` (empty cache)");
group.bench_function("set", |b| b.iter(|| empty_cache::set()));
group.bench_function("get_mut", |b| b.iter(|| empty_cache::get_mut()));
group.bench_function("set", |b| b.iter(empty_cache::set));
group.bench_function("get_mut", |b| b.iter(empty_cache::get_mut));
group.finish();
Ok(())
})
......
......@@ -75,7 +75,7 @@ fn storage_layout_enum(s: &synstructure::Structure) -> TokenStream2 {
let field_layouts = field_layout(variant);
quote! {
{
let mut __variant_key_ptr = __key_ptr.clone();
let mut __variant_key_ptr = *__key_ptr;
let mut __key_ptr = &mut __variant_key_ptr;
(
::ink_metadata::layout::Discriminant::from(#discriminant),
......
......@@ -12,6 +12,14 @@
// See the License for the specific language governing permissions and
// limitations under the License.
// These tests are partly testing if code is expanded correctly.
// Hence the syntax contains a number of verbose statements which
// are not properly cleaned up.
#![allow(clippy::absurd_extreme_comparisons)]
#![allow(clippy::identity_op)]
#![allow(clippy::eq_op)]
#![allow(clippy::match_single_binding)]
use crate::spread_layout_derive;
#[test]
......
......@@ -120,7 +120,7 @@ fn clike_enum_works() {
::ink_metadata::layout::LayoutKey::from(dispatch_key),
vec![
{
let mut __variant_key_ptr = __key_ptr.clone();
let mut __variant_key_ptr = *__key_ptr;
let mut __key_ptr = &mut __variant_key_ptr;
(
::ink_metadata::layout::Discriminant::from(0usize),
......@@ -128,7 +128,7 @@ fn clike_enum_works() {
)
},
{
let mut __variant_key_ptr = __key_ptr.clone();
let mut __variant_key_ptr = *__key_ptr;
let mut __key_ptr = &mut __variant_key_ptr;
(
::ink_metadata::layout::Discriminant::from(1usize),
......@@ -136,7 +136,7 @@ fn clike_enum_works() {
)
},
{
let mut __variant_key_ptr = __key_ptr.clone();
let mut __variant_key_ptr = *__key_ptr;
let mut __key_ptr = &mut __variant_key_ptr;
(
::ink_metadata::layout::Discriminant::from(2usize),
......@@ -177,7 +177,7 @@ fn mixed_enum_works() {
::ink_metadata::layout::LayoutKey::from(dispatch_key),
vec![
{
let mut __variant_key_ptr = __key_ptr.clone();
let mut __variant_key_ptr = *__key_ptr;
let mut __key_ptr = &mut __variant_key_ptr;
(
::ink_metadata::layout::Discriminant::from(0usize),
......@@ -185,7 +185,7 @@ fn mixed_enum_works() {
)
},
{
let mut __variant_key_ptr = __key_ptr.clone();
let mut __variant_key_ptr = *__key_ptr;
let mut __key_ptr = &mut __variant_key_ptr;
(
::ink_metadata::layout::Discriminant::from(1usize),
......@@ -206,7 +206,7 @@ fn mixed_enum_works() {
)
},
{
let mut __variant_key_ptr = __key_ptr.clone();
let mut __variant_key_ptr = *__key_ptr;
let mut __key_ptr = &mut __variant_key_ptr;
(
::ink_metadata::layout::Discriminant::from(2usize),
......
......@@ -152,7 +152,7 @@ fn peek_works() {
fn peek_and_pop_works() {
let data = vec![2, 4, 6, 2, 1, 8, 10, 3, 5, 7, 0, 9, 1];
let mut sorted = data.clone();
sorted.sort();
sorted.sort_unstable();
let mut heap = heap_from_slice(&data);
while !heap.is_empty() {
assert_eq!(heap.peek().unwrap(), sorted.last().unwrap());
......@@ -192,7 +192,7 @@ fn min_heap_works() {
.map(|x| Reverse::new(*x))
.collect::<Vec<_>>();
let mut sorted = data.clone();