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

Construct PortableRegistry dynamically at runtime #164

Merged
merged 51 commits into from Oct 7, 2022

Conversation

ascjones
Copy link
Member

@ascjones ascjones commented Aug 11, 2022

Updates all type builders to allow construction of PortableForm types, and introduces the PortableRegistryBuilder to allow construction of a PortableRegistry dynamically. Currently this is only possible from an instance of the Registry which requires static type definitions.

The motivation for this is for solang to be able to generate metadata for ink! contracts, where static rust types are not available (since it is compiling Solidity contracts).

It is also generally useful to be able to dynamically define portable type hierarchies e.g. for post processing of statically generated metadata, or for testing.

Note: It is a goal that this should not be a breaking change and can be a minor release. So reviewers please look out for that.

src/registry.rs Outdated Show resolved Hide resolved
src/meta_type.rs Outdated Show resolved Hide resolved
@ascjones
Copy link
Member Author

Modified this to allow constructing of PortableRegistry and temporarily made type fields public to allow direct construction with PortableForm. See use-ink/cargo-contract#659 (comment)

@ascjones ascjones changed the title Experiment: construct MetaType at runtime Experiment: construct PortableRegistry dynamically at runtime Aug 22, 2022
@xermicus
Copy link
Member

Modified this to allow constructing of PortableRegistry and temporarily made type fields public to allow direct construction with PortableForm. See paritytech/cargo-contract#659 (comment)

Thanks! I couldn't get it to work (or figure a sane way to do so) neither with the static fn pointer. This looks promising now, I'll let you know!

@@ -337,7 +337,7 @@ impl<T> TypeInfo for PhantomData<T> {
// Fields of this type should be filtered out and never appear in the type graph.
Type::builder()
.path(Path::prelude("PhantomData"))
.docs(&["PhantomData placeholder, this type should be filtered out"])
.docs(["PhantomData placeholder, this type should be filtered out"])
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that this is not a required change, just a clippy suggestion since arrays implement IntoIter

src/ty/mod.rs Outdated
Comment on lines 221 to 223
pub fn new<S>(name: S, ty: Option<T::Type>) -> Self
where
S: Into<T::String>,
Copy link
Contributor

@jsdw jsdw Oct 7, 2022

Choose a reason for hiding this comment

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

This is very silly/edge casey but it's possible for this to be a breaking change because the type checker doesn't know quite so much about what name could be any more. Stupid contrived example:

fn into_string(val: impl Into<String>) {}
fn concrete_string(val: String) {}

fn main() {
    let maybe_string = ['a','b','c'].into_iter().collect();
    // breaks if changed to into_string:
    concrete_string(maybe_string);
}

Is it worth caring about? quite possibly not!

Copy link
Member Author

@ascjones ascjones Oct 7, 2022

Choose a reason for hiding this comment

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

Possibly, but I have split this anyway into new (legacy for MetaForm) and added a new new_portable method as I have for other types here. c9149c2

src/ty/path.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@jsdw jsdw left a comment

Choose a reason for hiding this comment

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

Looks good to me! Excited about the new PortableRegistryBuilder; I'm almos certain there will be places I can use that to simplify stuff.

As we chatted about, I think that while this technically could introduce breaking changes owing to loosening of types (and less type information for the compiler to work with), as you said the acid test will be whether it fails to compile on some crates that use this stuff heavily like substrate, scale-decode and whatever else. So I'd be happy with this being a minor bump if it doesn't cause any breakage on this set of crates known to use it a bunch (a mini crater run if you will).

@ascjones
Copy link
Member Author

ascjones commented Oct 7, 2022

as you said the acid test will be whether it fails to compile on some crates that use this stuff heavily like substrate, scale-decode and whatever else. So I'd be happy with this being a minor bump if it doesn't cause any breakage on this set of crates known to use it a bunch (a mini crater run if you will).

Indeed, as part of the release prep for 2.3 I will definitely check that all these things compile with no changes.

@ascjones ascjones merged commit a69ac55 into master Oct 7, 2022
@ascjones ascjones deleted the aj/custom-meta-types branch October 7, 2022 11:06
@xermicus xermicus mentioned this pull request Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants