Derive encode for variants can result in having multiple variants having same index.

[gui1117]

for instance this example does compile without warning:

#[test]
fn discriminant_variant_counted_in_default_index() {
	#[derive(Encode)]
	enum T {
		A = 1,
		B,
	}

	assert_eq!(T::A.encode(), vec![1]);
	assert_eq!(T::B.encode(), vec![1]);
}

We should make this a compile_error or try to give some warning in some way

[gui1117]

as warning is difficult in proc_macro, an idea would just be compile-error when it happens and adding a new attribute "#[codec(AllowVariantIndexConflict)]"

[gui1117]

implementation is actually difficult because disciminant is an expression: user can write:

enum A { A = 1+1 };

Thus the only solution I see is to implement a tests which test that no index are same

[gui1117]

unfortunately it is not possible to derive some test if the macro is called inside a function like this:

fn enum_struct_test() {
	#[derive(Encode, Decode, Debug)]
	enum A {
		A = 1,
		B,
		C,
	}
}

It result in:

warning: cannot test inner items
 --> tests/skip.rs:6:11
  |
6 |     #[derive(Encode, Decode, Debug)]
  |              ^^^^^^
  |
  = note: `#[warn(unnameable_test_items)]` on by default
  = note: this warning originates in an attribute macro (in Nightly builds, run with -Z macro-backtrace for more info)

[gslancaster]

The Rust Reference indicates that the correct discriminant value for T::B in the example given in the OP is 2.

https://doc.rust-lang.org/reference/items/enumerations.html#implicit-discriminants

It is unfortunate that the SCALE serialized discriminant is 1.

SCALE is free to decide its own discriminator values for serialized data, I suppose. However, creating ambiguity between two discriminants, where the standard Rust discriminants are unambiguous, seems like an unintentional bug. It can create the situation where you encode one enum value (e.g., T::B) but decode another (e.g., T::A).

[gui1117]

yes, but it seems difficult to implement like this in scale codec because discriminant is an expression, and we don't have access to the result AFAIK.
Also ideally it would be great to have a way forward without breaking any change.

[gui1117]

We could still implement the correct discriminant by reusing the expression and adding +1.
but we also introduced the skip attribute for variant, and this attribute can create some ambiguity too.

their was also this proposal #305 to improve enum encoding: we could use this opportunity to implement unambiguously

EDIT: a good implemetation would be to use the discriminant and skip attribute doesn't change the index at all it just skip the encoding/decoding of the variant.
this would be a breaking change for: enum with some discriminant define and some not define, and also enum with skipped variant which are not last.

[gslancaster]

The best implementation, if starting today, would be to have the Rust in-memory discriminant and SCALE serialised discriminant agree. But it is too late for that now.

Suggested workaround: if one enum variant has an explicit discriminant, then so should all subsequent enum variants. If this rule is followed then SCALE and Rust will agree on discriminant values. At first make failures to follow this rule a warning then, with the next major version, an error.