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.

Sign up for GitHub

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

Jump to bottom

New JSON ABI format supporting generics #2524

Merged
merged 23 commits into from
Aug 18, 2022
Merged

New JSON ABI format supporting generics #2524

merged 23 commits into from
Aug 18, 2022

Conversation

mohammadfawaz
Copy link
Contributor

@mohammadfawaz mohammadfawaz commented Aug 12, 2022 •
edited
Loading

Closes #2386

The main changes are as follows:

  • Introduce initial_type_id or initial_return_type in various places in the type system. This is needed because the original type information (potentially generic) is often lost and cannot be inferred at the end of the type checking phase. Keeping that information around helps generate the JSON ABI information needed in a much cleaner way.
  • New data structures for the new JSON ABI format in sway-types
  • Generate the new JSON ABI format... Most of the work in the type_system directory.
  • Currently generate both the old and the new JSON ABI files. We will quickly deprecate the old one as soon as the SDKs are ready to make the switch (hopefully within a week or two)
  • Added testing for contract tests for now. The tests show how the new JSON looks like. Again, most of the old stuff will be cleaned up and replaced with the new stuff.
  • In forc-pkg, I added code to standardize the output of the JSON by
    • Removing duplicate types
    • Ordering the types in alphabetical order
    • Assigning new IDs to the types according to the alphabetical order above.

The standardization of the output is not really required but helps make the JSON more compact and more robust for testing in case of future changes in the type system, given that the type IDs that the type engine hands out are arbitrary (deterministic but arbitrary)

@mohammadfawaz mohammadfawaz self-assigned this Aug 12, 2022
@mohammadfawaz mohammadfawaz added ABI Everything to do the ABI, especially the JSON representation compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen compiler: parser Everything to do with the parser P: critical Should be looked at before anything else labels Aug 12, 2022
@mohammadfawaz mohammadfawaz changed the title New JSON ABI format. Starting with full support of generic structs New JSON ABI format supporting generics Aug 12, 2022
@mohammadfawaz mohammadfawaz mentioned this pull request Aug 12, 2022
Closed
@mohammadfawaz
Copy link
Contributor Author

mohammadfawaz commented Aug 12, 2022 •
edited
Loading

This is nearly done but needs some tests. For now, the SDK team can experiment with the JSON file generated - there should be no blockers.

The old JSON file is still there so that nothing breaks just yet... the new JSON is called <proj_name>-flat-abi.json.

@mohammadfawaz mohammadfawaz requested a review from a team August 15, 2022 19:45
@mohammadfawaz mohammadfawaz marked this pull request as ready for review August 15, 2022 19:45
@mohammadfawaz mohammadfawaz requested review from a team August 15, 2022 19:45
@mohammadfawaz
Copy link
Contributor Author

mohammadfawaz commented Aug 16, 2022 •
edited
Loading

Need to fix something related to the (in)stable ordering of the output. Turning into a draft temporarily.

@mohammadfawaz mohammadfawaz marked this pull request as draft August 16, 2022 01:30
@mohammadfawaz mohammadfawaz marked this pull request as ready for review August 16, 2022 02:27
@luizstacio
Copy link
Member

luizstacio commented Aug 16, 2022 •
edited
Loading

@mohammadfawaz On the examples, we created, typeArguments was an object which looked cleaner by just having the typeArguments: [4, 5].

What are the motivations for having the object instead?

@mohammadfawaz
Copy link
Contributor Author

@mohammadfawaz On the examples, we created, typeArguments was an object which looked cleaner by just having the typeArguments: [4, 5].

What are the motivations for having the object instead?

Good question! It turns out that this is not enough. A type argument can be complex such as MyStruct<MyEnum<str[4]>> for example. Here, MyEnum<str[4]> has to be a JsonTypeApplication and not a JsonTypeDeclaration, and it's not enough to pass the ID of MyEnum<T> for example.

emilyaherbert
emilyaherbert approved these changes Aug 17, 2022
View reviewed changes
Copy link
Contributor

@emilyaherbert emilyaherbert left a comment

Choose a reason for hiding this comment

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

so beautiful... so flat! 🙌

In a future PR, let's make sure to write a blurb for the book documenting the new design of the ABI.

forc-pkg/src/pkg.rs
storage_slots,
tree_type,
};
Ok((compiled, source_map))
}

/// Standardize the JSON ABI data structure by eliminating duplicate types. This is an iterative
/// process because every time two types are merged, new opportunities for more merging arise.
fn standardize_json_abi_types(json_abi_program: &mut JsonABIProgram) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch, I didn't think about this possibility... it makes me want to implement this in the TypeEngine 😄

@emilyaherbert emilyaherbert requested a review from a team August 17, 2022 21:32
@mohammadfawaz mohammadfawaz mentioned this pull request Aug 17, 2022
Closed
@mohammadfawaz
Copy link
Contributor Author

In a future PR, let's make sure to write a blurb for the book documenting the new design of the ABI.

100%. I will start by adding the new format to the spec (FuelLabs/fuel-specs#401) and then go from there.

digorithm
digorithm approved these changes Aug 18, 2022
View reviewed changes
Copy link
Member

@digorithm digorithm left a comment

Choose a reason for hiding this comment

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

Approving on behalf of the SDK team (and regarding SDK/ABI-related stuff only); All tests with the new experimental fuels-rs using this new flat-abi.json outputted from a local copy of this branch (including sway's SDK harness tests) are passing!

@mohammadfawaz mohammadfawaz merged commit f9ecd20 into master Aug 18, 2022
@mohammadfawaz mohammadfawaz deleted the mohammadfawaz/2386 branch August 18, 2022 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@segfault-magnet segfault-magnet segfault-magnet left review comments

@emilyaherbert emilyaherbert emilyaherbert approved these changes

@sezna sezna sezna approved these changes

@digorithm digorithm digorithm approved these changes

Assignees

@mohammadfawaz mohammadfawaz

Labels
ABI Everything to do the ABI, especially the JSON representation compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen compiler: parser Everything to do with the parser P: critical Should be looked at before anything else
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Generalize the handling of generics in JSON ABI
6 participants
@mohammadfawaz @luizstacio @digorithm @sezna @emilyaherbert @segfault-magnet