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

Implement trait support - Part 1/3 - New ink! IR #451

Merged
merged 260 commits into from
Jul 2, 2020

Conversation

Robbepop
Copy link
Collaborator

@Robbepop Robbepop commented Jun 14, 2020

Implements & closes #441.

tl;dr

The PR packs all of the new definitions into a new ink! crate, called ink_lang_ir.
This new crate provides abstractions to parse, analyze and inspect ink! smart contract code on an IR level.
Other utilities, such as the planned new ink! codegen can then depend on ink_lang_ir and use its API to do their jobs.

A rough outline of the API surface is shown in the screenshot below:

2020-06-28-202734_1069x1163_scrot

ToDo List

  • Write unit tests for conflicting ink! attributes for:
    • Event
    • Storage
    • Message
    • Constructor
  • Implement interface for composed selectors.
    • Avoid accessing user provided selector. (This is just confusing anyways.)
    • Integrate composed selector access with iteration over impl block messages and constructors.
    • Write tests for composed selector access through this iteration interface.
  • Rename salt to namespace.
  • Implement check to ensure no selectors overlap
    • Implement check
    • Implement tests
    • Add additional test for selectors of different trait impls with same name methods.
  • Make all in-doc code examples compile.
  • Implement selector computation that treats trait implementation blocks different from inherent implementation blocks.
    • Implement unit tests.
  • Implement event topics.
    • Write failure unit tests for event topics.
    • Write unit tests for event fields iterator.
    • Register which fields are topic fields.
    • Provide iterator that yields (&Field, bool) where if the bool is true the &Field is a topic field.
  • Implement TryFrom<syn::Module> for ir2::Module.
    • Implement success unit tests.
    • Implement failure unit tests.
      • Missing storage struct
      • Too many storage structs
      • Missing constructor
      • Missing message
      • Invalid out-of-line module
      • Conflicting ink! attributes (any)
    • Implement check for missing and multiple #[ink(storage)] structs.
    • Implement check for missing #[ink(constructor)].
    • Implement check for missing #[ink(message)].
  • Implement Constructor and Message checks from within ir2::ItemImpl.
    • Check that Message of inherent ItemImpl has public visibility.
    • Check that Message of trait ItemImpl has inherited visibility. (Maybe not needed.)

In order to sanely implement traits we have to change the ink!'s IR to no longer be message & constructor focused but implementation block centric. That way we can also generate code that is more similar to the original user input. Also this allows to have more fine grained control over the output of ink! as a user.

By rewriting the ink! IR we are trying to find a design and implementation that suits other issues in the issue tracker, e.g. being able to apply multiple ink! markers to entities, e.g.:

#[ink(message)]
#[ink(payable)]
fn my_message(&self) { ... }

// or shorthand
#[ink(message, payable)]
fn my_message(&self) { ... }

Main Challenge & Problem

The main challenge behind ink!'s trait support is to somehow enforce on a syntactical level (we are still just a macro) that either none (normal trait implementation) or all methods lead to some properly specified ink! trait implementation.

E.g. when looking at a trait with default members or with super traits it is easy that we could implement a trait like this:

pub trait MyTrait {
    // This needs to be implemented!
    fn required_method(&self);

    // This can be omitted in an implementation.
    fn forward(&self) {
        self.required_method(&self);
    }
}

impl MyTrait for MyContract {
    #[ink(message)]
    fn required_method(&self) { ... }
}

While the above example is safe and ink! would be able to provide proper accessors to it so that users cannot misuse it or do cross-calls that end in nowhere the following example is more involved for ink! (or even impossible to safeguard).

// Might be defined in another crate so ink! doesn't necessarily have access to its contents.
pub trait SuperTrait {
    fn super_foo(&self);
}

// Might be defined in another crate so ink! doesn't necessarily have access to its contents.
pub trait BaseTrait: SuperTrait {
    fn foo(&sefl);
    fn bar(&self) {
        self.super_foo()
    }
}

impl BaseTrait for MyContract {
    #[ink(message)]
    fn foo(&self) { ... }
}

In this example, since ink! only ever has access to the syntactical portions of all items the ink! macro cannot know the internal structure of the SuperTrait or even the BaseTrait and only sees the required BaseTrait::foo method for which it creates the correct code. However, for BaseTrait::bar it will generate incorrect code that calls SuperTrait::super_foo without it being a proper cross-contract call on the chain. This is a huge problem and I currently do not see any solution to this. Since we cannot spot this we cannot even generate a warning in its place. The only potential solution is to require traits to be defined in the same ink! module where they have been implemented which kind of destroys the gains of having them in the first place.

We still think that the above mentioned problem can be fixed if we don't implement non-ink! marked traits for the cross-calling wrappers of a contract. We believe that this way we can again guarantee that all trait implementations will result in a proper cross-contract call.

