glayzzle / glayzzle/php-parser

AST type inconsistencies

Open
#1,008 0 comments 5 reactions 0 assignees View on GitHub
Dominant language
JavaScript
Stars
565
Forks
75
PR merge metrics
No merged PRs in 30d

Description

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.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.