ronantakizawa/codereview-bench huggingface dataset:
Repository: paritytech/substrate
File: client/service/test/src/client/mod.rs (Rust)

Code before:
	})
	.unwrap();
	let b1 = b1.build().unwrap().block;

	let check_block_a1 = BlockCheckParams {
		hash: a1.hash(),
		number: 0,
		parent_hash: *a1.header().parent_hash(),
		allow_missing_state: false,
		allow_missing_parent: false,
		import_existing: false,
	};

	assert_eq!(
		block_on(client.check_block(check_block_a1.clone())).unwrap(),
		ImportResult::imported(false),
	);
	assert_eq!(client.block_status(check_block_a1.hash).unwrap(), BlockStatus::Unknown,);

	block_on(client.import_as_final(BlockOrigin::Own, a1.clone())).unwrap();

	assert_eq!(
		block_on(client.check_block(check_block_a1.clone())).unwrap(),
		ImportResult::AlreadyInChain,
	);
	assert_eq!(client.block_status(check_block_a1.hash).unwrap(), BlockStatus::InChainWithState,);

	let a2 = client
		.new_block_at(&BlockId::Hash(a1.hash()), Default::default(), false)
		.unwrap()
		.build()
		.unwrap()
		.block;
	block_on(client.import_as_final(BlockOrigin::Own, a2.clone())).unwrap();

	let check_block_a2 = BlockCheckParams {
		hash: a2.hash(),
		number: 1,
		parent_hash: *a1.header().parent_hash(),
		allow_missing_state: false,
		allow_missing_parent: false,
		import_existing: false,
	};

	assert_eq!(
		block_on(client.check_block(check_block_a1.clone())).unwrap(),
		ImportResult::AlreadyInChain,
	);
	assert_eq!(client.block_status(check_block_a1.hash).unwrap(), BlockStatus::InChainPruned,);
	assert_eq!(
		block_on(client.check_block(check_block_a2.clone())).unwrap(),

Review comment (suggestion):
```suggestion
	assert_eq!(client.block_status(check_block_a1.hash).unwrap(), BlockStatus::InChainWithState);
```

Code after:
	})
	.unwrap();
	let b1 = b1.build().unwrap().block;

	let check_block_a1 = BlockCheckParams {
		hash: a1.hash(),
		number: 0,
		parent_hash: *a1.header().parent_hash(),
		allow_missing_state: false,
		allow_missing_parent: false,
		import_existing: false,
	};

	assert_eq!(
		block_on(client.check_block(check_block_a1.clone())).unwrap(),
		ImportResult::imported(false),
	);
	assert_eq!(client.block_status(check_block_a1.hash).unwrap(), BlockStatus::Unknown);

	block_on(client.import_as_final(BlockOrigin::Own, a1.clone())).unwrap();

	assert_eq!(
		block_on(client.check_block(check_block_a1.clone())).unwrap(),
		ImportResult::AlreadyInChain,
	);
	assert_eq!(client.block_status(check_block_a1.hash).unwrap(), BlockStatus::InChainWithState);

	let a2 = client
		.new_block_at(&BlockId::Hash(a1.hash()), Default::default(), false)
		.unwrap()
		.build()
		.unwrap()
		.block;
	block_on(client.import_as_final(BlockOrigin::Own, a2.clone())).unwrap();

	let check_block_a2 = BlockCheckParams {
		hash: a2.hash(),
		number: 1,
		parent_hash: *a1.header().parent_hash(),
		allow_missing_state: false,
		allow_missing_parent: false,
		import_existing: false,
	};

	assert_eq!(
		block_on(client.check_block(check_block_a1.clone())).unwrap(),
		ImportResult::AlreadyInChain,
	);
	assert_eq!(client.block_status(check_block_a1.hash).unwrap(), BlockStatus::InChainPruned);
	assert_eq!(
		block_on(client.check_block(check_block_a2.clone())).unwrap(),
Repository: paritytech/substrate
File: client/service/test/src/client/mod.rs (Rust)

Code before:
		ImportResult::AlreadyInChain,
	);
	assert_eq!(client.block_status(check_block_a1.hash).unwrap(), BlockStatus::InChainWithState,);

	let a2 = client
		.new_block_at(&BlockId::Hash(a1.hash()), Default::default(), false)
		.unwrap()
		.build()
		.unwrap()
		.block;
	block_on(client.import_as_final(BlockOrigin::Own, a2.clone())).unwrap();

	let check_block_a2 = BlockCheckParams {
		hash: a2.hash(),
		number: 1,
		parent_hash: *a1.header().parent_hash(),
		allow_missing_state: false,
		allow_missing_parent: false,
		import_existing: false,
	};

	assert_eq!(
		block_on(client.check_block(check_block_a1.clone())).unwrap(),
		ImportResult::AlreadyInChain,
	);
	assert_eq!(client.block_status(check_block_a1.hash).unwrap(), BlockStatus::InChainPruned,);
	assert_eq!(
		block_on(client.check_block(check_block_a2.clone())).unwrap(),
		ImportResult::AlreadyInChain,
	);
	assert_eq!(client.block_status(check_block_a2.hash).unwrap(), BlockStatus::InChainWithState,);

	let a3 = client
		.new_block_at(&BlockId::Hash(a2.hash()), Default::default(), false)
		.unwrap()
		.build()
		.unwrap()
		.block;

	block_on(client.import_as_final(BlockOrigin::Own, a3.clone())).unwrap();
	let check_block_a3 = BlockCheckParams {
		hash: a3.hash(),
		number: 2,
		parent_hash: *a2.header().parent_hash(),
		allow_missing_state: false,
		allow_missing_parent: false,
		import_existing: false,
	};

	// a1 and a2 are both pruned at this point
	assert_eq!(

Review comment (suggestion):
```suggestion
	assert_eq!(client.block_status(check_block_a1.hash).unwrap(), BlockStatus::InChainPruned);
```

Code after:
		ImportResult::AlreadyInChain,
	);
	assert_eq!(client.block_status(check_block_a1.hash).unwrap(), BlockStatus::InChainWithState);

	let a2 = client
		.new_block_at(&BlockId::Hash(a1.hash()), Default::default(), false)
		.unwrap()
		.build()
		.unwrap()
		.block;
	block_on(client.import_as_final(BlockOrigin::Own, a2.clone())).unwrap();

	let check_block_a2 = BlockCheckParams {
		hash: a2.hash(),
		number: 1,
		parent_hash: *a1.header().parent_hash(),
		allow_missing_state: false,
		allow_missing_parent: false,
		import_existing: false,
	};

	assert_eq!(
		block_on(client.check_block(check_block_a1.clone())).unwrap(),
		ImportResult::AlreadyInChain,
	);
	assert_eq!(client.block_status(check_block_a1.hash).unwrap(), BlockStatus::InChainPruned);
	assert_eq!(
		block_on(client.check_block(check_block_a2.clone())).unwrap(),
		ImportResult::AlreadyInChain,
	);
	assert_eq!(client.block_status(check_block_a2.hash).unwrap(), BlockStatus::InChainWithState);

	let a3 = client
		.new_block_at(&BlockId::Hash(a2.hash()), Default::default(), false)
		.unwrap()
		.build()
		.unwrap()
		.block;

	block_on(client.import_as_final(BlockOrigin::Own, a3.clone())).unwrap();
	let check_block_a3 = BlockCheckParams {
		hash: a3.hash(),
		number: 2,
		parent_hash: *a2.header().parent_hash(),
		allow_missing_state: false,
		allow_missing_parent: false,
		import_existing: false,
	};

	// a1 and a2 are both pruned at this point
	assert_eq!(
Repository: paritytech/substrate
File: frame/assets/src/functions.rs (Rust)

Code before:
		keep_alive: bool,
	) -> WithdrawConsequence<T::Balance> {
		use WithdrawConsequence::*;
		let details = match Asset::<T, I>::get(id) {
			Some(details) => details,
			None => return UnknownAsset,
		};
		if details.supply.checked_sub(&amount).is_none() {
			return Underflow
		}
		if details.status == AssetStatus::Frozen {
			return Frozen
		}
		if amount.is_zero() {
			return Success
		}
		let account = match Account::<T, I>::get(id, who) {
			Some(a) => a,
			None => return BalanceLow,
		};
		if account.is_frozen {
			return Frozen
		}
		if let Some(rest) = account.balance.checked_sub(&amount) {
			if let Some(frozen) = T::Freezer::frozen_balance(id, who) {
				match frozen.checked_add(&details.min_balance) {
					Some(required) if rest < required => return Frozen,
					None => return Overflow,
					_ => {},
				}
			}

			let is_provider = false;
			let is_required = is_provider && !frame_system::Pallet::<T>::can_dec_provider(who);
			let must_keep_alive = keep_alive || is_required;

			if rest < details.min_balance {
				if must_keep_alive {
					WouldDie
				} else {
					ReducedToZero(rest)
				}
			} else {
				Success
			}
		} else {
			BalanceLow
		}
	}

	// Maximum `amount` that can be passed into `can_withdraw` to result in a `WithdrawConsequence`

