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

(WIP) Add support for Yul AST nodes (addresses #163) #166

Open
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

d1ll0n
Copy link
Contributor

@d1ll0n d1ll0n commented Nov 11, 2022

Work in progress, not ready to merge

This is not ready yet, I'm just creating a PR to make it easier to track progress.

Summary

Introduce typed yul AST nodes with the same rich feature-set as solc's AST nodes. For more context, see #163

TODO

Features

Complete

  • Create classes for every Yul AST node type.
  • Add processor files for every Yul AST type to src/ast/modern
  • Add make and copy support to ASTNodeFactory
  • Replace the Yul AST writers with ones based on ASTNodeWriter that produce structured SrcDesc outputs.

TODO

  • Add new scope resolution methods to recognize YulBlock, YulForLoop, YulFunctionDefinition as scopes and recognize YulVariableDeclaration and YulFunctionDefinition as definitions.
  • Add pretty printers in src/types
  • Add types for the builtin assembly functions in src/types
  • Add constant evaluation support
  • Come up with some sort of node to represent member access of the form (>=0.7.0 sptr.slot, sptr.offset),(>=0.7.5 cdptr.offset, cdptr.length), (>=0.8.10 extfnvar.address, extfnvar.selector), (<0.7.0 sptr_slot, sptr_offset) and disambiguate with identifiers containing a dot (allowed in >0.5.7 <0.7.0)

Tests

  • Create specific unit tests for the Yul AST nodes.
  • Remove workarounds introduced in aaf54ed 7949cc6 and update the tests. Will need to prove that the same data is present between versions for the formatter and determine a Yul ID generation methodology acceptable to the team.
  • Create testing framework for scope resolution of Yul AST nodes. We don't have the benefit of solidity's referenced declarations here, so I think we'll need to do constant evaluation and compare to the EVM. Discussion: Add support for Yul AST nodes #163 (comment)

d1ll0n and others added 18 commits November 10, 2022 20:40
It is possible to generate IDs during AST processing by performing a deep search for the last ID in all the original source units; however, this breaks the current tests for the copy method in ASTNodeFactory, which expect hard-coded IDs for specific nodes. Since they are created *after* all of the original source units are processed, and those all have independent contexts, this does not seem like an actual issue or breaking change for consumers. Until the tests are updated or a better mechanism is determined, the current workaround is to offset yul AST nodes by 100000.
Tests use specific string constants to test the formatting behavior. This uses the .raw original AST node to print child yul ast nodes. This still breaks on the copy test because some fields in InlineAssembly print different types now that YulBlock replaced YulNode and .yul is part of children.
@d1ll0n d1ll0n mentioned this pull request Nov 11, 2022
6 tasks
Copy link
Contributor

@blitz-1306 blitz-1306 left a comment

Choose a reason for hiding this comment

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

Great job. Would like to highlight one option.

Comment on lines +50 to +54
/**
* Temporary workaround
*/
readonly yulIdStart = 1e5;
lastYulId = this.yulIdStart;
Copy link
Contributor

@blitz-1306 blitz-1306 Nov 21, 2022

Choose a reason for hiding this comment

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

Another option here is to consider using second standalone ASTContext for Yul nodes...

It seems a bit more clear and sound, however it would have more impact on code overall. Some pros/cons:

  1. It would be easier to keep it clean, as Solidity AST will not conflict with Yul AST references ever.
  2. There would would be no need to have hacks like 1e5, as ID space is separated.
  3. ASTReader and ASTNodeFactory would have to track two different AST contexts. Maybe YulNodes would also be affected if they refer to any ASTNodes as referenced declarations...

Unsure about consequences here. It may look cleaner but may also be impractical in the end. Still, I guess it may worth an attempt to see what would happend.

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.

2 participants