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

Correct handling string return values #1292

Merged
merged 4 commits into from
Apr 21, 2022
Merged

Conversation

mohammadfawaz
Copy link
Contributor

@mohammadfawaz mohammadfawaz commented Apr 18, 2022

Two things:

  • Fixing the size of a String.
  • Treating Strings of size 1 word or less as copy types. We might have to do the same for structs and other complex types when they're small enough to fit in registers.

The following contract, for example, now returns the correct receipts and can be tested from the SDK.

contract;

abi MyContract {
    fn get_small_string() -> str[8];
    fn get_large_string() -> str[9];
}

impl MyContract for Contract {
    fn get_small_string() -> str[8] {
        let my_string: str[8] = "gggggggg";
        my_string
    }
    fn get_large_string() -> str[9] {
        let my_string: str[9] = "ggggggggg";
        my_string
    }
}

The SDK has a test similar to the above that should be re-enabled (cc @digorithm). For now, I can't really check this from a script because I can't check what was returned from the contract call against a literal String using an assert. Relevant: #1291

@mohammadfawaz mohammadfawaz added bug Something isn't working compiler General compiler. Should eventually become more specific as the issue is triaged labels Apr 18, 2022
@mohammadfawaz mohammadfawaz self-assigned this Apr 18, 2022
@mohammadfawaz mohammadfawaz requested review from otrho April 18, 2022 12:17
@mohammadfawaz mohammadfawaz marked this pull request as draft April 18, 2022 12:37
@@ -20,7 +20,7 @@ fn main() -> bool {
assert(!is_reference_type::<byte>());
assert(!is_reference_type::<u64>());

assert(is_reference_type::<str[1]>());
assert(!is_reference_type::<str[1]>());
Copy link
Contributor Author

@mohammadfawaz mohammadfawaz Apr 19, 2022

Choose a reason for hiding this comment

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

If we go with this, then we should probably make it clear, in the language, that types that "fit in a word" are always copy types. That includes things like str[1], struct { u64 }, struct { struct { u64 } }, array[1], and so on. It is a bit weird, but I think this would make the specs a bit simpler where the optimization that we've been talking about is actually baked in the language and has to happen.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you're right. We'd absolutely need regression tests to enforce it for all our types too.

@mohammadfawaz mohammadfawaz marked this pull request as ready for review April 19, 2022 11:12
@@ -20,7 +20,7 @@ fn main() -> bool {
assert(!is_reference_type::<byte>());
assert(!is_reference_type::<u64>());

assert(is_reference_type::<str[1]>());
assert(!is_reference_type::<str[1]>());
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you're right. We'd absolutely need regression tests to enforce it for all our types too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working compiler General compiler. Should eventually become more specific as the issue is triaged
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants