Disallow duplicated enum variant indices #628

pkhry · 2024-09-11T08:38:43Z

Description

Disallows usage of duplicated variant indices during derivation.
e.g

enum Example {
    #[codec(index = 0)]
    A, 
    B = 0,  // Error here
    C,
}

Assigns unique indexes for each enum variant based on heuristic described below:
- if codec::index is present -> use it and add the index to the set of duplicates. If there are duplicate indexes -> return an error.
- if there is explicit discriminant(e.g. A = 3) -> use it and add it to the set of duplicates. If there are any duplicates in the set of used indexes or explicit indexes -> return an error.
- no discriminant/explicit index -> use an incrementing number starting from 0, if number is in the used set it will get incremented until its no longer in set.

enum Example {
    #[codec(index = 0)]
    A, 
    B = 1, 
    C, // Will have index `2`
}

enum Example {
    #[codec(index = 1)]
    A, 
    B = 2, 
    C, // Will have index `0`
}

added docs.
we need current to assign incremental indexes to variants in ascending order also we need to traverse the variants first and collect all the assigned indexes before we can process them to check for duplicates and have a set of indexes already used, so that when we encounter a variant without explicit discriminant/codec(index = ?) we could assign an index using UsedIndexes.current by incrementing it until the u8 is not contained inside the used set.

bkchr · 2024-09-24T18:45:12Z

derive/src/utils.rs

