Ref T4334. The catch clauses are optional when a finally is available.
Details
- Reviewers
joshuaspence epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T4334: Support PHP5.4+ syntax in XHPAST
- Commits
- rPHU5f573d2b7f27: Correctly support try..catch..finally in xhpast
run xhpast with some files. Also added unit tests
Diff Detail
- Repository
- rPHU libphutil
- Branch
- finally
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 2330 Build 8471: [Placeholder Plan] Wait for 30 Seconds Build 2334: [Placeholder Plan] Wait for 30 Seconds
Event Timeline
Note that the token constant values have changed because I built with Bison 3.0.2 instead of 2.7
The use of yyerror() explicitly is unusual, and does not seem to come from zend_language_parser.y. I'd rather see this prevented by the grammar itself. It looks like zend_language_parser.y accomplished this purely with grammar rules, at least as of 5.5.14 (the copy of the PHP source I have handy locally).
This is not true - it's handled in zend_compile.c. The grammar allows omitting catch/finally.
I can implement it in the grammar without too much problems. I really need this to work now.
If you didn't already, maybe poke around and see if there's a precedent for n_EMPTY vs n_X_LIST with no members? I think both are reasonable on their face, but suspect we may currently use n_EMPTY in other cases, although maybe there are no other cases or I could be misremembering. But if one variant is more consistent I think that's slightly preferable.
support/xhpast/parser.y | ||
---|---|---|
582 | Maybe this should be n_EMPTY? I don't actually recall which node would be more consistent with other rules. An empty n_CATCH_LIST also seems fine/reasonable in isolation. |
For lists, I generally see the "n_X_LIST with no members" approach.
parameter_list: non_empty_parameter_list | %empty { $$ = NNEW(n_DECLARATION_PARAMETER_LIST); } ;
inner_statement_list: inner_statement_list inner_statement { $$ = $1->appendChild($2); } | %empty { $$ = NNEW(n_STATEMENT_LIST); } ;
… and more alike.
support/xhpast/parser.y | ||
---|---|---|
582 | At least when looking at the left side of this diff, it was an n_CATCH_LIST. I think introducing n_EMPTY on an empty catch list would be a BC break. |
Works for me.
(The n_CATCH_LIST was previously always-nonempty, right? But those %empty examples are compelling to me.)
Hmm yes, that is true. It's hard to think of a real-world scenario in which it should matter though.