Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure unique types in codegen #967

Merged
merged 9 commits into from
May 23, 2023
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 42 additions & 1 deletion codegen/src/api/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -175,15 +175,56 @@ impl RuntimeGenerator {
///
/// Supported versions: v14 and v15.
pub fn new(metadata: RuntimeMetadataPrefixed) -> Self {
let metadata = match metadata.1 {
let mut metadata = match metadata.1 {
RuntimeMetadata::V14(v14) => metadata_v14_to_latest(v14),
RuntimeMetadata::V15(v15) => v15,
_ => panic!("Unsupported metadata version {:?}", metadata.1),
};

Self::ensure_unique_type_paths(&mut metadata);

RuntimeGenerator { metadata }
}

/// Ensure that every unique type we'll be generating or referring to also has a
/// unique path, so that types with matching paths don't end up overwriting each other
/// 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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, are these type hints really needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

for ty in &mut metadata.types.types {
// Ignore types without a path (ie prelude types).
if ty.ty.path.namespace().is_empty() {
continue;
}

let has_valid_type_params = ty.ty.type_params.iter().any(|tp| tp.ty.is_some());

// Ignore types which have generic params that the type generation will use.
// Ordinarily we'd expect that any two types with identical paths must be parameterized
// 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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

continue;
}

// Count how many times we've seen the same path already.
let visited_count = visited_path_counts
.entry(ty.ty.path.segments.clone())
.or_default();
*visited_count += 1;

// alter the type so that if it's been seen more than once, we append a number to
// its name to ensure that every unique type has a unique path, too.
if *visited_count > 1 {
if let Some(name) = ty.ty.path.segments.last_mut() {
*name = format!("{name}{visited_count}");
}
}
}
}

/// Generate the API for interacting with a Substrate runtime.
///
/// # Arguments
Expand Down
97 changes: 97 additions & 0 deletions testing/integration-tests/src/codegen/codegen_tests.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
// Copyright 2019-2023 Parity Technologies (UK) Ltd.
// This file is dual-licensed as Apache-2.0 or GPL-3.0.
// see LICENSE for license details.

use frame_metadata::{
v15::{ExtrinsicMetadata, RuntimeMetadataV15},
RuntimeMetadataPrefixed,
};
use scale_info::{meta_type, IntoPortable, TypeInfo};
use subxt_codegen::{CratePath, DerivesRegistry, RuntimeGenerator, TypeSubstitutes};

fn generate_runtime_interface_from_metadata(metadata: RuntimeMetadataPrefixed) -> String {
// Generate a runtime interface from the provided metadata.
let generator = RuntimeGenerator::new(metadata);
let item_mod = syn::parse_quote!(
pub mod api {}
);
let crate_path = CratePath::default();
let derives = DerivesRegistry::with_default_derives(&crate_path);
let type_substitutes = TypeSubstitutes::with_default_substitutes(&crate_path);
generator
.generate_runtime(item_mod, derives, type_substitutes, crate_path, false)
.expect("API generation must be valid")
.to_string()
}

#[test]
fn dupe_types_do_not_overwrite_each_other() {
fn generate_metadata_with_duplicate_types() -> RuntimeMetadataPrefixed {
#[derive(TypeInfo)]
struct Runtime;
#[derive(TypeInfo)]
enum RuntimeCall {}
#[derive(TypeInfo)]
enum RuntimeEvent {}
#[derive(TypeInfo)]
pub enum DispatchError {}

// We need these types for codegen to work:
let mut registry = scale_info::Registry::new();
let ty = registry.register_type(&meta_type::<Runtime>());
registry.register_type(&meta_type::<RuntimeCall>());
registry.register_type(&meta_type::<RuntimeEvent>());
registry.register_type(&meta_type::<DispatchError>());

// Now we duplicate some types with same type info:
enum Foo {}
impl TypeInfo for Foo {
type Identity = Self;
fn type_info() -> scale_info::Type {
scale_info::Type::builder()
.path(scale_info::Path::new("DuplicateType", "dupe_mod"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

.variant(
scale_info::build::Variants::new()
.variant("FirstDupeTypeVariant", |builder| builder.index(0)),
)
}
}
enum Bar {}
impl TypeInfo for Bar {
type Identity = Self;
fn type_info() -> scale_info::Type {
scale_info::Type::builder()
.path(scale_info::Path::new("DuplicateType", "dupe_mod"))
.variant(
scale_info::build::Variants::new()
.variant("SecondDupeTypeVariant", |builder| builder.index(0)),
)
}
}

registry.register_type(&meta_type::<Foo>());
registry.register_type(&meta_type::<Bar>());

let extrinsic = ExtrinsicMetadata {
ty: meta_type::<()>(),
version: 0,
signed_extensions: vec![],
}
.into_portable(&mut registry);
let metadata = RuntimeMetadataV15 {
types: registry.into(),
pallets: Vec::new(),
extrinsic,
ty,
apis: vec![],
};

RuntimeMetadataPrefixed::from(metadata)
}

let metadata = generate_metadata_with_duplicate_types();
let interface = generate_runtime_interface_from_metadata(metadata);

assert!(interface.contains("FirstDupeTypeVariant"));
Copy link
Member

@niklasad1 niklasad1 May 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ^

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

assert!(interface.contains("SecondDupeTypeVariant"));
}
3 changes: 2 additions & 1 deletion testing/integration-tests/src/codegen/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,5 @@
#[allow(clippy::all)]
mod polkadot;

mod codegen_documentation;
mod codegen_tests;
mod documentation;