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

Allow script main fns to have args #2592

Merged
merged 10 commits into from
Sep 2, 2022
Merged

Allow script main fns to have args #2592

merged 10 commits into from
Sep 2, 2022

Conversation

AlicanC
Copy link
Contributor

@AlicanC AlicanC commented Aug 19, 2022

This PR adds support for main args:

script;

struct TestStruct {
    val: u64,
}

fn main(baba: TestStruct, keke: u64) -> u64 {
    baba.val + keke
}

TODO:

  • This PR needs GTF so it might get blocked by Refactor std::tx to use gtf intrinsic #2482 if other items are finished before it lands on master. When it lands, the incomplete GTF support commit in this PR should be removed.
  • Predicate support.
  • Checking if the length of script data is correct. Removed.
  • Implement copying, test mutating the args. Disallow ref mut args in main fns.
  • Docs. Postponed.
  • Test w/ enums, strings, arrays

@AlicanC AlicanC added enhancement New feature or request compiler General compiler. Should eventually become more specific as the issue is triaged language feature Core language features visible to end users blocked labels Aug 19, 2022
@AlicanC AlicanC self-assigned this Aug 19, 2022
@AlicanC AlicanC linked an issue Aug 19, 2022 that may be closed by this pull request
@AlicanC AlicanC removed the blocked label Aug 19, 2022
@AlicanC AlicanC force-pushed the jc/script-main-args branch 2 times, most recently from 14bb605 to 6a862a5 Compare August 31, 2022 19:27
@AlicanC AlicanC force-pushed the jc/script-main-args branch from 6a862a5 to a54dbc3 Compare August 31, 2022 19:35
@AlicanC AlicanC marked this pull request as ready for review August 31, 2022 19:56
@AlicanC AlicanC requested a review from a team August 31, 2022 19:57
@mohammadfawaz
Copy link
Contributor

Do you mind adding a small section in the Sway book about this? Potentially here. We basically should mention that main() does accept arguments, that those arguments cannot be mutable, and where those arguments live (in script data, maybe a link to the specs?). Also, is there a way to exercise this feature today using forc? Or nothing can be done without support from the SDK?

Copy link
Contributor

@mohammadfawaz mohammadfawaz left a comment

Choose a reason for hiding this comment

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

Other than my comments about script data and other things it may contain, this looks great! I like the reuse of code.

Regarding testing, I'd feel a bit more confident with the change if you can add examples that use other complex data types such as enums, strings, and arrays.

sway-core/src/asm_generation/from_ir.rs Outdated Show resolved Hide resolved
sway-core/src/error.rs Outdated Show resolved Hide resolved
sway-core/src/asm_generation/from_ir.rs Outdated Show resolved Hide resolved
Co-authored-by: Mohammad Fawaz <[email protected]>
@AlicanC

This comment was marked as outdated.

@AlicanC
Copy link
Contributor Author

AlicanC commented Sep 1, 2022

Do you mind adding a small section in the Sway book about this? Potentially here. We basically should mention that main() does accept arguments, that those arguments cannot be mutable, and where those arguments live (in script data, maybe a link to the specs?). Also, is there a way to exercise this feature today using forc? Or nothing can be done without support from the SDK?

Both SDKs have a way of providing script data, but (at least on the TS side) the user is responsible of encoding both the args and the return value. I could doc this mentioning the script_data field but it will be confusing when SDKs eventually start managing this for the user. Maybe we should wait for SDK support to doc this. (Or I could just add some args to the example main() to signal support without mentioning how to get the data there.)

sezna
sezna previously approved these changes Sep 1, 2022
Copy link
Contributor

@sezna sezna left a comment

Choose a reason for hiding this comment

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

lgtm, although we should make an issue regarding avoiding the len check in bytecode when possible

@AlicanC AlicanC force-pushed the jc/script-main-args branch from 2077aa6 to e74ceeb Compare September 1, 2022 23:04
@AlicanC
Copy link
Contributor Author

AlicanC commented Sep 1, 2022

So I've:

What should I do with:

  • Docs? Leave it for another PR when SDKs support script args?
  • Length checking? Remove and let the SDKs handle it?

@AlicanC AlicanC requested a review from sezna September 1, 2022 23:35
@sezna
Copy link
Contributor

sezna commented Sep 2, 2022

Yeah we can postpone docs for now. Will circle back on the runtime check...

edit: circling back:
from @adlerjohn:

We could just err on assuming the script data is valid and not check it at all

So let's remove the check.

@sezna sezna merged commit d083ea6 into master Sep 2, 2022
@sezna sezna deleted the jc/script-main-args branch September 2, 2022 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler General compiler. Should eventually become more specific as the issue is triaged enhancement New feature or request language feature Core language features visible to end users
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Allow access of script data via arguments in the main function
3 participants