Fix breaking change: Do not enforce discriminant to be a literal + don't expect usize. (#695)

* fix

* tests

* fix ui test error

* more fix

* add test

* fmt

* fix typo

* wip

* fix

* remove dead code

* last fix

* remove convertion

* fix

* refactor

* fix conflicting implementation of len()

* license and doc

* trigger clippy again

* Revert "trigger clippy again"

This reverts commit 97246b8.

Author: gui1117

Cargo.lock

@@ -31,6 +31,7 @@ proptest = "1.6.0"
 trybuild = "1.0.103"
 paste = "1"
 rustversion = "1"
+enumflags2 = "0.7.11"
 
 [build-dependencies]
 rustversion = "1"

Cargo.toml

@@ -59,7 +59,7 @@ pub fn quote(
 
 				quote_spanned! { v.span() =>
 					#[allow(clippy::unnecessary_cast)]
-					__codec_x_edqy if __codec_x_edqy == #index as ::core::primitive::u8 => {
+					__codec_x_edqy if __codec_x_edqy == (#index) as ::core::primitive::u8 => {
 						// NOTE: This lambda is necessary to work around an upstream bug
 						// where each extra branch results in excessive stack usage:
 						//   https://github.com/rust-lang/rust/issues/34283

derive/src/decode.rs

@@ -333,7 +333,8 @@ fn impl_encode(data: &Data, type_name: &Ident, crate_path: &syn::Path) -> TokenS
 						let encoding_names = names.clone();
 						let encoding = quote_spanned! { f.span() =>
 							#type_name :: #name { #( ref #encoding_names, )* } => {
-								#dest.push_byte(#index as ::core::primitive::u8);
+								#[allow(clippy::unnecessary_cast)]
+								#dest.push_byte((#index) as ::core::primitive::u8);
 								#encode_fields
 							}
 						};
@@ -366,7 +367,8 @@ fn impl_encode(data: &Data, type_name: &Ident, crate_path: &syn::Path) -> TokenS
 						let encoding_names = names.clone();
 						let encoding = quote_spanned! { f.span() =>
 							#type_name :: #name ( #( ref #encoding_names, )* ) => {
-								#dest.push_byte(#index as ::core::primitive::u8);
+								#[allow(clippy::unnecessary_cast)]
+								#dest.push_byte((#index) as ::core::primitive::u8);
 								#encode_fields
 							}
 						};
@@ -383,7 +385,7 @@ fn impl_encode(data: &Data, type_name: &Ident, crate_path: &syn::Path) -> TokenS
 						let encoding = quote_spanned! { f.span() =>
 							#type_name :: #name => {
 								#[allow(clippy::unnecessary_cast)]
-								#dest.push_byte(#index as ::core::primitive::u8);
+								#dest.push_byte((#index) as ::core::primitive::u8);
 							}
 						};
 

derive/src/encode.rs

@@ -45,7 +45,13 @@ pub fn const_eval_check_variant_indexes(
 	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) });
+		// We convert to u8 same as in the generated code.
+		recurse_indices.push(quote_spanned! { ident.span() =>
+			(
+				(#index) as ::core::primitive::u8,
+				#ident_str
+			)
+		});
 	}
 	let len = recurse_indices.len();
 
