php-parser icon indicating copy to clipboard operation
php-parser copied to clipboard

AST type inconsistencies

Open deakjahn opened this issue 3 years ago • 0 comments

I encountered the following source discrepancies while moving my AST->PHP writer from any types to strict typing. They are tolerable in JavaScript but cause problems in a type safe TypeScript:

  • continue and break aren't consistent in their level, one uses a plain number while the other a Number extended from Node.
  • empty extends Expression and uses an expression property but it isn't actually defined as such (empty.js fails to mention the property, consequently, it isn't generated into types.d.ts).
  • isset is the same with the variables property,
  • print is the same with the expression property,
  • unset is the same with the variables property.
  • namedargument is lowercase, all other node types have PascalCase names.
  • String normally contains the whole string, with the appropriate quotes, as dictated by isDoubleQuote. However, EncapsedPart can also contain a String fragment and that has no quotes of its own. Although I can work around this, the perfectly correct solution would require either an isQuoted property for String or a new node type like EncapsedString to be used just here.
  • Repeated case labels are stored rather problematically, they are empty Case branches with no relation to the actual branch they belong to. They should be consolidated into a single Case instead, with a test property of Expression[] instead of a single expression.
  • Property and Constant both are supposed to have a name of simple type string. In reality, they receive an Identifier.
  • UseGroup defines an item property but actually populates one named items.
  • The type field of Function can be null, too.

And something not actually an error but a very needed, very small addition: Node should have a parent property and it should be populated when the AST is generated. It's of paramount importance for any tree manipulation. I can also work around this by using my additional union type and populating it during my own traversal but it would be a very cheap addition to the core.

deakjahn avatar Sep 23 '22 13:09 deakjahn