Issue checking duplicate variant indexes when using #[bitflags]

[serban300]

Related to #691 and #690

This will lead to a compilation error:

#[bitflags]
#[repr(u64)]
#[derive(Copy, Clone, DeriveEncode, DeriveDecode)]
pub enum EnumWithU64Repr {
	/// This item is transferable.
	Transferable,
	/// The metadata of this item can be modified.
	UnlockedMetadata,
	/// Attributes of this item can be modified.
	UnlockedAttributes,
}

That's because #[bitflags] does:

pub enum EnumWithU64Repr {
    /// This item is transferable.
    Transferable = 1,
    /// The metadata of this item can be modified.
    UnlockedMetadata =
        ::enumflags2::_internal::next_bit(EnumWithU64Repr::Transferable as u128) as u64,
    /// Attributes of this item can be modified.
    UnlockedAttributes = ::enumflags2::_internal::next_bit(
        EnumWithU64Repr::Transferable as u128 | EnumWithU64Repr::UnlockedMetadata as u128,
    ) as u64,
}

And this will not work well with the duplicate variant indexes checking logic introduced in #653 and #687

Example of polkadot-sdk structure using #[bitflags]: ItemSettings

[gui1117]

Maybe the error is correct though, encoding EnumWithU64Repr doesn't seem to make sense if multiple variant have the same index

[gui1117]

But it is true that there is also a breaking change that was introduced forcing the discriminant to be a literal.

Here can be a fix: #695

[serban300]

Maybe the error is correct though, encoding EnumWithU64Repr doesn't seem to make sense if multiple variant have the same index

But they shouldn't have the same index. From what I understand the indexes would be:

b1
b10
b100
b1000

For example:

#[bitflags]
#[repr(u64)]
#[derive(Copy, Clone)]
pub enum EnumWithU64Repr {
	Variant1,
	Variant2,
	Variant3,
	Variant4,
}

#[test]
fn print() {
	println!("{:?}", EnumWithU64Repr::Variant1 as usize);
	println!("{:?}", EnumWithU64Repr::Variant2 as usize);
	println!("{:?}", EnumWithU64Repr::Variant3 as usize);
	println!("{:?}", EnumWithU64Repr::Variant4 as usize);
}

This code outputs:

1
2
4
8

So the duplicate indexes check is wrong in this case

[serban300]

Yes, and the old implementation (v3.6.12 branch) was expanding to:

        fn encode_to<__CodecOutputEdqy: ::parity_scale_codec::Output + ?::core::marker::Sized>(
            &self,
            __codec_dest_edqy: &mut __CodecOutputEdqy,
        ) {
            match *self {
                EnumWithU64Repr::Variant1 => {
                    #[allow(clippy::unnecessary_cast)]
                    __codec_dest_edqy.push_byte(1 as ::core::primitive::u8);
                }
                EnumWithU64Repr::Variant2 => {
                    #[allow(clippy::unnecessary_cast)]
                    __codec_dest_edqy
                        .push_byte(::enumflags2::_internal::next_bit(
                            EnumWithU64Repr::Variant1 as u128,
                        ) as u64 as ::core::primitive::u8);
                }
                EnumWithU64Repr::Variant3 => {
                    #[allow(clippy::unnecessary_cast)]
                    __codec_dest_edqy
                        .push_byte(::enumflags2::_internal::next_bit(
                            EnumWithU64Repr::Variant1 as u128 | EnumWithU64Repr::Variant2 as u128,
                        ) as u64 as ::core::primitive::u8);
                }
                EnumWithU64Repr::Variant4 => {
                    #[allow(clippy::unnecessary_cast)]
                    __codec_dest_edqy.push_byte(::enumflags2::_internal::next_bit(
                        EnumWithU64Repr::Variant1 as u128
                            | EnumWithU64Repr::Variant2 as u128
                            | EnumWithU64Repr::Variant3 as u128,
                    ) as u64
                        as ::core::primitive::u8);
                }
                _ => (),
            }
        }
    }

