Ensure unique types in codegen #967
Conversation
codegen/src/api/mod.rs
Outdated
| /// in the codegen. We ignore any types with generics; Subxt actually endeavours to | ||
| /// de-duplicate those into single types with a generic. | ||
| fn ensure_unique_type_paths(metadata: &mut RuntimeMetadataV15) { | ||
| let mut visited_path_counts = std::collections::HashMap::<Vec<String>, usize>::new(); |
There was a problem hiding this comment.
hmm, are these type hints really needed?
There was a problem hiding this comment.
I needed the usize bit because nothing else determines that number type, and so I figure we may as well have both for clarity :) (I did import Hashmap though to shorten this line)
| type Identity = Self; | ||
| fn type_info() -> scale_info::Type { | ||
| scale_info::Type::builder() | ||
| .path(scale_info::Path::new("DuplicateType", "dupe_mod")) |
There was a problem hiding this comment.
dq: this does generate two types with the same name but different variants?
Such as:
enum DuplicateType {
FirstDupeTypeVariant
}
enum DuplicateTyp {
SecondDupeTypeVariant
}or
enum DuplicateType {
FirstDupeTypeVariant
SecondDupeTypeVariant
}There was a problem hiding this comment.
We end up with two different types with the same path and name here, ie the first case.
I think scale-info uses std::any::TypeId, so if I tried adding the same type to the registry twice it would not add the second one, hence the need for two separate types with the same info :)
| let metadata = generate_metadata_with_duplicate_types(); | ||
| let interface = generate_runtime_interface_from_metadata(metadata); | ||
|
|
||
| assert!(interface.contains("FirstDupeTypeVariant")); |
There was a problem hiding this comment.
see my comment above, I don't understand if the the variants are merged to a single type or that path has been modified to format!("{name}{visited_path}")?
Looking at the code you added, the path should be modified but I don't really understand by looking the test code ^
There was a problem hiding this comment.
We end up with 2 variant types in the registry that both have the same path and name, and then when doing the codegen we spot that the paths are identical and add a "2" to one of the names to make sure that they don't override eachother. (ie if you remove the ensure_unique_types line and run this test again it'll fail)
| // in order to share the path. However scale-info doesn't understand all forms of generics | ||
| // properly I think (eg generics that have associated types that can differ), and so in | ||
| // those cases we need to fix the paths for Subxt to generate correct code. | ||
| if has_valid_type_params { |
There was a problem hiding this comment.
Can't we just skip this check and modify duplicate the path the regardless to be on the safe side?
Maybe I don't understand the comment properly but I regard it as if it's generic is safe to have
duplicate types because when those are instantiated it isn't an issue?
Would be cool to to have test for it :)
There was a problem hiding this comment.
If the path is same and the type has generics, scale-info already handles that case ok.
For example, in the metadata you might see types like Vec<u8>, Vec<bool> and Vec<Foo> mentioned. When we generate the code, we see that they all have the same path but don't care, so we just end up with a single Vec<Something> in the generated output. (I didn't realise this at first and indeed didn't have this check to begin with, but then the codegen ended up with Vec1<T>, Vec2<T>, Vec3<T> and so on for loads of types).
The problem we ran into here was that there were two types mentioned in the metadata, pallet::Call and pallet::Call. They actually had some different types in their variants, but didn't have any generics that the code gen could use, so appeared the same. Now, we spot that case and change one to pallet::Call2 so that both get generated in the output and one doesn't end up overriding the other.
Yeah I can probably add a test to show that generics end up being output as just one type :)
|
@niklasad1 I added another test to show that types with generics + same path are de-duplicated; hopefully with my other replies that addresses things :) |
This feels like a bit of a hack, but basically the issue is:
Closes #966