-
Notifications
You must be signed in to change notification settings - Fork 0
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
Print ast
to Solidity Source Code
#201
base: main
Are you sure you want to change the base?
Conversation
@@ -438,5 +438,5 @@ func (p *Parameter) getStorageLocationFromCtx(ctx *parser.ParameterDeclarationCo | |||
} | |||
} | |||
|
|||
return ast_pb.StorageLocation_MEMORY | |||
return ast_pb.StorageLocation_DEFAULT |
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.
I changed this from Memory
to Default
because we needed a way to print the storage location "memory" only when it is specified in the original antlr ast.
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.
We print "" on Default
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.
Good catch!
@@ -22,6 +22,7 @@ type StateVariableDeclaration struct { | |||
Visibility ast_pb.Visibility `json:"visibility"` // Visibility of the state variable declaration | |||
StorageLocation ast_pb.StorageLocation `json:"storage_location"` // Storage location of the state variable declaration | |||
StateMutability ast_pb.Mutability `json:"mutability"` // State mutability of the state variable declaration | |||
Override bool `json:"is_override"` // Indicates if the state variable is an override |
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.
Introduced an Override for StateVariableDeclaration, eg address public override owner;
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.
Nice one, I missed that one 🎃
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.
It's all going nice way! Thank you a lot man!
Some of the things that I believe needs to be a part of this PR:
- Documentation in general, it's lacking.
- There are no unit tests, please at least add few of them. Does not have to be 100% code coverage even tho 80+ is preferred. We at least need to have for example 1-2 contracts, a table driven test that produces expected results back.
@@ -186,6 +186,9 @@ func (i *IfStatement) Parse( | |||
body.ParseBlock(unit, contractNode, fnNode, statementCtx.Block()) | |||
break | |||
} | |||
// Edge case for single statement if | |||
// Eg: if (a) b; | |||
body.parseStatements(unit, contractNode, fnNode, statementCtx.GetChild(0)) |
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.
This too me sounds like that it could have potential side-effects. It should probably be checked if block itself is not nil? Going to test it as well and should for sure, as we started be part of the different PR, not this one.
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.
Will remove this from the PR!
@@ -438,5 +438,5 @@ func (p *Parameter) getStorageLocationFromCtx(ctx *parser.ParameterDeclarationCo | |||
} | |||
} | |||
|
|||
return ast_pb.StorageLocation_MEMORY | |||
return ast_pb.StorageLocation_DEFAULT |
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.
Good catch!
@@ -22,6 +22,7 @@ type StateVariableDeclaration struct { | |||
Visibility ast_pb.Visibility `json:"visibility"` // Visibility of the state variable declaration | |||
StorageLocation ast_pb.StorageLocation `json:"storage_location"` // Storage location of the state variable declaration | |||
StateMutability ast_pb.Mutability `json:"mutability"` // State mutability of the state variable declaration | |||
Override bool `json:"is_override"` // Indicates if the state variable is an override |
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.
Nice one, I missed that one 🎃
@@ -207,6 +208,8 @@ func (v *StateVariableDeclaration) Parse( | |||
v.Constant = constantCtx != nil | |||
} | |||
|
|||
v.Override = ctx.GetOverrideSpecifierSet() |
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.
Could this panic in case that there's no specifier set? basically if not nil to set it?
@@ -270,6 +273,8 @@ func (v *StateVariableDeclaration) ParseGlobal( | |||
v.Constant = constantCtx != nil | |||
} | |||
|
|||
v.Override = ctx.GetOverrideSpecifierSet() |
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.
Same as above
Introduced a new package called
ast_printer
to printast.Node[ast.NodeType]
to solidity source code.Right now, we do not support the printing of all the nodes, and we haven't tested it rigorously yet, but would love to get some feedback on what changes need to be made to integrate this into the codebase 🤩
Heres an example of how you would use this: