chore!: don’t unnecessarily add types recursively in borsh::BorshSchema derives
#251
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Firstly, change add_definition function to return whether the definition has been added or it already existed in the map. This avoids double map lookup in places which conditionally add component definitions. Secondly, use the check consistently in all places so that component definitions aren’t processed unnecessarily.
dj8yfo
changed the title
dj8yfo
changed the title
borsh::BorshSchema derives
dj8yfo
force-pushed
the
c
branch
2 times, most recently
from
1a17c9b to
3c7c713
Compare
dj8yfo
reviewed
Oct 25, 2023
Comment on lines
+44
to
+50
| #[test] | ||
| fn test_implicit_conflict_vec() { | ||
| let mut defs = Default::default(); | ||
| <Vec<i64> as borsh::BorshSchema>::add_definitions_recursively(&mut defs); | ||
| <Vec<ConflictingSchema> as borsh::BorshSchema>::add_definitions_recursively(&mut defs); | ||
| // NOTE: the contents of `defs` depend on the order of 2 above lines | ||
| } |
There was a problem hiding this comment.
similar test on test_schema_conflicts branch emits
thread 'test_implicit_conflict_vec' panicked at borsh/src/schema.rs:253:13:
assertion `left == right` failed: Redefining type schema for i64. Types with the same names are not supported.
left: Primitive(8)
right: Struct { fields: Empty }
Comment on lines
+52
to
+60
| #[test] | ||
| fn test_implicit_conflict_range() { | ||
| let mut defs = Default::default(); | ||
| <core::ops::Range<i64> as borsh::BorshSchema>::add_definitions_recursively(&mut defs); | ||
| <core::ops::Range<ConflictingSchema> as borsh::BorshSchema>::add_definitions_recursively( | ||
| &mut defs, | ||
| ); | ||
| // NOTE: the contents of `defs` depend on the order of 2 above lines | ||
| } |
There was a problem hiding this comment.
similar test on test_schema_conflict branch emits
thread 'test_implicit_conflict_range' panicked at borsh/src/schema.rs:253:13:
assertion `left == right` failed: Redefining type schema for i64. Types with the same names are not supported.
left: Primitive(8)
right: Struct { fields: Empty }
Comment on lines
+62
to
+68
| #[test] | ||
| fn test_implicit_conflict_slice() { | ||
| let mut defs = Default::default(); | ||
| <[i64] as borsh::BorshSchema>::add_definitions_recursively(&mut defs); | ||
| <[ConflictingSchema] as borsh::BorshSchema>::add_definitions_recursively(&mut defs); | ||
| // NOTE: the contents of `defs` depend on the order of 2 above lines | ||
| } |
There was a problem hiding this comment.
similar test on test_schema_conflict branch emits
thread 'test_implicit_conflict_slice' panicked at borsh/src/schema.rs:253:13:
assertion `left == right` failed: Redefining type schema for i64. Types with the same names are not supported.
left: Primitive(8)
right: Struct { fields: Empty }
Comment on lines
+70
to
+76
| #[test] | ||
| fn test_implicit_conflict_array() { | ||
| let mut defs = Default::default(); | ||
| <[i64; 10] as borsh::BorshSchema>::add_definitions_recursively(&mut defs); | ||
| <[ConflictingSchema; 10] as borsh::BorshSchema>::add_definitions_recursively(&mut defs); | ||
| // NOTE: the contents of `defs` depend on the order of 2 above lines | ||
| } |
There was a problem hiding this comment.
similar test on test_schema_conflict branch emits
thread 'test_implicit_conflict_array' panicked at borsh/src/schema.rs:253:13:
assertion `left == right` failed: Redefining type schema for i64. Types with the same names are not supported.
left: Primitive(8)
right: Struct { fields: Empty }
Comment on lines
+78
to
+84
| #[test] | ||
| fn test_implicit_conflict_option() { | ||
| let mut defs = Default::default(); | ||
| <Option<i64> as borsh::BorshSchema>::add_definitions_recursively(&mut defs); | ||
| <Option<ConflictingSchema> as borsh::BorshSchema>::add_definitions_recursively(&mut defs); | ||
| // NOTE: the contents of `defs` depend on the order of 2 above lines | ||
| } |
There was a problem hiding this comment.
similar test on test_schema_conflict branch emits
thread 'test_implicit_conflict_option' panicked at borsh/src/schema.rs:253:13:
assertion `left == right` failed: Redefining type schema for i64. Types with the same names are not supported.
left: Primitive(8)
right: Struct { fields: Empty }
Comment on lines
+133
to
+145
| #[allow(unused)] | ||
| #[derive(borsh::BorshSchema)] | ||
| enum SelfConflictingEnum { | ||
| A { field: i64 }, | ||
| B { field: ConflictingSchema }, | ||
| } | ||
|
|
||
| #[test] | ||
| #[should_panic(expected = "Redefining type schema for i64")] | ||
| fn test_implicit_conflict_self_conflicting_enum() { | ||
| let mut defs = Default::default(); | ||
| <SelfConflictingEnum as borsh::BorshSchema>::add_definitions_recursively(&mut defs); | ||
| } |
There was a problem hiding this comment.
this test passes, but if the enum definition changes to add some indirection
// SelfConflictingEnum change
#[allow(unused)]
#[derive(borsh::BorshSchema)]
enum SelfConflictingEnum {
A { field: Vec<i64> },
B { field: Vec<ConflictingSchema> },
}the test stops to pass, unlike test_schema_conflict branch
Comment on lines
+147
to
+153
| #[test] | ||
| fn test_implicit_conflict_result() { | ||
| let mut defs = Default::default(); | ||
| <Result<u8, i64> as borsh::BorshSchema>::add_definitions_recursively(&mut defs); | ||
| <Result<u8, ConflictingSchema> as borsh::BorshSchema>::add_definitions_recursively(&mut defs); | ||
| // NOTE: the contents of `defs` depend on the order of 2 above lines | ||
| } |
There was a problem hiding this comment.
similar test on test_schema_conflict branch emits
thread 'test_implicit_conflict_result' panicked at borsh/src/schema.rs:253:13:
assertion `left == right` failed: Redefining type schema for i64. Types with the same names are not supported.
left: Primitive(8)
right: Struct { fields: Empty }
Comment on lines
+155
to
+161
| #[test] | ||
| fn test_implicit_conflict_btreemap() { | ||
| let mut defs = Default::default(); | ||
| <BTreeMap<i64, u8> as borsh::BorshSchema>::add_definitions_recursively(&mut defs); | ||
| <BTreeMap<ConflictingSchema, u8> as borsh::BorshSchema>::add_definitions_recursively(&mut defs); | ||
| // NOTE: the contents of `defs` depend on the order of 2 above lines | ||
| } |
There was a problem hiding this comment.
similar test on test_schema_conflict branch emits
thread 'test_implicit_conflict_btreemap' panicked at borsh/src/schema.rs:253:13:
assertion `left == right` failed: Redefining type schema for i64. Types with the same names are not supported.
left: Primitive(8)
right: Struct { fields: Empty }
Comment on lines
+163
to
+169
| #[test] | ||
| fn test_implicit_conflict_btreeset() { | ||
| let mut defs = Default::default(); | ||
| <BTreeSet<i64> as borsh::BorshSchema>::add_definitions_recursively(&mut defs); | ||
| <BTreeSet<ConflictingSchema> as borsh::BorshSchema>::add_definitions_recursively(&mut defs); | ||
| // NOTE: the contents of `defs` depend on the order of 2 above lines | ||
| } |
There was a problem hiding this comment.
similar test on test_schema_conflict branch emits
thread 'test_implicit_conflict_btreeset' panicked at borsh/src/schema.rs:253:13:
assertion `left == right` failed: Redefining type schema for i64. Types with the same names are not supported.
left: Primitive(8)
right: Struct { fields: Empty }
Comment on lines
+171
to
+177
| #[test] | ||
| fn test_implicit_conflict_tuple() { | ||
| let mut defs = Default::default(); | ||
| <(i64, u8) as borsh::BorshSchema>::add_definitions_recursively(&mut defs); | ||
| <(ConflictingSchema, u8) as borsh::BorshSchema>::add_definitions_recursively(&mut defs); | ||
| // NOTE: the contents of `defs` depend on the order of 2 above lines | ||
| } |
There was a problem hiding this comment.
similar test on test_schema_conflict branch emits
thread 'test_implicit_conflict_tuple' panicked at borsh/src/schema.rs:253:13:
assertion `left == right` failed: Redefining type schema for i64. Types with the same names are not supported.
left: Primitive(8)
right: Struct { fields: Empty }
dj8yfo
requested changes
Oct 25, 2023
Comment on lines
-66
to
-68
| let no_recursion_flag = definitions.get(&<Self as #cratename::BorshSchema>::declaration()).is_none(); | ||
| #cratename::schema::add_definition(<Self as #cratename::BorshSchema>::declaration(), definition, definitions); | ||
| if no_recursion_flag { |
There was a problem hiding this comment.
In my opinion, there's no point in propagating this single loophole, needed for derives of BorshSchema for recursive structs/enums, everywhere, and, changing the signature of borsh::schema::add_definition as a convenience method for that purpose,
because it decreases the number of cases where schema Definition collisions are detected (2 Definitions mapped to the same Declaration).
Unfortunately, collissions illustrated by test_implicit_conflict_struct and test_implicit_conflict_enum aren't detected on current master either, but a more robust solution would probably require more significant changes to BorshSchema trait and much more complex code overall.
Let's keep it as it is now.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Firstly, change add_definition function to return whether the
definition has been added or it already existed in the map. This
avoids double map lookup in places which conditionally add component
definitions.
Secondly, use the check consistently in all places so that component
definitions aren’t processed unnecessarily.
NOTE: formally, this is a breaking change, as
borsh-derivewith this change of version, say,1.2.0won't be compatible withborsh1.1.0. Hopefully, tilde version requirement adresses this problem.