stephens2424 / php

Parser for PHP written in Go
BSD 3-Clause "New" or "Revised" License
529 stars 77 forks source link

Inconsistent parsing of function calls #13

Open jochil opened 9 years ago

jochil commented 9 years ago

Hi, I think I have found a problem with the parsing of simple function calls. Given the following code:

<?php
sort($myArray);

sort() will be parsed to ast.ExpressionStmt. If you use a the function call in an assignment like this:

<?php
$ok = sort($myArray);

sort() will be parsed as ast.FunctionCallExpression. Maybe I missed some point but it seems a bit confusing. If this is a real bug I could fix it by myself... I would appreciate some hints :)

stephens2424 commented 9 years ago

So I think what's going on here is that ast.ExpressionStmt is hiding a real type from you. In the first case, it's hiding *ast.FunctionCallExpression and in the second case it's hiding *ast.AssignmentExpression. So that's why you don't see the function call node in the first case.

Even if this is the case, it's still producing ASTs that are difficult to print and follow. I don't remember why these things are getting wrapped in that type, so if you want to look into making this print better, I'd be happy to help.

Honestly, it might just be that the parseStmt function returns an ast.Statement. That was defined very early on in the project and could certainly be revisited.

Perhaps the way to go would be to put an unexported method in Statement and define that method on all AST types that can stand alone as a statement and do the same thing for Expression. The two almost totally overlap...but not quite. I'm not sure this is the right way to go. I'd have to see how more or less "smelly" the code seems afterward, but you could try it. I'm open to other ideas as well.

stephens2424 commented 9 years ago

Oh, an added benefit of that idea: one of my thoughts with how the ast package is organized: if you can write it as a struct literal, it should equate to valid PHP. Adding the internal requirement to ast.Statement and ast.Expression would get the package closer to that ideal.