BUGFIX: edgecasy parenthesis in expressions #18
Conversation
previously `ExpressionNode::fromString("(foo)")` couldnt be parsed (it worked though when used in return statement of a module)
but the real fix, is that you couldnt write `return ((foo))`
as $tokens is reassigned but not passed by ref
mhsdesign
changed the title
for example ExpressionNode::fromString('(true) ? 42 : "foo"')
would previously only tokenize `(true)` due to the brace
this is not an issue, when used in the return statement
There was a problem hiding this comment.
Hi @mhsdesign,
sorry that I'm taking so long to review your changes. Please know, that I really appreciate the hard work you're putting into this. I only have trouble keeping up with your pace :D
For this particular fix, I think I understand the problem, you're trying to solve. Yet, there's a couple of questions that arose while I was reading the code. But I think, it'll be easy to clear those up and get this fix on the road :)
EDIT: I may stand corrected on my comments below, because as pointed out in #10 (comment), I have been running the wrong tests. I'll have another look and keep you posted.
| $tokens = (function(): \Generator { | ||
| throw new \Exception('Once debugged, $tokens is empty.'); | ||
| // @phpstan-ignore-next-line | ||
| yield; | ||
| })(); |
There was a problem hiding this comment.
This should rather be removed, right?
| if (!Scanner::isEnd($this->tokens)) { | ||
| yield from $this->tokens; | ||
| } |
There was a problem hiding this comment.
| if (!Scanner::isEnd($this->tokens)) { | |
| yield from $this->tokens; | |
| } | |
| yield from $this->tokens; |
Scanner::isEnd only checks whether $this->tokens is still valid. This is implied in yield from - the extra check is not needed.
EDIT: Oh my god! Not true at all. Indeed if I remove this check, the unit tests fail... This is highly unexpected. I honestly don't know what's going on here.
|
Thanks for taking care @grebaldi and sorry for not responding ^^ its hard to have so many side projects xD |
| if (Scanner::isEnd($tokens) || $precedence->mustStopAt(Scanner::type($tokens))) { | ||
| return new self( | ||
| root: $root | ||
| ); | ||
| } | ||
|
|
||
| while (!Scanner::isEnd($tokens) && !$precedence->mustStopAt(Scanner::type($tokens))) { | ||
| Scanner::skipSpaceAndComments($tokens); |
There was a problem hiding this comment.
The additions that are here are not necessary. The condition is also handled by the "while" loop and later comes the return.
I think i messed stuff up because the partial fix belongs to #10
either way, it works as it is, just not super clean.
previously
ExpressionNode::fromString("(foo)")couldnt be parsed (it worked though when used in return statement of a module)but the real fix, is that you couldnt write
return ((foo))as $tokens is reassigned but not passed by ref(foo === ((null))) ? 1 : (((0)))this seemed to worked previously but not this:(((foo)) === ((null))) ? 1 : (((0)))