-						.expect("Internal error, index attribute must have been checked");
-					return Some(byte);
+pub struct UsedIndexes {
+	used_set: HashSet<u8>,


Could be a HashMap<u8, Span> to show a proper error message.

done, not it's a hashMap. Now when we encounter a duplicate variant index we would also see span of the previous use site

gui1117 · 2024-10-02T07:10:46Z

This looks like a difficult breaking change for users.

They would have to manually check all their usage to see if any index is changing.

Maybe it would be better to have a safe transition.

Also if we are to change the indices, the most idiomatic order could be to follow the rust index implementation maybe.

enum Foo
    A = 3,
    B, // is 4

This discussion can be relevant: #173

gui1117 · 2024-10-02T07:17:19Z

derive/src/utils.rs

+				}
+			} else if let Some((
+				_,
+				expr @ syn::Expr::Lit(ExprLit { lit: syn::Lit::Int(lit_int), .. }),


I think this doesn't capture any discriminant, user can write this:

#[derive(Encode, Decode)] enum A { A = 3 + 4, }

I think it can be ok to constraint our implementation, but then we should compile-error if the expression is not a int literal

updated with to disallow this pattern

gui1117 · 2024-10-02T12:09:34Z

I like the PR, but I think we could keep the order as it is currently, compile an error if the discriminant expression is not a int literal, and compile an error if there is some duplicated indices.

pkhry · 2024-10-02T14:03:36Z

This looks like a difficult breaking change for users.

They would have to manually check all their usage to see if any index is changing.

Maybe it would be better to have a safe transition.

Also if we are to change the indices, the most idiomatic order could be to follow the rust index implementation maybe.
enum Foo
    A = 3,
    B, // is 4
This discussion can be relevant: #173

I feel like it's very hard to achieve safe transition, initial impl that i pushed tried to index each variant in a manner that was somewhat compatible with some of the things already existing.

I also pushed an update that indexes variants in a way that's closer to the rust spec

gui1117 · 2024-10-02T16:36:47Z

This looks like a difficult breaking change for users.
They would have to manually check all their usage to see if any index is changing.
Maybe it would be better to have a safe transition.
Also if we are to change the indices, the most idiomatic order could be to follow the rust index implementation maybe.
enum Foo
    A = 3,
    B, // is 4
This discussion can be relevant: #173
I feel like it's very hard to achieve safe transition, initial impl that i pushed tried to index each variant in a manner that was somewhat compatible with some of the things already existing.

I also pushed an update that indexes variants in a way that's closer to the rust spec

I see the commit for the compile error on complex expression, but not the other commit?

Thinking again the index variant closer to rust spec is not so important. I think the most important is to not break user code.

IMO we don't need to change the index, I know the index is not ideal, but any index will feel not so ideal (some people will prefer your initial implementation which is great, some other will prefer to follow rust discriminant order).

So maybe we should just not change the order and instead have a compile error when there is conflict? what do you think?

also revert indexing behaviour to the initial approach

pkhry · 2024-10-15T14:14:15Z

This looks like a difficult breaking change for users.
They would have to manually check all their usage to see if any index is changing.
Maybe it would be better to have a safe transition.
Also if we are to change the indices, the most idiomatic order could be to follow the rust index implementation maybe.
enum Foo
    A = 3,
    B, // is 4
This discussion can be relevant: #173
I feel like it's very hard to achieve safe transition, initial impl that i pushed tried to index each variant in a manner that was somewhat compatible with some of the things already existing.
I also pushed an update that indexes variants in a way that's closer to the rust spec
I see the commit for the compile error on complex expression, but not the other commit?

Thinking again the index variant closer to rust spec is not so important. I think the most important is to not break user code.

IMO we don't need to change the index, I know the index is not ideal, but any index will feel not so ideal (some people will prefer your initial implementation which is great, some other will prefer to follow rust discriminant order).

So maybe we should just not change the order and instead have a compile error when there is conflict? what do you think?

reverted to the indexing strategy used initially, we now just give out a compile error after encountering duplicates.
added ui tests for failing cases

gui1117

PR is good, some refactor and then I will approve.

gui1117 · 2024-10-19T14:42:40Z

tests/scale_codec_ui.rs

@@ -19,4 +19,5 @@ fn scale_codec_ui_tests() {
 	let t = trybuild::TestCases::new();
 	t.compile_fail("tests/scale_codec_ui/*.rs");
 	t.pass("tests/scale_codec_ui/pass/*.rs");
+	t.compile_fail("tests/scale_codec_ui/fail/*.rs");


why not put those tests at "tests/scale_codec_ui/" ?
or move all failing test in the fail dir.

(anyway I don't mind)

gui1117 · 2024-10-19T14:44:42Z

tests/scale_codec_ui/fail/codec_duplicate_index.stderr

@@ -0,0 +1,23 @@
+error: Duplicate variant index. qed


QED means: quod erat demonstrandum = which was to be demonstrated.

I think it doesn't fit in an error message.

Maybe writing: scale codec error: Invalid variant index, the variant index is duplicated.

derive/src/utils.rs

gui1117 · 2024-11-04T04:39:10Z

Also I just found out we can assert that variant indices are unique using constant evaluation.
No need to enforce that discriminant is a literal (so no breaking change at all)

A draft implementation look like this:

diff --git a/derive/src/encode.rs b/derive/src/encode.rs
index a29eec7..798237f 100644
--- a/derive/src/encode.rs
+++ b/derive/src/encode.rs
@@ -345,7 +345,7 @@ fn impl_encode(data: &Data, type_name: &Ident, crate_path: &syn::Path) -> TokenS
                                                        }
                                                };
 
-                                               [hinting, encoding]
+                                               [hinting, encoding, index]
                                        },
                                        Fields::Unnamed(ref fields) => {
                                                let fields = &fields.unnamed;
@@ -378,7 +378,7 @@ fn impl_encode(data: &Data, type_name: &Ident, crate_path: &syn::Path) -> TokenS
                                                        }
                                                };
 
-                                               [hinting, encoding]
+                                               [hinting, encoding, index]
                                        },
                                        Fields::Unit => {
                                                let hinting = quote_spanned! { f.span() =>
@@ -394,13 +394,14 @@ fn impl_encode(data: &Data, type_name: &Ident, crate_path: &syn::Path) -> TokenS
                                                        }
                                                };
 
-                                               [hinting, encoding]
+                                               [hinting, encoding, index]
                                        },
                                }
                        });
 
