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

Add Result<Self, Error> return type for constructors #1446

Merged
merged 30 commits into from
Oct 27, 2022

Conversation

SkymanOne
Copy link
Contributor

@SkymanOne SkymanOne commented Oct 21, 2022

This PR closes #988.

The PR also includes an example return_err to illustrate the usage:

Addition

If we define storage and error similar to

/* Snippet */
#[ink(storage)]
pub struct ReturnErr {
    count: i32,
}

#[derive(Debug, PartialEq, Eq, scale::Encode, scale::Decode)]
#[cfg_attr(feature = "std", derive(::scale_info::TypeInfo))]
pub enum Error {
    Foo,
}
/* Snippet */

We can now create a constructor that can safely fail without the need to panic!

/* Snippet */
#[ink(constructor, payable)]
// can also return core::result::Result<T, Error> for explicitness
pub fn new2(fail: bool) -> Result<T, Error> {
    if fail {
        Err(Error::Foo)
    } else {
        Ok(Default::default())
    }
}
    /* Snippet */

Alternatively, you can define a type alias for convenience:

 /* Snippet */
pub type Result<T> = core::result::Result<T, Error>;
#[ink(constructor, payable)]
pub fn new2(fail: bool) -> Result<Self> {
    if fail {
        Err(Error::Foo)
    } else {
        Ok(Default::default())
    }
}
/* Snippet */

@SkymanOne SkymanOne marked this pull request as ready for review October 23, 2022 18:45
@codecov-commenter
Copy link

codecov-commenter commented Oct 24, 2022

Codecov Report

Merging #1446 (b5e99fe) into master (4156112) will decrease coverage by 1.67%.
The diff coverage is 45.45%.

❗ Current head b5e99fe differs from pull request most recent head ae37fe6. Consider uploading reports for the commit ae37fe6 to get more accurate results

@@            Coverage Diff             @@
##           master    #1446      +/-   ##
==========================================
- Coverage   70.89%   69.21%   -1.68%     
==========================================
  Files         193      199       +6     
  Lines        6071     6124      +53     
==========================================
- Hits         4304     4239      -65     
- Misses       1767     1885     +118     
Impacted Files Coverage Δ
crates/ink/src/codegen/dispatch/execution.rs 0.00% <0.00%> (ø)
crates/ink/src/reflect/dispatch.rs 0.00% <ø> (ø)
.../ink/tests/ui/contract/pass/constructor-aliases.rs 11.11% <11.11%> (ø)
...i/contract/pass/constructor-return-result-alias.rs 16.66% <16.66%> (ø)
.../ui/contract/pass/constructor-return-result-err.rs 16.66% <16.66%> (ø)
...ontract/pass/constructor-return-result-explicit.rs 16.66% <16.66%> (ø)
...s/ui/contract/pass/constructor-return-result-ok.rs 16.66% <16.66%> (ø)
crates/ink/codegen/src/generator/dispatch.rs 94.54% <50.00%> (-0.41%) ⬇️
...tests/ui/contract/pass/example-return-err-works.rs 80.95% <80.95%> (ø)
crates/ink/ir/src/ir/item_impl/constructor.rs 91.80% <85.71%> (-1.95%) ⬇️
... and 14 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@athei athei left a comment

Choose a reason for hiding this comment

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

We can not create a constructor that can safely fail without the need to panic!

I don't parse this sentence at all. Can you elaborate?

Also, why are we reliant on type aliases here while we aren't for messages? For messages we can just write any Result<T, E> as return type. For constructors you would just stricten that to Result<Self, E> and nothing more.

examples/return_err/lib.rs Outdated Show resolved Hide resolved
examples/return_err/lib.rs Outdated Show resolved Hide resolved
@SkymanOne
Copy link
Contributor Author

We can not create a constructor that can safely fail without the need to panic!

I don't parse this sentence at all. Can you elaborate?

Also, why are we reliant on type aliases here while we aren't for messages? For messages we can just write any Result<T, E> as return type. For constructors you would just stricten that to Result<Self, E> and nothing more.

We aren't relying on type aliases at all. They are just used for convenience because I've seen them used in other examples like like DNS or ERC721. We can use Result<Self, ()> on its own

We can not create a constructor that can safely fail without the need to panic!

This is a typo, should be:

We can now create a constructor that can safely fail without the need to panic!

@athei
Copy link
Contributor

athei commented Oct 24, 2022

This is a typo, should be:

We can now create a constructor that can safely fail without the need to panic!

Ahh lol yeah this really fried my brain.

We aren't relying on type aliases at all. They are just used for convenience because I've seen them used in other examples like like DNS or ERC721. We can use Result<Self, ()> on its own

Good. I just misread the UI test I think.

@SkymanOne
Copy link
Contributor Author

SkymanOne commented Oct 26, 2022

Update

  • Removed syntactical check for constructors from ir
  • There is already a type check in ConstructorReturnType<C> trait. It will only accept either a contract type or Result<C, ...> where C - contract type.
  • Changed the UI tests to test against the new error messages
  • It is also now possible to define a type aliases for return type of a constructor. So these lines are now allowed:
    • type MyType = <Contract Type>;
    • type MyReturnResult = Result<C, Error>;
    • The above defined aliases can be used a constructor return type
  • While the error message is not helpful at all which affects developer experience, I propose to create a follow-up issue to add a lint rule for constructor return type

If we are happy with my changes, I will add more UI tests to cover type aliases.
P.S. Added UI test for type aliases

Copy link
Collaborator

@Robbepop Robbepop left a comment

Choose a reason for hiding this comment

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

Looks a lot better now!

Copy link
Collaborator

@Robbepop Robbepop left a comment

Choose a reason for hiding this comment

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

LGTM

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!

crates/ink/ir/src/ir/item_impl/constructor.rs Outdated Show resolved Hide resolved
examples/return_err/lib.rs Outdated Show resolved Hide resolved
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.

Support Result<Self, E> returning ink! constructors
5 participants