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

feat: enhance asm functions to support all* of the Fift-asm syntax #855

Merged
merged 7 commits into from
Sep 24, 2024

Conversation

novusnota
Copy link
Member

@novusnota novusnota commented Sep 20, 2024

Note, that while it's possible to support the syntax via Ohm without constucting ASTs, it's impossible to support the AST generation without outsourcing that part of the parser to the recursive-descent one, which would also keep track of dictionary of words/instructions and the TVM stack, at least approximating it. See this for more info.

P.S.: all* in the title means that we support everything, but with two caveats:

  1. Out of place } and { (when shadowing them, for example) will break the parsing. However, the non-shadowing uses, lists { } and any words containing } inside are allowed as long as they are not just a single }.
  2. Usage of newly created words that use word may not parse well, as they might contain the } inside of them separated by whitespace, which means that it's another variant of the 1st caveat.

P.P.S: Essentially, our parser only has one flaw, which stems from { } being used as a function block. If we were to use, say, """ or {{{, the problem would kind of mitigate itself. Just until someone defines the """ or {{{ as a new word in their Fift program, and then we (and FunC), would inevitably fail to parse it. Until we have a stack-keeping parser, that is.

Issue

Closes #857.

Checklist

  • I have updated CHANGELOG.md
  • I have added tests to demonstrate the contribution is correctly implemented: this usually includes both positive and negative tests, showing the happy path(s) and featuring intentionally broken cases
  • I have run all the tests locally and no test failure was reported
  • I have run the linter, formatter and spellchecker
  • I did not do unrelated and/or undiscussed refactorings

Note, that while it's possible to support the syntax via Ohm without
constucting ASTs, it's impossible to support the AST generation without
outsourcing that part of the parser to the recursive-descent one, which
would also keep track of dictionary of words/instructions and the TVM
stack, at least approximating it. [See this for more
info](#837 (comment)).
@novusnota novusnota requested a review from a team as a code owner September 20, 2024 03:45
@novusnota novusnota changed the title feat: enhance asm functions to support all of the Fift-asm syntax feat: enhance asm functions to support all* of the Fift-asm syntax Sep 21, 2024
The important test with arbitrary words/instructions containing `}` in
them was needed
Copy link
Member

@anton-trunov anton-trunov left a comment

Choose a reason for hiding this comment

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

Wait, this PR definitely does not close the linked issue as there is no AST model of asm-instructions.

src/grammar/grammar.ohm Outdated Show resolved Hide resolved
@anton-trunov anton-trunov self-assigned this Sep 23, 2024
@anton-trunov anton-trunov added this to the v1.6.0 milestone Sep 23, 2024
Co-authored-by: Anton Trunov <anton@ton.org>
@novusnota
Copy link
Member Author

novusnota commented Sep 23, 2024

Changed the linked issue to #857. I'm willing to return to #837 later, preferably when we start moving to Tact 2.0 and a new parser for it. Alternatively, can tackle it after other remaining issues on 1.6.0 milestone for me to resolve (both here and for docs)

P.S.: On the positive side of #837, our asm ASTs will be typechecked right away :)

Copy link
Member

@anton-trunov anton-trunov left a comment

Choose a reason for hiding this comment

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

We should definitely add more positive tests from some known Fift-projects.

Can we parse at least some part of Asm.fif?

Some examples can be taken from this list: https://docs.ton.org/develop/smart-contracts/examples#fift-smart-contracts and this list: https://github.com/ton-blockchain/ton/tree/master/crypto/smartcont.

And apply a small fix to the parser — we cannot prohibit shadowing or
removal of instructions because both are used in Fift library and
beyond.

However, that means that out of place `{` and `}` are possible when
shadowing them. See the comment in `embed-fift-fif.tact` about such
case, and why it's ok to move forward with it
@novusnota
Copy link
Member Author

novusnota commented Sep 23, 2024

@anton-trunov added Fift library to the test suite and made sure that we don't create unnecessary snapshots for its contents

Copy link
Member

@anton-trunov anton-trunov left a comment

Choose a reason for hiding this comment

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

Amazing job!

@anton-trunov anton-trunov merged commit e67f431 into main Sep 24, 2024
17 checks passed
@anton-trunov anton-trunov deleted the closes-837 branch September 24, 2024 05:52
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.

Current asm functions parser does not allow the full range of Fift-asm syntax
2 participants