decl_runtime_apis! - check that a method without changed_in exists (#5635)

* `decl_runtime_apis!` - check that a method without `changed_in` exists

This adds another check to the macro that ensures that not only methods
with `changed_in` exists, but there are also the default methods exist.

* Update primitives/api/proc-macro/src/decl_runtime_apis.rs

Co-Authored-By: Nikolay Volf <nikvolf@gmail.com>

* Fix test

Co-authored-by: Nikolay Volf <nikvolf@gmail.com>

substrate/primitives/api/proc-macro/src/decl_runtime_apis.rs

@@ -876,6 +876,53 @@ struct CheckTraitDecl {
 	errors: Vec<Error>,
 }
 
+impl CheckTraitDecl {
+	/// Check the given trait.
+	///
+	/// All errors will be collected in `self.errors`.
+	fn check(&mut self, trait_: &ItemTrait) {
+		self.check_method_declarations(trait_.items.iter().filter_map(|i| match i {
+			TraitItem::Method(method) => Some(method),
+			_ => None,
+		}));
+
+		visit::visit_item_trait(self, trait_);
+	}
+
+	/// Check that the given method declarations are correct.
+	///
+	/// Any error is stored in `self.errors`.
+	fn check_method_declarations<'a>(&mut self, methods: impl Iterator<Item = &'a TraitItemMethod>) {
+		let mut method_to_signature_changed = HashMap::<Ident, Vec<Option<u64>>>::new();
+
+		methods.into_iter().for_each(|method| {
+			let attributes = remove_supported_attributes(&mut method.attrs.clone());
+
+			let changed_in = match get_changed_in(&attributes) {
+				Ok(r) => r,
+				Err(e) => { self.errors.push(e); return; },
+			};
+
+			method_to_signature_changed
+				.entry(method.sig.ident.clone())
+				.or_default()
+				.push(changed_in);
+		});
+
+		method_to_signature_changed.into_iter().for_each(|(f, changed)| {
+			// If `changed_in` is `None`, it means it is the current "default" method that calls
+			// into the latest implementation.
+			if changed.iter().filter(|c| c.is_none()).count() == 0 {
+				self.errors.push(Error::new(
+					f.span(),
+					"There is no 'default' method with this name (without `changed_in` attribute).\n\
+					 The 'default' method is used to call into the latest implementation.",
+				));
+			}
+		});
+	}
+}
+
 impl<'ast> Visit<'ast> for CheckTraitDecl {
 	fn visit_fn_arg(&mut self, input: &'ast FnArg) {
 		if let FnArg::Receiver(_) = input {
@@ -923,7 +970,7 @@ impl<'ast> Visit<'ast> for CheckTraitDecl {
 /// Check that the trait declarations are in the format we expect.
 fn check_trait_decls(decls: &[ItemTrait]) -> Result<()> {
 	let mut checker = CheckTraitDecl { errors: Vec::new() };
-	decls.iter().for_each(|decl| visit::visit_item_trait(&mut checker, &decl));
+	decls.iter().for_each(|decl| checker.check(decl));
 
 	if let Some(err) = checker.errors.pop() {
 		Err(checker.errors.into_iter().fold(err, |mut err, other| {

substrate/primitives/api/src/lib.rs

@@ -113,7 +113,9 @@ use std::{panic::UnwindSafe, cell::RefCell};
 /// change is highlighted with the `#[changed_in(2)]` attribute above a method. A method that is
 /// tagged with this attribute is callable by the name `METHOD_before_version_VERSION`. This
 /// method will only support calling into wasm, trying to call into native will fail (change the
-/// spec version!). Such a method also does not need to be implemented in the runtime.
+/// spec version!). Such a method also does not need to be implemented in the runtime. It is
+/// required that there exist the "default" of the method without the `#[changed_in(_)]` attribute,
+/// this method will be used to call the current default implementation.
 ///
 /// ```rust
 /// sp_api::decl_runtime_apis! {

substrate/primitives/api/test/tests/ui/changed_in_no_default_method.rs

@@ -0,0 +1,19 @@
+use sp_runtime::traits::GetNodeBlockType;
+use substrate_test_runtime_client::runtime::Block;
+
+/// The declaration of the `Runtime` type and the implementation of the `GetNodeBlockType`
+/// trait are done by the `construct_runtime!` macro in a real runtime.
+struct Runtime {}
+impl GetNodeBlockType for Runtime {
+	type NodeBlock = Block;
+}
+
+sp_api::decl_runtime_apis! {
+	#[api_version(2)]
+	pub trait Api {
+		#[changed_in(2)]
+		fn test(data: u64);
+	}
+}
+
+fn main() {}

substrate/primitives/api/test/tests/ui/changed_in_no_default_method.stderr

@@ -0,0 +1,6 @@
+error: There is no 'default' method with this name (without `changed_in` attribute).
+The 'default' method is used to call into the latest implementation.
+  --> $DIR/changed_in_no_default_method.rs:15:6
+   |
+15 |         fn test(data: u64);
+   |            ^^^^