Review comment (suggestion):
`let is_provider = details.is_sufficient;` should be the correct solution.

You are not allowed to let the account die when there is a consumer and you need to remove the provider.

Code after:
		if details.supply.checked_sub(&amount).is_none() {
			return Underflow
		}
		if details.status == AssetStatus::Frozen {
			return Frozen
		}
		if amount.is_zero() {
			return Success
		}
		let account = match Account::<T, I>::get(id, who) {
			Some(a) => a,
			None => return NoFunds,
		};
		if account.is_frozen {
			return Frozen
		}
		if let Some(rest) = account.balance.checked_sub(&amount) {
			if let Some(frozen) = T::Freezer::frozen_balance(id, who) {
				match frozen.checked_add(&details.min_balance) {
					Some(required) if rest < required => return Frozen,
					None => return Overflow,
					_ => {},
				}
			}

			if rest < details.min_balance {
				if keep_alive {
					WouldDie
				} else {
					ReducedToZero(rest)
				}
			} else {
				Success
			}
		} else {
			NoFunds
		}
	}

	// Maximum `amount` that can be passed into `can_withdraw` to result in a `WithdrawConsequence`
	// of `Success`.
	pub(super) fn reducible_balance(
		id: T::AssetId,
		who: &T::AccountId,
		keep_alive: bool,
	) -> Result<T::Balance, DispatchError> {
		let details = Asset::<T, I>::get(id).ok_or(Error::<T, I>::Unknown)?;
		ensure!(details.status == AssetStatus::Live, Error::<T, I>::AssetNotLive);

		let account = Account::<T, I>::get(id, who).ok_or(Error::<T, I>::NoAccount)?;
		ensure!(!account.is_frozen, Error::<T, I>::Frozen);
Repository: paritytech/substrate
File: primitives/npos-elections/src/approval_voting.rs (Rust)

Code before:
// This file is part of Substrate.

// Copyright (C) 2020-2023 Parity Technologies (UK) Ltd.
// SPDX-License-Identifier: Apache-2.0

// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// 	http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

//! Implementation of the approval voting election method.
//!
//! This method allows voters to select many candidates and backing each of them with the same
//! vote weight. The candidates with the most backing are the election winners.

use crate::{setup_inputs, ElectionResult, IdentifierT, PerThing128, VoteWeight};
use sp_arithmetic::{
	helpers_128bit::multiply_by_rational_with_rounding,
	traits::{Bounded, Zero},
	Rounding,
};

Review comment (suggestion):
```suggestion
// Copyright (C) 2023 Parity Technologies (UK) Ltd.
```

Code after:
// This file is part of Substrate.

// Copyright (C) 2023 Parity Technologies (UK) Ltd.
// SPDX-License-Identifier: Apache-2.0

// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// 	http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

//! Implementation of the approval voting election method.
//!
//! This method allows voters to select many candidates and backing each of them with the same
//! vote weight. The candidates with the most backing are the election winners.

use crate::{setup_inputs, ElectionResult, IdentifierT, PerThing128, VoteWeight};
use sp_arithmetic::traits::Zero;
use sp_std::{cmp::Reverse, vec::Vec};

/// Execute an approvals voting election scheme. The return type is a list of winners and a weight
/// distribution vector of all voters who contribute to the winners.
Repository: paritytech/substrate
File: frame/support/test/tests/pallet.rs (Rust)

Code before:
	}

	#[pallet::pallet]
	#[pallet::generate_store(pub(crate) trait Store)]
	#[pallet::storage_version(STORAGE_VERSION)]
	#[pallet::without_storage_info]
	pub struct Pallet<T>(_);

	#[pallet::hooks]
	impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T>
	where
		T::AccountId: From<SomeType1> + SomeAssociation1,
	{
		fn on_initialize(_: BlockNumberFor<T>) -> Weight {
			Self::deposit_event(Event::Something(11));
			Weight::zero()
		}
		fn on_finalize(_: BlockNumberFor<T>) {
			Self::deposit_event(Event::Something(21));
		}
		fn on_runtime_upgrade() -> Weight {
			Self::deposit_event(Event::Something(31));

			if dbg!(UpdateStorageVersion::get()) {
				dbg!(Self::current_storage_version()).put::<Self>();
			}

			Weight::zero()
		}
	}

	#[pallet::call]
	impl<T: Config> Pallet<T> where T::AccountId: From<SomeType1> + SomeAssociation1 {}

	#[pallet::storage]
	pub type SomeValue<T: Config> = StorageValue<_, Vec<u32>>;

	#[pallet::storage]
	pub type SomeCountedStorageMap<T> =
		CountedStorageMap<Hasher = Twox64Concat, Key = u8, Value = u32>;

	#[pallet::event]
	#[pallet::generate_deposit(fn deposit_event)]
	pub enum Event {
		/// Something
		Something(u32),
	}

	#[pallet::genesis_config]
	pub struct GenesisConfig<T: Config>
	where

