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

Print ast to Solidity Source Code #201

Open
wants to merge 30 commits into
base: main
Choose a base branch
from

Conversation

xianlinc
Copy link

Introduced a new package called ast_printer to print ast.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:

func printRoot(root *ast.RootNode) {
	str, ok := ast_printer.Print(root.GetSourceUnits()[0])
	if !ok {
		zap.L().Error("Failed to print root")
	}
	fmt.Println(str)
}

@@ -438,5 +438,5 @@ func (p *Parameter) getStorageLocationFromCtx(ctx *parser.ParameterDeclarationCo
}
}

return ast_pb.StorageLocation_MEMORY
return ast_pb.StorageLocation_DEFAULT
Copy link
Author

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.

Copy link
Author

Choose a reason for hiding this comment

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

We print "" on Default

Copy link
Contributor

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
Copy link
Author

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;

Copy link
Contributor

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 🎃

Copy link
Contributor

@0x19 0x19 left a 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))
Copy link
Contributor

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.

Copy link
Author

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
Copy link
Contributor

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
Copy link
Contributor

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()
Copy link
Contributor

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()
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

printer/ast_printer/ast_printer.go Show resolved Hide resolved
printer/ast_printer/ast_printer.go Show resolved Hide resolved
@0x19 0x19 added enhancement New feature or request ast Abstract Syntax Tree labels Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ast Abstract Syntax Tree enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants