Disallow duplicate indexes using constant evaluation (#653)

* init

* Improve error message (#681)

* improve error message

* Apply suggestions from code review

Co-authored-by: Bastian Köcher <[email protected]>

* finally understanding rust const environment

* fmt

---------

Co-authored-by: Bastian Köcher <[email protected]>

---------

Co-authored-by: Guillaume Thiolliere <[email protected]>
Co-authored-by: Bastian Köcher <[email protected]>

Author: 3 people

Cargo.lock

@@ -20,6 +20,7 @@ byte-slice-cast = { version = "1.2.2", default-features = false }
 generic-array = { version = "0.14.7", optional = true }
 arbitrary = { version = "1.4.1", features = ["derive"], optional = true }
 impl-trait-for-tuples = "0.2.3"
+const_format = { version = "0.2.34" }
 
 [dev-dependencies]
 criterion = "0.4.0"

Cargo.toml

@@ -70,12 +70,20 @@ pub fn quote(
 					},
 				}
 			});
+			let recurse_indices = variants
+				.iter()
+				.enumerate()
+				.map(|(i, v)| (v.ident.clone(), utils::variant_index(v, i)));
+
+			let const_eval_check =
+				utils::const_eval_check_variant_indexes(recurse_indices, crate_path);
 
 			let read_byte_err_msg =
 				format!("Could not decode `{type_name}`, failed to read variant byte");
 			let invalid_variant_err_msg =
 				format!("Could not decode `{type_name}`, variant doesn't exist");
 			quote! {
+				#const_eval_check
 				match #input.read_byte()
 					.map_err(|e| e.chain(#read_byte_err_msg))?
 				{

derive/src/decode.rs

@@ -17,7 +17,7 @@ use std::str::from_utf8;
 use proc_macro2::{Ident, Span, TokenStream};
 use syn::{punctuated::Punctuated, spanned::Spanned, token::Comma, Data, Error, Field, Fields};
 
-use crate::utils;
+use crate::utils::{self, const_eval_check_variant_indexes};
 
 type FieldsList = Punctuated<Field, Comma>;
 
@@ -338,7 +338,7 @@ fn impl_encode(data: &Data, type_name: &Ident, crate_path: &syn::Path) -> TokenS
 							}
 						};
 
-						[hinting, encoding]
+						(hinting, encoding, index, name.clone())
 					},
 					Fields::Unnamed(ref fields) => {
 						let fields = &fields.unnamed;
@@ -371,7 +371,7 @@ fn impl_encode(data: &Data, type_name: &Ident, crate_path: &syn::Path) -> TokenS
 							}
 						};
 
-						[hinting, encoding]
+						(hinting, encoding, index, name.clone())
 					},
 					Fields::Unit => {
 						let hinting = quote_spanned! { f.span() =>
@@ -387,13 +387,14 @@ fn impl_encode(data: &Data, type_name: &Ident, crate_path: &syn::Path) -> TokenS
 							}
 						};
 
-						[hinting, encoding]
+						(hinting, encoding, index, name.clone())
 					},
 				}
 			});
 
-			let recurse_hinting = recurse.clone().map(|[hinting, _]| hinting);
-			let recurse_encoding = recurse.clone().map(|[_, encoding]| encoding);
+			let recurse_hinting = recurse.clone().map(|(hinting, _, _, _)| hinting);
+			let recurse_encoding = recurse.clone().map(|(_, encoding, _, _)| encoding);
+			let recurse_variant_indices = recurse.clone().map(|(_, _, index, name)| (name, index));
 
 			let hinting = quote! {
 				// The variant index uses 1 byte.
@@ -403,7 +404,11 @@ fn impl_encode(data: &Data, type_name: &Ident, crate_path: &syn::Path) -> TokenS
 				}
 			};
 
+			let const_eval_check =
+				const_eval_check_variant_indexes(recurse_variant_indices, crate_path);
+
 			let encoding = quote! {
+				#const_eval_check
 				match *#self_ {
 					#( #recurse_encoding )*,
 					_ => (),

derive/src/encode.rs

@@ -38,6 +38,61 @@ where
 	})
 }
 
+pub fn const_eval_check_variant_indexes(
+	recurse_variant_indices: impl Iterator<Item = (syn::Ident, TokenStream)>,
+	crate_path: &syn::Path,
+) -> TokenStream {
+	let mut recurse_indices = vec![];
+	for (ident, index) in recurse_variant_indices {
+		let ident_str = ident.to_string();
+		recurse_indices.push(quote! { (#index, #ident_str) });
+	}
+	let len = recurse_indices.len();
+
+	if len == 0 {
+		return quote! {};
+	}
+
+	quote! {
+		const _: () = {
+			const indices: [(usize, &'static str); #len] = [#( #recurse_indices ,)*];
+
+			// Returns if there is duplicate, and if there is some the duplicate indexes.
+			const fn duplicate_info(array: &[(usize, &'static str); #len]) -> (bool, usize, usize) {
+				let len = array.len();
+				let mut i = 0;
+				while i < len {
+						let mut j = i + 1;
+						while j < len {
+								if array[i].0 == array[j].0 {
+									return (true, i, j);
+								}
+								j += 1;
+						}
+						i += 1;
+				}
+				(false, 0, 0)
+			}
+
+			const DUP_INFO: (bool, usize, usize) = duplicate_info(&indices);
+
+			if DUP_INFO.0 {
+				let msg = #crate_path::__private::concatcp!(
+					"Found variants that have duplicate indexes. Both `",
+					indices[DUP_INFO.1].1,
+					"` and `",
+					indices[DUP_INFO.2].1,
+					"` have the index `",
+					indices[DUP_INFO.1].0,
+					"`. Use different indexes for each variant."
+				);
+
+				::core::panic!("{}", msg);
+			}
+		};
+	}
+}
+
 /// Look for a `#[scale(index = $int)]` attribute on a variant. If no attribute
 /// is found, fall back to the discriminant or just the variant index.
 pub fn variant_index(v: &Variant, i: usize) -> TokenStream {
@@ -47,7 +102,7 @@ pub fn variant_index(v: &Variant, i: usize) -> TokenStream {
 			if nv.path.is_ident("index") {
 				if let Expr::Lit(ExprLit { lit: Lit::Int(ref v), .. }) = nv.value {
 					let byte = v
-						.base10_parse::<u8>()
+						.base10_parse::<usize>()
 						.expect("Internal error, index attribute must have been checked");
 					return Some(byte);
 				}

derive/src/utils.rs

@@ -41,6 +41,14 @@ pub mod alloc {
 	pub use std::{alloc, borrow, boxed, collections, rc, string, sync, vec};
 }
 
+/// Private module to reexport items used by derive macros.
+// We don't feature gate this module with `derive` to avoid compilation error when
+// `parity-scale-codec-derive` is used on its own and this crate doesn't have the feature enabled.
+#[doc(hidden)]
+pub mod __private {
+	pub use const_format::concatcp;
+}
+
 #[cfg(feature = "bit-vec")]
 mod bit_vec;
 mod btree_utils;

src/lib.rs

@@ -0,0 +1,16 @@
+#[derive(::parity_scale_codec::Decode, ::parity_scale_codec::Encode)]
+#[codec(crate = ::parity_scale_codec)]
+enum T {
+	A = 1,
+	B,
+}
+
+#[derive(::parity_scale_codec::Decode, ::parity_scale_codec::Encode)]
+#[codec(crate = ::parity_scale_codec)]
+enum T2 {
+	#[codec(index = 1)]
+	A,
+	B,
+}
+
+fn main() {}

tests/scale_codec_ui/codec_discriminiant_variant_counted_in_default_index.rs

@@ -0,0 +1,31 @@
+error[E0080]: evaluation of constant value failed
+ --> tests/scale_codec_ui/codec_discriminiant_variant_counted_in_default_index.rs:1:10
+  |
+1 | #[derive(::parity_scale_codec::Decode, ::parity_scale_codec::Encode)]
+  |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the evaluated program panicked at 'Found variants that have duplicate indexes. Both `A` and `B` have the index `1`. Use different indexes for each variant.', $DIR/tests/scale_codec_ui/codec_discriminiant_variant_counted_in_default_index.rs:1:10
+  |
+  = note: this error originates in the macro `$crate::panic::panic_2021` which comes from the expansion of the macro `::core::panic` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error[E0080]: evaluation of constant value failed
+ --> tests/scale_codec_ui/codec_discriminiant_variant_counted_in_default_index.rs:1:40
+  |
+1 | #[derive(::parity_scale_codec::Decode, ::parity_scale_codec::Encode)]
+  |                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the evaluated program panicked at 'Found variants that have duplicate indexes. Both `A` and `B` have the index `1`. Use different indexes for each variant.', $DIR/tests/scale_codec_ui/codec_discriminiant_variant_counted_in_default_index.rs:1:40
+  |
+  = note: this error originates in the macro `$crate::panic::panic_2021` which comes from the expansion of the macro `::core::panic` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error[E0080]: evaluation of constant value failed
+ --> tests/scale_codec_ui/codec_discriminiant_variant_counted_in_default_index.rs:8:10
+  |
+8 | #[derive(::parity_scale_codec::Decode, ::parity_scale_codec::Encode)]
+  |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the evaluated program panicked at 'Found variants that have duplicate indexes. Both `A` and `B` have the index `1`. Use different indexes for each variant.', $DIR/tests/scale_codec_ui/codec_discriminiant_variant_counted_in_default_index.rs:8:10
+  |
+  = note: this error originates in the macro `$crate::panic::panic_2021` which comes from the expansion of the macro `::core::panic` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error[E0080]: evaluation of constant value failed
+ --> tests/scale_codec_ui/codec_discriminiant_variant_counted_in_default_index.rs:8:40
+  |
+8 | #[derive(::parity_scale_codec::Decode, ::parity_scale_codec::Encode)]
+  |                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the evaluated program panicked at 'Found variants that have duplicate indexes. Both `A` and `B` have the index `1`. Use different indexes for each variant.', $DIR/tests/scale_codec_ui/codec_discriminiant_variant_counted_in_default_index.rs:8:40
+  |
+  = note: this error originates in the macro `$crate::panic::panic_2021` which comes from the expansion of the macro `::core::panic` (in Nightly builds, run with -Z macro-backtrace for more info)

tests/scale_codec_ui/codec_discriminiant_variant_counted_in_default_index.stderr

@@ -0,0 +1,17 @@
+#[derive(::parity_scale_codec::Decode, ::parity_scale_codec::Encode)]
+#[codec(crate = ::parity_scale_codec)]
+enum T {
+	A = 3,
+	#[codec(index = 3)]
+	B,
+}
+
+#[derive(::parity_scale_codec::Decode, ::parity_scale_codec::Encode)]
+#[codec(crate = ::parity_scale_codec)]
+enum T1 {
+	A,
+	#[codec(index = 0)]
+	B,
+}
+
+fn main() {}

tests/scale_codec_ui/codec_duplicate_index.rs

@@ -0,0 +1,31 @@
+error[E0080]: evaluation of constant value failed
+ --> tests/scale_codec_ui/codec_duplicate_index.rs:1:10
+  |
+1 | #[derive(::parity_scale_codec::Decode, ::parity_scale_codec::Encode)]
+  |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the evaluated program panicked at 'Found variants that have duplicate indexes. Both `A` and `B` have the index `3`. Use different indexes for each variant.', $DIR/tests/scale_codec_ui/codec_duplicate_index.rs:1:10
+  |
+  = note: this error originates in the macro `$crate::panic::panic_2021` which comes from the expansion of the macro `::core::panic` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error[E0080]: evaluation of constant value failed
+ --> tests/scale_codec_ui/codec_duplicate_index.rs:1:40
+  |
+1 | #[derive(::parity_scale_codec::Decode, ::parity_scale_codec::Encode)]
+  |                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the evaluated program panicked at 'Found variants that have duplicate indexes. Both `A` and `B` have the index `3`. Use different indexes for each variant.', $DIR/tests/scale_codec_ui/codec_duplicate_index.rs:1:40
+  |
+  = note: this error originates in the macro `$crate::panic::panic_2021` which comes from the expansion of the macro `::core::panic` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error[E0080]: evaluation of constant value failed
+ --> tests/scale_codec_ui/codec_duplicate_index.rs:9:10
+  |
+9 | #[derive(::parity_scale_codec::Decode, ::parity_scale_codec::Encode)]
+  |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the evaluated program panicked at 'Found variants that have duplicate indexes. Both `A` and `B` have the index `0`. Use different indexes for each variant.', $DIR/tests/scale_codec_ui/codec_duplicate_index.rs:9:10
+  |
+  = note: this error originates in the macro `$crate::panic::panic_2021` which comes from the expansion of the macro `::core::panic` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error[E0080]: evaluation of constant value failed
+ --> tests/scale_codec_ui/codec_duplicate_index.rs:9:40
+  |
+9 | #[derive(::parity_scale_codec::Decode, ::parity_scale_codec::Encode)]
+  |                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the evaluated program panicked at 'Found variants that have duplicate indexes. Both `A` and `B` have the index `0`. Use different indexes for each variant.', $DIR/tests/scale_codec_ui/codec_duplicate_index.rs:9:40
+  |
+  = note: this error originates in the macro `$crate::panic::panic_2021` which comes from the expansion of the macro `::core::panic` (in Nightly builds, run with -Z macro-backtrace for more info)

tests/scale_codec_ui/codec_duplicate_index.stderr

@@ -1,18 +1,6 @@
 use parity_scale_codec::Encode;
 use parity_scale_codec_derive::Encode as DeriveEncode;
 
-#[test]
-fn discriminant_variant_counted_in_default_index() {
-	#[derive(DeriveEncode)]
-	enum T {
-		A = 1,
-		B,
-	}
-
-	assert_eq!(T::A.encode(), vec![1]);
-	assert_eq!(T::B.encode(), vec![1]);
-}
-
 #[test]
 fn skipped_variant_not_counted_in_default_index() {
 	#[derive(DeriveEncode)]
@@ -25,16 +13,3 @@ fn skipped_variant_not_counted_in_default_index() {
 	assert_eq!(T::A.encode(), vec![]);
 	assert_eq!(T::B.encode(), vec![0]);
 }
-
-#[test]
-fn index_attr_variant_counted_and_reused_in_default_index() {
-	#[derive(DeriveEncode)]
-	enum T {
-		#[codec(index = 1)]
-		A,
-		B,
-	}
-
-	assert_eq!(T::A.encode(), vec![1]);
-	assert_eq!(T::B.encode(), vec![1]);
-}