Review comment (suggestion):
```suggestion
			if UpdateStorageVersion::get() {
				Self::current_storage_version().put::<Self>();
			}
```

Code after:
	}

	impl frame_support::inherent::IsFatalError for InherentError {
		fn is_fatal_error(&self) -> bool {
			matches!(self, InherentError::Fatal)
		}
	}

	pub const INHERENT_IDENTIFIER: InherentIdentifier = *b"testpall";
}

// Test that a pallet with non generic event and generic genesis_config is correctly handled
// and that a pallet with the attribute without_storage_info is correctly handled.
#[frame_support::pallet]
pub mod pallet2 {
	use super::{SomeAssociation1, SomeType1, UpdateStorageVersion};
	use frame_support::pallet_prelude::*;
	use frame_system::pallet_prelude::*;

	pub(crate) const STORAGE_VERSION: StorageVersion = StorageVersion::new(2);

	#[pallet::config]
	pub trait Config: frame_system::Config
	where
		<Self as frame_system::Config>::AccountId: From<SomeType1> + SomeAssociation1,
	{
		type RuntimeEvent: From<Event> + IsType<<Self as frame_system::Config>::RuntimeEvent>;
	}

	#[pallet::pallet]
	#[pallet::storage_version(STORAGE_VERSION)]
	#[pallet::without_storage_info]
	pub struct Pallet<T>(_);