-                       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_indices = recurse.clone().map(|[_, _, index]| index);
 
                        let hinting = quote! {
                                // The variant index uses 1 byte.
@@ -411,6 +412,23 @@ fn impl_encode(data: &Data, type_name: &Ident, crate_path: &syn::Path) -> TokenS
                        };
 
                        let encoding = quote! {
+                                       const _: () = {
+                                               let indices = [#( #recurse_indices ,)*];
+                                               let len = indices.len();
+
+                                               // Check each pair for uniqueness
+                                               let mut i = 0;
+                                               while i < len {
+                                                       let mut j = i + 1;
+                                                       while j < len {
+                                                               if indices[i] == indices[j] {
+                                                                       panic!("TODO: good error message with variant names and indices");
+                                                               }
+                                                               j += 1;
+                                                       }
+                                                       i += 1;
+                                               }
+                                       };
                                match *#self_ {
                                        #( #recurse_encoding )*,
                                        _ => (),

pkhry · 2024-11-04T10:33:17Z

Also I just found out we can assert that variant indices are unique using constant evaluation. No need to enforce that discriminant is a literal (so no breaking change at all)

A draft implementation look like this:

diff --git a/derive/src/encode.rs b/derive/src/encode.rs
index a29eec7..798237f 100644
--- a/derive/src/encode.rs
+++ b/derive/src/encode.rs
@@ -345,7 +345,7 @@ fn impl_encode(data: &Data, type_name: &Ident, crate_path: &syn::Path) -> TokenS
                                                        }
                                                };
 
-                                               [hinting, encoding]
+                                               [hinting, encoding, index]
                                        },
                                        Fields::Unnamed(ref fields) => {
                                                let fields = &fields.unnamed;
@@ -378,7 +378,7 @@ fn impl_encode(data: &Data, type_name: &Ident, crate_path: &syn::Path) -> TokenS
                                                        }
                                                };
 
-                                               [hinting, encoding]
+                                               [hinting, encoding, index]
                                        },
                                        Fields::Unit => {
                                                let hinting = quote_spanned! { f.span() =>
@@ -394,13 +394,14 @@ fn impl_encode(data: &Data, type_name: &Ident, crate_path: &syn::Path) -> TokenS
                                                        }
                                                };
 
-                                               [hinting, encoding]
+                                               [hinting, encoding, index]
                                        },
                                }
                        });
 
