Page MenuHomePhabricator

Correctly support try..catch..finally in xhpast
ClosedPublic

Authored by richardvanvelzen on Aug 22 2014, 12:50 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Apr 19, 2:08 AM
Unknown Object (File)
Fri, Apr 19, 2:08 AM
Unknown Object (File)
Fri, Apr 19, 2:08 AM
Unknown Object (File)
Fri, Apr 19, 2:08 AM
Unknown Object (File)
Fri, Apr 19, 2:08 AM
Unknown Object (File)
Fri, Apr 19, 2:08 AM
Unknown Object (File)
Thu, Apr 11, 10:46 AM
Unknown Object (File)
Thu, Apr 11, 3:31 AM
Tokens
"Doubloon" token, awarded by epriestley.

Details

Summary

Ref T4334. The catch clauses are optional when a finally is available.

Test Plan

run xhpast with some files. Also added unit tests

Diff Detail

Repository
rPHU libphutil
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

richardvanvelzen retitled this revision from to Correctly support try..catch..finally in xhpast.
richardvanvelzen updated this object.
richardvanvelzen edited the test plan for this revision. (Show Details)
richardvanvelzen added a reviewer: epriestley.

Note that the token constant values have changed because I built with Bison 3.0.2 instead of 2.7

epriestley edited edge metadata.

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 revision now requires changes to proceed.Nov 23 2015, 3:53 PM

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.

richardvanvelzen edited reviewers, added: joshuaspence; removed: richardvanvelzen.

Let me take this back for a bit.

richardvanvelzen edited edge metadata.

Implement the restriction via the grammar

richardvanvelzen edited edge metadata.

Make sure to not skip staging (sorry about that)

epriestley edited edge metadata.

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
592

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.

This revision is now accepted and ready to land.Mar 17 2016, 1:09 PM

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
592

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.)

This revision was automatically updated to reflect the committed changes.

(The n_CATCH_LIST was previously always-nonempty, right? … )

Hmm yes, that is true. It's hard to think of a real-world scenario in which it should matter though.