Parse catch multiple exceptions #107
Conversation
There was a problem hiding this comment.
@runz0rd You're on a roll! 🚀
Left a few comments, and +1 after that!
|
|
||
| catch (BadMethodCallException | InvalidArgumentException $e) { | ||
| echo "L0: In catch-block multiple Exception\n"; | ||
| } |
There was a problem hiding this comment.
Were these changes intentional? (this file is autogenerated from the PHP spec tests)
There was a problem hiding this comment.
Some of the grammar tests from this file were failing. This was intentionally added to test if a valid case would pass. Want this one removed?
There was a problem hiding this comment.
Yep, let's revert this file but just make sure that there are handwritten test cases added for this scenario (which it looks like you already added). The spec tests are generated using this script and will be overwritten whenever we take updates from php-langspec. Original source here.
| return $this->parseDelimitedList( | ||
| DelimitedList\QualifiedNameList::class, | ||
| TokenKind::CommaToken, | ||
| $delimiter, |
There was a problem hiding this comment.
Oh, I see what you meant in the delimited list PR - yeah, I don't think this is a super common case to handle, so I think it makes sense to special case this usage (or come up with some other idea that doesn't add additional memory usage.)
src/DiagnosticsProvider.php
Outdated
| @@ -79,6 +79,17 @@ public static function getDiagnostics($node) : \Generator { | |||
| } | |||
| } | |||
| } | |||
| elseif ($node instanceof Node\CatchClause) { | |||
| $lastElement = \end($node->qualifiedNameList->children); | |||
| if($lastElement instanceof Token && $lastElement->kind == TokenKind::BarToken) { | |||
There was a problem hiding this comment.
nit: space between if and paren here and everywhere else. sorry, we don't have automatic code style checking yet #83
src/Parser.php
Outdated
| $catchClause->qualifiedNameList = $this->parseQualifiedNameList($catchClause, TokenKind::BarToken); | ||
| if(\is_null($catchClause->qualifiedNameList)) { | ||
| $catchClause->qualifiedNameList = new MissingToken(TokenKind::QualifiedName, $this->getCurrentToken()->fullStart); | ||
| } |
| @@ -1863,14 +1863,18 @@ private function parseCatchClause($parentNode) { | |||
| $catchClause->parent = $parentNode; | |||
| $catchClause->catch = $this->eat(TokenKind::CatchKeyword); | |||
| $catchClause->openParen = $this->eat(TokenKind::OpenParenToken); | |||
| $catchClause->qualifiedName = $this->parseQualifiedName($catchClause); // TODO generate missing token or error if null | |||
| $catchClause->qualifiedNameList = $this->parseQualifiedNameList($catchClause, TokenKind::BarToken); | |||
There was a problem hiding this comment.
Let's also add a new test case to handle this scenario.
Cool idea, and probably best left as an exercise to the consumer 😉. For that, we'd need to resolve imports, build a symbol table, etc., which are more "language service-y" things to do. cc @felixfbecker |
src/Parser.php
Outdated
| @@ -1863,14 +1863,18 @@ private function parseCatchClause($parentNode) { | |||
| $catchClause->parent = $parentNode; | |||
| $catchClause->catch = $this->eat(TokenKind::CatchKeyword); | |||
| $catchClause->openParen = $this->eat(TokenKind::OpenParenToken); | |||
| $catchClause->qualifiedName = $this->parseQualifiedName($catchClause); // TODO generate missing token or error if null | |||
| $catchClause->qualifiedNameList = $this->parseQualifiedNameList($catchClause, TokenKind::BarToken); | |||
| if(\is_null($catchClause->qualifiedNameList)) { | |||
There was a problem hiding this comment.
using === null is a lot faster and does the same
|
I agree that checking the class extends |
|
Hey @runz0rd, this looks ok to me - sorry it fell through the cracks. If you're still up for it, I would accept it if you can get it merged with the latest stuff in master. Also, where did the |
@roblourens those look like the raw php language spec test format. The files in If upstream changes happen in php/php-langspec, they can be pulled in using git subtree. Then run |
|
Got it, thanks! |
|
This will be handled in #244, and now would be a breaking change. |
For #103
Theres still the issue of detecting class names that do not implement Throwable. Any idea where can this fit in?