@@ -55,11 +61,12 @@ pub fn const_eval_check_variant_indexes(
 
 	quote! {
 		const _: () = {
-			const indices: [(usize, &'static str); #len] = [#( #recurse_indices ,)*];
+			#[allow(clippy::unnecessary_cast)]
+			const indices: [(u8, &'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();
+			const fn duplicate_info(array: &[(u8, &'static str); #len]) -> (bool, usize, usize) {
+				let len = #len;
 				let mut i = 0;
 				while i < len {
 						let mut j = i + 1;
@@ -97,7 +104,7 @@ pub fn const_eval_check_variant_indexes(
 /// is found, fall back to the discriminant or just the variant index.
 pub fn variant_index(v: &Variant, i: usize) -> TokenStream {
 	// first look for an attribute
-	let mut index = find_meta_item(v.attrs.iter(), |meta| {
+	let index = find_meta_item(v.attrs.iter(), |meta| {
 		if let Meta::NameValue(ref nv) = meta {
 			if nv.path.is_ident("index") {
 				if let Expr::Lit(ExprLit { lit: Lit::Int(ref v), .. }) = nv.value {
@@ -112,21 +119,13 @@ pub fn variant_index(v: &Variant, i: usize) -> TokenStream {
 		None
 	});
 
-	// if no attribute, we fall back to an explicit discriminant (ie 'enum A { Foo = 1u32 }').
-	if index.is_none() {
-		if let Some((_, Expr::Lit(ExprLit { lit: Lit::Int(disc_lit), .. }))) = &v.discriminant {
-			index = disc_lit.base10_parse::<usize>().ok()
-		}
-	}
-
-	// fall back to the variant index if no attribute or explicit discriminant is found.
-	let index = index.unwrap_or(i);
-
-	// output the index with no suffix (ie 1 rather than 1usize) so that these can be quoted into
-	// an array of usizes and will work regardless of what type the discriminant values actually
-	// are.
-	let s = proc_macro2::Literal::usize_unsuffixed(index);
-	quote! { #s }
+	// then fallback to discriminant or just index
+	index.map(|i| quote! { #i }).unwrap_or_else(|| {
+		v.discriminant
+			.as_ref()
+			.map(|(_, expr)| quote! { #expr })
+			.unwrap_or_else(|| quote! { #i })
+	})
 }
 
 /// Look for a `#[codec(encoded_as = "SomeType")]` outer attribute on the given
@@ -394,12 +393,6 @@ pub fn check_attributes(input: &DeriveInput) -> syn::Result<()> {
 						check_field_attribute(attr)?;
 					}
 				}
-				// While we're checking things, also ensure that
-				// any explicit discriminants are within 0..=255
-				let discriminant = variant.discriminant.as_ref().map(|(_, d)| d);
-				if let Some(expr) = discriminant {
-					check_variant_discriminant(expr)?;
-				}
 			},
 		Data::Union(_) => (),
 	}
@@ -479,21 +472,6 @@ fn check_variant_attribute(attr: &Attribute) -> syn::Result<()> {
 	}
 }
 
-// Ensure a variant discriminant, if provided, can be parsed into
-// something in the range 0..255.
-fn check_variant_discriminant(discriminant: &Expr) -> syn::Result<()> {
-	if let Expr::Lit(ExprLit { lit: Lit::Int(lit_int), .. }) = discriminant {
-		lit_int.base10_parse::<u8>().map(|_| ()).map_err(|_| {
-			syn::Error::new(lit_int.span(), "Discriminant index must be in the range 0..255")
-		})
-	} else {
-		Err(syn::Error::new(
-			discriminant.span(),
-			"Discriminant must be an integer literal in the range 0..255",
-		))
-	}
-}
-
 // Only `#[codec(dumb_trait_bound)]` is accepted as top attribute
 fn check_top_attribute(attr: &Attribute) -> syn::Result<()> {
 	let top_error = "Invalid attribute: only `#[codec(dumb_trait_bound)]`, \

derive/src/utils.rs

@@ -0,0 +1,26 @@
+// Copyright 2017, 2018 Parity Technologies
+//
+// 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.
+
+use parity_scale_codec_derive::{Decode, Encode};
+
+// test for regression
+#[enumflags2::bitflags]
+#[repr(u64)]
+#[derive(Copy, Clone, Encode, Decode)]
+pub enum EnumWithU64ReprAndBitflags {
+	Variant1,
+	Variant2,
+	Variant3,
+	Variant4,
+}

tests/bitflags.rs

@@ -931,3 +931,13 @@ fn deserializing_of_big_recursively_nested_enum_works() {
 	let obj_d2 = Enum::decode_with_depth_limit(40, &mut &data[..]).unwrap();
 	assert!(obj == obj_d2);
 }
+
+#[test]
+fn non_literal_variant_discriminant() {
+	const A: isize = 1;
+	#[derive(PartialEq, Eq, DeriveDecode, DeriveDecodeWithMemTracking, DeriveEncode)]
+	enum Enum {
+		A = A,
+		B = A + 1,
+	}
+}