-                       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_indices = recurse.clone().map(|[_, _, index]| index);
 
                        let hinting = quote! {
                                // The variant index uses 1 byte.
@@ -411,6 +412,23 @@ fn impl_encode(data: &Data, type_name: &Ident, crate_path: &syn::Path) -> TokenS
                        };
 
                        let encoding = quote! {
+                                       const _: () = {
+                                               let indices = [#( #recurse_indices ,)*];
+                                               let len = indices.len();
+
+                                               // Check each pair for uniqueness
+                                               let mut i = 0;
+                                               while i < len {
+                                                       let mut j = i + 1;
+                                                       while j < len {
+                                                               if indices[i] == indices[j] {
+                                                                       panic!("TODO: good error message with variant names and indices");
+                                                               }
+                                                               j += 1;
+                                                       }
+                                                       i += 1;
+                                               }
+                                       };
                                match *#self_ {
                                        #( #recurse_encoding )*,
                                        _ => (),

it's possible, but should we do const eval in that place?

Decode would still be using the new variant indexing code.
adds additional need to keep the variant spans around in case when we need the error.

gui1117 · 2024-11-04T12:59:26Z

I'm good with the current PR.
I just didn't know it was possible with const evaluation.

If we go the const evaluation way, we should do for both decode and encode. There won't be overhead as it is const evaluation. The error will be a bit less pretty but probably ok.

Anyway we can go with the PR as it is.

gui1117 · 2024-11-07T07:44:54Z

don't hate me, but I am conflicted, the current PR is good but it is a breaking change. We no longer support enums with discriminant being an expression rather than a literal.

And there to achieve the goal of this PR we can do it without a breaking change, by using constant evaluation.

People regularly complains that libraries are breaking too often, maybe we should avoid this breaking change and implement with the constant evaluation.

pkhry · 2024-11-07T08:20:44Z

don't hate me, but I am conflicted, the current PR is good but it is a breaking change. We no longer support enums with discriminant being an expression rather than a literal.

And there to achieve the goal of this PR we can do it without a breaking change, by using constant evaluation.

People regularly complains that libraries are breaking too often, maybe we should avoid this breaking change and implement with the constant evaluation.

All good, I'll re-do it

pkhry · 2024-11-08T15:07:58Z

don't hate me, but I am conflicted, the current PR is good but it is a breaking change. We no longer support enums with discriminant being an expression rather than a literal.

And there to achieve the goal of this PR we can do it without a breaking change, by using constant evaluation.

People regularly complains that libraries are breaking too often, maybe we should avoid this breaking change and implement with the constant evaluation.

Ok, it seems like string interpolation is not possible without external dependency inside constant blocks, see alternate PR's resulting error message. the only way to have string interpolation in constant blocks is to use const_format from what i've understood.

pkhry added 2 commits September 11, 2024 10:05

Add limit for codec indexes

e6b0b0f

closes #507

fixup tests

c986e06

pkhry requested a review from koute September 11, 2024 08:38

pkhry added 2 commits September 11, 2024 10:42

clippy

56c6b3a

fmt

722f02e

bkchr reviewed Sep 24, 2024

View reviewed changes

review comments

f2cc6c0

gui1117 reviewed Oct 2, 2024

View reviewed changes

review comments

7aa0d89

pkhry requested a review from gui1117 October 2, 2024 14:03

pkhry added 2 commits October 15, 2024 15:58

just dissalow duplicate indexes

7b92913

also revert indexing behaviour to the initial approach

merge master

614f2b7

pkhry added 2 commits October 18, 2024 11:28

clippy

9e3c8a6

fixup test

df86890

pkhry requested a review from bkchr October 21, 2024 11:29

pkhry and others added 2 commits October 30, 2024 11:47

Merge branch 'master' into pkhry/codec-indexes

6f31b67

syn 1.0 -> 2.0

f2255ee

gui1117 reviewed Nov 3, 2024

View reviewed changes

pkhry added 3 commits November 4, 2024 11:33

review comments

add9eae

remove const eval

77396b9

fixup ui tests

a25db72

pkhry requested a review from gui1117 November 5, 2024 12:13

fmt

9112b92

pkhry mentioned this pull request Nov 8, 2024

Disallow duplicate indexes using constant evaluation #653

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disallow duplicated enum variant indices #628

Disallow duplicated enum variant indices #628

pkhry commented Sep 11, 2024 •

edited

Loading

bkchr Sep 12, 2024

pkhry Oct 1, 2024

bkchr Sep 24, 2024

pkhry Oct 1, 2024

bkchr Sep 24, 2024

pkhry Oct 1, 2024

gui1117 commented Oct 2, 2024 •

edited

Loading

gui1117 Oct 2, 2024 •

edited

Loading

pkhry Oct 2, 2024

gui1117 commented Oct 2, 2024

pkhry commented Oct 2, 2024

gui1117 commented Oct 2, 2024

pkhry commented Oct 15, 2024

gui1117 left a comment

gui1117 Oct 19, 2024

gui1117 Oct 19, 2024

gui1117 commented Nov 4, 2024

pkhry commented Nov 4, 2024

gui1117 commented Nov 4, 2024

gui1117 commented Nov 7, 2024

pkhry commented Nov 7, 2024

pkhry commented Nov 8, 2024

+              	assert_eq!(T::B.encode(), vec![0]);
+              }
+              #[test]
+              fn index_attr_vairant_duplicates_indices() {

Disallow duplicated enum variant indices #628

Are you sure you want to change the base?

Disallow duplicated enum variant indices #628

Conversation

pkhry commented Sep 11, 2024 • edited Loading

Description

See also:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gui1117 commented Oct 2, 2024 • edited Loading

gui1117 Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gui1117 commented Oct 2, 2024

pkhry commented Oct 2, 2024

gui1117 commented Oct 2, 2024

pkhry commented Oct 15, 2024

gui1117 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gui1117 commented Nov 4, 2024

pkhry commented Nov 4, 2024

gui1117 commented Nov 4, 2024

gui1117 commented Nov 7, 2024

pkhry commented Nov 7, 2024

pkhry commented Nov 8, 2024

pkhry commented Sep 11, 2024 •

edited

Loading

gui1117 commented Oct 2, 2024 •

edited

Loading

gui1117 Oct 2, 2024 •

edited

Loading