-
Notifications
You must be signed in to change notification settings - Fork 61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ConstExprParser: support numeric literal separator #189
ConstExprParser: support numeric literal separator #189
Conversation
d25945f
to
57f6787
Compare
|
||
ConstantFloatExp | ||
= "e" ["-"] 1*ByteDecDigit | ||
= "e" ["-"] 1*ByteDecDigit *("_" 1*ByteDecDigit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please see phpstan/phpstan-src#2358 (comment), +
before exponent should be supported, for consistency, also before any number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, it makes sense to support the same range of expressions as PHP does. But it feels out of scope of this PR; feel free to follow up with a fix :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will, this PR is merged, thus there will be no conflicts... One question, why is the grammar defined on two places - doc/grammars/type.abnf and src/Lexer/Lexer.php, is one file generated from the other one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doc/grammars/type.abnf
is mostly for documentation purposes and it's very likely out of sync. There's also FuzzyTest using it but I'm not sure about its purpose.
9785f59
to
65e237e
Compare
Thank you very much! I'm gonna release this as part of phpdoc-parser 1.21 but it's not going to be right away, because I just did this massive amount of work 308c57c which I still need to improve a bit :) |
Sure, no rush :) |
@@ -47,7 +48,7 @@ public function parse(TokenIterator $tokens, bool $trimStrings = false): Ast\Con | |||
|
|||
return $this->enrichWithAttributes( | |||
$tokens, | |||
new Ast\ConstExpr\ConstExprFloatNode($value), | |||
new Ast\ConstExpr\ConstExprFloatNode(str_replace('_', '', $value)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason for str_replace
here? It seems there are no other text transformations done in general, for example \d\.
is not normalized to \d\.\d
etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping the underscore here would be a BC break because phpstan-src simply casts this value to int or float, and (int) '1_000_000'
evaluates to 1
.
This PR adds support for an underscore separator in numeric literals, e.g.
/** @var int<0, 999_999_999> */
The logic should be the same as supported by PHP (https://wiki.php.net/rfc/numeric_literal_separator). To maintain BC, this is also implemented similarly to how PHP does it: the separator is supported by the lexer but then stripped