@Robbepop Robbepop marked this pull request as draft June 16, 2020 17:29
@Robbepop Robbepop force-pushed the robin-implement-trait-support branch from e67b4c3 to eae7db9 Compare June 17, 2020 15:09
@Robbepop Robbepop marked this pull request as ready for review June 27, 2020 14:19
@Robbepop Robbepop changed the title Implement trait support for ink! Implement trait support - Part 1/3 - New ink! IR Jun 27, 2020
@codecov-commenter
Copy link

codecov-commenter commented Jun 27, 2020

Codecov Report

Merging #451 into master will decrease coverage by 2.02%.
The diff coverage is 80.60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #451      +/-   ##
==========================================
- Coverage   86.83%   84.80%   -2.03%     
==========================================
  Files         135      125      -10     
  Lines        5877     5343     -534     
==========================================
- Hits         5103     4531     -572     
- Misses        774      812      +38     
Impacted Files Coverage Δ
core/src/env/call/utils.rs 100.00% <ø> (+55.55%) ⬆️
core/src/env/engine/off_chain/db/exec_context.rs 82.00% <ø> (-0.70%) ⬇️
core/src/env/engine/off_chain/test_api.rs 62.50% <ø> (-7.50%) ⬇️
core/src/env/types.rs 23.52% <ø> (ø)
core/src/storage2/alloc/allocation.rs 100.00% <ø> (ø)
core/src/storage2/alloc/allocator.rs 83.33% <0.00%> (-16.67%) ⬇️
core/src/storage2/alloc/boxed/storage.rs 100.00% <ø> (ø)
core/src/storage2/alloc/boxed/tests.rs 100.00% <ø> (ø)
core/src/storage2/alloc/tests.rs 100.00% <ø> (ø)
core/src/storage2/collections/bitstash/counts.rs 92.30% <ø> (ø)
... and 133 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 30fe10d...bae235d. Read the comment docs.

lang/ir/src/ast/config.rs Outdated Show resolved Hide resolved
lang/ir/src/ast/config.rs Outdated Show resolved Hide resolved
lang/ir/src/ast/mod.rs Outdated Show resolved Hide resolved
lang/ir/src/ir/contract.rs Outdated Show resolved Hide resolved
lang/ir/src/ir/contract.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

WIP 7/30 files

lang/ir/src/ast/config.rs Outdated Show resolved Hide resolved
lang/ir/src/ast/mod.rs Outdated Show resolved Hide resolved
lang/ir/src/ir/attrs.rs Show resolved Hide resolved
lang/ir/src/ir/attrs.rs Outdated Show resolved Hide resolved
lang/ir/src/ir/attrs.rs Outdated Show resolved Hide resolved
lang/ir/src/ir/attrs.rs Outdated Show resolved Hide resolved
lang/ir/src/ir/config.rs Outdated Show resolved Hide resolved
lang/ir/src/ir/item_impl/callable.rs Show resolved Hide resolved
Copy link
Collaborator

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

WIP 12/30

lang/ir/src/ir/contract.rs Outdated Show resolved Hide resolved
lang/ir/src/ir/contract.rs Outdated Show resolved Hide resolved
lang/ir/src/ir/item/event.rs Outdated Show resolved Hide resolved
lang/ir/src/ir/item/storage.rs Outdated Show resolved Hide resolved
lang/ir/src/ir/item/storage.rs Outdated Show resolved Hide resolved
lang/ir/src/ir/item/storage.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

30/30. Left some comments - looking good

lang/ir/src/ir/item_impl/mod.rs Outdated Show resolved Hide resolved
lang/ir/src/ir/item_impl/mod.rs Outdated Show resolved Hide resolved
lang/ir/src/ir/item_impl/callable.rs Show resolved Hide resolved
lang/ir/src/ir/item_impl/constructor.rs Outdated Show resolved Hide resolved
lang/ir/src/ir/item_mod.rs Outdated Show resolved Hide resolved
lang/ir/src/ir/item_mod.rs Outdated Show resolved Hide resolved
lang/ir/src/ir/item_mod.rs Show resolved Hide resolved
Copy link
Collaborator

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

LGTM

@Robbepop Robbepop force-pushed the robin-implement-trait-support branch from afc5436 to 5e4b6ae Compare July 2, 2020 11:40
Copy link
Collaborator

@cmichi cmichi left a comment

Choose a reason for hiding this comment

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

LGTM!

@Robbepop Robbepop merged commit c24f0ff into master Jul 2, 2020
@Robbepop Robbepop deleted the robin-implement-trait-support branch July 2, 2020 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Design Proposal: Trait Support
4 participants