solidity-parser / parser

A Solidity parser for JS built on top of a robust ANTLR4 grammar
MIT License
157 stars 44 forks source link

Parameter in `FunctionCall` to resolve the signature of function being called #64

Closed MatthewEthanTam closed 2 years ago

MatthewEthanTam commented 2 years ago

Feature Description

Adding a parameter called methodFullName in FunctionCall that will allow resolution of the function's type and will contain the signature of the function.

Why I need this:

I am currently creating a static analysis tool using code property graphs and would like to resolve the type of the function being called, I will also be using ConsenSys/Surya with this feature to add to both Surya and Parser tools

Example usage:

parser.visit(ast, {
            FunctionCall: function FunctionCall(node) {
                if (node.expression.typeName != undefined) {
                    if (node.expression.typeName.name  != null) {
                        node.methodFullName = node.expression.typeName.name 
                    } else {
                        node.methodFullName = null
                    }
                    if (node.arguments.length != 0) {
                        var args = "";
                        args = node.arguments[0].name;
                        for (var i = 1; i < node.arguments.length; i++) {
                            args = ","+node.arguments[i].name;
                        }
                        node.methodFullName += "("+args+")"

                    }
                } else if (node.expression.name != undefined) {
                    if (functionsPerContract[contractName][node.expression.name]) {
                    node.methodFullName = functionsPerContract[contractName][node.expression.name]
                    } else {
                        node.methodFullName = null
                    }
                } else {
                    node.methodFullName = null
                }
            }

        }); 
fvictorio commented 2 years ago

Hi @MatthewEthanTam, thanks for opening this. I don't think I understand exactly what would be the value of methodFullName. For example, for a function call like token.transfer(address, 12345), what would be the value of that property?

fvictorio commented 2 years ago

This also seems like something that can be done by the consumer code with a helper function. I'm not against adding redundant properties, but I would also prefer to avoid it if possible. Can you explain why it makes more sense to have it in the parser itself?

MatthewEthanTam commented 2 years ago

Hi @fvictorio for the example of token.transfer(address, 12345), the methodFullName would be token.transfer:void(address, uint256), this would help with understanding the properties of the FunctionCall, because currently there is no way to see the properties of the function unless you do another parse through the AST, so we wouldn't know by the AST that transfer is a void function with parameters of address and uint256. And yes this can be done with consumer code of a helper function, it would be nice to keep it in the AST so that a consumer could populate it with the relevant information of the call if necessary.

But I understand If it isn't added to the parser. I am using it in my static analysis tool and just wanted to see if it could be helpful for others to use.

MatthewEthanTam commented 2 years ago

I think its benificial to add it to the parser itself because you can populate the methodFullName right after calling parser.parse, by using parser.visit.

fvictorio commented 2 years ago

Hey @MatthewEthanTam, thanks for the explanation.

I think this shouldn't be part of the parser though. The reason is that this is adding semantic information to the AST, which I believe is not the place to have it (I'm not an expert though). Plus, if I understand correctly, this means doing some type resolution work (in my previous example, you need to know the type of token), which is hard and out of scope for the parser.

Hope you don't mind! And please let me know if you disagree or if I'm not understanding the feature request correctly.

MatthewEthanTam commented 2 years ago

Hey @fvictorio, Thanks for the consideration and I totally understand!