decl_storage! check for duplicate config()/get() (#3936)

* `decl_storage!` check for duplicate `config()`/`get()`

* Fix tests

substrate/srml/support/procedural/src/storage/parse.rs

@@ -17,7 +17,7 @@
 //! Parsing of decl_storage input.
 
 use srml_support_procedural_tools::{ToTokens, Parse, syn_ext as ext};
-use syn::{Ident, Token};
+use syn::{Ident, Token, spanned::Spanned};
 
 mod keyword {
 	syn::custom_keyword!(hiddencrate);
@@ -265,7 +265,6 @@ fn get_module_instance(
 
 pub fn parse(input: syn::parse::ParseStream) -> syn::Result<super::DeclStorageDef> {
 	use syn::parse::Parse;
-	use syn::spanned::Spanned;
 
 	let def = StorageDefinition::parse(input)?;
 
@@ -303,9 +302,31 @@ pub fn parse(input: syn::parse::ParseStream) -> syn::Result<super::DeclStorageDe
 		}
 	}
 
-	let mut storage_lines = vec![];
+	let storage_lines = parse_storage_line_defs(def.content.content.inner.into_iter())?;
 
-	for line in def.content.content.inner.into_iter() {
+	Ok(super::DeclStorageDef {
+		hidden_crate: def.hidden_crate.inner.map(|i| i.ident.content),
+		visibility: def.visibility,
+		module_name: def.module_ident,
+		store_trait: def.ident,
+		module_runtime_generic: def.mod_param_generic,
+		module_runtime_trait: def.mod_param_bound,
+		where_clause: def.where_clause,
+		crate_name: def.crate_ident,
+		module_instance,
+		extra_genesis_build,
+		extra_genesis_config_lines,
+		storage_lines,
+	})
+}
+
+/// Parse the `DeclStorageLine` into `StorageLineDef`.
+fn parse_storage_line_defs(
+	defs: impl Iterator<Item = DeclStorageLine>,
+) -> syn::Result<Vec<super::StorageLineDef>> {
+	let mut storage_lines = Vec::<super::StorageLineDef>::new();
+
+	for line in defs {
 		let getter = line.getter.inner.map(|o| o.getfn.content.ident);
 		let config = if let Some(config) = line.config.inner {
 			if let Some(ident) = config.expr.content {
@@ -315,14 +336,28 @@ pub fn parse(input: syn::parse::ParseStream) -> syn::Result<super::DeclStorageDe
 			} else {
 				return Err(syn::Error::new(
 					config.span(),
-					"Invalid storage definiton, couldn't find config identifier: storage must either have a get \
-					identifier `get(fn ident)` or a defined config identifier `config(ident)`"
+					"Invalid storage definition, couldn't find config identifier: storage must \
+					either have a get identifier `get(fn ident)` or a defined config identifier \
+					`config(ident)`",
 				))
 			}
 		} else {
 			None
 		};
 
+		if let Some(ref config) = config {
+			storage_lines.iter().filter_map(|sl| sl.config.as_ref()).try_for_each(|other_config| {
+				if other_config == config {
+					Err(syn::Error::new(
+						config.span(),
+						"`config()`/`get()` with the same name already defined.",
+					))
+				} else {
+					Ok(())
+				}
+			})?;
+		}
+
 		let storage_type = match line.storage_type {
 			DeclStorageType::Map(map) => super::StorageLineTypeDef::Map(
 				super::MapDef {
@@ -365,18 +400,5 @@ pub fn parse(input: syn::parse::ParseStream) -> syn::Result<super::DeclStorageDe
 		})
 	}
 
-	Ok(super::DeclStorageDef {
-		hidden_crate: def.hidden_crate.inner.map(|i| i.ident.content),
-		visibility: def.visibility,
-		module_name: def.module_ident,
-		store_trait: def.ident,
-		module_runtime_generic: def.mod_param_generic,
-		module_runtime_trait: def.mod_param_bound,
-		where_clause: def.where_clause,
-		crate_name: def.crate_ident,
-		module_instance,
-		extra_genesis_build,
-		extra_genesis_config_lines,
-		storage_lines,
-	})
+	Ok(storage_lines)
 }

substrate/srml/support/test/Cargo.toml

@@ -12,7 +12,7 @@ support = { package = "srml-support", version = "2", path = "../", default-featu
 inherents = { package = "substrate-inherents", path = "../../../core/inherents", default-features = false }
 sr-primitives = { package = "sr-primitives", path = "../../../core/sr-primitives", default-features = false }
 primitives = { package = "substrate-primitives", path = "../../../core/primitives", default-features = false }
-trybuild = "1.0.14"
+trybuild = "1.0.17"
 pretty_assertions = "0.6.1"
 
 [features]

substrate/srml/support/test/src/lib.rs

@@ -16,9 +16,3 @@
 
 //! Test crate for srml_support. Allow to make use of `support::decl_storage`.
 //! See tests directory.
-
-#[test]
-fn reserved_keyword() {
-    let t = trybuild::TestCases::new();
-    t.compile_fail("tests/reserved_keyword/*.rs");
-}

substrate/srml/support/test/tests/decl_storage_ui.rs

@@ -0,0 +1,24 @@
+// Copyright 2019 Parity Technologies (UK) Ltd.
+// This file is part of Substrate.
+
+// Substrate is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+
+// Substrate is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License
+// along with Substrate.  If not, see <http://www.gnu.org/licenses/>.
+
+#[test]
+fn decl_storage_ui() {
+	// As trybuild is using `cargo check`, we don't need the real WASM binaries.
+	std::env::set_var("BUILD_DUMMY_WASM_BINARY", "1");
+
+	let t = trybuild::TestCases::new();
+	t.compile_fail("tests/decl_storage_ui/*.rs");
+}

substrate/srml/support/test/tests/decl_storage_ui/config_duplicate.rs

@@ -0,0 +1,33 @@
+// Copyright 2019 Parity Technologies (UK) Ltd.
+// This file is part of Substrate.
+
+// Substrate is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+
+// Substrate is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License
+// along with Substrate.  If not, see <http://www.gnu.org/licenses/>.
+
+pub trait Trait {
+	type Origin;
+	type BlockNumber: codec::Codec + codec::EncodeLike + Default + Clone;
+}
+
+support::decl_module! {
+	pub struct Module<T: Trait> for enum Call where origin: T::Origin {}
+}
+
+support::decl_storage!{
+	trait Store for Module<T: Trait> as FinalKeysNone {
+		pub Value config(value): u32;
+		pub Value2 config(value): u32;
+	}
+}
+
+fn main() {}

substrate/srml/support/test/tests/decl_storage_ui/config_duplicate.stderr

@@ -0,0 +1,5 @@
+error: `config()`/`get()` with the same name already defined.
+  --> $DIR/config_duplicate.rs:29:21
+   |
+29 |         pub Value2 config(value): u32;
+   |                           ^^^^^

substrate/srml/support/test/tests/decl_storage_ui/config_get_duplicate.rs

@@ -0,0 +1,33 @@
+// Copyright 2019 Parity Technologies (UK) Ltd.
+// This file is part of Substrate.
+
+// Substrate is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+
+// Substrate is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License
+// along with Substrate.  If not, see <http://www.gnu.org/licenses/>.
+
+pub trait Trait {
+	type Origin;
+	type BlockNumber: codec::Codec + codec::EncodeLike + Default + Clone;
+}
+
+support::decl_module! {
+	pub struct Module<T: Trait> for enum Call where origin: T::Origin {}
+}
+
+support::decl_storage!{
+	trait Store for Module<T: Trait> as FinalKeysNone {
+		pub Value get(fn value) config(): u32;
+		pub Value2 config(value): u32;
+	}
+}
+
+fn main() {}

substrate/srml/support/test/tests/decl_storage_ui/config_get_duplicate.stderr

@@ -0,0 +1,5 @@
+error: `config()`/`get()` with the same name already defined.
+  --> $DIR/config_get_duplicate.rs:29:21
+   |
+29 |         pub Value2 config(value): u32;
+   |                           ^^^^^

substrate/srml/support/test/tests/decl_storage_ui/get_duplicate.rs

@@ -0,0 +1,33 @@
+// Copyright 2019 Parity Technologies (UK) Ltd.
+// This file is part of Substrate.
+
+// Substrate is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+
+// Substrate is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License
+// along with Substrate.  If not, see <http://www.gnu.org/licenses/>.
+
+pub trait Trait {
+	type Origin;
+	type BlockNumber: codec::Codec + codec::EncodeLike + Default + Clone;
+}
+
+support::decl_module! {
+	pub struct Module<T: Trait> for enum Call where origin: T::Origin {}
+}
+
+support::decl_storage!{
+	trait Store for Module<T: Trait> as FinalKeysNone {
+		pub Value get(fn value) config(): u32;
+		pub Value2 get(fn value) config(): u32;
+	}
+}
+
+fn main() {}

substrate/srml/support/test/tests/decl_storage_ui/get_duplicate.stderr

@@ -0,0 +1,5 @@
+error: `config()`/`get()` with the same name already defined.
+  --> $DIR/get_duplicate.rs:29:21
+   |
+29 |         pub Value2 get(fn value) config(): u32;
+   |                           ^^^^^

substrate/srml/support/test/tests/instance.rs

@@ -13,6 +13,7 @@
 
 // You should have received a copy of the GNU General Public License
 // along with Substrate.  If not, see <http://www.gnu.org/licenses/>.
+
 #![recursion_limit="128"]
 
 use sr_primitives::{generic, BuildStorage, traits::{BlakeTwo256, Block as _, Verify}};

substrate/srml/support/test/tests/reserved_keyword.rs

@@ -0,0 +1,24 @@
+// Copyright 2019 Parity Technologies (UK) Ltd.
+// This file is part of Substrate.
+
+// Substrate is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+
+// Substrate is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License
+// along with Substrate.  If not, see <http://www.gnu.org/licenses/>.
+
+#[test]
+fn reserved_keyword() {
+	// As trybuild is using `cargo check`, we don't need the real WASM binaries.
+	std::env::set_var("BUILD_DUMMY_WASM_BINARY", "1");
+
+	let t = trybuild::TestCases::new();
+	t.compile_fail("tests/reserved_keyword/*.rs");
+}