	#[pallet::hooks]
	impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T>
	where
		T::AccountId: From<SomeType1> + SomeAssociation1,
	{
		fn on_initialize(_: BlockNumberFor<T>) -> Weight {
			Self::deposit_event(Event::Something(11));
			Weight::zero()
		}
		fn on_finalize(_: BlockNumberFor<T>) {
			Self::deposit_event(Event::Something(21));
		}
		fn on_runtime_upgrade() -> Weight {
			Self::deposit_event(Event::Something(31));

			if UpdateStorageVersion::get() {
				Self::current_storage_version().put::<Self>();
Repository: paritytech/substrate
File: frame/support/procedural/src/pallet/expand/documentation.rs (Rust)

Code before:
	///
	/// The string literal represents the file `PATH`.
	///
	/// `#[doc = include_str!(PATH)]`
	Path(Lit),
}

impl Parse for DocMetaValue {
	fn parse(input: ParseStream) -> syn::Result<Self> {
		let _token: Token![=] = input.parse()?;

		if input.peek(Lit) {
			return input.parse().map(DocMetaValue::Lit)
		}

		let ident: Ident = input.parse()?;
		if ident != INCLUDE_STR {
			return Err(input.error("expected include_str ident"))
		}
		let _token: Token![!] = input.parse()?;

		// We must have a path literal inside `(...)`
		let content;
		parenthesized!(content in input);
		content.parse().map(DocMetaValue::Path)
	}
}

impl ToTokens for DocMetaValue {
	fn to_tokens(&self, tokens: &mut TokenStream) {
		match self {
			DocMetaValue::Lit(lit) => lit.to_tokens(tokens),
			DocMetaValue::Path(path_lit) => {
				let decl = quote::quote!(include_str!(#path_lit));
				tokens.extend(decl)
			},
		}
	}
}

/// Extract the documentation from the given pallet definition
/// to include in the runtime metadata.
///
/// Implement a `pallet_documentation_metadata` function to fetch the
/// documentation that is included in the metadata.
///
/// The documentation is placed at the top of the module similar to:
///
/// ```ignore
/// #[pallet]
/// /// Documentation for pallet

Review comment (bug):
btw you could use `derive_syn_parse` to do this very compactly. It has facilities for parsing enums and peeking, not that there is anything wrong with deriving `Parse` manually

Code after:
}

/// Parse the include_str attribute.
#[derive(Debug, Parse)]
struct IncludeDocParser {
	_include_str: keywords::include_str,
	_eq_token: syn::token::Bang,
	#[paren]
	_paren: syn::token::Paren,
	#[inside(_paren)]
	lit: Lit,
}

/// Parse the doc literal.
#[derive(Debug, Parse)]
struct DocParser {
	_eq_token: syn::token::Eq,
	#[peek(Lit)]
	lit: Option<Lit>,
	#[parse_if(lit.is_none())]
	include_doc: Option<IncludeDocParser>,
}

/// Supported documentation tokens.
#[derive(Debug)]
enum DocMetaValue {
	/// Documentation with string literals.
	///
	/// `#[doc = "Lit"]`
	Lit(Lit),
	/// Documentation with `include_str!` macro.
	///
	/// The string literal represents the file `PATH`.
	///
	/// `#[doc = include_str!(PATH)]`
	Path(Lit),
}

impl ToTokens for DocMetaValue {
	fn to_tokens(&self, tokens: &mut TokenStream) {
		match self {
			DocMetaValue::Lit(lit) => lit.to_tokens(tokens),
			DocMetaValue::Path(path_lit) => {
				let decl = quote::quote!(include_str!(#path_lit));
				tokens.extend(decl)
			},
		}
	}
}

/// Extract the documentation from the given pallet definition
Repository: paritytech/substrate
File: client/network/src/service/out_events.rs (Rust)

Code before:
		Ok(Self { event_streams: Vec::new(), metrics: Arc::new(metrics) })
	}

	/// Adds a new [`Sender`] to the collection.
	pub fn push(&mut self, sender: Sender) {
		let mut metrics = sender.metrics.lock();
		debug_assert!(metrics.is_none());
		*metrics = Some(self.metrics.clone());
		drop(metrics);

		if let Some(metrics) = &*self.metrics {
			metrics.num_channels.with_label_values(&[sender.name]).inc();
		}

		self.event_streams.push(sender);
	}

	/// Sends an event.
	pub fn send(&mut self, event: Event) {
		self.event_streams.retain_mut(|sender| {
			if sender.inner.len() == sender.queue_size_warning && !sender.warning_fired {
				sender.warning_fired = true;
				sender.creation_backtrace.resolve();
				error!(
					"The number of unprocessed events in channel `{}` exceeded {}.\n\
					 The channel was created at:\n{:?}",
					sender.name, sender.queue_size_warning, sender.creation_backtrace,
				);
			}
			sender.inner.try_send(event.clone()).is_ok()
		});

		if let Some(metrics) = &*self.metrics {
			for ev in &self.event_streams {
				metrics.event_in(&event, ev.name);
			}
		}
	}
}

impl fmt::Debug for OutChannels {
	fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
		f.debug_struct("OutChannels")
			.field("num_channels", &self.event_streams.len())
			.finish()
	}
}

struct Metrics {
	// This list is ordered alphabetically
	events_total: CounterVec<U64>,

Review comment (suggestion):
Hm.... shouldn't the backtrace be printed out with `{}` instead of `{:?}`?

Maybe also print out the backtrace from which this send is being made? I can imagine that could also be useful in some cases. It's a one time warning that should only trigger in exceptional cases, so I think we can afford it.

Code after:
		Ok(Self { event_streams: Vec::new(), metrics: Arc::new(metrics) })
	}

	/// Adds a new [`Sender`] to the collection.
	pub fn push(&mut self, sender: Sender) {
		let mut metrics = sender.metrics.lock();
		debug_assert!(metrics.is_none());
		*metrics = Some(self.metrics.clone());
		drop(metrics);

		if let Some(metrics) = &*self.metrics {
			metrics.num_channels.with_label_values(&[sender.name]).inc();
		}

		self.event_streams.push(sender);
	}

	/// Sends an event.
	pub fn send(&mut self, event: Event) {
		self.event_streams.retain_mut(|sender| {
			if sender.inner.len() >= sender.queue_size_warning && !sender.warning_fired {
				sender.warning_fired = true;
				error!(
					"The number of unprocessed events in channel `{}` exceeded {}.\n\
					 The channel was created at:\n{:}\n
					 The last event was sent from:\n{:}",
					sender.name,
					sender.queue_size_warning,
					sender.creation_backtrace,
					Backtrace::force_capture(),
				);
			}
			sender.inner.try_send(event.clone()).is_ok()
		});

		if let Some(metrics) = &*self.metrics {
			for ev in &self.event_streams {
				metrics.event_in(&event, ev.name);
			}
		}
	}
}

impl fmt::Debug for OutChannels {
	fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
		f.debug_struct("OutChannels")
			.field("num_channels", &self.event_streams.len())
			.finish()
	}
}
Repository: paritytech/substrate
File: frame/support/test/tests/pallet_ui/deprecated_store_attr.rs (Rust)

Code before:
#[frame_support::pallet]
mod pallet {
	use frame_support::pallet_prelude::Hooks;
	use frame_system::pallet_prelude::BlockNumberFor;
	use frame_support::pallet_prelude::StorageValue;

	#[pallet::config]
	pub trait Config: frame_system::Config {}

	#[pallet::pallet]
	#[pallet::generate_store(trait Store)]
	pub struct Pallet<T>(core::marker::PhantomData<T>);

	#[pallet::hooks]
	impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {}

	#[pallet::call]
	impl<T: Config> Pallet<T> {}

	#[pallet::storage]
	type Foo<T> = StorageValue<_, u8>;
}

fn main() {}


Review comment (suggestion):
```suggestion
```

None of the pallet parts highlighted here are mandatory.

Code after:
#[frame_support::pallet]
mod pallet {
	#[pallet::config]
	pub trait Config: frame_system::Config {}

	#[pallet::pallet]
	#[pallet::generate_store(trait Store)]
	pub struct Pallet<T>(core::marker::PhantomData<T>);
}

fn main() {}