Skip to content
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

Support underscores in number literals #4487

Merged
merged 1 commit into from
Dec 2, 2024

Conversation

smnandre
Copy link
Contributor

{{ 1000 == 1_000 ? 'yes' : 'no' }}
# now: syntax error
# this PR: "yes"

As of PHP 7.4.0, integer literals may contain underscores (_) between digits, for better readability of literals. These underscores are removed by PHP's scanner.

https://www.php.net/manual/en/language.types.integer.php

This PR replicates that behaviour, using the regexp to match the literals and then remove the "_".

I'm targeting Twig4 but maybe 3.x would be ok?

I cannot think of a real case that

  • does not trigger an error currently
  • would work differently after this PR

@fabpot
Copy link
Contributor

fabpot commented Nov 30, 2024

LGTM on 3.x
Can you also add an entry in the changelog and add a note in the docs ?

@smnandre smnandre changed the base branch from 4.x to 3.x November 30, 2024 08:58
@smnandre
Copy link
Contributor Author

@fabpot updated to target 3.x, mention syntax in documentation & add a line in changelog

CHANGELOG Outdated Show resolved Hide resolved
src/Lexer.php Outdated
@@ -44,7 +44,7 @@ class Lexer
public const STATE_INTERPOLATION = 4;

public const REGEX_NAME = '/[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*/A';
public const REGEX_NUMBER = '/[0-9]+(?:\.[0-9]+)?([Ee][\+\-][0-9]+)?/A';
public const REGEX_NUMBER = '/[0-9]+(?:_[0-9]+)*(?:\.[0-9]+(?:_[0-9]+)*)?([Ee][\+\-][0-9]+)?/A';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can do the same as in https://github.com/symfony/symfony/blob/7.3/src/Symfony/Component/ExpressionLanguage/Lexer.php#L42-L43 where the exponent can also accept _.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fabpot
Copy link
Contributor

fabpot commented Nov 30, 2024

What about doing the same changes as we’ve done on Symfony? Both the regex and the logic.

@smnandre
Copy link
Contributor Author

Yep i have been interrupted earlier.. will finish later today!

@smnandre
Copy link
Contributor Author

smnandre commented Nov 30, 2024

Update: i missread things.. i'm sorry.

@smnandre
Copy link
Contributor Author

smnandre commented Nov 30, 2024

Update: i missread the scanner code.. 🤕

@smnandre
Copy link
Contributor Author

smnandre commented Dec 1, 2024

@fabpot it took time to hit me ... it should be as you expected now.

Only thing missing is the "float without integer" syntax (ie .99).

What do you think we handle this in following PR ?
I'm afraid it would require other (bigger?) changes.

So, i had to update a bit the symfony regep, to disallow the "decimal part without integer part".

    public const REGEX_NUMBER = '/
                (?(DEFINE)(?P<LNUM>[0-9]+(_[0-9]+)*))
-                (?:\.(?&LNUM)|(?&LNUM)(?:\.(?!\.)(?&LNUM)?)?)(?:[eE][+-]?(?&LNUM))?/Ax';
+                (?:(?&LNUM)(?:\.(?!\.)(?&LNUM)?)?)(?:[eE][+-]?(?&LNUM))?/Ax';

The following tests pass with the current state of this PR, but would fail if we kept the original Symfony regexp.

❌ nested.definedArray.0

Twig\Error\Error: Twig\Error\SyntaxError: Unexpected token "number" ("end of print statement" expected) in "index.twig" at line 16.

Pretty much the same thing/cause

{{ nested.definedArray.0          is     defined ? 'ok' : 'ko' }}

https://github.com/twigphp/Twig/blob/6823b64cad3ce8f9300ff69cfa7d23e20d61f10c/tests/Fixtures/tests/defined.test

if items.3 is defined

Twig\Error\Error: Twig\Error\SyntaxError: Unexpected token "number" of value "0.3" ("end of statement block" expected) in "index.twig" at line 7

{% if items.3 is defined %}

I guess we are in the same situation you explained me quite recently (with a block name "cb-1")

Not sure how would fix this though 🤷‍♂️

--

src/Lexer.php Show resolved Hide resolved
phpstan-baseline.neon Outdated Show resolved Hide resolved
@fabpot
Copy link
Contributor

fabpot commented Dec 2, 2024

Thank you @smnandre.

@fabpot fabpot merged commit d894f92 into twigphp:3.x Dec 2, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants