-
Notifications
You must be signed in to change notification settings - Fork 24
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
base: master
Are you sure you want to change the base?
Conversation
…hChildren conflicts
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.
…rsions as deprecated
Simplifies checking if an object is an instance of multiple classes
There was a problem hiding this 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.
/** | ||
* Temporary workaround | ||
*/ | ||
readonly yulIdStart = 1e5; | ||
lastYulId = this.yulIdStart; |
There was a problem hiding this comment.
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:
- It would be easier to keep it clean, as Solidity AST will not conflict with Yul AST references ever.
- There would would be no need to have hacks like
1e5
, as ID space is separated. ASTReader
andASTNodeFactory
would have to track two different AST contexts. MaybeYulNode
s would also be affected if they refer to anyASTNode
s 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.
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
make
andcopy
support to ASTNodeFactorySrcDesc
outputs.TODO
YulBlock
,YulForLoop
,YulFunctionDefinition
as scopes and recognizeYulVariableDeclaration
andYulFunctionDefinition
as definitions.sptr.slot, sptr.offset
),(>=0.7.5cdptr.offset, cdptr.length
), (>=0.8.10extfnvar.address, extfnvar.selector
), (<0.7.0sptr_slot, sptr_offset
) and disambiguate with identifiers containing a dot (allowed in >0.5.7 <0.7.0)Tests