While the new one (master) expands to:

        fn encode_to<__CodecOutputEdqy: ::parity_scale_codec::Output + ?::core::marker::Sized>(
            &self,
            __codec_dest_edqy: &mut __CodecOutputEdqy,
        ) {
            const _: () = {
                const indices: [(usize, &'static str); 4usize] = [
                    (1, "Variant1"),
                    (1, "Variant2"),
                    (2, "Variant3"),
                    (3, "Variant4"),
                ];
                const fn discriminant_overflow_u8(
                    array: &[(usize, &'static str); 4usize],
                ) -> (bool, usize) {
                    let len = array.len();
                    let mut i = 0;
                    while i < len {
                        if array[i].0 > 255 {
                            return (true, i);
                        }
                        i += 1;
                    }
                    (false, 0)
                }
                const OVERFLOW: (bool, usize) = discriminant_overflow_u8(&indices);
                if OVERFLOW.0 {
                    let msg = ::const_format::pmr::__AssertStr {
                        x: {
                            use ::const_format::__cf_osRcTFl4A;
                            ({
                                #[doc(hidden)]
                                #[allow(unused_mut, non_snake_case)]
                                const CONCATP_NHPMWYD3NJA: &[__cf_osRcTFl4A::pmr::PArgument] = {
                                    let fmt = __cf_osRcTFl4A::pmr::FormattingFlags::NEW;
                                    &[
                                        __cf_osRcTFl4A::pmr::PConvWrapper(
                                            "Discriminant must be in the range 0..255, variant `",
                                        )
                                        .to_pargument_display(fmt),
                                        __cf_osRcTFl4A::pmr::PConvWrapper(indices[OVERFLOW.1].1)
                                            .to_pargument_display(fmt),
                                        __cf_osRcTFl4A::pmr::PConvWrapper("` index is `")
                                            .to_pargument_display(fmt),
                                        __cf_osRcTFl4A::pmr::PConvWrapper(indices[OVERFLOW.1].0)
                                            .to_pargument_display(fmt),
                                        __cf_osRcTFl4A::pmr::PConvWrapper("`.")
                                            .to_pargument_display(fmt),
                                    ]
                                };
                                {
                                    #[doc(hidden)]
                                    const ARR_LEN: usize = ::const_format::pmr::PArgument::calc_len(
                                        CONCATP_NHPMWYD3NJA,
                                    );
                                    #[doc(hidden)]
                                    const CONCAT_ARR: &::const_format::pmr::LenAndArray<
                                        [u8; ARR_LEN],
                                    > = &::const_format::pmr::__priv_concatenate(
                                        CONCATP_NHPMWYD3NJA,
                                    );
                                    #[doc(hidden)]
                                    #[allow(clippy::transmute_ptr_to_ptr)]
                                    const CONCAT_STR: &str = unsafe {
                                        let slice = ::const_format::pmr::transmute::<
                                            &[u8; ARR_LEN],
                                            &[u8; CONCAT_ARR.len],
                                        >(
                                            &CONCAT_ARR.array
                                        );
                                        {
                                            let bytes: &'static [::const_format::pmr::u8] = slice;
                                            let string: &'static ::const_format::pmr::str = {
                                                ::const_format::__hidden_utils::PtrToRef {
                                                    ptr: bytes as *const [::const_format::pmr::u8]
                                                        as *const str,
                                                }
                                                .reff
                                            };
                                            string
                                        }
                                    };
                                    CONCAT_STR
                                }
                            })
                        },
                    }
                    .x;
                    {
                        #[cold]
                        #[track_caller]
                        #[inline(never)]
                        #[rustc_const_panic_str]
                        #[rustc_do_not_const_check]
                        const fn panic_cold_display<T: ::core::fmt::Display>(arg: &T) -> ! {
                            ::core::panicking::panic_display(arg)
                        }
                        panic_cold_display(&msg);
                    };
                }
                const fn duplicate_info(
                    array: &[(usize, &'static str); 4usize],
                ) -> (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 = ::const_format::pmr::__AssertStr {
                        x: {
                            use ::const_format::__cf_osRcTFl4A;
                            ({
                                #[doc(hidden)]
                                #[allow(unused_mut, non_snake_case)]
                                const CONCATP_NHPMWYD3NJA: &[__cf_osRcTFl4A::pmr::PArgument] = {
                                    let fmt = __cf_osRcTFl4A::pmr::FormattingFlags::NEW;
                                    &[
                                        __cf_osRcTFl4A::pmr::PConvWrapper(
                                            "Found variants that have duplicate indexes. Both `",
                                        )
                                        .to_pargument_display(fmt),
                                        __cf_osRcTFl4A::pmr::PConvWrapper(indices[DUP_INFO.1].1)
                                            .to_pargument_display(fmt),
                                        __cf_osRcTFl4A::pmr::PConvWrapper("` and `")
                                            .to_pargument_display(fmt),
                                        __cf_osRcTFl4A::pmr::PConvWrapper(indices[DUP_INFO.2].1)
                                            .to_pargument_display(fmt),
                                        __cf_osRcTFl4A::pmr::PConvWrapper("` have the index `")
                                            .to_pargument_display(fmt),
                                        __cf_osRcTFl4A::pmr::PConvWrapper(indices[DUP_INFO.1].0)
                                            .to_pargument_display(fmt),
                                        __cf_osRcTFl4A::pmr::PConvWrapper(
                                            "`. Use different indexes for each variant.",
                                        )
                                        .to_pargument_display(fmt),
                                    ]
                                };
                                {
                                    #[doc(hidden)]
                                    const ARR_LEN: usize = ::const_format::pmr::PArgument::calc_len(
                                        CONCATP_NHPMWYD3NJA,
                                    );
                                    #[doc(hidden)]
                                    const CONCAT_ARR: &::const_format::pmr::LenAndArray<
                                        [u8; ARR_LEN],
                                    > = &::const_format::pmr::__priv_concatenate(
                                        CONCATP_NHPMWYD3NJA,
                                    );
                                    #[doc(hidden)]
                                    #[allow(clippy::transmute_ptr_to_ptr)]
                                    const CONCAT_STR: &str = unsafe {
                                        let slice = ::const_format::pmr::transmute::<
                                            &[u8; ARR_LEN],
                                            &[u8; CONCAT_ARR.len],
                                        >(
                                            &CONCAT_ARR.array
                                        );
                                        {
                                            let bytes: &'static [::const_format::pmr::u8] = slice;
                                            let string: &'static ::const_format::pmr::str = {
                                                ::const_format::__hidden_utils::PtrToRef {
                                                    ptr: bytes as *const [::const_format::pmr::u8]
                                                        as *const str,
                                                }
                                                .reff
                                            };
                                            string
                                        }
                                    };
                                    CONCAT_STR
                                }
                            })
                        },
                    }
                    .x;
                    {
                        #[cold]
                        #[track_caller]
                        #[inline(never)]
                        #[rustc_const_panic_str]
                        #[rustc_do_not_const_check]
                        const fn panic_cold_display<T: ::core::fmt::Display>(arg: &T) -> ! {
                            ::core::panicking::panic_display(arg)
                        }
                        panic_cold_display(&msg);
                    };
                }
            };
            match *self {
                EnumWithU64Repr::Variant1 => {
                    #[allow(clippy::unnecessary_cast)]
                    __codec_dest_edqy.push_byte(1 as ::core::primitive::u8);
                }
                EnumWithU64Repr::Variant2 => {
                    #[allow(clippy::unnecessary_cast)]
                    __codec_dest_edqy.push_byte(1 as ::core::primitive::u8);
                }
                EnumWithU64Repr::Variant3 => {
                    #[allow(clippy::unnecessary_cast)]
                    __codec_dest_edqy.push_byte(2 as ::core::primitive::u8);
                }
                EnumWithU64Repr::Variant4 => {
                    #[allow(clippy::unnecessary_cast)]
                    __codec_dest_edqy.push_byte(3 as ::core::primitive::u8);
                }
                _ => (),
            }
        }

which is wrong

[gui1117]

I see, the discriminant is ignored when it is an expression: https://github.com/paritytech/parity-scale-codec/pull/687/files#r1937043283

[gui1117]

I added the fix in the branch #695
it seems to work

[gui1117]

fixed on master